feat: add rpc `VerifyMembershipProof` - querier approach for conditional clients #5821

damiannolan · 2024-02-07T14:40:44Z

Description

Adds the new Query service rpc VerifyMembershipProof outlined in #5310. Big thanks to @srdtrk for the initial deep dive on this issue.

closes: #5310

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against the correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

damiannolan · 2024-02-07T14:42:50Z

Would like to add a couple of more tests:

08-wasm stargate querier
Gas is correctly consumed from context

Please raise any more in the comments if applicable!

proto/ibc/core/client/v1/query.proto

modules/core/02-client/keeper/grpc_query.go

proto/ibc/core/client/v1/query.proto

…ries

modules/light-clients/08-wasm/types/querier_test.go

crodriguezvega

Nice and clean. Thank you for the PR, @damiannolan; and thank you @srdtrk for the ground work.

proto/ibc/core/client/v1/query.proto

modules/light-clients/08-wasm/types/querier_test.go

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

DimitrisJim

lgtm, left one minor nit for your viewing pleasure

modules/light-clients/08-wasm/types/querier.go

modules/core/02-client/keeper/grpc_query.go

…ests for discarded state checks

…ceptList

crodriguezvega · 2024-02-13T22:31:11Z

modules/core/02-client/keeper/grpc_query.go

@@ -328,3 +328,55 @@ func (k Keeper) UpgradedConsensusState(c context.Context, req *types.QueryUpgrad
 		UpgradedConsensusState: protoAny,
 	}, nil
 }
+
+// VerifyMembership implements the Query/VerifyMembership gRPC method
+func (k Keeper) VerifyMembership(c context.Context, req *types.QueryVerifyMembershipRequest) (*types.QueryVerifyMembershipResponse, error) {


Should we document this addition and the fact that there is going to be a set of default allowed queries made available to the querier? Maybe in the 08-wasm docs?

crodriguezvega · 2024-02-13T22:52:20Z

I added the backport-to-v8.2.x label to the PR, but I guess we also need to create a new release branch for 08-wasm that will depend on ibc-go v8.2 and backport to that branch the 08-wasm changes of the PR? Like release/v0.2.x+ibc-go-v8.2.x-wasmvm-v2.0.x (and then we can also upgrade to the latest wasmvm version)?

colin-axner · 2024-02-14T10:32:03Z

modules/core/02-client/keeper/grpc_query.go

+	if err := host.ClientIdentifierValidator(req.ClientId); err != nil {
+		return nil, status.Error(codes.InvalidArgument, err.Error())
+	}


I think we should error if the client type is solo machine?

localhost as well?

do we also want to error if the client is not active, i.e.

k.GetClientStatus(cachedCtx, clientState, req.ClientId) != exported.Active

I think we should error if the client type is solo machine?

This would involve adding solomachine types as a concrete dependency in 02-client which I think is currently not the case, (it is already with localhost though).

I think we should error if the client type is solo machine?

Why would we error if it is solomachine client type? State changes are discarded so no sequence incrementing will happen. Shouldn't it be fine to let it go through, just sig verify on whatever bytes? Is it that solomachine is only signing one particular set of bytes for a one sequence? i.e. you will be able to verify for example, a channel end signed at sequence x? so in theory the VerifyMembership query is only available for proof one value at a particular point in time. Does that make sense?

localhost as well?

Yeah, if you query a chain for a proof and have access to an rpc for a header to get the app hash then you should be able to verify off chain and it would be spammy(?) I guess to allow a query to go through like that. Fee like there's no reason to allow it.

This would involve adding solomachine types as a concrete dependency in 02-client which I think is currently not the case, (it is already with localhost though)

Solomachine client type is in exported afaik.

Created this issue: #5848

solomachine client type is a string, it can be duplicated

Is it that solomachine is only signing one particular set of bytes for a one sequence?

yes it is.

colin-axner

Excellent work!!! Thanks for all the followup changes. My only requested addition is to disable light clients on the verify membership query which we don't want to support initially to limit any potential side effects we aren't trying to account for. I'm okay doing this in a followup though since the diffs should be quite minimal and mainly an addition

modules/core/02-client/keeper/grpc_query.go

colin-axner · 2024-02-14T10:34:30Z

modules/core/02-client/keeper/grpc_query.go

+	if req.ProofHeight.IsZero() {
+		return nil, status.Error(codes.InvalidArgument, "proof height must be non-zero")
+	}
+


I think proof height is allowed to be empty? (for localhost?)

if we disable localhost then we should be sweet? :)

colin-axner · 2024-02-14T10:34:36Z

modules/core/02-client/keeper/grpc_query.go

