Optimise fork choice attestation dequeueing

[michaelsproul]

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]

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]

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

[michaelsproul]

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

[mergify]

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.

[michaelsproul]

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

[mergify]

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

[mergify]

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

[dapplion]

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

[michaelsproul]

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

[michaelsproul]

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

[michaelsproul]

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]

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

[mergify]

Hi @michaelsproul, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.

[mergify]

Hi @michaelsproul, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.