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.

Sign up for GitHub

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

Jump to bottom

chore: add solomachine client storage #1144

Merged

damiannolan merged 7 commits into 02-client-refactor from damian/solomachine-client-storage

Mar 28, 2022

Member

damiannolan commented Mar 21, 2022

Description

Adds storage operations to UpdateState and UpdateStateOnMisbehaviour

closes: #XXXX

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 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
Review Codecov Report in the comment section below once CI passes


          WIP: solomachine UpdateState now handles storage of ClientState and C…

9ce3b69

…onsensusState

damiannolan commented

View reviewed changes

modules/light-clients/06-solomachine/types/update.go Outdated

Comment on lines 137 to 138

		// TODO: Should be telemetry and events be emitted from 02-client?
		// The clientID would need to be passed an additional arg here

Member Author

damiannolan Mar 21, 2022

I think we need to make a decision on whether telemetry and events should remain to be emitted from 02-client or from within the ClientState implementation. We may need to pass the client ID as an additional arg if we want to do this in the lightclient implementation, or otherwise we may need to return the consensus height. Any thoughts on this? cc. @colin-axner @seantking

Contributor

colin-axner Mar 21, 2022 •

edited

Loading

We should not require light client implementations to emit events. This decision informed the proposed design. We should allow light client to optionally emit their own events and telemetry. This is technically possible by constructing a ClientMessage which contains the clientID, but I'm not opposed to adding the clientID as an argument to the function since 02-client already has that information. It doesn't provide any usefulness outside of event emission/telemetry since the client store is prefixed by the clientID

Contributor

colin-axner Mar 21, 2022

To expand further on why we should not require light client implementations to emit events:

Relayers make use of the events we have in 02-client. By passing this to light client implementations, we lose standardization and risk light client implementators making mistakes. Since the functionality called by 02-client is very verbose now, 02-client can emit appropriate events when doing updates or updates on misbehaviour

Member Author

damiannolan Mar 21, 2022

Okay, great, that makes a lot of sense! Thanks, I'll remove the events and telemetry from here.

colin-axner reviewed

View reviewed changes

modules/light-clients/06-solomachine/types/update.go Outdated Show resolved Hide resolved

modules/light-clients/06-solomachine/types/update.go Outdated Show resolved Hide resolved

modules/light-clients/06-solomachine/types/update.go Outdated Show resolved Hide resolved

modules/light-clients/06-solomachine/types/update.go Outdated

Comment on lines 129 to 134

+              	// set default consensus height with header height
+              	consensusHeight := smHeader.GetHeight()
+              	if cs.ClientType() != exported.Localhost {
+              		clientStore.Set(host.ConsensusStateKey(consensusHeight), types.MustMarshalConsensusState(cdc, consensusState))
+              	} else {
+              		consensusHeight = types.GetSelfHeight(ctx)

Contributor

colin-axner Mar 21, 2022

where is this coming from? Solo machines don't store consensus states

Member Author

damiannolan Mar 21, 2022

Slightly confused here, isn't this code run for solomachines too?

Contributor

colin-axner Mar 21, 2022

It is, but it shouldn't be. This was another justification for why setting the consensus state within the light client implementation is useful. I thought I documented this clearly, but this was all I could find:

This would require light client developers to set the consensus states themselves (allowing header.GetHeight to be removed and stopping unnecessary state storage for solo machines).

Solo machines shouldn't be setting a consensus state ever. It has a single consensus state which is constantly updated and thus it is stored within the client state. The code linked above was an inefficiency, performing unnecessary storage

Member Author

damiannolan Mar 22, 2022

Ahh, okay! Tbh I was confused why ConsensusState existed as a field on the solomachine ClientState and the extra storage was happening.
This should all be much easier to reason about when the refactor is done. Thank you for clarifying this and providing context, much appreciated!

Updated and removed this code :)

modules/light-clients/06-solomachine/types/update.go Outdated Show resolved Hide resolved

modules/light-clients/06-solomachine/types/update.go Outdated

Comment on lines 137 to 138

		// TODO: Should be telemetry and events be emitted from 02-client?
		// The clientID would need to be passed an additional arg here

Contributor

colin-axner Mar 21, 2022

To expand further on why we should not require light client implementations to emit events:

Relayers make use of the events we have in 02-client. By passing this to light client implementations, we lose standardization and risk light client implementators making mistakes. Since the functionality called by 02-client is very verbose now, 02-client can emit appropriate events when doing updates or updates on misbehaviour


          applying suggestions for review

ae972c3

colin-axner reviewed

View reviewed changes

modules/light-clients/06-solomachine/types/update.go Outdated Show resolved Hide resolved

damiannolan added 2 commits

March 22, 2022 09:41


          removing unnecessary consensus state storage

c749c70


          Merge branch '02-client-refactor' into damian/solomachine-client-storage

9628a60

damiannolan marked this pull request as ready for review

March 22, 2022 08:47

damiannolan requested review from fedekunze and AdityaSripal as code owners

March 22, 2022 08:47

damiannolan changed the title ~~WIP: solomachine client storage~~ chore: add solomachine client storage

damiannolan requested a review from seantking

March 22, 2022 08:48

colin-axner reviewed

View reviewed changes

modules/light-clients/06-solomachine/types/update_test.go Outdated

		@@ -615,7 +623,7 @@ func (suite *SoloMachineTestSuite) TestUpdateState() {
		suite.Require().Equal(consensusState, cs.(*types.ClientState).ConsensusState)

Contributor

colin-axner Mar 23, 2022

can you add checks on the successful case that the correct client state is set in the store?

colin-axner reviewed

View reviewed changes

modules/light-clients/06-solomachine/types/update.go

-              func (cs ClientState) UpdateStateOnMisbehaviour(
-              	_ sdk.Context, _ codec.BinaryCodec, _ sdk.KVStore, // prematurely include args for self storage of consensus state
-              ) (*ClientState, exported.ConsensusState, error) {
+              func (cs ClientState) UpdateStateOnMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore) (*ClientState, exported.ConsensusState, error) {

Contributor

colin-axner Mar 23, 2022

Is there a test for this function? Can you check that the correct client state is set in the store?

Member Author

damiannolan Mar 23, 2022

Yeah, there is a test function for this. I was initially just doing an assertion on the returned value but I can adapt it to query the store. Will do the same as you suggest for UpdateState 👍

Contributor

colin-axner Mar 23, 2022

Sweet, I just figure better to do now so when we remove the return values we don't have to add testing code

Member Author

damiannolan Mar 23, 2022

Definitely! Agreed 💯

damiannolan added 2 commits

March 23, 2022 15:41


          updating tests to use store in favour of return values

663ede8


          Merge branch 'damian/solomachine-client-storage' of github.com:cosmos…

ca18137

…/ibc-go into damian/solomachine-client-storage

colin-axner approved these changes

View reviewed changes

Contributor

colin-axner left a comment

Great work!

seantking approved these changes

View reviewed changes


          Merge branch '02-client-refactor' into damian/solomachine-client-storage

cdf7d35

damiannolan enabled auto-merge (squash)

March 28, 2022 14:28

damiannolan added the automerge label

damiannolan merged commit b0dd49d into 02-client-refactor

damiannolan deleted the damian/solomachine-client-storage branch

March 28, 2022 14:36

colin-axner mentioned this pull request

07-tendermint should set own client/consensus states in UpdateState #1190

Closed

3 tasks

seunlanlege pushed a commit to ComposableFi/ibc-go that referenced this pull request


          chore: add solomachine client storage (cosmos#1144)

0261e83

* WIP: solomachine UpdateState now handles storage of ClientState and ConsensusState

* applying suggestions for review

* removing unnecessary consensus state storage

* updating tests to use store in favour of return values

CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this pull request


          test: use pkg httptest to avoid port conflict (cosmos#1144)

91e1c64

## Overview

This PR uses pkg `httptest` to avoid port conflict when running tests
under `da/test` i.e. `TestLifecycle`, `TestRetrieve`.

This allows the tests to pass even if an address is bound to the default
port i.e. `127.0.0.1:26658`

## Checklist

<!-- 
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [ ] New and updated code has appropriate documentation
- [ ] New and updated code has new and/or updated testing
- [ ] Required CI checks are passing
- [ ] Visual proof for any user facing features like CLI or
documentation updates
- [ ] Linked issues closed with keywords

CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this pull request


          test: update mock.Server to use httptest.Server (cosmos#1145)

3967fc2

## Overview

This PR improves upon cosmos#1144 by simplifying `mock.Server` to use
`httptest` directly, avoiding a duplicate `http.Server`. `Server.Start`
doesn't need a listener anymore, and it returns the URL at which the
`httptest.Server` is running.

## Checklist

<!-- 
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [ ] New and updated code has appropriate documentation
- [ ] New and updated code has new and/or updated testing
- [ ] Required CI checks are passing
- [ ] Visual proof for any user facing features like CLI or
documentation updates
- [ ] Linked issues closed with keywords

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels