Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve error messages, indicate already relayed packets #184

Merged
merged 11 commits into from
May 27, 2021
20 changes: 16 additions & 4 deletions modules/core/04-channel/keeper/packet.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,8 @@ func (k Keeper) RecvPacket(
_, found := k.GetPacketReceipt(ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
if found {
return sdkerrors.Wrapf(
types.ErrInvalidPacket,
"packet sequence (%d) already has been received", packet.GetSequence(),
types.ErrPacketReceived,
"packet sequence (%d)", packet.GetSequence(),
)
}

Expand All @@ -262,9 +262,17 @@ func (k Keeper) RecvPacket(
)
}

// helpful error message for relayers
if packet.GetSequence() < nextSequenceRecv {
return sdkerrors.Wrapf(
types.ErrPacketReceived,
"packet sequence (%d), next sequence receive (%d)", packet.GetSequence(), nextSequenceRecv,
)
}

if packet.GetSequence() != nextSequenceRecv {
return sdkerrors.Wrapf(
types.ErrInvalidPacket,
types.ErrPacketReceiptOutOfOrder,
"packet sequence ≠ next receive sequence (%d ≠ %d)", packet.GetSequence(), nextSequenceRecv,
)
}
Expand Down Expand Up @@ -461,6 +469,10 @@ func (k Keeper) AcknowledgePacket(

commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())

if len(commitment) == 0 {
return sdkerrors.Wrapf(types.ErrPacketAcknowledged, "packet commitment does not exist")
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

packetCommitment := types.CommitPacket(k.cdc, packet)

// verify we sent the packet and haven't cleared it out yet
Expand All @@ -472,7 +484,7 @@ func (k Keeper) AcknowledgePacket(
ctx, connectionEnd, proofHeight, proof, packet.GetDestPort(), packet.GetDestChannel(),
packet.GetSequence(), acknowledgement,
); err != nil {
return sdkerrors.Wrap(err, "packet acknowledgement verification failed")
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
return err
}

// assert packets acknowledged in order
Expand Down
118 changes: 106 additions & 12 deletions modules/core/04-channel/keeper/packet_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,29 @@ func (suite *KeeperTestSuite) TestRecvPacket() {
// attempts to receive packet 2 without receiving packet 1
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
}, true},
{"packet already relayed ORDERED channel", func() {
path.SetChannelOrdered()
suite.coordinator.Setup(path)

packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
err := path.EndpointA.SendPacket(packet)
suite.Require().NoError(err)
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)

err = path.EndpointB.RecvPacket(packet.(types.Packet))
suite.Require().NoError(err)
}, false},
{"packet already relayed UNORDERED channel", func() {
// setup uses an UNORDERED channel
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
err := path.EndpointA.SendPacket(packet)
suite.Require().NoError(err)
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)

err = path.EndpointB.RecvPacket(packet.(types.Packet))
suite.Require().NoError(err)
}, false},
{"out of order packet failure with ORDERED channel", func() {
path.SetChannelOrdered()
suite.coordinator.Setup(path)
Expand Down Expand Up @@ -285,6 +308,8 @@ func (suite *KeeperTestSuite) TestRecvPacket() {
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
}, false},
{"connection not found", func() {
suite.coordinator.Setup(path)

// pass channel check
suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetChannel(
suite.chainB.GetContext(),
Expand Down Expand Up @@ -323,9 +348,11 @@ func (suite *KeeperTestSuite) TestRecvPacket() {
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
}, false},
{"next receive sequence is not found", func() {
path := ibctesting.NewPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

path.EndpointA.ChannelID = ibctesting.FirstChannelID
path.EndpointB.ChannelID = ibctesting.FirstChannelID

// manually creating channel prevents next recv sequence from being set
suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetChannel(
suite.chainB.GetContext(),
Expand All @@ -336,10 +363,13 @@ func (suite *KeeperTestSuite) TestRecvPacket() {
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)

// manually set packet commitment
suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetPacketCommitment(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, packet.GetSequence(), ibctesting.MockPacketData)
suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetPacketCommitment(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, packet.GetSequence(), types.CommitPacket(suite.chainA.App.AppCodec(), packet))
suite.chainB.CreateChannelCapability(suite.chainB.GetSimApp().ScopedIBCMockKeeper, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)

channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)

path.EndpointA.UpdateClient()
path.EndpointB.UpdateClient()
}, false},
{"receipt already stored", func() {
suite.coordinator.Setup(path)
Expand All @@ -366,7 +396,7 @@ func (suite *KeeperTestSuite) TestRecvPacket() {

// get proof of packet commitment from chainA
packetKey := host.PacketCommitmentKey(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
proof, proofHeight := suite.chainA.QueryProof(packetKey)
proof, proofHeight := path.EndpointA.QueryProof(packetKey)

err := suite.chainB.App.GetIBCKeeper().ChannelKeeper.RecvPacket(suite.chainB.GetContext(), channelCap, packet, proof, proofHeight)

Expand Down Expand Up @@ -520,6 +550,41 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {

channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
}, true},
{"packet already acknowledged ordered channel", func() {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
path.SetChannelOrdered()
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
// create packet commitment
err := path.EndpointA.SendPacket(packet)
suite.Require().NoError(err)

// create packet receipt and acknowledgement
err = path.EndpointB.RecvPacket(packet)
suite.Require().NoError(err)

channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

err = path.EndpointA.AcknowledgePacket(packet, ack.Acknowledgement())
suite.Require().NoError(err)
}, false},
{"packet already acknowledged unordered channel", func() {
// setup uses an UNORDERED channel
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)

// create packet commitment
err := path.EndpointA.SendPacket(packet)
suite.Require().NoError(err)

// create packet receipt and acknowledgement
err = path.EndpointB.RecvPacket(packet)
suite.Require().NoError(err)

channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

err = path.EndpointA.AcknowledgePacket(packet, ack.Acknowledgement())
suite.Require().NoError(err)
}, false},
{"channel not found", func() {
// use wrong channel naming
suite.coordinator.Setup(path)
Expand Down Expand Up @@ -560,11 +625,13 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
}, false},
{"connection not found", func() {
suite.coordinator.Setup(path)

// pass channel check
suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetChannel(
suite.chainB.GetContext(),
path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID,
types.NewChannel(types.OPEN, types.ORDERED, types.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{connIDB}, path.EndpointB.ChannelConfig.Version),
suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetChannel(
suite.chainA.GetContext(),
path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID,
types.NewChannel(types.OPEN, types.ORDERED, types.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID), []string{"connection-1000"}, path.EndpointA.ChannelConfig.Version),
)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
suite.chainA.CreateChannelCapability(suite.chainA.GetSimApp().ScopedIBCMockKeeper, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
Expand Down Expand Up @@ -601,23 +668,50 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
path.EndpointA.SendPacket(packet)
channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
}, false},
{"packet commitment bytes do not match", func() {
// setup uses an UNORDERED channel
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)

// create packet commitment
err := path.EndpointA.SendPacket(packet)
suite.Require().NoError(err)

// create packet receipt and acknowledgement
err = path.EndpointB.RecvPacket(packet)
suite.Require().NoError(err)

channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

packet.Data = []byte("invalid packet commitment")
}, false},
{"next ack sequence not found", func() {
path := ibctesting.NewPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)

path.EndpointA.ChannelID = ibctesting.FirstChannelID
path.EndpointB.ChannelID = ibctesting.FirstChannelID

// manually creating channel prevents next sequence acknowledgement from being set
suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetChannel(
suite.chainA.GetContext(),
path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID,
types.NewChannel(types.OPEN, types.ORDERED, types.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID), []string{path.EndpointA.ConnectionID}, path.EndpointA.ChannelConfig.Version),
)

packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
// manually set packet commitment
suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetPacketCommitment(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, packet.GetSequence(), ibctesting.MockPacketData)
suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetPacketCommitment(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, packet.GetSequence(), types.CommitPacket(suite.chainA.App.AppCodec(), packet))

// manually set packet acknowledgement and capability
suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetPacketAcknowledgement(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, packet.GetSequence(), ibctesting.MockAcknowledgement)
suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetPacketAcknowledgement(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, packet.GetSequence(), types.CommitAcknowledgement(ack.Acknowledgement()))

suite.chainA.CreateChannelCapability(suite.chainA.GetSimApp().ScopedIBCMockKeeper, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

suite.coordinator.CommitBlock(path.EndpointA.Chain, path.EndpointB.Chain)

path.EndpointA.UpdateClient()
path.EndpointB.UpdateClient()
}, false},
{"next ack sequence mismatch ORDERED", func() {
path.SetChannelOrdered()
Expand Down Expand Up @@ -646,7 +740,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
tc.malleate()

packetKey := host.PacketAcknowledgementKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
proof, proofHeight := suite.chainB.QueryProof(packetKey)
proof, proofHeight := path.EndpointB.QueryProof(packetKey)

err := suite.chainA.App.GetIBCKeeper().ChannelKeeper.AcknowledgePacket(suite.chainA.GetContext(), channelCap, packet, ack.Acknowledgement(), proof, proofHeight)
pc := suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetPacketCommitment(suite.chainA.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
Expand Down
4 changes: 4 additions & 0 deletions modules/core/04-channel/keeper/timeout.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ func (k Keeper) TimeoutPacket(

commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())

if len(commitment) == 0 {
return sdkerrors.Wrapf(types.ErrPacketTimedOut, "packet sequence (%d)", packet.GetSequence())
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

packetCommitment := types.CommitPacket(k.cdc, packet)

// verify we sent the packet and haven't cleared it out yet
Expand Down
25 changes: 25 additions & 0 deletions modules/core/04-channel/keeper/timeout_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,31 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
// need to update chainA's client representing chainB to prove missing ack
path.EndpointA.UpdateClient()
}, true},
{"packet already timed out: ORDERED", func() {
ordered = true
path.SetChannelOrdered()

suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.GetSelfHeight(suite.chainB.GetContext()), uint64(suite.chainB.GetContext().BlockTime().UnixNano()))
path.EndpointA.SendPacket(packet)
// need to update chainA's client representing chainB to prove missing ack
path.EndpointA.UpdateClient()

err := path.EndpointA.TimeoutPacket(packet)
suite.Require().NoError(err)
}, false},
{"packet already timed out: UNORDERED", func() {
ordered = false

suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.GetSelfHeight(suite.chainB.GetContext()), disabledTimeoutTimestamp)
path.EndpointA.SendPacket(packet)
// need to update chainA's client representing chainB to prove missing ack
path.EndpointA.UpdateClient()

err := path.EndpointA.TimeoutPacket(packet)
suite.Require().NoError(err)
}, false},
{"channel not found", func() {
// use wrong channel naming
suite.coordinator.Setup(path)
Expand Down
13 changes: 10 additions & 3 deletions modules/core/04-channel/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,14 @@ var (
ErrTooManyConnectionHops = sdkerrors.Register(SubModuleName, 15, "too many connection hops")
ErrInvalidAcknowledgement = sdkerrors.Register(SubModuleName, 16, "invalid acknowledgement")
ErrPacketCommitmentNotFound = sdkerrors.Register(SubModuleName, 17, "packet commitment not found")
ErrPacketReceived = sdkerrors.Register(SubModuleName, 18, "packet already received")
ErrAcknowledgementExists = sdkerrors.Register(SubModuleName, 19, "acknowledgement for packet already exists")
ErrInvalidChannelIdentifier = sdkerrors.Register(SubModuleName, 20, "invalid channel identifier")
ErrAcknowledgementExists = sdkerrors.Register(SubModuleName, 18, "acknowledgement for packet already exists")
ErrInvalidChannelIdentifier = sdkerrors.Register(SubModuleName, 19, "invalid channel identifier")

// packets already relayed errors
ErrPacketReceived = sdkerrors.Register(SubModuleName, 20, "packet already received")
ErrPacketAcknowledged = sdkerrors.Register(SubModuleName, 21, "packet acknowledgement already processed")
ErrPacketTimedOut = sdkerrors.Register(SubModuleName, 22, "packet already timed out")

// ORDERED channel error
ErrPacketReceiptOutOfOrder = sdkerrors.Register(SubModuleName, 23, "packet cannot be received out of order")
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
)
4 changes: 2 additions & 2 deletions modules/core/23-commitment/types/merkle.go
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,8 @@ func verifyChainedMembershipProof(root []byte, specs []*ics23.ProofSpec, proofs
value = subroot
case *ics23.CommitmentProof_Nonexist:
return sdkerrors.Wrapf(ErrInvalidProof,
"chained membership proof contains nonexistence proof at index %d. If this is unexpected, please ensure that proof was queried from the height that contained the value in store and was queried with the correct key.",
i)
"chained membership proof contains nonexistence proof at index %d. If this is unexpected, please ensure that proof was queried from a height that contained the value in store and was queried with the correct key. The key used: %s",
i, keys)
default:
return sdkerrors.Wrapf(ErrInvalidProof,
"expected proof type: %T, got: %T", &ics23.CommitmentProof_Exist{}, proofs[i].Proof)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ func produceVerificationArgs(
if cs.GetLatestHeight().LT(height) {
return commitmenttypes.MerkleProof{}, nil, sdkerrors.Wrapf(
sdkerrors.ErrInvalidHeight,
"client state height < proof height (%d < %d)", cs.GetLatestHeight(), height,
"client state height < proof height (%d < %d), please ensure the client has been updated", cs.GetLatestHeight(), height,
)
}

Expand Down
Loading