Enable local snapshot restore (backport: #733) #728

yihuang · 2023-04-19T03:42:52Z

Closes: #29
Original PR: tendermint/tendermint#9541

Sync state in Handshake via RPC

CV Cleanup dead code

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments

Sync state in Handshake via RPC CV Cleanup dead code

adizere

Thanks for the initial pass at this solution, very grateful for the work here!

I left some questions / suggestions. I would be glad to fix myself some of the concerns I raised directly in the dev branch, if that's ok with you @yihuang and @chillyvee; but will of course give you priority to make commits on the branch.

adizere · 2023-04-20T08:45:44Z

node/node.go

-		if err := doHandshake(stateStore, state, blockStore, genDoc, eventBus, proxyApp, consensusLogger); err != nil {
+
+	// RPC recovery is required if blockStore Height is 0
+	if blockStore.Height() == 0 || !stateSync {


Shouldn't we also check that state.LastBlockHeight == 0 in addition to blockStore.Height() == 0?

Something like:

Suggested change

if blockStore.Height() == 0 || !stateSync {

if (blockStore.Height() == 0 && state.LastBlockHeight == 0) || !stateSync {

This is worth discussing since there are gaps in knowledge that need to be filled. There are three cases where the comet height is zero.

Actually genesis

Statesync completely over the network as we use today

LocalStatesync where the application.db is already restored, but comet is at 0

One of the cosmos-sdk to comet integration challenges for local state sync is communicating the height of appstate/application.db

In the initial implementation, an extra applicationd.db height parameter was passed from cosmos-sdk to comet. However a previous code review tried to eliminate that extra parameter and simply check if comet height was 0, and start RPC statesync.

However that actually causes some issues because a normal network statesync also has comet height == 0, but application.db is also 0.

We need an agreeable way to determine the height of application.db from cosmos-sdk. Since a previous code review determined a parameter was not desirable, is there a better way to query the application height? Is there an object we should look at, or make an ABCI call?

I see, thanks for the detailed notes and history here, I wasn't aware of the subtlety here!

Is there an object we should look at, or make an ABCI call?

An ABCI call (info) would indeed provide the app height I believe. But not sure that's the best way.

Originally it was added as a variable to configure when cosmos-sdk created the Node{}, but it was asked to be removed during code-review, probably to avoid creating a new dependency between cosmos-sdk and comet

Now that we know the removal causes an issue, it's up to us to pick a new way to compare the heights of comet vs cosmos-sdk app.

Is there general guidance on how to communicate variables between comet and cosmos-sdk?

So, for context, a valid CometBFT state requires:

blockStore.Height() - 1 <= state.LastBlockHeight <= blockStore.Height()

So, if the commit of block at height blockStore.Height() has been completed, the state height should be the same. If it hasn't, as the node crashed in the while, the state height should be 1 unit below.

Having said so, if the state of CometBFT is valid and blockStore.Height() == 0 then state.LastBlockHeight == 0 is mandatory.

adizere · 2023-04-20T08:52:04Z

consensus/replay.go

+	// Optimistically build new state, so we don't discover any light client failures at the end.
+	state, err = h.stateProvider.State(pctx, appBlockHeight)
+	if err != nil {
+		h.logger.Error("failed to fetch and verify tendermint state", "err", err)


Nit: Should we use "comet" instead of "tendermint" ?

adizere · 2023-04-20T08:53:56Z

consensus/replay.go

+		if err == light.ErrNoWitnesses {
+			return sm.State{}, nil, err
+		}
+		return sm.State{}, nil, errors.New("snapshot was rejected")


Can we be more specific in this error message? It seems to me that what happened here was that Comet's light client verified the app hash present in the local snapshot of app.db, and this app hash was incorrect. Is this accurate?

Referencing func (s *lightClientStateProvider) State(ctx context.Context, height uint64) (sm.State, error) {

The error conditions are failure to VerifyLightBlockAtHeight , RPC Client Creation Failure (Bad URL), and failure to fetch consensus parameters for the height.

What do you think of wrapping the error to add detail?

Yes, that's a good idea. Let's wrap the err to save the detail and return it to the caller.

There is not snapshot involved here.

The error should say that the application state was rejected, instead.

adizere · 2023-04-20T08:54:40Z

consensus/replay.go

+		if err == light.ErrNoWitnesses {
+			return sm.State{}, nil, err
+		}
+		return sm.State{}, nil, errors.New("snapshot was rejected")


Same comment as above: Would it be good to have more explicit error message here?

What do you think of wrapping the error to add detail?

yihuang · 2023-04-21T04:59:21Z

Haven't looked into detail, but find an unexpected behavior in test: when the app height is 0, statesync enabled, it still try to init with genesis.json, which don't happen with normal version.

github-actions · 2023-05-05T00:12:17Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

cason · 2023-05-05T10:12:20Z

consensus/replay.go

+		if err == light.ErrNoWitnesses {
+			return sm.State{}, nil, err
+		}
+		return sm.State{}, nil, errors.New("snapshot was rejected")


There is not snapshot involved here.

The error should say that the application state was rejected, instead.

cason · 2023-05-05T10:18:45Z

consensus/replay.go

+	if err := h.stateStore.Save(state); err != nil {
+		return sm.State{}, nil, err
+	}
+	if err := h.store.(*store.BlockStore).SaveSeenCommit(int64(appBlockHeight), commit); err != nil {


Following the original implementation, which is Node.startStateSync (I don't know the branch to refer, so no link, sorry), you should use state.LastBlockHeight here for compatibility. It should be equal to appBlockHeight, except for uint/int stuff, but for coherence it is better to follow the same approach.

cason · 2023-05-05T10:19:20Z

consensus/replay.go

+		h.logger.Info("localSync resulting state.Version.Consensus.Block", "state", state.Version.Consensus.Block)
+
+		// Assume Heights Restored, Update Heights for edge cases and constraints on the storeBlockHeight and storeBlockBase.
+		storeBlockHeight = appBlockHeight


I would say that these values should be read from the stores, not blindly updated here.

cason · 2023-05-05T10:21:07Z

node/node.go

+	var err error
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+	stateProvider, err := statesync.NewLightClientStateProvider(


If we are not using this "local sync", this would error, right?

We have to still support the case in which the node is simply recovering, and will restore all state from disk.

cason · 2023-05-05T10:21:39Z

node/node.go

@@ -989,8 +1007,9 @@ func (n *Node) OnStart() error {
 		return fmt.Errorf("could not dial peers from persistent_peers field: %w", err)
 	}

-	// Run state sync
+	// Statesync reqeusting all data from P2P


Suggested change

// Statesync reqeusting all data from P2P

// State sync fetching snapshots from peers

cason · 2023-05-05T10:23:11Z

node/node.go

 	if n.stateSync {
+		// If state and blockStore.Height are both at the same height, skip the P2P Statesync and immediately enter consensus


I couldn't understand this comment.

github-actions · 2023-05-16T00:12:53Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

cason · 2023-05-17T06:17:37Z

Are we proceeding with this approach/solution attempt? If so, we should add the wip label to the PR. Otherwise, GitHub will close it.

yihuang · 2023-05-17T06:19:37Z

Are we proceeding with this approach/solution attempt? If so, we should add the wip label to the PR. Otherwise, GitHub will close it.

I think no need to pursue

adizere · 2023-05-22T09:41:48Z

Closing, a different solution was chosen.

chillyvee · 2023-05-22T11:19:49Z

Which other solution was chosen? Just had a break in schedule to work on the Juno v14 local state sync demo today.

yihuang · 2023-05-22T11:21:48Z

Which other solution was chosen? Just had a break in schedule to work on the Juno v14 local state sync demo today.

cosmos/cosmos-sdk#16061
cosmos/cosmos-sdk#16067

chillyvee · 2023-05-22T11:22:49Z

Thank you, will review

chillyvee · 2023-05-22T11:59:44Z

nice code @yihuang :)

Enable local snapshot restore

68b5d3f

Sync state in Handshake via RPC CV Cleanup dead code

yihuang requested a review from a team as a code owner April 19, 2023 03:42

yihuang mentioned this pull request Apr 19, 2023

State sync from local snapshot: Implementation #29

Closed

3 tasks

adizere mentioned this pull request Apr 19, 2023

ADR 103 Local State Sync Support (ported from Tendermint Core repo) #729

Closed

4 tasks

Merge branch 'v0.34.x' into cv_local_statesync-squash

8809b7a

yihuang changed the title ~~Enable local snapshot restore~~ Enable local snapshot restore (backport: #733) Apr 20, 2023

yihuang marked this pull request as draft April 20, 2023 01:45

adizere reviewed Apr 20, 2023

View reviewed changes

adizere mentioned this pull request Apr 26, 2023

Enable local snapshot restore #733

Closed

3 tasks

github-actions bot added the stale For use by stalebot label May 5, 2023

cason reviewed May 5, 2023

View reviewed changes

github-actions bot removed the stale For use by stalebot label May 6, 2023

github-actions bot added the stale For use by stalebot label May 16, 2023

github-actions bot removed the stale For use by stalebot label May 18, 2023

adizere closed this May 22, 2023

adizere added this to the 2023-Q2 milestone May 22, 2023

yihuang deleted the cv_local_statesync-squash branch May 23, 2023 01:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable local snapshot restore (backport: #733) #728

Enable local snapshot restore (backport: #733) #728

yihuang commented Apr 19, 2023 •

edited

Loading

adizere left a comment

adizere Apr 20, 2023

chillyvee Apr 20, 2023

adizere Apr 24, 2023

chillyvee Apr 24, 2023

cason May 5, 2023 •

edited

Loading

adizere Apr 20, 2023

chillyvee Apr 20, 2023

adizere Apr 20, 2023

chillyvee Apr 20, 2023 •

edited

Loading

adizere Apr 24, 2023

cason May 5, 2023

adizere Apr 20, 2023

chillyvee Apr 20, 2023

yihuang commented Apr 21, 2023 •

edited

Loading

github-actions bot commented May 5, 2023

cason May 5, 2023

cason May 5, 2023

cason May 5, 2023

cason May 5, 2023

cason May 5, 2023

cason May 5, 2023

github-actions bot commented May 16, 2023

cason commented May 17, 2023

yihuang commented May 17, 2023

adizere commented May 22, 2023

chillyvee commented May 22, 2023

yihuang commented May 22, 2023

chillyvee commented May 22, 2023

chillyvee commented May 22, 2023

	if blockStore.Height() == 0 \|\| !stateSync {
	if (blockStore.Height() == 0 && state.LastBlockHeight == 0) \|\| !stateSync {

	// Statesync reqeusting all data from P2P
	// State sync fetching snapshots from peers

		if n.stateSync {
		// If state and blockStore.Height are both at the same height, skip the P2P Statesync and immediately enter consensus

Enable local snapshot restore (backport: #733) #728

Enable local snapshot restore (backport: #733) #728

Conversation

yihuang commented Apr 19, 2023 • edited Loading

PR checklist

adizere 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

cason May 5, 2023 • edited Loading

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

chillyvee Apr 20, 2023 • edited Loading

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

yihuang commented Apr 21, 2023 • edited Loading

github-actions bot commented May 5, 2023

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

github-actions bot commented May 16, 2023

cason commented May 17, 2023

yihuang commented May 17, 2023

adizere commented May 22, 2023

chillyvee commented May 22, 2023

yihuang commented May 22, 2023

chillyvee commented May 22, 2023

chillyvee commented May 22, 2023

yihuang commented Apr 19, 2023 •

edited

Loading

cason May 5, 2023 •

edited

Loading

chillyvee Apr 20, 2023 •

edited

Loading

yihuang commented Apr 21, 2023 •

edited

Loading