Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 22 commits into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
7388af9
feat: adding protobuf msgs and rpc for VerifyMembershipProof
damiannolan Feb 7, 2024
18e5404
feat: adding VerifyMembershipProof query implementation and wiring
damiannolan Feb 7, 2024
1f720b6
chore(08-wasm): add VerifyMembershipProof to stargate query acceptlist
damiannolan Feb 7, 2024
37435a6
test: adding failure case unit tests for VerifyMembershipProof query
damiannolan Feb 7, 2024
2569bd4
fix: correct protodoc
damiannolan Feb 7, 2024
06d2f59
chore: proto-swagger-gen
damiannolan Feb 7, 2024
a2fc9a7
chore: protodocs
damiannolan Feb 7, 2024
a2f139d
test: adding additional test cases
damiannolan Feb 7, 2024
9cc6cdc
Merge branch 'main' into damian/5310-conditional-clients-querier
damiannolan Feb 7, 2024
b72e541
test: assert gas consumed in tests
damiannolan Feb 8, 2024
c19d6ed
chore: rename rpc to VerifyMembership and update tests
damiannolan Feb 8, 2024
889be6e
chore: update service definition URL in 08-wasm stargate accepted que…
damiannolan Feb 8, 2024
7ef470e
test: adding verify membership test to 08-wasm querier
damiannolan Feb 8, 2024
e64af3a
Update proto/ibc/core/client/v1/query.proto
damiannolan Feb 12, 2024
da3b02e
chore: review items - log error at debug, pass cachedCtx and adjust t…
damiannolan Feb 13, 2024
143d7c9
chore: add doc comment to querier test, address nit to move defaultAc…
damiannolan Feb 13, 2024
1ecb660
chore: regen protos and swagger doc
damiannolan Feb 13, 2024
83fbeec
Merge branch 'main' into damian/5310-conditional-clients-querier
damiannolan Feb 13, 2024
8396c1c
Merge branch 'main' into damian/5310-conditional-clients-querier
chatton Feb 14, 2024
294bacf
nit: update comment in querier
damiannolan Feb 15, 2024
41b6e5c
Merge branch 'main' into damian/5310-conditional-clients-querier
damiannolan Feb 15, 2024
00f59e8
imp: add more info to godoc for VerifyMembership rpc
damiannolan Feb 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
387 changes: 387 additions & 0 deletions docs/client/swagger-ui/swagger.yaml

Large diffs are not rendered by default.

52 changes: 52 additions & 0 deletions modules/core/02-client/keeper/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -328,3 +328,55 @@ func (k Keeper) UpgradedConsensusState(c context.Context, req *types.QueryUpgrad
UpgradedConsensusState: protoAny,
}, nil
}

// VerifyMembership implements the Query/VerifyMembership gRPC method
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
func (k Keeper) VerifyMembership(c context.Context, req *types.QueryVerifyMembershipRequest) (*types.QueryVerifyMembershipResponse, error) {
Copy link
Contributor

@crodriguezvega crodriguezvega Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if req == nil {
return nil, status.Error(codes.InvalidArgument, "empty request")
}

if err := host.ClientIdentifierValidator(req.ClientId); err != nil {
return nil, status.Error(codes.InvalidArgument, err.Error())
}
Comment on lines +340 to +342
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

localhost as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created this issue: #5848

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if len(req.Proof) == 0 {
return nil, status.Error(codes.InvalidArgument, "empty proof")
}

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

Comment on lines +348 to +351
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if req.MerklePath.Empty() {
return nil, status.Error(codes.InvalidArgument, "empty merkle path")
}

if len(req.Value) == 0 {
return nil, status.Error(codes.InvalidArgument, "empty value")
}

ctx := sdk.UnwrapSDKContext(c)
// cache the context to ensure clientState.VerifyMembership does not change state
cachedCtx, _ := ctx.CacheContext()

// make sure we charge the higher level context even on panic
defer func() {
ctx.GasMeter().ConsumeGas(cachedCtx.GasMeter().GasConsumed(), "verify membership query")
}()

clientState, found := k.GetClientState(cachedCtx, req.ClientId)
if !found {
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

k.Logger(ctx).Debug("proof verification failed", "key", req.MerklePath, "error", err)
return &types.QueryVerifyMembershipResponse{
Success: false,
}, nil
}

return &types.QueryVerifyMembershipResponse{
Success: true,
}, nil
}
144 changes: 144 additions & 0 deletions modules/core/02-client/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"errors"
"fmt"

