Warp sync part II #9284

arkpar · 2021-07-06T08:49:29Z

This enables background block download after warp sync. The bulk of the changes is epoch management for GRANDPA to allow for verification and import of old blocks.

Polkadot companion: paritytech/polkadot#3564

Also required for warp sync to work properly on kusama: #9239

cheme

I did a first read, looks good, even if I am not sure IIUC a few points.

I also wonder how will behave state pruning for the blocks imported from the gap?

client/cli/src/commands/check_block_cmd.rs

cheme · 2021-08-04T09:53:20Z

client/consensus/babe/src/aux_schema.rs

 			load_decode::<_, EpochChangesFor<Block, EpochV0>>(backend, BABE_EPOCH_CHANGES_KEY)?
 				.map(|v1| v1.map(|_, _, epoch| epoch.migrate(config))),
+		Some(2) =>
+			load_decode::<_, EpochChangesForV1<Block, Epoch>>(backend, BABE_EPOCH_CHANGES_KEY)?


Should we use EpochChangeForV2 instead of EpochChangeForV1 ?

There's some confusion to versions here. V1 means version of the epoch definition, rather than epoch changes. I'll rename the type to indicate the version of epoch changes instead.

cheme · 2021-08-04T09:54:47Z

client/consensus/babe/src/aux_schema.rs

 			load_decode::<_, EpochChangesForV0<Block, EpochV0>>(backend, BABE_EPOCH_CHANGES_KEY)?
 				.map(|v0| v0.migrate().map(|_, _, epoch| epoch.migrate(config))),
 		Some(1) =>
 			load_decode::<_, EpochChangesFor<Block, EpochV0>>(backend, BABE_EPOCH_CHANGES_KEY)?


Is it EpochChangeForV1 now?

cheme · 2021-08-04T09:58:55Z

client/consensus/babe/src/aux_schema.rs

 				.map(|v0| v0.migrate().map(|_, _, epoch| epoch.migrate(config))),
 		Some(1) =>
 			load_decode::<_, EpochChangesFor<Block, EpochV0>>(backend, BABE_EPOCH_CHANGES_KEY)?
 				.map(|v1| v1.map(|_, _, epoch| epoch.migrate(config))),


This function being called 'migrate' is confusing to me (fwiu it migrates from v2 to v1 and the new migrate does V2 to V3), maybe renaming it to 'migrate_to_previous_version' or anything could be better?

both migrate functions migrate to the latest version, not the previous.

client/consensus/epochs/src/lib.rs

cheme · 2021-08-04T12:30:13Z

client/consensus/epochs/src/lib.rs

+				Ok(()) => return Ok(()),
+				Err(e) => e,
+			}
+		} else if !self.epochs.is_empty() && matches!(epoch, PersistedEpoch::Genesis(_, _)) {


IIUC import genesis when already an epoch indicates warp sync case, maybe worth a comment if it is the case.

client/db/src/utils.rs

cheme · 2021-08-04T13:09:55Z

client/db/src/lib.rs

+							&(start, end).encode(),
+						);
+					}
+				} else if number > best_num + One::one() &&


The point checked here is first block imported after a warp sync triggers gap download?
I first thought it should be 'number > best_num',
but maybe here 'best_num' is not changed by warp_sync and is the start of gap (gap init suggests that).

The point checked here is first block imported after a warp sync triggers gap download?

This is triggered when we import a warp block. I.e. we start at genesis and warp to block 10000. Block 10000 is imported with state. Gap is detected. No need to wait for the next block.

With number > best_num it would be a false detection. E.g. block 43 is imported after 42 is the normal case. Only if we import 44 after 42 there's a gap.

client/finality-grandpa/src/authorities.rs

client/network/test/src/sync.rs

arkpar · 2021-08-06T08:04:38Z

I also wonder how will behave state pruning for the blocks imported from the gap?

These blocks are not executed and don't have any state. Only headers are verified. State pruning starts working normally starting from the warp target block.

cheme · 2021-08-06T08:13:03Z

client/network/src/protocol/sync.rs

+										origin: block_data.origin,
+										allow_missing_state: true,
+										import_existing: self.import_existing,
+										skip_execution: true,


👍 (+ demonstrates babe/grandpa do not need chain state to run)

cheme

babe/grandpa warpsync LGTM.

Co-authored-by: cheme <[email protected]>

andresilva

The logic for dealing with gaps on BABE and GRANDPA seems correct to me. I'd like to give this one more pass for the rest of the changes.

andresilva · 2021-08-18T15:35:24Z

client/consensus/epochs/src/lib.rs

