From 0240148b7366aa60469d047c81d07ca945b710d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 19 Nov 2024 13:43:46 +0100 Subject: [PATCH] imp: cleanup verifcation arg code for 23-commitment (#7493) * imp: cleanup verifcation arg code for 23-commitment * Update modules/core/23-commitment/types/merkle.go * Update modules/core/23-commitment/types/merkle_test.go * chore: review followups * Update modules/core/23-commitment/types/merkle.go --------- Co-authored-by: DimitrisJim Co-authored-by: Damian Nolan --- .../core/23-commitment/types/bench_test.go | 15 ----- modules/core/23-commitment/types/merkle.go | 65 ++++++------------- .../core/23-commitment/types/merkle_test.go | 5 -- 3 files changed, 20 insertions(+), 65 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 05294e9c419..9b6e4b0d073 100644 --- a/modules/core/23-commitment/types/merkle.go +++ b/modules/core/23-commitment/types/merkle.go @@ -3,13 +3,10 @@ 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" "github.com/cosmos/ibc-go/v9/modules/core/exported" ) @@ -86,19 +83,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,17 +106,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 } // VerifyNonMembership will verify the absence of key in lowest subtree, and then chain inclusion proofs @@ -198,30 +188,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) -} - -// 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 { - if proof.Empty() { - return errorsmod.Wrap(ErrInvalidMerkleProof, "proof cannot be empty") +// 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.GetProofs() == nil { + return errorsmod.Wrap(ErrInvalidMerkleProof, "proof must not be empty") } if root == nil || root.Empty() { @@ -229,9 +202,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..9c8657cc237 100644 --- a/modules/core/23-commitment/types/merkle_test.go +++ b/modules/core/23-commitment/types/merkle_test.go @@ -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