"google.golang.org/grpc/codes"
Expand All @@ -12,9 +13,12 @@ import (
"github.com/cosmos/cosmos-sdk/types/query"

"github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
commitmenttypes "github.com/cosmos/ibc-go/v8/modules/core/23-commitment/types"
host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
"github.com/cosmos/ibc-go/v8/modules/core/exported"
ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint"
ibctesting "github.com/cosmos/ibc-go/v8/testing"
"github.com/cosmos/ibc-go/v8/testing/mock"
)

func (suite *KeeperTestSuite) TestQueryClientState() {
Expand Down Expand Up @@ -770,3 +774,143 @@ func (suite *KeeperTestSuite) TestQueryClientParams() {
res, _ := suite.chainA.QueryServer.ClientParams(ctx, &types.QueryClientParamsRequest{})
suite.Require().Equal(&expParams, res.Params)
}

func (suite *KeeperTestSuite) TestQueryVerifyMembershipProof() {
var (
path *ibctesting.Path
req *types.QueryVerifyMembershipRequest
)

testCases := []struct {
name string
malleate func()
expError error
}{
{
"success",
func() {
channel := path.EndpointB.GetChannel()
bz, err := suite.chainB.Codec.Marshal(&channel)
suite.Require().NoError(err)

channelProof, proofHeight := path.EndpointB.QueryProof(host.ChannelKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID))

merklePath := commitmenttypes.NewMerklePath(host.ChannelPath(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID))
merklePath, err = commitmenttypes.ApplyPrefix(suite.chainB.GetPrefix(), merklePath)
suite.Require().NoError(err)

req = &types.QueryVerifyMembershipRequest{
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
ClientId: path.EndpointA.ClientID,
Proof: channelProof,
ProofHeight: proofHeight,
MerklePath: merklePath,
Value: bz,
}
},
nil,
},
{
"req is nil",
func() {
req = nil
},
errors.New("empty request"),
},
{
"invalid client ID",
func() {
req = &types.QueryVerifyMembershipRequest{
ClientId: "//invalid_id",
}
},
host.ErrInvalidID,
},
{
"empty proof",
func() {
req = &types.QueryVerifyMembershipRequest{
ClientId: ibctesting.FirstClientID,
Proof: []byte{},
}
},
errors.New("empty proof"),
},
{
"invalid proof height",
func() {
req = &types.QueryVerifyMembershipRequest{
ClientId: ibctesting.FirstClientID,
Proof: []byte{0x01},
ProofHeight: types.ZeroHeight(),
}
},
errors.New("proof height must be non-zero"),
},
{
"empty merkle path",
func() {
req = &types.QueryVerifyMembershipRequest{
ClientId: ibctesting.FirstClientID,
Proof: []byte{0x01},
ProofHeight: types.NewHeight(1, 100),
}
},
errors.New("empty merkle path"),
},
{
"empty value",
func() {
req = &types.QueryVerifyMembershipRequest{
ClientId: ibctesting.FirstClientID,
Proof: []byte{0x01},
ProofHeight: types.NewHeight(1, 100),
MerklePath: commitmenttypes.NewMerklePath("/ibc", host.ChannelPath(mock.PortID, ibctesting.FirstChannelID)),
}
},
errors.New("empty value"),
},
{
"client not found",
func() {
req = &types.QueryVerifyMembershipRequest{
ClientId: types.FormatClientIdentifier(exported.Tendermint, 100), // use a sequence which hasn't been created yet
Proof: []byte{0x01},
ProofHeight: types.NewHeight(1, 100),
MerklePath: commitmenttypes.NewMerklePath("/ibc", host.ChannelPath(mock.PortID, ibctesting.FirstChannelID)),
Value: []byte{0x01},
}
},
types.ErrClientNotFound,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest() // reset

path = ibctesting.NewPath(suite.chainA, suite.chainB)
path.Setup()

tc.malleate()

ctx := suite.chainA.GetContext()
initialGas := ctx.GasMeter().GasConsumed()
res, err := suite.chainA.QueryServer.VerifyMembership(ctx, req)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

expPass := tc.expError == nil
if expPass {
suite.Require().NoError(err)
suite.Require().True(res.Success, "failed to verify membership proof")

gasConsumed := ctx.GasMeter().GasConsumed()
suite.Require().Greater(gasConsumed, initialGas, "gas consumed should be greater than initial gas")
} else {
suite.Require().ErrorContains(err, tc.expError.Error())

gasConsumed := ctx.GasMeter().GasConsumed()
suite.Require().GreaterOrEqual(gasConsumed, initialGas, "gas consumed should be greater than or equal to initial gas")
}
})
}
}
Loading
Loading