Load sync states from the chain spec on the light client by expenses · Pull Request #7329 · paritytech/substrate

expenses · 2020-10-15T11:59:38Z

A follow-up to #7225 and part of #6804.

https://github.com/paritytech/substrate/blob/a9b22d4e901c2633f92fefc6cbfbf1d3a9971ecc/bin/node/cli/res/flaming-fir.json managed to sync to the head of the chain.
https://github.com/paritytech/substrate/blob/e1180c25423dfe45b8027e331bcf99676424f1a5/bin/node/cli/res/flaming-fir.json does not.

Some questions about the PR that we should:

Is there a better way of getting a sync state header to load correctly without all the hacky allow_missing_parent code?
Is there a better way to debug problems in the code other than replacing errors with panics?
How can we best prune the EpochChanges tree? See Add missing fields to the light sync state #7225 (comment).

…-state

…nges

…-state-WIP-loading

…-state

…te-WIP-loading

…-state-WIP-loading

…rove-sync-state-WIP-loading

This reverts commit a8f0966.

…-state

…oading`

…te-WIP-loading

…-state

…te-WIP-loading

expenses · 2020-10-15T12:20:36Z

* https://github.com/paritytech/substrate/blob/a9b22d4e901c2633f92fefc6cbfbf1d3a9971ecc/bin/node/cli/res/flaming-fir.json managed to sync to the head of the chain.

* https://github.com/paritytech/substrate/blob/e1180c25423dfe45b8027e331bcf99676424f1a5/bin/node/cli/res/flaming-fir.json does not.

We should probably try and find out what the difference between the two is 🤔

expenses · 2020-10-15T12:29:32Z

* https://github.com/paritytech/substrate/blob/a9b22d4e901c2633f92fefc6cbfbf1d3a9971ecc/bin/node/cli/res/flaming-fir.json managed to sync to the head of the chain.

* https://github.com/paritytech/substrate/blob/e1180c25423dfe45b8027e331bcf99676424f1a5/bin/node/cli/res/flaming-fir.json does not.

We should probably try and find out what the difference between the two is thinking

The first chain spec for commit a9b22d4e901c2633f92fefc6cbfbf1d3a9971ecc is for block 118347 which contains a new session and new grandpa authorities.
The second for commit e1180c25423dfe45b8027e331bcf99676424f1a5 is for block 198161 which seems to be a more regular block.

cheme

I do not know if 'allow_missing_parent' can be avoided, if it can't, it could also think of naming it 'snapshot_syncing' or something that refer to the actual action it is used for.
Still 'allow_missing_parent' is similar to existing 'allow_missing_state' (probably for light client), so maybe that is not a good idea at all place.
Generally loading from a snapshot do not use the same trust assumption, so I do not find this that surprising it is needed (maybe some specific api or trait could make it looks less hacky, I think making code generic may help here but that should be next steps).

cheme · 2020-10-15T13:07:47Z

client/consensus/babe/src/aux_schema.rs


 /// Write the cumulative chain-weight of a block ot aux storage.
-pub(crate) fn write_block_weight<H: Encode, F, R>(
+pub fn write_block_weight<H: Encode, F, R>(


why not have a specific 'load_snapshot_state' method that is also public but makes it clear that in should not be use lighty?
Could be part of a trait.

cheme · 2020-10-15T13:11:13Z

client/consensus/babe/src/lib.rs

 			// used by pruning may not know about the block that is being
 			// imported.
 			let prune_and_import = || {
+				/*


Wonder if light pruning method from https://github.com/paritytech/substrate/pull/6851/files would work here, really just a wild guess.

cheme · 2020-10-15T13:12:11Z

client/api/src/in_mem.rs

 		let best_tree_route = {
 			let best_hash = self.storage.read().best_hash;
-			if &best_hash == header.parent_hash() {
+			if &best_hash == header.parent_hash() || allow_missing_parent {


I wonder if modifying tree_route to not fail on missing parent may be better, or if it could break some assumption and make existing code unsecure.

gnunicorn · 2020-11-25T08:33:59Z

Closing due to the PR being stale for a while.

expenses and others added 30 commits September 23, 2020 17:06

Initial draft

d3e8c97

Add an iterator that helps us get most items

1eb1297

Merge remote-tracking branch 'origin/master' into ashley-improve-sync…

a118cea

…-state

Revert changes to grandpa

ccfb11d

Merge remote-tracking branch 'origin/master' into ashley-improve-sync…

b3e8f09

…-state

Change fields to just be the grandpa authority set and babe epoch cha…

35a580a

…nges

Only use the fields we need from the shared authority set

a8f0966

Made a start at getting the sync state to load in substrate

833c007

Add info to panic

bad0a28

babe: example code to filter epoch changes tree

96c1748

babe: don't try to prune epoch changes tree to avoid unknown headers

3e7391b

Merge remote-tracking branch 'origin/master' into ashley-improve-sync…

14cc62a

…-state-WIP-loading

Filter before saving

b2b0860

Merge remote-tracking branch 'origin/master' into ashley-improve-sync…

447b676

…-state-WIP-loading

Switch to RPC call

fdd282f

Merge remote-tracking branch 'origin/master' into ashley-improve-sync…

32c12ed

…-state

Merge branch 'ashley-improve-sync-state' into ashley-improve-sync-sta…

6a5db06

…te-WIP-loading

Merge remote-tracking branch 'origin/master' into ashley-improve-sync…

186e248

…-state-WIP-loading

Reset flaming fir network

a7f607e

Merge remote-tracking branch 'origin/dd-flamingfir-9' into ashley-imp…

40f19e2

…rove-sync-state-WIP-loading

Revert "Only use the fields we need from the shared authority set"

7f28aa9

This reverts commit a8f0966.

Merge branch 'ashley-improve-sync-state' into HEAD

5dd7405

Merge remote-tracking branch 'origin/master' into ashley-improve-sync…

6e7855b

…-state

Add babe_finalized_block_weight from `ashley-improve-sync-state-WIP-l…

