Optimise fork choice attestation dequeueing #8378

michaelsproul · 2025-11-06T01:07:49Z

Proposed Changes

Optimise dequeuing of attestations in fork choice by avoiding reallocating the queue after every dequeue.

The included benchmarks shows that this takes ~35% off the runtime of dequeue_attestations over multiple slots:

dequeue_attestations/93750
                        time:   [10.603 ms 10.607 ms 10.613 ms]
                        change: [-34.468% -34.367% -34.274%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  4 (4.00%) high mild
  3 (3.00%) high severe

The baseline branch with just the benchmark added to unstable can be found here: https://github.com/michaelsproul/lighthouse/tree/dequeue-attestation-baseline-benchmark

Additional Info

I played around with several VecDeque-based implementations. The one with partition_point and rotate_left/split_off is substantially faster than using pop_front or drain.

The risk of this PR is that the allocation for queued_attestations is never shrunk/contracted, so in unusual circumstances it could grow very large. We could consider adding a hard cap on the number of items in the queue, as we've also had this queue grow in an unbounded way during times of memory corruption:

Expensive fork-choice queued attestation mutation #6206

michaelsproul · 2025-11-10T01:29:24Z

consensus/fork_choice/src/fork_choice.rs

        PersistedForkChoice {
            proto_array: self.proto_array().as_ssz_container(),
-            queued_attestations: self.queued_attestations().to_vec(),
+            queued_attestations: self.queued_attestations().iter().cloned().collect(),


This is potentially slower than if we implemented Encode for VecDeque. The conversion from VecDeque to Vec is non-trivial in most cases where the VecDeque doesn't start at index 0:

https://doc.rust-lang.org/std/collections/struct.VecDeque.html#impl-From%3CVecDeque%3CT,+A%3E%3E-for-Vec%3CT,+A%3E

I'll check fork choice persistence times to make sure this isn't having a substantial impact.

michaelsproul · 2025-11-10T01:30:08Z

consensus/fork_choice/src/fork_choice.rs

            fc_store,
            proto_array,
-            queued_attestations: persisted.queued_attestations,
+            queued_attestations: persisted.queued_attestations.into(),


This conversion Vec => VecDeque is trivial and cheap, but only happens once on startup anyway.

michaelsproul · 2025-11-10T01:40:18Z

consensus/fork_choice/benches/benches.rs

+use types::{Epoch, Hash256, Slot};
+
+fn all_benches(c: &mut Criterion) {
+    let num_attestations = 1_500_000_usize / 16;


This is ~2 slots worth of attestations on a 1.5M validator network

mergify · 2025-11-10T01:57:57Z

Some required checks have failed. Could you please take a look @michaelsproul? 🙏

Copilot

Pull Request Overview

This PR optimizes the attestation queue management in fork choice by replacing Vec<QueuedAttestation> with VecDeque<QueuedAttestation> and improving the dequeue_attestations algorithm to preserve allocation capacity and avoid unnecessary reallocations.

Key Changes

Replaced Vec with VecDeque for the queued attestations data structure across fork choice components
Optimized dequeue_attestations function using partition_point, rotate_left, and split_off to maintain capacity while removing old attestations
Made dequeue_attestations public and exported it for external use, including in benchmarks

Reviewed Changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
consensus/fork_choice/src/fork_choice.rs	Core implementation changes: converted queued_attestations to VecDeque, optimized dequeue logic with capacity preservation, made QueuedAttestation fields public, added Debug derive
consensus/fork_choice/src/lib.rs	Added public export of dequeue_attestations function
consensus/fork_choice/tests/tests.rs	Updated test helper function signature to use VecDeque reference
consensus/fork_choice/benches/benches.rs	Added comprehensive benchmark testing dequeue performance with realistic attestation volumes
consensus/fork_choice/Cargo.toml	Added criterion dev-dependency and benchmark harness configuration
Cargo.lock	Updated with criterion dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

consensus/fork_choice/src/fork_choice.rs

michaelsproul · 2025-11-10T02:50:19Z

dequeue_attestations is no longer visible in the profiling flamegraph after this change 🔥

mergify · 2025-11-10T03:17:35Z

Some required checks have failed. Could you please take a look @michaelsproul? 🙏

mergify · 2025-11-12T03:43:02Z

This pull request has merge conflicts. Could you please resolve them @michaelsproul? 🙏

dapplion · 2025-11-19T19:40:26Z

consensus/fork_choice/src/fork_choice.rs

+    let to_pop = queued_attestations.partition_point(|a| a.slot < current_slot);
+
+    // Rotate the entries to remove into the *end* of the vec deque.
+    queued_attestations.rotate_left(to_pop);


Why not change self.queued_attestations into a map of Slot -> Vec< QueuedAttestation> and when that slot arrives we just remove the entire vec? No need to rotate or split_off

Good idea, I'll try it and see how the benchmark looks

It will have the same problem of re-allocating memory for the Vecs though.

It's 4x slower 💀

dequeue_attestations/93750 time: [41.906 ms 41.928 ms 41.953 ms] Found 8 outliers among 100 measurements (8.00%) 5 (5.00%) high mild 3 (3.00%) high severe

Impl here: michaelsproul@daf3617.

One of the problems is that I didn't try optimising out the concatenation of Vecs. dequeue_attestations could definitely return like a Vec<Vec<_>> or something, but I didn't bother refactoring any further, because I think the lack of memory-reuse just makes it worse than the VecDeque.

michaelsproul · 2025-11-26T22:57:49Z

Putting this back to waiting-for-author so we can address the unbounded memory growth. Going to try the hashmap approach with pre-allocated Vecs

Optimise fork choice attestation dequeueing

95d06c9

michaelsproul added work-in-progress PR is a work-in-progress optimization Something to make Lighthouse run more efficiently. fork-choice labels Nov 6, 2025

michaelsproul added 4 commits November 6, 2025 16:50

Add benchmark

c797f33

Update benchmark

635611c

Update benchmark for vecdeque

2654fe6

Use partition_point

41944a4

michaelsproul commented Nov 10, 2025

View reviewed changes

michaelsproul added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Nov 10, 2025

michaelsproul added 2 commits November 10, 2025 12:57

Clippy

b5829aa

Improve comments

776a4ff

mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Nov 10, 2025

michaelsproul added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Nov 10, 2025

michaelsproul requested review from Copilot and dapplion November 10, 2025 02:16

Copilot AI reviewed Nov 10, 2025

View reviewed changes

consensus/fork_choice/src/fork_choice.rs Outdated Show resolved Hide resolved

Apply copilot review suggestion

1f6257a

mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Nov 10, 2025

michaelsproul added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Nov 10, 2025

mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Nov 12, 2025

Merge remote-tracking branch 'origin/unstable' into dequeue-optimisation

cc3d27e

michaelsproul added v8.1.0 Post-Fulu release ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Nov 12, 2025

dapplion reviewed Nov 19, 2025

View reviewed changes

Merge branch 'unstable' into dequeue-optimisation

4dfccf5

michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Nov 26, 2025

Optimise fork choice attestation dequeueing #8378

Are you sure you want to change the base?

Optimise fork choice attestation dequeueing #8378

Uh oh!

Conversation

michaelsproul commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Additional Info

Uh oh!

michaelsproul Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

michaelsproul Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

michaelsproul Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Nov 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Uh oh!

michaelsproul commented Nov 10, 2025

Uh oh!

mergify bot commented Nov 10, 2025

Uh oh!

mergify bot commented Nov 12, 2025

Uh oh!

dapplion Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

michaelsproul Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

michaelsproul Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

michaelsproul Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

michaelsproul commented Nov 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

michaelsproul commented Nov 6, 2025 •

edited

Loading