+	if err := host.ClientIdentifierValidator(req.ClientId); err != nil {
+		return nil, status.Error(codes.InvalidArgument, err.Error())
+	}


localhost as well?

modules/core/02-client/keeper/grpc_query_test.go

colin-axner · 2024-02-14T10:36:07Z

modules/core/02-client/keeper/grpc_query.go

+		return nil, status.Error(codes.NotFound, errorsmod.Wrap(types.ErrClientNotFound, req.ClientId).Error())
+	}
+
+	if err := clientState.VerifyMembership(cachedCtx, k.ClientStore(cachedCtx, req.ClientId), k.cdc, req.ProofHeight, req.TimeDelay, req.BlockDelay, req.Proof, req.MerklePath, req.Value); err != nil {


if we ever add a mock client, I think this would be a good test case we could use it on. I think it is nice to be testing the state change in 02-client rather than 08-wasm, but it is fine for now

chatton

Looks good! Left some small suggestions but nothing major 👍

chatton · 2024-02-14T11:17:54Z

modules/core/02-client/keeper/grpc_query.go

+	if err := host.ClientIdentifierValidator(req.ClientId); err != nil {
+		return nil, status.Error(codes.InvalidArgument, err.Error())
+	}


do we also want to error if the client is not active, i.e.

k.GetClientStatus(cachedCtx, clientState, req.ClientId) != exported.Active

chatton · 2024-02-14T11:20:50Z

modules/core/02-client/keeper/grpc_query.go

+	if err := host.ClientIdentifierValidator(req.ClientId); err != nil {
+		return nil, status.Error(codes.InvalidArgument, err.Error())
+	}


I think we should error if the client type is solo machine?

This would involve adding solomachine types as a concrete dependency in 02-client which I think is currently not the case, (it is already with localhost though).

chatton · 2024-02-14T11:23:20Z

modules/light-clients/08-wasm/types/querier.go

@@ -130,7 +135,7 @@ func NewDefaultQueryPlugins() *QueryPlugins {
 func AcceptListStargateQuerier(acceptedQueries []string) func(sdk.Context, *wasmvmtypes.StargateQuery) ([]byte, error) {
 	return func(ctx sdk.Context, request *wasmvmtypes.StargateQuery) ([]byte, error) {
 		// A default list of accepted queries can be added here.


think this comment can be adjusted since we are adding this default list now

…nal clients (#5821) * feat: adding protobuf msgs and rpc for VerifyMembershipProof * feat: adding VerifyMembershipProof query implementation and wiring * chore(08-wasm): add VerifyMembershipProof to stargate query acceptlist * test: adding failure case unit tests for VerifyMembershipProof query * fix: correct protodoc * chore: proto-swagger-gen * chore: protodocs * test: adding additional test cases * test: assert gas consumed in tests * chore: rename rpc to VerifyMembership and update tests * chore: update service definition URL in 08-wasm stargate accepted queries * test: adding verify membership test to 08-wasm querier * Update proto/ibc/core/client/v1/query.proto Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * chore: review items - log error at debug, pass cachedCtx and adjust tests for discarded state checks * chore: add doc comment to querier test, address nit to move defaultAcceptList * chore: regen protos and swagger doc * nit: update comment in querier * imp: add more info to godoc for VerifyMembership rpc --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Cian Hatton <cian@interchain.io> (cherry picked from commit ed9bf74) # Conflicts: # docs/client/swagger-ui/swagger.yaml # modules/light-clients/08-wasm/types/querier.go # modules/light-clients/08-wasm/types/querier_test.go

…nal clients (backport #5821) (#5850) * feat: add rpc `VerifyMembershipProof` - querier approach for conditional clients (#5821) * feat: adding protobuf msgs and rpc for VerifyMembershipProof * feat: adding VerifyMembershipProof query implementation and wiring * chore(08-wasm): add VerifyMembershipProof to stargate query acceptlist * test: adding failure case unit tests for VerifyMembershipProof query * fix: correct protodoc * chore: proto-swagger-gen * chore: protodocs * test: adding additional test cases * test: assert gas consumed in tests * chore: rename rpc to VerifyMembership and update tests * chore: update service definition URL in 08-wasm stargate accepted queries * test: adding verify membership test to 08-wasm querier * Update proto/ibc/core/client/v1/query.proto Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * chore: review items - log error at debug, pass cachedCtx and adjust tests for discarded state checks * chore: add doc comment to querier test, address nit to move defaultAcceptList * chore: regen protos and swagger doc * nit: update comment in querier * imp: add more info to godoc for VerifyMembership rpc --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Cian Hatton <cian@interchain.io> (cherry picked from commit ed9bf74) # Conflicts: # docs/client/swagger-ui/swagger.yaml # modules/light-clients/08-wasm/types/querier.go # modules/light-clients/08-wasm/types/querier_test.go * chore: rm 08-wasm module files * fix: setup path using coordinator in tests --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com>

…nal clients (#5821) * feat: adding protobuf msgs and rpc for VerifyMembershipProof * feat: adding VerifyMembershipProof query implementation and wiring * chore(08-wasm): add VerifyMembershipProof to stargate query acceptlist * test: adding failure case unit tests for VerifyMembershipProof query * fix: correct protodoc * chore: proto-swagger-gen * chore: protodocs * test: adding additional test cases * test: assert gas consumed in tests * chore: rename rpc to VerifyMembership and update tests * chore: update service definition URL in 08-wasm stargate accepted queries * test: adding verify membership test to 08-wasm querier * Update proto/ibc/core/client/v1/query.proto Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * chore: review items - log error at debug, pass cachedCtx and adjust tests for discarded state checks * chore: add doc comment to querier test, address nit to move defaultAcceptList * chore: regen protos and swagger doc * nit: update comment in querier * imp: add more info to godoc for VerifyMembership rpc --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Cian Hatton <cian@interchain.io> (cherry picked from commit ed9bf74) # Conflicts: # docs/client/swagger-ui/swagger.yaml # modules/light-clients/08-wasm/types/querier.go # modules/light-clients/08-wasm/types/querier_test.go

…nal clients (backport #5821) (#6105) * feat: add rpc `VerifyMembershipProof` - querier approach for conditional clients (#5821) * feat: adding protobuf msgs and rpc for VerifyMembershipProof * feat: adding VerifyMembershipProof query implementation and wiring * chore(08-wasm): add VerifyMembershipProof to stargate query acceptlist * test: adding failure case unit tests for VerifyMembershipProof query * fix: correct protodoc * chore: proto-swagger-gen * chore: protodocs * test: adding additional test cases * test: assert gas consumed in tests * chore: rename rpc to VerifyMembership and update tests * chore: update service definition URL in 08-wasm stargate accepted queries * test: adding verify membership test to 08-wasm querier * Update proto/ibc/core/client/v1/query.proto Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * chore: review items - log error at debug, pass cachedCtx and adjust tests for discarded state checks * chore: add doc comment to querier test, address nit to move defaultAcceptList * chore: regen protos and swagger doc * nit: update comment in querier * imp: add more info to godoc for VerifyMembership rpc --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Cian Hatton <cian@interchain.io> (cherry picked from commit ed9bf74) # Conflicts: # docs/client/swagger-ui/swagger.yaml # modules/light-clients/08-wasm/types/querier.go # modules/light-clients/08-wasm/types/querier_test.go * fix conflicts --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

damiannolan added 8 commits February 7, 2024 14:24

feat: adding protobuf msgs and rpc for VerifyMembershipProof

7388af9

feat: adding VerifyMembershipProof query implementation and wiring

18e5404

chore(08-wasm): add VerifyMembershipProof to stargate query acceptlist

1f720b6

test: adding failure case unit tests for VerifyMembershipProof query

37435a6

fix: correct protodoc

2569bd4

chore: proto-swagger-gen

06d2f59

chore: protodocs

a2fc9a7

test: adding additional test cases

a2f139d

damiannolan marked this pull request as ready for review February 7, 2024 14:41

damiannolan requested review from colin-axner, AdityaSripal, crodriguezvega, charleenfei, chatton, DimitrisJim and srdtrk as code owners February 7, 2024 14:41

damiannolan commented Feb 7, 2024

View reviewed changes

proto/ibc/core/client/v1/query.proto Outdated Show resolved Hide resolved

damiannolan commented Feb 7, 2024

View reviewed changes

modules/core/02-client/keeper/grpc_query.go Outdated Show resolved Hide resolved

Merge branch 'main' into damian/5310-conditional-clients-querier

9cc6cdc

DimitrisJim reviewed Feb 7, 2024

View reviewed changes

proto/ibc/core/client/v1/query.proto Outdated Show resolved Hide resolved

damiannolan added 4 commits February 8, 2024 12:04

test: assert gas consumed in tests

b72e541

chore: rename rpc to VerifyMembership and update tests

c19d6ed

chore: update service definition URL in 08-wasm stargate accepted que…

889be6e

…ries

test: adding verify membership test to 08-wasm querier

7ef470e

damiannolan commented Feb 9, 2024

View reviewed changes

modules/light-clients/08-wasm/types/querier_test.go Show resolved Hide resolved

crodriguezvega approved these changes Feb 10, 2024

View reviewed changes

proto/ibc/core/client/v1/query.proto Outdated Show resolved Hide resolved

modules/light-clients/08-wasm/types/querier_test.go Show resolved Hide resolved

Update proto/ibc/core/client/v1/query.proto

e64af3a

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

DimitrisJim approved these changes Feb 12, 2024

View reviewed changes

modules/light-clients/08-wasm/types/querier.go Outdated Show resolved Hide resolved

colin-axner reviewed Feb 12, 2024

View reviewed changes

modules/core/02-client/keeper/grpc_query.go Outdated Show resolved Hide resolved

damiannolan added 4 commits February 13, 2024 11:49

chore: review items - log error at debug, pass cachedCtx and adjust t…

da3b02e

…ests for discarded state checks

chore: add doc comment to querier test, address nit to move defaultAc…

143d7c9

…ceptList

chore: regen protos and swagger doc

1ecb660

Merge branch 'main' into damian/5310-conditional-clients-querier

83fbeec

crodriguezvega reviewed Feb 13, 2024

View reviewed changes

crodriguezvega added the backport-to-v8.2.x label Feb 13, 2024

colin-axner reviewed Feb 14, 2024

View reviewed changes

colin-axner approved these changes Feb 14, 2024

View reviewed changes

chatton approved these changes Feb 14, 2024

View reviewed changes

Merge branch 'main' into damian/5310-conditional-clients-querier

8396c1c

This was referenced Feb 15, 2024

Document addition of VerifyMembership rpc and default accepted queries #5846

Closed

Disable selected client types on VerifyMembership rpc #5848

Closed

damiannolan added 3 commits February 15, 2024 13:20

nit: update comment in querier

294bacf

Merge branch 'main' into damian/5310-conditional-clients-querier

41b6e5c

imp: add more info to godoc for VerifyMembership rpc

00f59e8

damiannolan enabled auto-merge (squash) February 15, 2024 12:28

damiannolan merged commit ed9bf74 into main Feb 15, 2024
77 of 79 checks passed

damiannolan deleted the damian/5310-conditional-clients-querier branch February 15, 2024 12:32

mergify bot mentioned this pull request Feb 15, 2024

feat: add rpc VerifyMembershipProof - querier approach for conditional clients (backport #5821) #5850

Merged

damiannolan mentioned this pull request Feb 15, 2024

Backport 08-wasm default accepted query list to v8 compatibility branch #5851

Closed

2 tasks

coderabbitai bot mentioned this pull request Mar 5, 2024

docs: add changelog for callbacks and 08-wasm #5910

Merged

9 tasks

crodriguezvega added backport-to-v8.3.x and removed backport-to-v8.2.x labels Apr 8, 2024

mergify bot mentioned this pull request Apr 8, 2024

feat: add rpc VerifyMembershipProof - querier approach for conditional clients (backport #5821) #6105

Merged

9 tasks

coderabbitai bot mentioned this pull request Apr 28, 2024

feat(08-wasm): expose migrate entry point for 08-wasm #6231

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add rpc `VerifyMembershipProof` - querier approach for conditional clients #5821

feat: add rpc `VerifyMembershipProof` - querier approach for conditional clients #5821

damiannolan commented Feb 7, 2024

damiannolan commented Feb 7, 2024 •

edited

Loading

crodriguezvega left a comment

DimitrisJim left a comment

crodriguezvega Feb 13, 2024 •

edited

Loading

damiannolan Feb 15, 2024

crodriguezvega commented Feb 13, 2024

colin-axner Feb 14, 2024

colin-axner Feb 14, 2024

chatton Feb 14, 2024

chatton Feb 14, 2024

damiannolan Feb 15, 2024

damiannolan Feb 15, 2024

colin-axner Feb 15, 2024

colin-axner left a comment

colin-axner Feb 14, 2024

damiannolan Feb 15, 2024

colin-axner Feb 14, 2024

colin-axner Feb 14, 2024

chatton left a comment

chatton Feb 14, 2024

chatton Feb 14, 2024

chatton Feb 14, 2024

feat: add rpc VerifyMembershipProof - querier approach for conditional clients #5821

feat: add rpc VerifyMembershipProof - querier approach for conditional clients #5821

Conversation

damiannolan commented Feb 7, 2024

Description

damiannolan commented Feb 7, 2024 • edited Loading

crodriguezvega left a comment

Choose a reason for hiding this comment

DimitrisJim left a comment

Choose a reason for hiding this comment

crodriguezvega Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega commented Feb 13, 2024

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

colin-axner 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

Choose a reason for hiding this comment

chatton 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

feat: add rpc `VerifyMembershipProof` - querier approach for conditional clients #5821

feat: add rpc `VerifyMembershipProof` - querier approach for conditional clients #5821

damiannolan commented Feb 7, 2024 •

edited

Loading

crodriguezvega Feb 13, 2024 •

edited

Loading