Skip to content
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.

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

Enable warp sync #901

Merged
merged 42 commits into from
May 6, 2023
Merged

Enable warp sync #901

merged 42 commits into from
May 6, 2023

Conversation

shunsukew
Copy link
Member

@shunsukew shunsukew commented Apr 6, 2023

Pull Request Summary
Enable warp sync feature for local and parachain services.

Check list

  • added or updated unit tests
  • updated Astar official documentation
  • added OnRuntimeUpgrade hook for precompile revert code registration
  • updated spec version
  • updated semver

@Dinonard
Copy link
Member

Dinonard commented Apr 7, 2023

@shunsukew about this PR, have you tested how it works?

I'm asking becausae I found this PR while doing the uplift analysis and my plan was just to create a follow-up item to implement this later, once all parts have been merged. But I guess they already are?

@shunsukew
Copy link
Member Author

@shunsukew about this PR, have you tested how it works?

I'm asking becausae I found this PR while doing the uplift analysis and my plan was just to create a follow-up item to implement this later, once all parts have been merged. But I guess they already are?

yes, I'm testing it now

Base automatically changed from feat/polkadot-v0.9.39 to master April 10, 2023 13:45
@shunsukew
Copy link
Member Author

shunsukew commented Apr 17, 2023

Although Warp sync is unstable, Normal block sync works as expected.
For more details about warp sync, please check paritytech/polkadot-sdk#14.

@shunsukew shunsukew marked this pull request as ready for review April 19, 2023 07:51
@shunsukew
Copy link
Member Author

shunsukew commented Apr 24, 2023

@bkchr @cheme
Thank you so much for checking and finding out problems we encounter here.
paritytech/polkadot-sdk#14

We will conduct state_version migration.
Is this pallet for migration?
https://github.com/paritytech/substrate/tree/master/frame/state-trie-migration
https://hackmd.io/@kizi/HyoSO3lf9

I see, lazy migration is ongoing just changed state_version to 1, but we can hasten its completion by using pallet-state-trie-migration. And without its completion, warp sync doesn't work.

cc. @akru @Dinonard

@shunsukew shunsukew mentioned this pull request Apr 24, 2023
@cheme
Copy link

cheme commented Apr 24, 2023

@bkchr @cheme Thank you so much for checking and finding out problems we encounter here. paritytech/polkadot-sdk#14 (comment)

We will conduct state_version migration. Is this pallet for migration? https://github.com/paritytech/substrate/tree/master/frame/state-trie-migration https://hackmd.io/@kizi/HyoSO3lf9

I see, lazy migration is ongoing just changed state_version to 1, but we can hasten its completion by using pallet-state-trie-migration. And without its completion, warp sync doesn't work.

cc. @akru @Dinonard

Yess that is the right infos, did a quick sumup here: paritytech/polkadot-sdk#14

Note that a "lazy migration" may never complete (if a single value of size > 32 byte is not rewritten in state which is very likely).

@bkchr
Copy link

bkchr commented Apr 24, 2023

Note that a "lazy migration" may never complete (if a single value of size > 32 byte is not rewritten in state which is very likely).

I don't get this. Could you please explain what you mean by this?

@cheme
Copy link

cheme commented Apr 24, 2023

IIUC "lazy migration" is the hybrid mode, one can wait for all the state to be rewritten and thus in state version 1.
But this is a bit optimistic, for instance if your account is bigger than 32 byte, and the user lost its key info, there is no way for it to be updated and the node will never switch from 0 to 1:

@bkchr
Copy link

bkchr commented Apr 24, 2023

Ahh! By "lazy migration" you meant we just wait for the entire state getting "touched". Makes sense now! Ty for the explanation.

// This is done for example when gap syncing and it is expected that the block after the gap
// was checked/chosen properly, e.g. by warp syncing to this block using a finality proof.
// Or when we are importing state only and can not verify the seal.
if block_import.with_state() || block_import.state_action.skip_execution_checks() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +422 to +424
relay_chain_interface: relay_chain_interface.clone(),
})
.await?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just interested, how come we can get rid of block_announce_validator_builder? Is it not required or deprecated?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build_network is doing this internally.

Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shunsukew bump the version and let's merge it 👍

@shunsukew shunsukew merged commit 7d2bbeb into master May 6, 2023
@shunsukew shunsukew deleted the feature/enable-warp-sync branch May 6, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants