perf(blockchain): sign a node's attestations in parallel

[MegaRedHand]

🗒️ Description / Motivation

A node signs the attestations of all its validators at interval 1, but it signed them one after another. One XMSS signature takes about 22 ms, so on devnet-5 (32 validators per node, 8 s slots) a node published its first vote at 1.61 s into the slot and its last at about 2.3 s.

Aggregators pay for that tail. On aggregator 0, the first raw proof of a slot usually covered 96 of its subnet's 128 votes; the last node's 32 arrived while it ran, so they needed a second raw proof (done at ~3.9 s) and one more merge level before the subnet was whole. Measured over 120 slots, across all 32 nodes' logs:

first aggregate reaching	p10	p50	p90
128 participants	2.75 s	2.94 s	3.26 s
256	4.12 s	4.44 s	4.98 s
512	5.66 s	6.03 s	6.19 s
683 (2/3)	6.22 s	6.49 s	6.75 s

The next block is built at 6.40 s (interval 4), so in most slots the 2/3-wide aggregate did not exist yet: only 24 of 115 blocks carried their parent slot's votes at or above the threshold, and every target was justified one block late. Moving the base of the aggregation tree earlier moves every level above it.

The keys are independent and ValidatorSecretKey::sign takes &self (each key's cache sits behind its own lock), so the signatures can run concurrently.

What Changed

• crates/blockchain/src/key_manager.rs
  • KeyManager::sign_attestations(&self, validator_ids, data): signs with every listed attestation key, spread over at most one thread per core, and returns one result per id in input order.
  • for_each_batched (the scoped-thread batcher the key warm uses) becomes a thin wrapper over a new map_batched, which returns results in item order and takes the minimum batch size and thread name as arguments. The warm keeps its 4-key minimum batch; signing uses 1, since a signature costs far more than a thread spawn.
  • sign_with_attestation_key takes &self instead of &mut self.
• crates/blockchain/src/lib.rs: produce_attestations signs all of the node's attestations first, then self-delivers and publishes each in validator order, as before.

Correctness / Behavior Guarantees

• Same signatures, same messages, same publish order; only the signing runs concurrently. An unknown validator id still yields ValidatorKeyNotFound in its own slot of the result list.
• Signing a key concurrently with a background warm of the same key is unchanged: both take the key's cache lock, and a signature that races the warm reuses the rebuilt subtree.
• A panic on a signing thread is re-raised on the caller with resume_unwind, the same thing thread::scope did for the warm threads before. A thread that cannot be spawned runs its batch on the calling thread.
• Publication of the first vote now waits for the whole batch: about 4 signatures' worth on an 8-core host with 32 validators (~90 ms), against the ~700 ms the last vote used to wait.
• The new threads do no proving. leanVM's arena (--prover-arena) only serves ArenaVec allocations in the proving crates, and the xmss crate never uses it, so signing threads cannot claim arena slabs.

Tests Added / Run

• sign_attestations_signs_each_id_in_order: real (tiny) keys, ids in shuffled order plus an unknown id in the middle; each result verifies against that validator's public key, the unknown id reports ValidatorKeyNotFound.
• map_batched_returns_results_in_item_order: lengths 0-40, thread caps 0-64, minimum batches 0, 1 and 4.
• map_batched_with_unit_batches_uses_one_thread_per_item.
• The existing for_each_batched and warm tests pass unchanged.

cargo fmt --all --check
cargo test -p ethlambda-blockchain --lib --bins --profile release-fast   # 146 passed
cargo clippy --locked --workspace --all-targets --profile release-fast -- -D warnings
cargo clippy --locked --workspace --profile release-fast --test forkchoice_spectests --test signature_spectests --test stf_spectests --test ssz_spectests -- -D warnings

The same commit, stacked on #603, is rolling onto devnet-5 now as devnet5-leanvm-b-603-psign; I will post the before/after of the table above once it has run for a while.

Related Issues / PRs

• Related to refactor(blockchain): keep the aggregation worker running at all times #603 (the always-on aggregation worker; this change is independent of it)

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean (at release-fast)
• Ran make test (test-consensus plus test-node, at release-fast) — ran ethlambda-blockchain unit tests only

[github-actions]

🤖 Kimi Code Review

I'll review this PR focusing on the concurrent attestation signing implementation and its integration.

Overall Assessment

The PR introduces parallel XMSS attestation signing to reduce latency when multiple validators run on the same node. The core logic is sound, but there are several issues ranging from minor correctness concerns to potential performance pitfalls.

---

Detailed Findings

1. Potential for Excessive Thread Spawning (Performance)

File: crates/blockchain/src/key_manager.rs, Lines 330-335

let max_threads = thread::available_parallelism().map_or(1, NonZeroUsize::get);

available_parallelism() returns the total hardware threads, not idle capacity. On a 64-core server running multiple node services, spawning 64 CPU-bound XMSS signing threads can cause severe contention. Consider capping this or making it configurable.

Suggestion: Add a reasonable upper bound or configuration parameter:

const MAX_SIGNING_THREADS: usize = 16; // or configurable
let max_threads = thread::available_parallelism()
    .map_or(1, NonZeroUsize::get)
    .min(MAX_SIGNING_THREADS);

---

2. Thread Name Allocation on Every Call (Performance/Memory)

File: crates/blockchain/src/key_manager.rs, Lines 349-350

.name(thread_name.into())

thread_name.into() allocates a String from &str for every spawned thread, on every attestation slot. With 32 validators and 12-second slots, this is ~2.6M allocations/hour. Thread names are primarily for debugging; consider using a static name or only setting names in debug builds.

Suggestion: Use a 'static name or cfg!(debug_assertions) guard:

#[cfg(debug_assertions)]
let builder = thread::Builder::new().name("xmss-sign".into());
#[cfg(not(debug_assertions))]
let builder = thread::Builder::new();

---

3. unwrap_or_else with resume_unwind is Redundant

File: crates/blockchain/src/key_manager.rs, Lines 361-364

.join()
.unwrap_or_else(|panic| std::panic::resume_unwind(panic)),

std::panic::resume_unwind is exactly what JoinHandle::join().unwrap() does internally. This is unnecessarily verbose.

Suggestion: Replace with:

.join().unwrap(),

Or if you want explicit clarity:

.join().expect("xmss-sign thread panicked"),

---

4. sign_with_attestation_key Signature Change - &mut self to &self

File: crates/blockchain/src/key_manager.rs, Lines 212-222

The change from &mut self to &self is correct for concurrent access, but verify that XmssKeyPair::sign() is truly thread-safe. The PR doesn't show XmssKeyPair internals—ensure it uses interior mutability correctly (e.g., Mutex or atomic index advancement) and doesn't have data races on the WOTS+ chain state.

Action needed: Confirm XmssKeyPair::sign() documentation or implementation guarantees thread safety.

---

5. Test map_batched_with_unit_batches_uses_one_thread_per_item is Flaky

File: crates/blockchain/src/key_manager.rs, Lines 521-528

fn map_batched_with_unit_batches_uses_one_thread_per_item() {
    let items: Vec<usize> = (0..5).collect();
    let threads: HashSet<_> = map_batched(&items, 64, 1, "test", |_| thread::current().id())
        .into_iter()
        .collect();
    assert_eq!(threads.len(), items.len());
}

This test assumes each item gets its own thread, but thread::scope + spawn_scoped doesn't guarantee OS thread creation—Rust may reuse threads or the OS scheduler may behave unexpectedly. More critically, with min_batch: 1 and max_threads: 64, batch_len = 5/64 = 1 (with div_ceil), so 5 batches of 1—but the first batch runs on the calling thread, so at most 4 spawned threads. The assertion expects 5 threads.

Wait—re-checking: items.len() = 5, max_threads = 64, div_ceil(5, 64) = 1, max(1, 1) = 1. First batch: item 0 (calling thread). Remaining 4 items in 4 batches of 1. So expected threads: 1 calling + up to 4 spawned = 5 total. But if any spawn fails, it falls back to calling thread, reducing uniqueness. The test could fail under thread exhaustion.

Suggestion: Soften the assertion or document the assumption:

assert!(threads.len() >= 2, "Expected multiple threads, got {}", threads.len());
// Or check that at least some items ran on different threads

---

6. Missing Error Propagation in BlockChainServer

File: crates/blockchain/src/lib.rs, Lines 742-753

let signatures = self
    .key_manager
    .sign_attestations(&validator_ids, &attestation_data);
for (validator_id, signature) in validator_ids.into_iter().zip(signatures) {
    let Ok(signature) = signature.inspect_err(
        |err| error!(%slot, %validator_id, %err, "Failed to sign attestation"),
    ) else {
        continue;
    };

The continue silently drops failed signatures. For production monitoring, consider a metrics counter for signing failures to detect systemic issues (e.g., key corruption, XMSS state exhaustion).

Suggestion: Add a counter increment:

} else {
    metrics::counter!("ethlambda.attestation_sign_failed").increment(1);
    continue;
}

---

7. validator_ids Clone Efficiency

File: crates/blockchain/src/lib.rs, Lines 741-742

let validator_ids = self.key_manager.validator_ids();

If validator_ids() returns Vec<u64> or similar owned collection, this allocates. With frequent attestations, consider if this can return a borrowed slice or if the allocation matters.

---

8. map_batched Panic Safety on min_batch: 0

File: crates/blockchain/src/key_manager.rs, Lines 336-338

.max(min_batch.max(1));

Good catch with .max(1) to prevent zero-sized batches. However, the test at line 511 uses min_batch: 0 which exercises this path—verify this doesn't cause division by zero elsewhere. The div_ceil happens before .max(), so 0/64 = 0, then .max(0.max(1)) = 1. Correct.

---

Positive Observations

1. Correct result ordering: map_batched preserves item order despite concurrency—critical for matching signatures to validator IDs. The test at line 502 verifies this.

2. Graceful degradation: Failed thread spawns run inline rather than panicking, which is the right behavior for consensus code.

3. XMSS state safety: The &self change enables parallelism without external locking; assuming XmssKeyPair handles interior mutability correctly, this avoids lock contention.

4. Comprehensive tests: The parameterized test at line 502 covers edge cases (empty, single, various thread counts).

---

Summary Table

Priority	Item	Location	Action
Medium	Unbounded thread spawning	key_manager.rs:330	Cap max_threads
Low	Thread name allocation	key_manager.rs:349	Use static name or conditional
Low	Redundant resume_unwind	key_manager.rs:363	Simplify to .unwrap()
High	Verify XmssKeyPair thread safety	Not in diff	Code review/audit required
Low	Flaky test	key_manager.rs:521	Soften assertion
Low	Missing failure metrics	lib.rs:747	Add counter
Low	validator_ids allocation	lib.rs:741	Consider optimization

---

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

[github-actions]

🤖 Claude Code Review

Review: parallel attestation signing (PR 655)

The change is correct and well scoped. I found no blocking issues.

What I checked

• map_batched preserves input order. The calling thread takes the first batch, the rest are joined in spawn order, and the batches are contiguous chunks. Results therefore line up with validator_ids, and the zip in produce_attestations (lib.rs:746) is sound.
• The max_threads.max(1) and min_batch.max(1) guards rule out a zero-sized chunks panic. An empty input returns early.
• Spawn failure falls back to running that batch inline instead of panicking. Worker panics are re-raised with resume_unwind, which is the right behaviour for a consensus actor.
• Each key is signed by exactly one thread, so there is no contention beyond the per-key cache lock. That lock also makes a race with the background warm safe, as the doc comment describes.
• Signing is not proving, so the single leanvm-prover thread and arena rule in CLAUDE.md doesn't apply.
• Tests cover ordering, thread caps and unit batches.

Minor notes

• lib.rs:742: validator_ids() iterates a HashMap, so the order is arbitrary. This is harmless, because results are zipped back to the same vector. The publish order is nondeterministic, though, and was already so before this change.
• key_manager.rs:162: sign_attestation (singular, &mut self) now appears to have no non-test caller. Remove it, or fold it into sign_attestations, to avoid drift between the two paths.
• key_manager.rs:193: sign_attestations takes &self and so does sign_with_attestation_key. Keep sign_attestation's &mut self signature in mind if it is kept, because it is needlessly restrictive.
• Signing now uses up to one thread per core at interval 1. If the node also runs an aggregation worker or a warm thread at that moment, they will compete for CPU. The warm is called only once nothing is left to sign, so this is likely fine. The devnet latency numbers in the commit message are the real validation.
• produce_attestations is still synchronous on the actor, so the actor stays blocked for the longest batch. That is much shorter than before, and it is an acceptable trade-off.
• Hygiene: the commit snapshot includes unrelated files (.cargo/config.toml, .claude/settings.json, docs). Confirm these are not meant to be part of this PR's diff.

Overall, this looks good to merge.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Codex Code Review

Looks good overall. I don’t see a consensus-safety regression in fork choice, attestation validation, justification/finalization, state transition, XMSS verification, or SSZ handling from this patch; it is narrowly scoped to local attestation signing and preserves output ordering.

• crates/blockchain/src/key_manager.rs:182 — sign_attestations computes one hash_tree_root and signs per-validator in parallel, returning Vec<Result<...>> in input order. That preserves the validator→signature association, which is critical for correct attestation publication.
• crates/blockchain/src/key_manager.rs:211 — changing sign_with_attestation_key from &mut self to &self looks sound because keys are already stored behind Arc and signing/prepare comments indicate the underlying cache is internally synchronized. I don’t see an aliasing or memory-safety issue here.
• crates/blockchain/src/key_manager.rs:346 — map_batched preserves batch order by collecting spawned handles in iterator order and joining/extending in the same order. Good choice; this avoids subtle validator/signature misalignment bugs.
• crates/blockchain/src/key_manager.rs:355 — spawn failure falls back to inline execution rather than dropping work. That is the right failure mode for validator duties: latency degradation, not missed attestations due to an internal scheduling error.
• crates/blockchain/src/lib.rs:742 — validator_ids() is still the single source for iteration, and the subsequent zip(signatures) is safe because sign_attestations always returns exactly one result per input ID. No correctness concern there.

Minor note:

• crates/blockchain/src/key_manager.rs:189 — using available_parallelism() for attestation signing is reasonable, but if this actor ever shares a core-constrained environment with proving or networking, this may increase tail latency under load. Not a blocker, just something to watch in benchmarks.

Given the scope, this PR looks good to merge.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[MegaRedHand]

devnet-5 before/after

The same commit, stacked on #603 (perf/parallel-attestation-signing, image devnet5-leanvm-b-603-psign), was rolled onto devnet-5 (32 ethlambda nodes, 32 validators each, 8 subnets, 8 s slots), keeping each node's DB. The baseline is the #603 image on the same chain.

• before: 120 slots, 19:30-19:46Z
• after: 85 slots, from 20:44Z, once all 32 nodes were rolled

Both are measured from every node's logs, plus the central Prometheus for the finality gap.

Votes

	before	after
a node's last vote published (s into slot, p50)	~2.30	1.77
first raw aggregation job starts (p50)	2.25	1.79

Aggregation tree (first aggregate anywhere in the fleet reaching N participants, p50, s into slot)

N	before	after	Δ
128	2.94	2.32	−0.62
256	4.44	4.14	−0.30
512	6.03	5.68	−0.35
683 (2/3)	6.49	6.36	−0.13
1024	7.66	7.14	−0.52

The 2/3 crossing p10/p90 went from 6.22/6.75 s to 6.10/6.50 s. The next block is built at 6.40 s, so what matters is how often the crossing beats it:

	before	after
2/3 aggregate exists before the 6.40 s build	38/119 (32%)	60/85 (71%)
proposer holds a ≥683 aggregate at build start	26/120 (22%)	52/85 (61%)
block N carries ≥683 of slot N−1's votes	24/115 (21%)	50/83 (60%)
median slot N−1 votes in block N	608	736

Finality (head minus checkpoint, slots, sampled every slot)

	before p10 / p50 / p90	after p10 / p50 / p90
head − finalized	4 / 7 / 10	3 / 5 / 9
head − justified	2 / 3 / 5	2 / 3 / 4

Notes

• The base of the tree moved by 0.62 s, but the 256 level moved by only 0.30 s. A proof that finishes before interval 2 (3.2 s) is held until then before it is gossiped (publishes_immediately), and the 128-wide proofs now finish at ~2.3 s. Publishing those immediately is the next lever, and is out of scope here.
• The rest of the finality gap is mostly late blocks rather than aggregation. Proposers on the 8-core hosts take 2.2-5.5 s to build, so their blocks land after the 1.6 s attestation deadline; this change does not touch that.
• There were no restarts during or after the roll, and no ERROR lines.