[IBC] Implement ICS-23 CommitmentProof verification for the SMT by h5law · Pull Request #845 · pokt-network/pocket

h5law · 2023-06-19T15:29:58Z

Description

Summary generated by Reviewpad on 29 Jun 23 20:46 UTC

This pull request includes various changes across multiple files.

In the README.md file, a new section titled "ICS-23 Vector Commitments" was added under the "Components" section, along with a link to the ICS-23 specification. Additionally, the link to the ICS-24 specification was updated to point to the ics24.md file.

In the go.sum file, new dependencies were added for the libraries github.com/cosmos/gogoproto and github.com/h5law/ics23/go, with specific version tags. The existing dependency on github.com/pokt-network/smt was also updated to a newer version.

The ics23.md file provides implementation details of ICS-23 Vector Commitments, including the benefits of using the cosmos/ics23 library and the changes made for Pocket's implementation. It also explains the proof verification process for membership and non-membership proofs, accompanied by flowchart diagrams. The implementation logic can be found in the file proofs_ics23.go.

A new file proofs_ics23_test.go was added to the ibc/store package, containing test functions for generating and verifying commitment proofs using the ICS23 library. The tests cover membership and non-membership proofs for a sparse Merkle tree.

The go.mod file shows various changes, including the addition of replace directives for modules github.com/cosmos/ics23/go and github.com/regen-network/gocuke, updates to module versions, and the addition of a new module github.com/cosmos/gogoproto.

The file proofs_ics23.go is a new addition to the ibc/store package, containing functions and constants for verifying membership and non-membership in a Sparse Merkle Tree.

Lastly, in the file where the NextCode constant is defined, it was updated to a new value and a new constant and error function were added.

Please review these changes in the pull request.

Issue

Fixes #841

Type of change

Please mark the relevant option(s):

List of changes

Add proof generation and verification in accordance with cosmos/ics23
Add SMT proof -> ics23 proof conversion

Testing

make develop_test; if any code changes were made
make test_e2e on k8s LocalNet; if any code changes were made
e2e-devnet-test passes tests on DevNet; if any code was changed
Docker Compose LocalNet; if any major functionality was changed or introduced
k8s LocalNet; if any infrastructure or configuration changes were made

Required Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
I have tested my changes using the available tooling
I have updated the corresponding CHANGELOG

If Applicable Checklist

I have updated the corresponding README(s); local and/or global
I have added tests that prove my fix is effective or that my feature works
I have added, or updated, mermaid.js diagrams in the corresponding README(s)
I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

ibc/docs/README.md

shared/core/types/error.go

ibc/docs/ics23.md

ibc/store/proofs_ics23.go

ibc/store/proofs_ics23_test.go

h5law · 2023-06-27T09:25:47Z

🚨 I HAVE REWRITTEN HISTORY 🚨

@Olshansk I went through all the comments, and rebased off of main. The PR is now much smaller without all the unecissary files from other PRs.

ibc/docs/README.md

ibc/docs/ics23.md

Olshansk · 2023-06-28T22:20:14Z

ibc/store/proofs_ics23.go

+)
+
+var (
+	// Custom SMT spec as the store does not hash values


The hashing and nonhashing makes sense to me in the context of IBC and SMT in independence.

It's not a blocker for this PR, but my question/concern is moreso for the larger context of the blockchain to make sure that I'm personally not missing/misunderstanding something. For the rest of the persistence module, question is: Should we hash the values for the other trees or not?

@dylanlott do you have thoughts/opinion here?

ibc/store/proofs_ics23_test.go

Olshansk · 2023-06-28T22:26:55Z

ibc/store/proofs_ics23.go

+	return steps
+}
+
+// getPathBit takes the hash of a key (the path) and a position (depth) and returns whether at


See the ref - Ref: https://github.com/pokt-network/smt/blob/main/utils.go

Yea, this is definitely non-trivial to implement...

This business logic is VERY closely coupled with our specific SMT implementation. I'd suggest the following:

exposing GetPathBit in the smt and reusing it here

Move the comments you added to the smt

Add an isLeftChild helper and use it here

I have added a techdebt comment here as I have opened smt#14 which will cover this. As I have just release v0.6.0 for the SMT i think that its best to bundle these small changes into releases to save on constantly updating the go.mod. As the functionality will not change until this issue in in progress I think its alright to leave it until then.

LMK wdyt

Going to push back on this.

