chore: add GetTimestampAtHeight to client state #888

notbdu · 2022-02-09T06:04:04Z

Description

Add the GetTimetstampAtHeight to each client state + unit tests

06-solomachine
07-tendermint
09-localhost

PR 1/2 for: #876

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

crodriguezvega

Thanks for this PR, @notbdu!

crodriguezvega · 2022-02-09T07:46:46Z

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

@@ -39,6 +39,20 @@ func (cs ClientState) GetLatestHeight() exported.Height {
 	return clienttypes.NewHeight(0, cs.Sequence)
 }

+// GetTimestampAtHeight returns 0.


Is this comment correct? The implementation seems to indicate that a non-zero timestamp could also be returned...

Ah, forgot to update the comment here... 🤦‍♂️

crodriguezvega · 2022-02-09T07:49:46Z

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

+) (uint64, error) {
+	consensusState, err := GetConsensusState(clientStore, cdc, height)
+	if err != nil {
+		return 0, err


Is there a reason not to return the error wrapped like you do for the tendermint client (i.e. sdkerrors.Wrapf(err, "height (%s)", height,))?

colin-axner · 2022-02-10T11:02:40Z

Thanks for the pr's @notbdu. I might have forgotten to mention that we plan to merge all the 02-client changes into a separate feature branch. It still needs to be created, but I can make it now. Would you mind changing the target branch to it?

colin-axner

Nice start! Main requested changes is in relation to the solo machine

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

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

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

modules/light-clients/07-tendermint/types/client_state.go

modules/light-clients/09-localhost/types/client_state.go

modules/light-clients/09-localhost/types/client_state_test.go

codecov-commenter · 2022-03-08T12:26:07Z

Codecov Report

Merging #888 (15544bf) into 02-client-refactor (d5e2ba5) will increase coverage by 0.01%.
The diff coverage is 87.50%.

@@                  Coverage Diff                   @@
##           02-client-refactor     #888      +/-   ##
======================================================
+ Coverage               79.85%   79.86%   +0.01%     
======================================================
  Files                     150      150              
  Lines                   10803    10819      +16     
======================================================
+ Hits                     8627     8641      +14     
- Misses                   1752     1754       +2     
  Partials                  424      424

Impacted Files	Coverage Δ
modules/core/02-client/legacy/v100/solomachine.go	`6.34% <0.00%> (-0.21%)`	⬇️
...light-clients/06-solomachine/types/client_state.go	`63.53% <100.00%> (+0.69%)`	⬆️
.../light-clients/07-tendermint/types/client_state.go	`72.78% <100.00%> (+0.61%)`	⬆️
...s/light-clients/09-localhost/types/client_state.go	`93.28% <100.00%> (+0.09%)`	⬆️

notbdu · 2022-03-09T14:06:07Z

This is ready for another pass btw!

seantking

LGTM but would like a review from @colin-axner.

Thanks for the great work 🤝

colin-axner

Fantastic work! Really appreciated the clean tests. There's small fix regarding solo machines.

Could you add a changelog entry as well? Thanks again!

colin-axner · 2022-03-17T14:52:02Z

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

+	if !cs.GetLatestHeight().EQ(height) {
+		return 0, sdkerrors.Wrapf(ErrInvalidSequence, "not latest height (%s)", height)
+	}


this can be removed. The solo machine only has a single consensus state (the latest consensus state). This function is used when checking if a packet has timed out, so we want to compare the packet timestamp against the latest consensus state, as this would be the consensus state used process the timeout proof

Actually on further consideration, we should return an error if the height passed in is not associated with an expected proof height of cs.GetLatestHeight().Increment()

notbdu · 2022-03-25T05:32:40Z

Thanks for the review! I just rebased and pushed.

colin-axner · 2022-03-28T15:17:05Z

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

+	cdc codec.BinaryCodec,
+	height exported.Height,
+) (uint64, error) {
+	if !cs.GetLatestHeight().Increment().EQ(height) {


Suggested change

if !cs.GetLatestHeight().Increment().EQ(height) {

if !cs.GetLatestHeight().EQ(height) {

Sorry I was incorrect!

The solo machine stores the current sequence of the next proof it'll verify. In the timeout case it'll either be verifying absence of next seq recv (ordered channels) or packet receipt absence (unordered channels). I'm not sure why I got mixed up, but the sequence is incremented after verifying and this function is always called before verifying. So the height passed in should match the current sequence. The only time it won't is during connection handshake verification which doesn't need to rely on GetTimestampAtHeight

Co-authored-by: Bo Du <[email protected]>

colin-axner · 2022-07-12T10:46:13Z

closed by #1659

Co-authored-by: Bo Du <[email protected]>

Closes: cosmos#887 Copy parts of [share package](https://github.com/celestiaorg/celestia-app/tree/main/pkg/shares) relating to compact shares. We plan to later extract common code into a separate repo that both `celestia-app` and `rollkit` can use. Changes needed to this code for use in rollkit will be handled in a later PR. Also, extracts parts needed to resolve dependencies, specifically, `namespace`, `appconsts`, and `testfactory.

notbdu requested review from AdityaSripal, colin-axner and fedekunze as code owners February 9, 2022 06:04

crodriguezvega reviewed Feb 9, 2022

View reviewed changes

notbdu force-pushed the shake/move-getts branch from b964d7d to 01fd5a3 Compare February 10, 2022 00:49

colin-axner suggested changes Feb 10, 2022

View reviewed changes

notbdu changed the base branch from main to 02-client-refactor February 11, 2022 00:25

seantking approved these changes Mar 11, 2022

View reviewed changes

colin-axner suggested changes Mar 17, 2022

View reviewed changes

notbdu and others added 7 commits March 25, 2022 01:31

feat: add GetTimestampAtHeight to client state

86281cd

chore: add space

111b137

chore: address pr feedback

5c4c95b

fix: increment height + check err

176340c

chore: address PR feedback

397342c

fix: expected height is next height

64573b5

chore: add changelog entry

dcec344

notbdu force-pushed the shake/move-getts branch from 15544bf to dcec344 Compare March 25, 2022 05:32

notbdu requested review from charleenfei and damiannolan as code owners March 25, 2022 05:32

colin-axner reviewed Mar 28, 2022

View reviewed changes

crodriguezvega mentioned this pull request Jun 27, 2022

update to 02-client-refactor and apply requested changes polymerdao/ibc-go#1

Closed

9 tasks

charleenfei mentioned this pull request Jul 5, 2022

chore: add GetTimestampAtHeight to client state #888 #1659

Merged

9 tasks

charleenfei added a commit that referenced this pull request Jul 6, 2022

chore: add GetTimestampAtHeight to client state #888 (#1659)

32c4827

Co-authored-by: Bo Du <[email protected]>

colin-axner closed this Jul 12, 2022

oshorefueled pushed a commit to ComposableFi/ibc-go that referenced this pull request Aug 9, 2022

chore: add GetTimestampAtHeight to client state cosmos#888 (cosmos#1659)

bf1a42d

Co-authored-by: Bo Du <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add GetTimestampAtHeight to client state #888

chore: add GetTimestampAtHeight to client state #888

notbdu commented Feb 9, 2022 •

edited

Loading

crodriguezvega left a comment

crodriguezvega Feb 9, 2022

notbdu Feb 9, 2022

crodriguezvega Feb 9, 2022

colin-axner commented Feb 10, 2022

colin-axner left a comment

codecov-commenter commented Mar 8, 2022

notbdu commented Mar 9, 2022

seantking left a comment

colin-axner left a comment

colin-axner Mar 17, 2022

colin-axner Mar 17, 2022

notbdu commented Mar 25, 2022

colin-axner Mar 28, 2022

colin-axner commented Jul 12, 2022

	if !cs.GetLatestHeight().Increment().EQ(height) {
	if !cs.GetLatestHeight().EQ(height) {

chore: add GetTimestampAtHeight to client state #888

chore: add GetTimestampAtHeight to client state #888

Conversation

notbdu commented Feb 9, 2022 • edited Loading

Description

crodriguezvega left a comment

Choose a reason for hiding this comment

crodriguezvega Feb 9, 2022

Choose a reason for hiding this comment

notbdu Feb 9, 2022

Choose a reason for hiding this comment

crodriguezvega Feb 9, 2022

Choose a reason for hiding this comment

colin-axner commented Feb 10, 2022

colin-axner left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 8, 2022

Codecov Report

notbdu commented Mar 9, 2022

seantking left a comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

colin-axner Mar 17, 2022

Choose a reason for hiding this comment

colin-axner Mar 17, 2022

Choose a reason for hiding this comment

notbdu commented Mar 25, 2022

colin-axner Mar 28, 2022

Choose a reason for hiding this comment

colin-axner commented Jul 12, 2022

notbdu commented Feb 9, 2022 •

edited

Loading