Use separate prefix store for each client #5850

AdityaSripal · 2020-03-22T19:31:38Z

Closes: #5502

Description

Creates prefix store with prefix of clientID for each client instance.

Test case on 04-channel was failing. Fixed it by simply changing the expected value to match actual

For contributor use:

Targeted PR against 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.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

x/ibc/02-client/keeper/keeper.go

cwgoes

The store keys need to be changed (in all instances I think), we don't want to duplicate the client identifier.

Also, the tests fail locally, not sure what's up with CI - failing tests should be expected anyways, as all of the paths for proofs will need to change. I suggest separate functions for calculating store keys & then prefixing them with a client identifier, which should also be used in the verification code (e.g. verifyClientConsensusState et al.)

x/ibc/02-client/keeper/keeper.go

cwgoes · 2020-03-23T11:13:25Z

Also note my question on this issue, which is relevant - cosmos/ibc#387 (comment).

…os-sdk into aditya/prefix-client

golangcibot · 2020-03-23T16:09:35Z

x/ibc/03-connection/keeper/handshake_test.go

@@ -205,8 +205,9 @@ func (suite *KeeperTestSuite) TestConnOpenAck() {
 			connectionKey := ibctypes.KeyConnection(testConnectionIDB)
 			proofTry, proofHeight := queryProof(suite.chainB, connectionKey)

-			consensusKey := ibctypes.KeyConsensusState(testClientIDA, uint64(consensusHeight))
+			consensusKey := append([]byte(testClientIDA+"/"), ibctypes.KeyConsensusState(uint64(consensusHeight))...)


unnecessary conversion (from unconvert)

golangcibot · 2020-03-24T00:12:29Z

x/ibc/03-connection/keeper/handshake_test.go

@@ -115,7 +115,7 @@ func (suite *KeeperTestSuite) TestConnOpenTry() {
 			connectionKey := ibctypes.KeyConnection(testConnectionIDA)
 			proofInit, proofHeight := queryProof(suite.chainA, connectionKey)

-			consensusKey := ibctypes.KeyConsensusState(testClientIDB, consensusHeight)
+			consensusKey := prefixedClientKey(testClientIDB, ibctypes.KeyConsensusState(uint64(consensusHeight)))


unnecessary conversion (from unconvert)

golangcibot · 2020-03-24T00:12:30Z

x/ibc/03-connection/keeper/handshake_test.go

@@ -205,7 +205,7 @@ func (suite *KeeperTestSuite) TestConnOpenAck() {
 			connectionKey := ibctypes.KeyConnection(testConnectionIDB)
 			proofTry, proofHeight := queryProof(suite.chainB, connectionKey)

-			consensusKey := ibctypes.KeyConsensusState(testClientIDA, uint64(consensusHeight))
+			consensusKey := prefixedClientKey(testClientIDA, ibctypes.KeyConsensusState(uint64(consensusHeight)))


unnecessary conversion (from unconvert)

golangcibot · 2020-03-24T00:12:30Z

x/ibc/03-connection/keeper/verify_test.go

@@ -66,7 +66,7 @@ func (suite *KeeperTestSuite) TestVerifyClientConsensusState() {

 			// TODO: is this the right consensus height
 			consensusHeight := uint64(suite.chainA.Header.Height)
-			consensusKey := ibctypes.KeyConsensusState(testClientIDA, consensusHeight)
+			consensusKey := prefixedClientKey(testClientIDA, ibctypes.KeyConsensusState(uint64(consensusHeight)))


unnecessary conversion (from unconvert)

AdityaSripal · 2020-03-24T00:12:45Z

x/ibc/02-client/client/utils/utils.go

@@ -44,7 +44,7 @@ func QueryClientState(
 ) (types.StateResponse, error) {
 	req := abci.RequestQuery{
 		Path:  "store/ibc/key",
-		Data:  ibctypes.KeyClientState(clientID),
+		Data:  ibctypes.KeyClientState(),


This file need to be fixed to still prefix clientID

AdityaSripal · 2020-03-24T00:13:38Z

x/ibc/03-connection/keeper/handshake_test.go

@@ -115,7 +115,7 @@ func (suite *KeeperTestSuite) TestConnOpenTry() {
 			connectionKey := ibctypes.KeyConnection(testConnectionIDA)
 			proofInit, proofHeight := queryProof(suite.chainA, connectionKey)

-			consensusKey := ibctypes.KeyConsensusState(testClientIDB, consensusHeight)
+			consensusKey := prefixedClientKey(testClientIDB, ibctypes.KeyConsensusState(uint64(consensusHeight)))


Currently using this helper function rather than creating a prefix and using ApplyPrefix

AdityaSripal · 2020-03-24T00:17:17Z

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

@@ -114,7 +114,8 @@ func (cs ClientState) VerifyClientConsensusState(
 	proof commitmentexported.Proof,
 	consensusState clientexported.ConsensusState,
 ) error {
-	path, err := commitmenttypes.ApplyPrefix(prefix, ibctypes.ConsensusStatePath(counterpartyClientIdentifier, consensusHeight))
+	clientPrefixedPath := counterpartyClientIdentifier + "/" + ibctypes.ConsensusStatePath(consensusHeight)


Simply appending here rather than using ApplyPrefix since the Prefix that gets used by all paths is simply ibc. Not sure if i should create a seperate Prefix object for ClientConsensusState and pass it in here or if I should continue hardcoding this append

If I create a prefix, I must have some code in the caller that does something like:

ibcPrefix := prefix.Bytes() clientPrefix := commitment.NewPrefix(append(ibcPrefix, counterPartyClientID....) // pass in clientPrefix to VerifyClientConsensusState, ibcPrefix into VerifyConnectionState

I think from the perspective of the spec the full path just needs to be correct, I'm not sure what's considered more ergonomic in the SDK.

AdityaSripal · 2020-03-24T06:18:25Z

x/ibc/02-client/keeper/keeper.go

@@ -147,6 +148,7 @@ func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height uint64) (exported.
 // objects. For each State object, cb will be called. If the cb returns true,
 // the iterator will close and stop.
 func (k Keeper) IterateClients(ctx sdk.Context, cb func(exported.ClientState) bool) {
+	// TODO: determine how to fix this function with new key format


Unsure of best way to fix this function with current key format that puts clientID before "client_state"

This should only be called outside of the state machine (e.g. in queriers), so I think it's OK if it's not the most efficient (note that all clients should still be prefixed by client/{clientID}, so you shouldn't need to iterate over other things).

Hmm currently think clientstates are stored under ibc/{clientID}/client_state and consensus states are stored under ibc/{clientID}/consensus_state/{height}

Could add another client prefix in after ibc? then we would be iterating over client states and consensus states. would be somewhat inefficient but not terrible

ah - it should be ibc/clients/{clientID}/client_state and ibc/clients/{clientID}/consensus_state/{height}, I think

AdityaSripal · 2020-04-09T22:51:18Z

Closing in favor of #5980 since it was too annoying to try and rebase to master

AdityaSripal added 2 commits March 22, 2020 12:15

prefix client instances

982864f

fix expected value on msgpacket.GetBytes

52fd8ab

AdityaSripal requested review from alessio, alexanderbez, fedekunze, hschoenburg and nylira as code owners March 22, 2020 19:31

AdityaSripal requested a review from cwgoes March 22, 2020 19:31

golangcibot reviewed Mar 22, 2020

View reviewed changes

x/ibc/02-client/keeper/keeper.go Outdated Show resolved Hide resolved

fix keys and build

3235aaf

cwgoes reviewed Mar 23, 2020

View reviewed changes

x/ibc/02-client/keeper/keeper.go Outdated Show resolved Hide resolved

Update x/ibc/02-client/keeper/keeper.go

3bd1fb2

cwgoes suggested changes Mar 23, 2020

View reviewed changes

x/ibc/02-client/keeper/keeper.go Outdated Show resolved Hide resolved

AdityaSripal added 2 commits March 23, 2020 09:05

start fixing tests

4921b1d

Merge branch 'aditya/prefix-client' of https://github.com/cosmos/cosm…

1e3951a

…os-sdk into aditya/prefix-client

golangcibot reviewed Mar 23, 2020

View reviewed changes

AdityaSripal added 2 commits March 23, 2020 17:08

fix 03-connection tests

3c61423

add TODO

e53fa00

golangcibot reviewed Mar 24, 2020

View reviewed changes

AdityaSripal commented Mar 24, 2020

View reviewed changes

AdityaSripal added 2 commits March 24, 2020 14:21

prefix client keys correctly and fix iterateclients

a52874d

appease golint

2f7d6cc

AdityaSripal changed the base branch from ibc-alpha to master April 9, 2020 17:20

AdityaSripal changed the base branch from master to ibc-alpha April 9, 2020 17:20

fix 03 and 04 prefixes

5eb6607

AdityaSripal closed this Apr 9, 2020

AdityaSripal deleted the aditya/prefix-client branch April 9, 2020 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use separate prefix store for each client #5850

Use separate prefix store for each client #5850

AdityaSripal commented Mar 22, 2020

cwgoes left a comment

cwgoes commented Mar 23, 2020

golangcibot Mar 23, 2020

golangcibot Mar 24, 2020

golangcibot Mar 24, 2020

golangcibot Mar 24, 2020

AdityaSripal Mar 24, 2020

AdityaSripal Mar 24, 2020

AdityaSripal Mar 24, 2020

cwgoes Mar 24, 2020

AdityaSripal Mar 24, 2020

cwgoes Mar 24, 2020 •

edited

Loading

AdityaSripal Mar 24, 2020

cwgoes Mar 24, 2020

AdityaSripal commented Apr 9, 2020

Use separate prefix store for each client #5850

Use separate prefix store for each client #5850

Conversation

AdityaSripal commented Mar 22, 2020

Description

cwgoes left a comment

Choose a reason for hiding this comment

cwgoes commented Mar 23, 2020

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

cwgoes Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Apr 9, 2020

cwgoes Mar 24, 2020 •

edited

Loading