-
Notifications
You must be signed in to change notification settings - Fork 932
Attestation processing optimization with batching #8285
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
base: unstable
Are you sure you want to change the base?
Conversation
beacon_node/beacon_processor/src/scheduler/work_reprocessing_queue.rs
Outdated
Show resolved
Hide resolved
…hthouse into attestation-optimisation
beacon_node/beacon_processor/src/scheduler/work_reprocessing_queue.rs
Outdated
Show resolved
Hide resolved
| let mut attestations = GossipAttestationBatch::new(); | ||
| let mut iter = batch_attestation.0.into_iter(); | ||
|
|
||
| if let Some(first) = iter.next() { |
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.
Is it easier to read to do if !is_empty() { let first = batch_attestation.first().expect("not empty") }?
| pub const QUEUED_ATTESTATION_DELAY: Duration = Duration::from_secs(12); | ||
|
|
||
| /// Batched attestation delay. | ||
| pub const QUEUED_BATCH_ATTESTATION_DELAY: Duration = Duration::from_millis(50); |
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 must be a flag, and if set to 0 disable this mechanism. In case we observe propagation degradation have a way to turn it off. And upper bound the flag value to a reasonable value.
#8208
Current the implementation follows a naive approach of batching every 50ms, but I do have an idea for greedy batcher which I'll be implementing in following commits.