From 87315a6aabc0485268dde9d7f93ce64a78bf3a41 Mon Sep 17 00:00:00 2001 From: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Date: Wed, 18 Nov 2020 20:01:22 +0100 Subject: [PATCH] ibc: commit packet and hash (#7808) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ibc: commit packet and hash * minor changes * fix build * update err msgs * don't use Any * update commitment hash to used fixed length preimage The commitment hash will now use 8 bytes timeout timestamp + 8 bytes timeout height version number + 8 bytes timeout height version height + sha 256 hash of data * remove error from CommitPacket return values * update godoc * rm proto-tools-stamp Co-authored-by: Colin Axner Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- x/ibc/applications/transfer/types/trace.go | 5 +-- x/ibc/core/02-client/types/codec.go | 3 -- .../core/03-connection/keeper/verify_test.go | 3 +- x/ibc/core/04-channel/keeper/packet.go | 14 +++++--- x/ibc/core/04-channel/keeper/timeout.go | 12 ++++--- x/ibc/core/04-channel/types/packet.go | 32 +++++++++++++------ x/ibc/core/04-channel/types/packet_test.go | 16 ++++++++++ .../07-tendermint/types/client_state_test.go | 31 +++++++++--------- 8 files changed, 78 insertions(+), 38 deletions(-) diff --git a/x/ibc/applications/transfer/types/trace.go b/x/ibc/applications/transfer/types/trace.go index b4527d668f..f45113efa3 100644 --- a/x/ibc/applications/transfer/types/trace.go +++ b/x/ibc/applications/transfer/types/trace.go @@ -1,12 +1,12 @@ package types import ( + "crypto/sha256" "encoding/hex" "fmt" "sort" "strings" - "github.com/tendermint/tendermint/crypto/tmhash" tmbytes "github.com/tendermint/tendermint/libs/bytes" tmtypes "github.com/tendermint/tendermint/types" @@ -42,7 +42,8 @@ func ParseDenomTrace(rawDenom string) DenomTrace { // // hash = sha256(tracePath + "/" + baseDenom) func (dt DenomTrace) Hash() tmbytes.HexBytes { - return tmhash.Sum([]byte(dt.GetFullDenomPath())) + hash := sha256.Sum256([]byte(dt.GetFullDenomPath())) + return hash[:] } // GetPrefix returns the receiving denomination prefix composed by the trace info and a separator. diff --git a/x/ibc/core/02-client/types/codec.go b/x/ibc/core/02-client/types/codec.go index d3cb8f7a5f..64b2a3a4f1 100644 --- a/x/ibc/core/02-client/types/codec.go +++ b/x/ibc/core/02-client/types/codec.go @@ -27,9 +27,6 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) { registry.RegisterInterface( "ibc.core.client.v1.Height", (*exported.Height)(nil), - ) - registry.RegisterImplementations( - (*exported.Height)(nil), &Height{}, ) registry.RegisterInterface( diff --git a/x/ibc/core/03-connection/keeper/verify_test.go b/x/ibc/core/03-connection/keeper/verify_test.go index ace373e12a..3b047e741a 100644 --- a/x/ibc/core/03-connection/keeper/verify_test.go +++ b/x/ibc/core/03-connection/keeper/verify_test.go @@ -289,9 +289,10 @@ func (suite *KeeperTestSuite) TestVerifyPacketCommitment() { packet.Data = []byte(ibctesting.InvalidID) } + commitment := channeltypes.CommitPacket(suite.chainB.App.IBCKeeper.Codec(), packet) err = suite.chainB.App.IBCKeeper.ConnectionKeeper.VerifyPacketCommitment( suite.chainB.GetContext(), connection, malleateHeight(proofHeight, tc.heightDiff), proof, - packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence(), channeltypes.CommitPacket(packet), + packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence(), commitment, ) if tc.expPass { diff --git a/x/ibc/core/04-channel/keeper/packet.go b/x/ibc/core/04-channel/keeper/packet.go index e86887eee9..9af59745d5 100644 --- a/x/ibc/core/04-channel/keeper/packet.go +++ b/x/ibc/core/04-channel/keeper/packet.go @@ -109,9 +109,11 @@ func (k Keeper) SendPacket( ) } + commitment := types.CommitPacket(k.cdc, packet) + nextSequenceSend++ k.SetNextSequenceSend(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), nextSequenceSend) - k.SetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence(), types.CommitPacket(packet)) + k.SetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence(), commitment) // Emit Event with Packet data along with other packet information for relayer to pick up // and relay to other chain @@ -216,11 +218,13 @@ func (k Keeper) RecvPacket( ) } + commitment := types.CommitPacket(k.cdc, packet) + // verify that the counterparty did commit to sending this packet if err := k.connectionKeeper.VerifyPacketCommitment( ctx, connectionEnd, proofHeight, proof, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence(), - types.CommitPacket(packet), + commitment, ); err != nil { return sdkerrors.Wrap(err, "couldn't verify counterparty packet commitment") } @@ -443,9 +447,11 @@ func (k Keeper) AcknowledgePacket( commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) + packetCommitment := types.CommitPacket(k.cdc, packet) + // verify we sent the packet and haven't cleared it out yet - if !bytes.Equal(commitment, types.CommitPacket(packet)) { - return sdkerrors.Wrapf(types.ErrInvalidPacket, "commitment bytes are not equal: got (%v), expected (%v)", types.CommitPacket(packet), commitment) + if !bytes.Equal(commitment, packetCommitment) { + return sdkerrors.Wrapf(types.ErrInvalidPacket, "commitment bytes are not equal: got (%v), expected (%v)", packetCommitment, commitment) } if err := k.connectionKeeper.VerifyPacketAcknowledgement( diff --git a/x/ibc/core/04-channel/keeper/timeout.go b/x/ibc/core/04-channel/keeper/timeout.go index 490eb83c8f..1f3dac918f 100644 --- a/x/ibc/core/04-channel/keeper/timeout.go +++ b/x/ibc/core/04-channel/keeper/timeout.go @@ -80,9 +80,11 @@ func (k Keeper) TimeoutPacket( commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) + packetCommitment := types.CommitPacket(k.cdc, packet) + // verify we sent the packet and haven't cleared it out yet - if !bytes.Equal(commitment, types.CommitPacket(packet)) { - return sdkerrors.Wrapf(types.ErrInvalidPacket, "packet commitment bytes are not equal: got (%v), expected (%v)", commitment, types.CommitPacket(packet)) + if !bytes.Equal(commitment, packetCommitment) { + return sdkerrors.Wrapf(types.ErrInvalidPacket, "packet commitment bytes are not equal: got (%v), expected (%v)", commitment, packetCommitment) } switch channel.Ordering { @@ -216,9 +218,11 @@ func (k Keeper) TimeoutOnClose( commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) + packetCommitment := types.CommitPacket(k.cdc, packet) + // verify we sent the packet and haven't cleared it out yet - if !bytes.Equal(commitment, types.CommitPacket(packet)) { - return sdkerrors.Wrapf(types.ErrInvalidPacket, "packet commitment bytes are not equal: got (%v), expected (%v)", commitment, types.CommitPacket(packet)) + if !bytes.Equal(commitment, packetCommitment) { + return sdkerrors.Wrapf(types.ErrInvalidPacket, "packet commitment bytes are not equal: got (%v), expected (%v)", commitment, packetCommitment) } counterpartyHops, found := k.CounterpartyHops(ctx, channel) diff --git a/x/ibc/core/04-channel/types/packet.go b/x/ibc/core/04-channel/types/packet.go index 46c1a1f06e..18a3690b1e 100644 --- a/x/ibc/core/04-channel/types/packet.go +++ b/x/ibc/core/04-channel/types/packet.go @@ -1,8 +1,9 @@ package types import ( - "github.com/tendermint/tendermint/crypto/tmhash" + "crypto/sha256" + "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" clienttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types" @@ -10,19 +11,32 @@ import ( "github.com/cosmos/cosmos-sdk/x/ibc/core/exported" ) -// CommitPacket returns a packet commitment bytes. The commitment consists of: -// hash(timeout_timestamp + timeout_version_number + timeout_version_height + data) from a given packet. -func CommitPacket(packet exported.PacketI) []byte { +// CommitPacket returns the packet commitment bytes. The commitment consists of: +// sha256_hash(timeout_timestamp + timeout_height.VersionNumber + timeout_height.VersionHeight + sha256_hash(data)) +// from a given packet. This results in a fixed length preimage. +// NOTE: sdk.Uint64ToBigEndian sets the uint64 to a slice of length 8. +func CommitPacket(cdc codec.BinaryMarshaler, packet exported.PacketI) []byte { + timeoutHeight := packet.GetTimeoutHeight() + buf := sdk.Uint64ToBigEndian(packet.GetTimeoutTimestamp()) - buf = append(buf, sdk.Uint64ToBigEndian(packet.GetTimeoutHeight().GetVersionNumber())...) - buf = append(buf, sdk.Uint64ToBigEndian(packet.GetTimeoutHeight().GetVersionHeight())...) - buf = append(buf, packet.GetData()...) - return tmhash.Sum(buf) + + versionNumber := sdk.Uint64ToBigEndian(timeoutHeight.GetVersionNumber()) + buf = append(buf, versionNumber...) + + versionHeight := sdk.Uint64ToBigEndian(timeoutHeight.GetVersionHeight()) + buf = append(buf, versionHeight...) + + dataHash := sha256.Sum256(packet.GetData()) + buf = append(buf, dataHash[:]...) + + hash := sha256.Sum256(buf) + return hash[:] } // CommitAcknowledgement returns the hash of commitment bytes func CommitAcknowledgement(data []byte) []byte { - return tmhash.Sum(data) + hash := sha256.Sum256(data) + return hash[:] } var _ exported.PacketI = (*Packet)(nil) diff --git a/x/ibc/core/04-channel/types/packet_test.go b/x/ibc/core/04-channel/types/packet_test.go index 114352f5c9..12ed828e66 100644 --- a/x/ibc/core/04-channel/types/packet_test.go +++ b/x/ibc/core/04-channel/types/packet_test.go @@ -5,9 +5,25 @@ import ( "github.com/stretchr/testify/require" + "github.com/cosmos/cosmos-sdk/codec" + codectypes "github.com/cosmos/cosmos-sdk/codec/types" + clienttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types" "github.com/cosmos/cosmos-sdk/x/ibc/core/04-channel/types" ) +func TestCommitPacket(t *testing.T) { + packet := types.NewPacket(validPacketData, 1, portid, chanid, cpportid, cpchanid, timeoutHeight, timeoutTimestamp) + + registry := codectypes.NewInterfaceRegistry() + clienttypes.RegisterInterfaces(registry) + types.RegisterInterfaces(registry) + + cdc := codec.NewProtoCodec(registry) + + commitment := types.CommitPacket(cdc, &packet) + require.NotNil(t, commitment) +} + func TestPacketValidateBasic(t *testing.T) { testCases := []struct { packet types.Packet diff --git a/x/ibc/light-clients/07-tendermint/types/client_state_test.go b/x/ibc/light-clients/07-tendermint/types/client_state_test.go index a160484609..71f9f3fc3b 100644 --- a/x/ibc/light-clients/07-tendermint/types/client_state_test.go +++ b/x/ibc/light-clients/07-tendermint/types/client_state_test.go @@ -33,57 +33,57 @@ func (suite *TendermintTestSuite) TestValidate() { }{ { name: "valid client", - clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), + clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), expPass: true, }, { name: "valid client with nil upgrade path", - clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), "", false, false), + clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), "", false, false), expPass: true, }, { name: "invalid chainID", - clientState: types.NewClientState(" ", types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), + clientState: types.NewClientState(" ", types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), expPass: false, }, { name: "invalid trust level", - clientState: types.NewClientState(chainID, types.Fraction{Numerator: 0, Denominator: 1}, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), + clientState: types.NewClientState(chainID, types.Fraction{Numerator: 0, Denominator: 1}, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), expPass: false, }, { name: "invalid trusting period", - clientState: types.NewClientState(chainID, types.DefaultTrustLevel, 0, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), + clientState: types.NewClientState(chainID, types.DefaultTrustLevel, 0, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), expPass: false, }, { name: "invalid unbonding period", - clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, 0, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), + clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, 0, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), expPass: false, }, { name: "invalid max clock drift", - clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, 0, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), + clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, 0, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), expPass: false, }, { name: "invalid height", - clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, clienttypes.ZeroHeight(), commitmenttypes.GetSDKSpecs(), upgradePath, false, false), + clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, clienttypes.ZeroHeight(), commitmenttypes.GetSDKSpecs(), upgradePath, false, false), expPass: false, }, { name: "trusting period not less than unbonding period", - clientState: types.NewClientState(chainID, types.DefaultTrustLevel, ubdPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), + clientState: types.NewClientState(chainID, types.DefaultTrustLevel, ubdPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), expPass: false, }, { name: "proof specs is nil", - clientState: types.NewClientState(chainID, types.DefaultTrustLevel, ubdPeriod, ubdPeriod, maxClockDrift, height, nil, upgradePath, false, false), + clientState: types.NewClientState(chainID, types.DefaultTrustLevel, ubdPeriod, ubdPeriod, maxClockDrift, height, nil, upgradePath, false, false), expPass: false, }, { name: "proof specs contains nil", - clientState: types.NewClientState(chainID, types.DefaultTrustLevel, ubdPeriod, ubdPeriod, maxClockDrift, height, []*ics23.ProofSpec{ics23.TendermintSpec, nil}, upgradePath, false, false), + clientState: types.NewClientState(chainID, types.DefaultTrustLevel, ubdPeriod, ubdPeriod, maxClockDrift, height, []*ics23.ProofSpec{ics23.TendermintSpec, nil}, upgradePath, false, false), expPass: false, }, } @@ -119,7 +119,7 @@ func (suite *TendermintTestSuite) TestVerifyClientConsensusState() { // }, { name: "ApplyPrefix failed", - clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), + clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), consensusState: types.ConsensusState{ Root: commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), }, @@ -128,7 +128,7 @@ func (suite *TendermintTestSuite) TestVerifyClientConsensusState() { }, { name: "latest client height < height", - clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), + clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), consensusState: types.ConsensusState{ Root: commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), }, @@ -146,7 +146,7 @@ func (suite *TendermintTestSuite) TestVerifyClientConsensusState() { }, { name: "proof verification failed", - clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), + clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), upgradePath, false, false), consensusState: types.ConsensusState{ Root: commitmenttypes.NewMerkleRoot(suite.header.Header.GetAppHash()), NextValidatorsHash: suite.valsHash, @@ -396,9 +396,10 @@ func (suite *TendermintTestSuite) TestVerifyPacketCommitment() { store := suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), clientA) + commitment := channeltypes.CommitPacket(suite.chainA.App.IBCKeeper.Codec(), packet) err = clientState.VerifyPacketCommitment( store, suite.chainA.Codec, proofHeight, &prefix, proof, - packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence(), channeltypes.CommitPacket(packet), + packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence(), commitment, ) if tc.expPass {