+#[derive(Clone, Encode, Decode, Debug)]
+pub struct GapEpochs<Hash, Number, E: Epoch> {
+	current: (Hash, Number, PersistedEpoch<E>),
+	next: Option<(Hash, Number, PersistedEpoch<E>)>,


Next is always guaranteed to be PersistedEpoch::Regular right? Maybe we can change the type to just take E.

andresilva · 2021-08-18T15:37:27Z

client/consensus/epochs/src/lib.rs


+impl<E: Epoch> PersistedEpochHeader<E> {
+	/// Map the epoch header to a different type.
+	pub fn map<B>(self) -> PersistedEpochHeader<B>


Don't really have a strong opinion on this, map usually takes a function to do the mapping.

andresilva · 2021-08-18T15:48:57Z

client/network/src/protocol/sync.rs

 	/// catch up to the latest state while re-importing blocks.
 	import_existing: bool,
+	/// Gap download process.
+	gap_sync: Option<GapSync<B>>,


Would it be possible to somehow move this into warp_sync? If gap_sync is Some then we know we warp synced but we're still downloading the old blocks.

I would prefer to keep them separate. I see warp sync and old block download as two independent features. Even though the only way you can currently end up with the block gap is the warp sync, this may change in the future. I.e. when importing the state from a file or chain spec snapshot. Or after disabling block body pruning.

andresilva · 2021-08-18T15:56:20Z

utils/fork-tree/src/lib.rs

 			if *number < self.number {
 				return Ok(FindOutcome::Failure(false))
 			}
+			if *number == self.number && hash == &self.hash {


I don't remember all the details to know the implications of this, could you just drop a line why this was needed?

It's not actually needed. Just an optimization I've made while fixing an unrelated issue. Saves a database query for the header. I guess it does not belong to this PR indeed, so I'll remove it.

…sync2

stale · 2021-09-18T11:08:22Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

arkpar · 2021-09-29T08:57:02Z

Still waiting for reviews

…sync2

utils/fork-tree/src/lib.rs

bkchr · 2021-10-05T12:28:05Z

client/consensus/babe/src/aux_schema.rs

+			load_decode::<_, EpochChangesV1For<Block, Epoch>>(backend, BABE_EPOCH_CHANGES_KEY)?
+				.map(|v2| v2.migrate()),


Maybe a comment why we use EpochChangesV1For here as well?

client/consensus/epochs/src/lib.rs

client/finality-grandpa/src/import.rs

bkchr · 2021-10-06T09:23:27Z

client/finality-grandpa/src/import.rs

+		if number <= self.inner.info().finalized_number {
+			// Importing an old block. Just save justifications and authority set changes
+			if let Some(_) = self.check_new_change(&block.header, hash) {
+				assert!(block.justifications.is_some());


A proper proof here would be nice

Co-authored-by: Bastian Köcher <[email protected]>

arkpar · 2021-10-06T11:50:05Z

Companion approval would be appreciated

arkpar · 2021-10-07T09:31:29Z

bot merge

ghost · 2021-10-07T09:31:38Z

Trying merge.

github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jul 6, 2021

This was referenced Jul 17, 2021

Warp sync part I #9227

Merged

Add support for GrandPa warp sync in the node template #8922

Closed

arkpar force-pushed the a-warp-sync2 branch 2 times, most recently from 4157e96 to 2a643d2 Compare August 3, 2021 07:37

arkpar marked this pull request as ready for review August 3, 2021 14:54

arkpar requested a review from andresilva as a code owner August 3, 2021 14:54

arkpar mentioned this pull request Aug 3, 2021

Update for warp sync paritytech/polkadot#3564

Merged

arkpar requested review from bkchr and cheme August 3, 2021 15:01

cheme reviewed Aug 4, 2021

View reviewed changes

cheme reviewed Aug 6, 2021

View reviewed changes

cheme approved these changes Aug 6, 2021

View reviewed changes

arkpar and others added 5 commits August 6, 2021 12:08

Gap sync

cee8047

Gap epoch test

01d94a3

Simplified network requests

d139fed

Update client/db/src/utils.rs

1c343f4

Co-authored-by: cheme <[email protected]>

Fixed v1 migration and added some comments

5821cf1

arkpar force-pushed the a-warp-sync2 branch from f7f4722 to 5821cf1 Compare August 6, 2021 10:08

andresilva reviewed Aug 18, 2021

View reviewed changes

arkpar added 4 commits August 18, 2021 19:03

Merge branch 'master' of github.com:paritytech/substrate into a-warp-…

ff04426

…sync2

Next epoch is always regular

ca28a85

Removed fork tree change

08fa139

Merge branch 'master' of github.com:paritytech/substrate into a-warp-…

79f76d7

…sync2

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 18, 2021

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 29, 2021

andresilva approved these changes Oct 4, 2021

View reviewed changes

Merge branch 'master' of github.com:paritytech/substrate into a-warp-…

d090bc2

…sync2

bkchr approved these changes Oct 6, 2021

View reviewed changes

arkpar and others added 2 commits October 6, 2021 12:12

Apply suggestions from code review

a49de7b

Co-authored-by: Bastian Köcher <[email protected]>

Added a comment and converted assert to error

5923f49

ghost merged commit 97818e4 into master Oct 7, 2021

ghost deleted the a-warp-sync2 branch October 7, 2021 09:31

This was referenced Apr 7, 2022

Backport change to checkpoints format paritytech/smoldot#2219

Merged

Light sync state needs a proper format paritytech/polkadot-sdk#60

Open

aurexav added a commit to darwinia-network/darwinia-common that referenced this pull request Jun 10, 2022

Companion of paritytech/substrate#9284

4b9e8e4

lexnv mentioned this pull request Jul 20, 2023

chainSpec_lightSyncState: Provide light-client sync state paritytech/json-rpc-interface-spec#68

Open

This pull request was closed.

		load_decode::<_, EpochChangesV1For<Block, Epoch>>(backend, BABE_EPOCH_CHANGES_KEY)?
		.map(\|v2\| v2.migrate()),

Warp sync part II #9284

Warp sync part II #9284

Uh oh!

Conversation

arkpar commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cheme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arkpar commented Aug 6, 2021

Uh oh!

cheme Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheme left a comment

Choose a reason for hiding this comment

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Sep 18, 2021

Uh oh!

arkpar commented Sep 29, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkpar commented Oct 6, 2021

Uh oh!

arkpar commented Oct 7, 2021

Uh oh!

ghost commented Oct 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arkpar commented Jul 6, 2021 •

edited

Loading

cheme Aug 6, 2021 •

edited

Loading