-
Notifications
You must be signed in to change notification settings - Fork 248
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
Optimize block finalization #3347
Conversation
test_domain_tx_propagate hanged on macOS again |
|
||
while let Some(notification) = import_notification.next().await { | ||
// Wait for importing block to finish importing | ||
if notification.header.number() == &importing_block_number { |
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.
Should it be notification.header.number() >= &importing_block_number
? otherwise, there can be a race condition where the block is imported before we call client.every_import_notification_stream()
.
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.
We call this from the context of block import, so for practical purposes Substrate right mow will only import one block at a time. But even if not, it doesn't really matter because all we really try to do here is delay finalization, it will work fine even if we don't, just causes unnecessary delays in block propagation as described in #3321
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.
Okay, I see, the acknowledgement
in the block notification will avoid such race condition happening. Though, my concern is more about the archiver being stuck in this loop... Perhaps instead of block_importing_notification_stream
, we should use every_import_notification_stream
to derive the archiver forward so we don't need to wait for block import to finish.
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.
We are using every_import_notification_stream
though or am I missing something?
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.
I mean to replace this stream with every_import_notification_stream
:
subspace/crates/sc-consensus-subspace/src/archiver.rs
Lines 950 to 953 in eed2c70
// Subscribing synchronously before returning | |
let mut block_importing_notification_stream = subspace_link | |
.block_importing_notification_stream | |
.subscribe(); |
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.
That is importing
, not import
stream. It is already fired for every single block before it is imported. We can't replace it with stream that processes blocks after import without breaking archiving.
First commit simply moves code around to make review easier, second implements workaround described in #3321 (comment), which should improve situation significantly and I think we can consider that it resolves #3321 for now (thought there are still significantly optimization possibilities remaining in Substrate itself).
Did a quick test locally, it should at very least not break thing comparing to what we already have.
Code contributor checklist: