From 7e0deeaea66920d37127a2e4ad0904896495e4a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 21 Oct 2024 18:12:31 +0200 Subject: [PATCH 1/5] imp: cleanup verifcation arg code for 23-commitment --- modules/core/23-commitment/types/merkle.go | 44 +++++++------------ .../core/23-commitment/types/merkle_test.go | 7 +-- 2 files changed, 16 insertions(+), 35 deletions(-) diff --git a/modules/core/23-commitment/types/merkle.go b/modules/core/23-commitment/types/merkle.go index 319e3ea5b8b..e94f12e0091 100644 --- a/modules/core/23-commitment/types/merkle.go +++ b/modules/core/23-commitment/types/merkle.go @@ -10,7 +10,7 @@ import ( cmtcrypto "github.com/cometbft/cometbft/proto/tendermint/crypto" - "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2" + v2 "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2" "github.com/cosmos/ibc-go/v9/modules/core/exported" ) @@ -86,19 +86,16 @@ func ApplyPrefix(prefix exported.Prefix, path v2.MerklePath) (v2.MerklePath, err // VerifyMembership verifies the membership of a merkle proof against the given root, path, and value. // Note that the path is expected as []string{, }. func (proof MerkleProof) VerifyMembership(specs []*ics23.ProofSpec, root exported.Root, path exported.Path, value []byte) error { - if err := proof.validateVerificationArgs(specs, root); err != nil { - return err - } - - // VerifyMembership specific argument validation mpath, ok := path.(v2.MerklePath) if !ok { return errorsmod.Wrapf(ErrInvalidProof, "path %v is not of type MerklePath", path) } - if len(mpath.KeyPath) != len(specs) { - return errorsmod.Wrapf(ErrInvalidProof, "path length %d not same as proof %d", - len(mpath.KeyPath), len(specs)) + + if err := validateVerificationArgs(proof, mpath, specs, root); err != nil { + return err } + + // VerifyMembership specific argument validation if len(value) == 0 { return errorsmod.Wrap(ErrInvalidProof, "empty value in membership proof") } @@ -112,18 +109,13 @@ func (proof MerkleProof) VerifyMembership(specs []*ics23.ProofSpec, root exporte // VerifyNonMembership verifies a chained proof where the absence of a given path is proven // at the lowest subtree and then each subtree's inclusion is proved up to the final root. func (proof MerkleProof) VerifyNonMembership(specs []*ics23.ProofSpec, root exported.Root, path exported.Path) error { - if err := proof.validateVerificationArgs(specs, root); err != nil { - return err - } - - // VerifyNonMembership specific argument validation mpath, ok := path.(v2.MerklePath) if !ok { return errorsmod.Wrapf(ErrInvalidProof, "path %v is not of type MerkleProof", path) } - if len(mpath.KeyPath) != len(specs) { - return errorsmod.Wrapf(ErrInvalidProof, "path length %d not same as proof %d", - len(mpath.KeyPath), len(specs)) + + if err := validateVerificationArgs(proof, mpath, specs, root); err != nil { + return err } switch proof.Proofs[0].Proof.(type) { @@ -234,16 +226,8 @@ func (proof *MerkleProof) Empty() bool { return proof == nil || proto.Equal(proof, blankMerkleProof) || proto.Equal(proof, blankProofOps) } -// ValidateBasic checks if the proof is empty. -func (proof MerkleProof) ValidateBasic() error { - if proof.Empty() { - return ErrInvalidProof - } - return nil -} - // validateVerificationArgs verifies the proof arguments are valid -func (proof MerkleProof) validateVerificationArgs(specs []*ics23.ProofSpec, root exported.Root) error { +func validateVerificationArgs(proof MerkleProof, path v2.MerklePath, specs []*ics23.ProofSpec, root exported.Root) error { if proof.Empty() { return errorsmod.Wrap(ErrInvalidMerkleProof, "proof cannot be empty") } @@ -253,9 +237,11 @@ func (proof MerkleProof) validateVerificationArgs(specs []*ics23.ProofSpec, root } if len(specs) != len(proof.Proofs) { - return errorsmod.Wrapf(ErrInvalidMerkleProof, - "length of specs: %d not equal to length of proof: %d", - len(specs), len(proof.Proofs)) + return errorsmod.Wrapf(ErrInvalidMerkleProof, "length of specs: %d not equal to length of proof: %d", len(specs), len(proof.Proofs)) + } + + if len(path.KeyPath) != len(specs) { + return errorsmod.Wrapf(ErrInvalidProof, "path length %d not same as proof %d", len(path.KeyPath), len(specs)) } for i, spec := range specs { diff --git a/modules/core/23-commitment/types/merkle_test.go b/modules/core/23-commitment/types/merkle_test.go index dd1ec4e6172..b1c04d6606c 100644 --- a/modules/core/23-commitment/types/merkle_test.go +++ b/modules/core/23-commitment/types/merkle_test.go @@ -9,7 +9,7 @@ import ( storetypes "cosmossdk.io/store/types" "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types" - "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2" + v2 "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2" ) func (suite *MerkleTestSuite) TestVerifyMembership() { @@ -27,9 +27,6 @@ func (suite *MerkleTestSuite) TestVerifyMembership() { proof, err := types.ConvertProofs(res.ProofOps) require.NoError(suite.T(), err) - suite.Require().NoError(proof.ValidateBasic()) - suite.Require().Error(types.MerkleProof{}.ValidateBasic()) - cases := []struct { name string root []byte @@ -93,8 +90,6 @@ func (suite *MerkleTestSuite) TestVerifyNonMembership() { proof, err := types.ConvertProofs(res.ProofOps) require.NoError(suite.T(), err) - suite.Require().NoError(proof.ValidateBasic()) - cases := []struct { name string root []byte From 7a7fa8948310e68ccfbc1c5cbca3fd6bd37d160a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 21 Oct 2024 18:20:54 +0200 Subject: [PATCH 2/5] Update modules/core/23-commitment/types/merkle.go --- modules/core/23-commitment/types/merkle.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/23-commitment/types/merkle.go b/modules/core/23-commitment/types/merkle.go index e94f12e0091..8f06d5c2b4a 100644 --- a/modules/core/23-commitment/types/merkle.go +++ b/modules/core/23-commitment/types/merkle.go @@ -10,7 +10,7 @@ import ( cmtcrypto "github.com/cometbft/cometbft/proto/tendermint/crypto" - v2 "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2" + "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2" "github.com/cosmos/ibc-go/v9/modules/core/exported" ) From e61f947184478befc8f504f5789b51551471c02b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 21 Oct 2024 18:21:10 +0200 Subject: [PATCH 3/5] Update modules/core/23-commitment/types/merkle_test.go --- modules/core/23-commitment/types/merkle_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/23-commitment/types/merkle_test.go b/modules/core/23-commitment/types/merkle_test.go index b1c04d6606c..9c8657cc237 100644 --- a/modules/core/23-commitment/types/merkle_test.go +++ b/modules/core/23-commitment/types/merkle_test.go @@ -9,7 +9,7 @@ import ( storetypes "cosmossdk.io/store/types" "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types" - v2 "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2" + "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2" ) func (suite *MerkleTestSuite) TestVerifyMembership() { From 37f89a74ecd810b9fccdacedf405e8223c257c72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 23 Oct 2024 12:41:29 +0200 Subject: [PATCH 4/5] chore: review followups --- .../core/23-commitment/types/bench_test.go | 15 ----------- modules/core/23-commitment/types/merkle.go | 26 +++++-------------- 2 files changed, 7 insertions(+), 34 deletions(-) delete mode 100644 modules/core/23-commitment/types/bench_test.go diff --git a/modules/core/23-commitment/types/bench_test.go b/modules/core/23-commitment/types/bench_test.go deleted file mode 100644 index 83794fc6f6e..00000000000 --- a/modules/core/23-commitment/types/bench_test.go +++ /dev/null @@ -1,15 +0,0 @@ -package types - -import ( - "testing" -) - -func BenchmarkMerkleProofEmpty(b *testing.B) { - b.ReportAllocs() - var mk MerkleProof - for i := 0; i < b.N; i++ { - if !mk.Empty() { - b.Fatal("supposed to be empty") - } - } -} diff --git a/modules/core/23-commitment/types/merkle.go b/modules/core/23-commitment/types/merkle.go index 8f06d5c2b4a..8fbc379ee91 100644 --- a/modules/core/23-commitment/types/merkle.go +++ b/modules/core/23-commitment/types/merkle.go @@ -3,14 +3,11 @@ package types import ( "bytes" - "github.com/cosmos/gogoproto/proto" ics23 "github.com/cosmos/ics23/go" errorsmod "cosmossdk.io/errors" - cmtcrypto "github.com/cometbft/cometbft/proto/tendermint/crypto" - - "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2" + v2 "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2" "github.com/cosmos/ibc-go/v9/modules/core/exported" ) @@ -214,22 +211,13 @@ func verifyChainedMembershipProof(root []byte, specs []*ics23.ProofSpec, proofs return nil } -// blankMerkleProof and blankProofOps will be used to compare against their zero values, -// and are declared as globals to avoid having to unnecessarily re-allocate on every comparison. -var ( - blankMerkleProof = &MerkleProof{} - blankProofOps = &cmtcrypto.ProofOps{} -) - -// Empty returns true if the root is empty -func (proof *MerkleProof) Empty() bool { - return proof == nil || proto.Equal(proof, blankMerkleProof) || proto.Equal(proof, blankProofOps) -} - -// validateVerificationArgs verifies the proof arguments are valid +// validateVerificationArgs verifies the proof arguments are valid. +// The merkle path and merkle proof contain a list of keys and their proofs +// which correspond to individual trees. The length of these keys and their proofs +// must equal the length of the given specs. All arguments must be non-empty. func validateVerificationArgs(proof MerkleProof, path v2.MerklePath, specs []*ics23.ProofSpec, root exported.Root) error { - if proof.Empty() { - return errorsmod.Wrap(ErrInvalidMerkleProof, "proof cannot be empty") + if proof.GetProofs() == nil { + return errorsmod.Wrap(ErrInvalidMerkleProof, "proof must not be empty") } if root == nil || root.Empty() { From a43f25693e68a4c43ef6774fd5f5673381baeb4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 23 Oct 2024 12:43:02 +0200 Subject: [PATCH 5/5] Update modules/core/23-commitment/types/merkle.go --- modules/core/23-commitment/types/merkle.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/23-commitment/types/merkle.go b/modules/core/23-commitment/types/merkle.go index 3025f3b9995..5b0f5ba973d 100644 --- a/modules/core/23-commitment/types/merkle.go +++ b/modules/core/23-commitment/types/merkle.go @@ -7,7 +7,7 @@ import ( errorsmod "cosmossdk.io/errors" - v2 "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2" + "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2" "github.com/cosmos/ibc-go/v9/modules/core/exported" )