-
Notifications
You must be signed in to change notification settings - Fork 43
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
fix(l1): full-sync with both consensus and execution node #2081
base: fix/full-sync
Are you sure you want to change the base?
Conversation
|
crates/networking/p2p/sync.rs
Outdated
@@ -265,7 +265,7 @@ impl SyncManager { | |||
store.add_block_headers(block_hashes.clone(), block_headers)?; | |||
|
|||
if self.sync_mode == SyncMode::Full { | |||
self.download_and_run_blocks(&mut block_hashes, store.clone()) | |||
self.download_and_run_blocks(&mut block_hashes, store.clone(), sync_head) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe, instead of passing sync_head
, you should filter block_hashes
so that every block after sync_head
is filtered out. Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Actually, my approach for some reason wasn't working. This is simpler and it is working in the server. I will update the PR later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
crates/networking/p2p/sync.rs
Outdated
if sync_head_found { | ||
// Filter out everything after the sync_head | ||
block_hashes = block_hashes | ||
.iter() | ||
.take_while(|&hash| *hash != sync_head) | ||
.cloned() | ||
.collect(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic should be on both sync modes: snap and full.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 973719d!
Motivation
Currently, during full-sync, we download, execute and store blocks as canonical in chunks of
MAX_BLOCK_BODIES_TO_REQUEST=128
. However, we do not verify whether we are ahead of the consensus head, which results in invalidforkChoice
responses.Description
Closes #2066