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

Update MMR sync. #3104

Closed
wants to merge 2 commits into from
Closed

Conversation

shamil-gadelshin
Copy link
Member

This PR updates MMR-sync. It removes the existing limitation of using FINALIZATION_DEPTH_IN_SEGMENTS lag by calculating the proper temporary key by the block number and saving the MMR-node with a canon key in advance. This change allows us to snap sync to the confirmed domain block instead of to the block within FINALIZATION_DEPTH_IN_SEGMENTS blocks lag and sync several segments after that.

Code contributor checklist:

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Just one minor request, very glad to see this otherwise!

@@ -5,7 +5,6 @@ use sc_client_api::{AuxStore, Backend, ProofProvider};
use sc_consensus::{
BlockImport, BlockImportParams, ForkChoiceStrategy, ImportedState, StateAction, StorageChanges,
};
use sc_consensus_subspace::archiver::FINALIZATION_DEPTH_IN_SEGMENTS;
Copy link
Member

Choose a reason for hiding this comment

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

Please make this constant non-public again

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. I'd consider moving the constant next time we make it public.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good!

@shamil-gadelshin
Copy link
Member Author

Superseded by #3115

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.

3 participants