-
Notifications
You must be signed in to change notification settings - Fork 725
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
Refactor data column reconstruction and avoid blocking processing #6403
base: unstable
Are you sure you want to change the base?
Conversation
… code and avoids having to pass `DataColumnsToPublish` around and blocking other processing.
@@ -3166,15 +3192,13 @@ impl<T: BeaconChainTypes> BeaconChain<T> { | |||
|
|||
/// Remove any block components from the *processing cache* if we no longer require them. If the | |||
/// block was imported full or erred, we no longer require them. | |||
fn remove_notified_custody_columns<P>( | |||
fn remove_notified_custody_columns( |
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.
If the return value does not have P, we can probably remove the function in favor of remove_notified
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.
Nice, didn't realize they're identical!
"result" => "imported block and custody columns", | ||
"block_hash" => %hash, | ||
); | ||
self.chain.recompute_head_at_current_slot().await; |
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 we compute the head before publishing the columns? I don't have metrics but scheduling the columns for publishing should not block this thread for too long, and it's timing sensitive for the network to do so as soon as possible.
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.
Good catch! Recompute head should definitely happen after publish - publish is non blocking anyway.
Issue Addressed
Continuation of #5990.
I've taken the changes from #5990 with some cleanups. This should simplify the code a bit and reduce supernode bandwidth and performance (reconstruction no longer blocks DA processing for the block).
Proposed Changes
overflow_lru_cache
, this simplifies the code and avoids having to passDataColumnsToPublish
around.