WIP: Fix ICS23-Proofs #6178

AdityaSripal · 2020-05-08T22:17:38Z

Description

closes #5842, closes #5082 and closes #6091

auto-comment · 2020-05-08T22:17:43Z

AdityaSripal · 2020-05-08T22:18:47Z

x/ibc/23-commitment/types/merkle.go

 	}

-	runtime := rootmulti.DefaultProofRuntime()
-	return runtime.VerifyValue(proof.Proof, root.GetHash(), path.String(), value)
+	if !ics23.VerifyMembership(proof.Spec, root.GetHash(), proof.Proof, []byte(path.String()), value) {


Is the key in the confio VerifyMembership function the path? Or is it the last element of the path?

I assume it's the full path

x/ibc/23-commitment/types/merkle.go

fedekunze

can you pull master and update the proto files?

cwgoes · 2020-05-11T11:54:51Z

The ProofSpec should be fixed when a client (e.g. for a Tendermint chain) is created, then used for all future proof verifications performed with that client.

AdityaSripal · 2020-05-15T18:22:50Z

The ProofSpec should be fixed when a client (e.g. for a Tendermint chain) is created, then used for all future proof verifications performed with that client.

I imagine we cannot assume that all Tendermint clients are SDK chains. Thus we cannot make the IAVLProofSpec a standard for tendermint clients. Will the proof spec have to be negotiated on the connection handshake?

cwgoes · 2020-05-17T10:42:02Z

The ProofSpec should be fixed when a client (e.g. for a Tendermint chain) is created, then used for all future proof verifications performed with that client.

I imagine we cannot assume that all Tendermint clients are SDK chains. Thus we cannot make the IAVLProofSpec a standard for tendermint clients. Will the proof spec have to be negotiated on the connection handshake?

No, I mean that it is a parameter when a client is created, like the trusting period.

I suppose it could also be per-connection, but it will make the API a bit more complicated - is there likely to be a case where a single Tendermint consensus instance wants to use multiple proof formats for different connections?

x/ibc/07-tendermint/types/client_state.go

x/ibc/07-tendermint/types/msgs.go

Co-authored-by: Federico Kunze <[email protected]>

…to aditya/ics23-proofs

AdityaSripal · 2020-05-19T01:36:59Z

x/ibc/23-commitment/types/merkle.go

 // CONTRACT: provided path string MUST be a well formated path. See ICS24 for
 // reference.
-func ApplyPrefix(prefix exported.Prefix, path string) (MerklePath, error) {
-	err := host.DefaultPathValidator(path)
+func ApplyPrefix(prefix exported.Prefix, path exported.Path) (exported.Path, error) {


ApplyPrefix has changed to use the prefix to prepend a merkle path for the next tree in a chained proof. This corresponds with the current usage. Since the prefix ibc would be the path for the next tree in the chained multistore proof proving that the iavl roothash for ibc substore is included in multistore hash

AdityaSripal · 2020-05-19T01:39:06Z

x/ibc/07-tendermint/types/client_state.go

@@ -158,6 +177,8 @@ func (cs ClientState) VerifyClientConsensusState(
 		return err
 	}

+	// TODO: Inject proof specs of clientstate into proof or modify verify membership function to take proof specs


As @ethanfrey mentioned, the proof spec needs to be fixed by the client not passed in by the user. I can either inject the clientstate proofspecs into the MerkleProof struct (replacing whatever user passed in). Or i can remove specs from MerkleProof and pass it into the VerifyMembership function.

The latter requires a change in ICS spec

We should change ICS 23 to not put ProofSpec in the proofs

If this requires a spec change as well, we can do that

AdityaSripal · 2020-05-19T01:40:20Z

x/ibc/23-commitment/types/merkle.go

 }

 // VerifyNonMembership verifies the absence of a merkle proof against the given root and path.
+// 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(root exported.Root, path exported.Path) error {


Currently VerifyNonMembership will verify absence at the smallest subtree, and then prove inclusion of all subtree upto the root.

Thus in the multistore example, we can prove absence of a value in the ibc store, but we can't use this to prove that storeABC does not exist in multistore

That seems sufficient for all ibc usecases.

That's fine, let's just make sure we panic if we ever try to check a proof of absence of storeABC

AdityaSripal · 2020-05-19T01:43:29Z

x/ibc/23-commitment/types/merkle.go

 // It demonstrates membership or non-membership for an element or set of elements,
 // verifiable in conjunction with a known commitment root. Proofs should be
 // succinct.
 type MerkleProof struct {
-	Proof *merkle.Proof `json:"proof" yaml:"proof"`
+	Proofs []*ics23.CommitmentProof `json:"proofs" yaml:"proofs"`


Updated the SDK's MerkleProof to be a list of CommitmentProof and specs that are chained from smallest subtree to the top tree, each subroot serving as the value for the next proof.

This allows clients to define arbitrary chaining of proofs with different proofspecs in their state whether they are TM/SDK chains or not. Also doesn't require putting this chaining logic in confio package

I like this approach.

AdityaSripal · 2020-05-19T01:44:26Z

x/ibc/23-commitment/types/merkle.go

+		existProof, ok := p.Proof.(*ics23.CommitmentProof_Exist)
+		if !ok {
+			return sdkerrors.Wrap(ErrInvalidProof, "proof is not an existence proof")
+		}
+		// For subtree verification, we simply calculate the root from the proof and use it to prove
+		// against the value
+		subroot, err := existProof.Exist.Calculate()


Would be nice to make Calculate a method on CommitmentProof so I don't have to do this casting when I need to chain the proofs. cc: @ethanfrey

Ah, if it is available on all variants of the CommitmentProof, that we then expose it on the larger container and dispatch it to the subtypes as needed? I will check if they all have the same method signature (eg. with Batch proofs), but if so, then I can add.

Okay, checking the repo now. I find these examples of calculate:

ExistenceProof: https://github.com/confio/ics23/blob/master/go/proof.go#L72
Non-Existence Proof has none, but I could add one (just checking either of the included proofs)
Batch Proof has none either, and I guess we would have to check all included proofs and then ensure they are the same?
Compressed Batch Proof also has none, and this is even more expensive to implement - first decompression, then the batch proof algorithm.

What would make more sense is just a helper function, as you require existence. (Or do you want to extend this to non-existence also). Something like:

func CalculateExistProofRoot(p *ics23.CommitmentProof) ([]byte, error) { existProof, ok := p.Proof.(*ics23.CommitmentProof_Exist) if !ok { return nil, sdkerrors.Wrap(ErrInvalidProof, "proof is not an existence proof") } // For subtree verification, we simply calculate the root from the proof and use it to prove // against the value subroot, err := existProof.Exist.Calculate() // anything else to do here? return subroot, err }

This could fit just as well in this module as in ics23

Hmm, I'll have to get the root for all the proof types because the different proof types show up at the lowest subtree (above that it's all just existence proofs). For example, the VerifyNonMembership proves nonexistence at the lowest subtree, and then proves existence of each subroot up to the final root. Similarly for batch proofs.

I don't need the batch proofs to check that all included proofs commit to the same root. Just return the root of the first proof. I'll then just use the batch-verify functions you have and pass in that first root. This will verify that every proof in my batch proof commits to the same root.

Currently writing up the batchverify functions in this module, and i think it'll be clear why it's better to have Calculate method in ics23 even if it involves an annoying amount of calls to get to the first embedded ExistenceProof in all of these proof types and calculate root on that

Ok, happy for a pr on ics23 that adds that, or I can take a look next week myself

AdityaSripal

If a lot of this code can get pushed into a confio Calculate method on CommitmentProof, then the code here becomes much more readable

Could do the type switch and calculate for each proof type in here with a helper function as well, but seems cleaner to have it in confio/ics23

cc: @ethanfrey

AdityaSripal · 2020-05-20T22:36:58Z

x/ibc/23-commitment/types/merkle.go

+			bproof, ok := p.Proof.(*ics23.CommitmentProof_Batch)
+			if ok {
+				if len(bproof.Batch.GetEntries()) == 0 || bproof.Batch.GetEntries()[0] == nil {
+					return sdkerrors.Wrap(ErrInvalidProof, "batch proof has empty entry")
+				}
+				bexist := bproof.Batch.GetEntries()[0].GetExist()
+				if bexist == nil {
+					return sdkerrors.Wrap(ErrInvalidProof, "batch proof is not an existence proof")
+				}
+				subroot, err = bexist.Calculate()


BatchProof root calculation

AdityaSripal · 2020-05-20T22:37:27Z

x/ibc/23-commitment/types/merkle.go

+				}
+				subroot, err = bexist.Calculate()
+			} else {
+				return sdkerrors.Wrap(ErrInvalidProof, "not a batch proof, compressed batch proof currently unimplemented")


Not doing compressed batch proof calculation manually here but similar principle

AdityaSripal · 2020-05-20T22:37:44Z

x/ibc/23-commitment/types/merkle.go

+			bproof, ok := p.Proof.(*ics23.CommitmentProof_Batch)
+			if ok {
+				if len(bproof.Batch.GetEntries()) == 0 || bproof.Batch.GetEntries()[0] == nil {
+					return sdkerrors.Wrap(ErrInvalidProof, "batch proof has empty entry")
+				}
+				bnonexist := bproof.Batch.GetEntries()[0].GetNonexist()
+				if bnonexist == nil {
+					return sdkerrors.Wrap(ErrInvalidProof, "batch proof is not a nonexistence proof")
+				}
+				subroot, err = bnonexist.Left.Calculate()
+			} else {
+				return sdkerrors.Wrap(ErrInvalidProof, "not a batch proof, compressed batch proof currently unimplemented")


ditto on nonexist batch proof

ethanfrey

I think there is a lot going on here, which could be done much better in a series of much smaller PRs.

Replacing the CommitInfo proof step from the whole (amino encoded?) struct to merkle.SimpleProof. That code looks pretty good and could be pulled out in a small PR and merged already.
Changing the hashing algorithm for merkleMap and merkle.SimpleMap. This seems to only be desired for some optimizations and should go through an ADR independent of the rest of this PR.
Adding a new op type and runtime decoder for ics23.CommitmentProof (or multiple ops for different subtypes?). Most of the verify logic you write in ics-23 can go in there, completely not breaking. This also allows all the ics code that refers to merkle.Proof to keep doing so.
Update the general query proof to return two ics23.ExistenceProof (wrapped in merkle.ProofOp) rather than ProofOpMultiStore and iavl.ProofOpIAVLValue. Pass those anywhere ics code is expecting a proof.
Create func ICSProofRuntime() (prt *merkle.ProofRuntime) that only has support for ics23 types and see if this works. If not, hunt down where the other types are leaking through.
Ensure all ibc code (including relayer on a branch with this code) is working with this - ideally before merging each PR to help Jack keep his sanity.

This should involve next to no changes in the ibc/02, 03, 07 or 23. We have a nice abstraction there already, eg: return runtime.VerifyValue(proof.Proof, root.GetHash(), path.String(), value). Let's use it and just fit ics23 under the entire merkle.ProofOp machinery. I believe this was the approach discussed. Live side by side, don't make them exclusive.

ethanfrey · 2020-06-01T19:09:42Z

go.mod

@@ -37,4 +37,6 @@ require (

 replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.2-alpha.regen.1

+replace github.com/tendermint/tendermint => github.com/AdityaSripal/tendermint v1.2.3


I just looked at this. Essentially changing the hashing algorithm for SimpleMerkleProof (which breaks any external tooling that uses it). Why was that done? And is there any goal to move that into master??

AdityaSripal/tendermint@b8be4a0

I would remove this replace. Step by step. No need to require merging a breaking change into tendermint core to get this working. If you think it is a nice optimiziation, it may be, and can be another PR process there, but it feels odd to pull it into this PR.

This isn't an optimization. The way simple proofs were being verified didn't line up with the way the stores were getting hashed. There was an extra rounds of hashing happening for leaf values in proof verification that wasn't happening with the simple hash. Thus the proof did not commit to the multistore hash, when just using SimpleHash and SimpleProof without any changes.

After tracking the issue down, I found that removing these extra layers of hashing made the proof verification line up with the multistore commit. Haven't found alternative solutions that also work, so I left this for now and moved on with rest of the code. Will look into this further and try to understand issue fully, because I too do not want to change tendermint if I don't have to.

If it's possible to have an SDK only solution, I will use it. If not, this may be necessary. I haven't figured out if it is necessary yet.

Are we still hashing somewhere? Maybe this was accidental, or maybe there was a reason for it - can we ask the Tendermint team (are they using this code anywhere)?

If they're not using this code anywhere, maybe we should move it out of Tendermint altogether.

ethanfrey · 2020-06-01T19:11:45Z

store/rootmulti/merkle_map.go

@@ -30,13 +29,9 @@ func newMerkleMap() *merkleMap {
 func (sm *merkleMap) set(key string, value []byte) {
 	sm.sorted = false

-	// The value is hashed, so you can check for equality with a cached value (say)


Okay, this is the same change as in tendermint. Again, why?

(Also this breaks ics23-tendermint, which needs a PR to update the format and regenerate the proofs)

https://github.com/confio/ics23-tendermint/pulls if you really want to make this.

But there should be no real reason to mix it in this PR.

Why do we have this code in two places in the first place?

ethanfrey · 2020-06-01T19:13:53Z

store/rootmulti/proof.go

 // XXX: This should be managed by the rootMultiStore which may want to register
 // more proof ops?
 func DefaultProofRuntime() (prt *merkle.ProofRuntime) {
 	prt = merkle.NewProofRuntime()
 	prt.RegisterOpDecoder(merkle.ProofOpSimpleValue, merkle.SimpleValueOpDecoder)
 	prt.RegisterOpDecoder(iavl.ProofOpIAVLValue, iavl.ValueOpDecoder)
 	prt.RegisterOpDecoder(iavl.ProofOpIAVLAbsence, iavl.AbsenceOpDecoder)
-	prt.RegisterOpDecoder(ProofOpMultiStore, MultiStoreProofOpDecoder)


If you can drop this one (:+1:) you an also add another proof op that returns ics23 format, no?

ethanfrey · 2020-06-01T19:17:36Z

store/rootmulti/store.go

@@ -714,3 +727,13 @@ func SimpleHashFromMap(m map[string][]byte) []byte {

 	return mm.hash()
 }
+
+func SimpleProofOpsFromMap(m map[string][]byte) map[string]merkle.SimpleValueOp {


No need to convert all the merkle.SimpleProofs into merkle.SimpleValueOp when we just use one of them.

Unless you actually use the multi-return value elsewhere (where this code would make sense), why not just:

// in real code handle the missing case better func SimpleProofOpFromMap(m map[string][]byte, key string) merkle.SimpleValueOp { _, proofs, _ := merkle.SimpleProofsFromMap(m) return merkle.NewSimpleValueOp([]byte(key), proofs[key]) }

ethanfrey · 2020-06-01T19:18:57Z

store/rootmulti/store.go

@@ -609,7 +621,8 @@ func (si storeInfo) Hash() []byte {
 		panic(err)
 	}

-	return hasher.Sum(nil)
+	hash := hasher.Sum(nil)


Why this change?

Missed reverting this change during debugging

ethanfrey · 2020-06-01T19:23:52Z

x/ibc/23-commitment/types/merkle.go

 	if err != nil {
 		return MerklePath{}, err
 	}

 	if prefix == nil || prefix.IsEmpty() {
 		return MerklePath{}, errors.New("prefix can't be empty")
 	}
-	return NewMerklePath([]string{string(prefix.Bytes()), path}), nil
+	mpath, ok := path.(MerklePath)


If you require the path to be a MerklePath here (runtime) but allow any exported.Path interface at compile time (function argument) you open up to annoying runtime failures. Why not just require the concrete type. Especially the return type is often much better as a struct (general Golang rule - accept interfaces, return structs).

I would just export MerklePath.

ethanfrey · 2020-06-01T19:29:58Z

x/ibc/23-commitment/types/merkle.go

+		if err != nil {
+			return sdkerrors.Wrap(ErrInvalidProof, err.Error())
+		}
+		if i != len(proof.Proofs)-1 {


Looking at this code, I think it would make more sense going from top-to-bottom, so we don't have to pre-calculate the expected root each time.

Or is the embedded key-value not passed along with the proof.

I mean, I first check there is a valid proof from header.AppHash to ("ibc", <sub-hash>).
If true, then I check if there is a valid proof from <sub-hash> to (<packet-key>, <packet-value>).

I assume that <sub-hash>, <packet-key> and <packet-value> are sent along with the message. If not, then my inversion doesn't work. Can you point me to all info we have in the calling function (or is this interface fixed). This function does take root hash, all keys and packet-value, so I do see why you have to go reverse... just seems a bit odd

subhash would not be sent with the message

ethanfrey · 2020-06-01T19:30:42Z

x/ibc/23-commitment/types/merkle.go

 	}

-	runtime := rootmulti.DefaultProofRuntime()
-	return runtime.VerifyAbsence(proof.Proof, root.GetHash(), path.String())
+	mpath, ok := path.(MerklePath)


Again you only invite user error when you accept an interface and immediately error if the user didn't pass in one implementation.

AdityaSripal · 2020-06-01T21:13:20Z

Working on making store.Query use ics23 proofs in a separate branch

cwgoes · 2020-06-02T14:34:07Z

I think there is a lot going on here, which could be done much better in a series of much smaller PRs.

I do think this would be easier to review as a set of smaller PRs, do you think that's feasible @AdityaSripal? It would also simplify the current merge conflicts.

cwgoes

Might be worth running through the proof-chaining logic in a synchronous discussion.

AdityaSripal · 2020-06-15T18:45:56Z

Superseded by #6323 #6324 #6374 #6390

integrate @ethanfrey work on confio/ics23 into 23-commitment

73462b2

AdityaSripal requested review from aaronc, alessio, alexanderbez, clevinson, fedekunze, hschoenburg, jgimeno and nylira as code owners May 8, 2020 22:17

AdityaSripal requested a review from cwgoes May 8, 2020 22:17

AdityaSripal added WIP labels May 8, 2020

AdityaSripal commented May 8, 2020

View reviewed changes

fedekunze reviewed May 8, 2020

View reviewed changes

x/ibc/23-commitment/types/merkle.go Outdated Show resolved Hide resolved

fedekunze reviewed May 8, 2020

View reviewed changes

fedekunze mentioned this pull request May 13, 2020

Potential attack vector with Merkle Commitments in ICS-23 #5440

Closed

start potential chain-proof implementation

59d5fd1

fedekunze reviewed May 19, 2020

View reviewed changes

x/ibc/07-tendermint/types/client_state.go Outdated Show resolved Hide resolved

x/ibc/07-tendermint/types/msgs.go Outdated Show resolved Hide resolved

AdityaSripal and others added 4 commits May 19, 2020 07:01

Update x/ibc/07-tendermint/types/client_state.go

567fc51

Co-authored-by: Federico Kunze <[email protected]>

Update x/ibc/07-tendermint/types/msgs.go

2da7b0d

Co-authored-by: Federico Kunze <[email protected]>

add GetProofSpecs method to localhost clientstate

4c38f3e

Merge branch 'aditya/ics23-proofs' of github.com:cosmos/cosmos-sdk in…

dba44d8

…to aditya/ics23-proofs

AdityaSripal commented May 19, 2020

View reviewed changes

AdityaSripal added 2 commits May 20, 2020 22:54

move specs out of proofs

d26a2d8

implement batch proofs

f605880

AdityaSripal commented May 20, 2020

View reviewed changes

replace MultiStoreProofOp with SimpleValueOp

6c66834

ethanfrey reviewed Jun 1, 2020

View reviewed changes

cwgoes reviewed Jun 2, 2020

View reviewed changes

AdityaSripal mentioned this pull request Jun 2, 2020

Switch 23-commitment to use ics23-proofs #6321

Closed

ethanfrey mentioned this pull request Jun 2, 2020

Use simple merkle proof for commit info #6323

Merged

8 tasks

AdityaSripal mentioned this pull request Jun 4, 2020

Make 23-Commitment use ICS23 proofs #6344

Closed

8 tasks

AdityaSripal closed this Jun 15, 2020

AdityaSripal deleted the aditya/ics23-proofs branch June 15, 2020 18:46

		@@ -37,4 +37,6 @@ require (

		replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.2-alpha.regen.1

		replace github.com/tendermint/tendermint => github.com/AdityaSripal/tendermint v1.2.3

WIP: Fix ICS23-Proofs #6178

WIP: Fix ICS23-Proofs #6178

Conversation

AdityaSripal commented May 8, 2020 • edited by fedekunze Loading

Description

auto-comment bot commented May 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze left a comment • edited Loading

Choose a reason for hiding this comment

cwgoes commented May 11, 2020 • edited Loading

AdityaSripal commented May 15, 2020

cwgoes commented May 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Jun 1, 2020

cwgoes commented Jun 2, 2020 • edited Loading

cwgoes left a comment

Choose a reason for hiding this comment

AdityaSripal commented Jun 15, 2020

AdityaSripal commented May 8, 2020 •

edited by fedekunze

Loading

fedekunze left a comment •

edited

Loading

cwgoes commented May 11, 2020 •

edited

Loading

cwgoes commented May 17, 2020 •

edited

Loading

AdityaSripal Jun 1, 2020 •

edited

Loading

cwgoes commented Jun 2, 2020 •

edited

Loading