I think pokt-network/smt#14 is a 1+ month research and development project, whereas exposing the GetPathBit function is something we can do before EOD.

@Olshansk pokt-network/smt#15

… verification

Olshansk · 2023-06-29T19:04:23Z

ibc/store/proofs_ics23.go

+	return steps
+}
+
+// getPathBit takes the hash of a key (the path) and a position (depth) and returns whether at


Going to push back on this.

I think pokt-network/smt#14 is a 1+ month research and development project, whereas exposing the GetPathBit function is something we can do before EOD.

Olshansk · 2023-06-29T19:12:45Z

ibc/docs/ics23.md

+     - `key` is in tree -> `Proof` is invalid -> exclusion QED
+
+```mermaid
+flowchart TD


Appreciate the new diagram!

Olshansk

ICS23!!!!!

* pokt/main: Update E2E_FEATURE_LIST.md [IBC] Implement ICS-23 CommitmentProof verification for the SMT (#845) [Utility] trustless relays servicer token validation (#803)

* feat/integrate-bg-router: Update E2E_FEATURE_LIST.md [IBC] Implement ICS-23 CommitmentProof verification for the SMT (#845) [Utility] trustless relays servicer token validation (#803)

h5law added e2e-devnet-test Runs E2E tests on devnet ibc IBC specific changes labels Jun 19, 2023

h5law added this to the M7: Pocket NoS (North Star) milestone Jun 19, 2023

h5law requested review from Olshansk, bryanchriswhite and dylanlott June 19, 2023 15:29

h5law self-assigned this Jun 19, 2023

reviewpad bot added large Pull request is large waiting-for-review labels Jun 19, 2023

h5law mentioned this pull request Jun 21, 2023

[IBC] Implement ICS-24 - Tracking IBC store transitions in the network state #847

Merged

20 tasks

h5law force-pushed the ibc/initial_module branch from 22b48dd to d1a4667 Compare June 23, 2023 10:36

h5law force-pushed the ibc/ics23_integration branch 2 times, most recently from 36a8056 to 8cd1fab Compare June 26, 2023 09:04

Olshansk requested changes Jun 27, 2023

View reviewed changes

Base automatically changed from ibc/initial_module to main June 27, 2023 08:11

h5law force-pushed the ibc/ics23_integration branch from 8cd1fab to f482e14 Compare June 27, 2023 09:24

h5law requested a review from Olshansk June 27, 2023 09:24

Olshansk requested changes Jun 28, 2023

View reviewed changes

Olshansk mentioned this pull request Jun 29, 2023

feat: Add support for SMT ExclusionProofs cosmos/ics23#153

Closed

h5law requested a review from Olshansk June 29, 2023 10:12

h5law added 9 commits June 29, 2023 13:00

Add ics23 integration

ab9ff72

Add SMT proof conversion to ics23 Existence and Exclusion proofs with…

b47402d

… verification

Add ICS23 docs

20fa2fc

Fix errors

52e62e4

Update next error comment

0656658

Address comments in docs

d631bb3

Add names back to tests

1beb4bd

Address comments

9a7a578

Address comments

327ed96

h5law force-pushed the ibc/ics23_integration branch from fdfd4a5 to 327ed96 Compare June 29, 2023 12:04

Olshansk requested changes Jun 29, 2023

View reviewed changes

Olshansk reviewed Jun 29, 2023

View reviewed changes

h5law added 3 commits June 29, 2023 20:31

add isLeft helper and use smt.GetPathBit()

bc5a878

go.mod

2f67dcd

Fix SMT repo

17e2419

Olshansk approved these changes Jun 29, 2023

View reviewed changes

h5law merged commit 7031c93 into main Jun 29, 2023

h5law deleted the ibc/ics23_integration branch June 29, 2023 21:22

h5law mentioned this pull request Jun 30, 2023

[Persistence] Update pocket/smt to have support for a Merkle Sum Trie #776

Closed

4 tasks

Conversation

h5law commented Jun 19, 2023 • edited by reviewpad bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Summary generated by Reviewpad on 29 Jun 23 20:46 UTC

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

h5law commented Jun 27, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Olshansk Jun 28, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Olshansk Jun 28, 2023

Choose a reason for hiding this comment

Uh oh!

h5law Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

Olshansk Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

h5law Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

Olshansk Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

Olshansk Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

Olshansk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

h5law commented Jun 19, 2023 •

edited by reviewpad bot

Loading