81ac0ab

…oading`

Merge branch 'ashley-improve-sync-state' into ashley-improve-sync-sta…

aac528b

…te-WIP-loading

Add sync state to flaming-fir.json

3dd9357

Fix rpc test

136231b

Move sync state rpc stuff into sc-sync-state-rpc

d94adf1

Merge remote-tracking branch 'origin/master' into ashley-improve-sync…

d56cafc

…-state

Merge branch 'ashley-improve-sync-state' into ashley-improve-sync-sta…

9a842ea

…te-WIP-loading

expenses added 3 commits October 12, 2020 14:24

Update sync state

a9b22d4

Comment out CHT creation

76afda0

Merge remote-tracking branch 'origin/master' into HEAD

f14c1d9

expenses added A0-please_review Pull request needs code review. A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Oct 15, 2020

expenses requested review from andresilva, bkchr, cheme, rakanalh and tomaka October 15, 2020 11:59

github-actions bot removed the A0-please_review Pull request needs code review. label Oct 15, 2020

cheme reviewed Oct 15, 2020

View reviewed changes

expenses mentioned this pull request Oct 19, 2020

Rewrite fork tree functions using loops. #7337

Closed

gnunicorn added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Nov 2, 2020

gnunicorn closed this Nov 25, 2020

expenses deleted the ashley-load-sync-state branch August 23, 2021 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load sync states from the chain spec on the light client#7329

Load sync states from the chain spec on the light client#7329
expenses wants to merge 33 commits intomasterfrom
ashley-load-sync-state

expenses commented Oct 15, 2020

Uh oh!

expenses commented Oct 15, 2020

Uh oh!

expenses commented Oct 15, 2020

Uh oh!

cheme left a comment

Uh oh!

cheme Oct 15, 2020

Uh oh!

cheme Oct 15, 2020

Uh oh!

cheme Oct 15, 2020

Uh oh!

gnunicorn commented Nov 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

expenses commented Oct 15, 2020

Uh oh!

expenses commented Oct 15, 2020

Uh oh!

expenses commented Oct 15, 2020

Uh oh!

cheme left a comment

Choose a reason for hiding this comment

Uh oh!

cheme Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

cheme Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

cheme Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

gnunicorn commented Nov 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants