refactor: type-guard recently-executed block provenance (leader epoch/round/digest)

[procdump]

The problem

The consensus bus's recently-executed-blocks channel handed out raw SealedHeaders. The epoch/round packed in a block's nonce are the committed sub-DAG leader's values (the output that ordered it), not the execution frontier — a block drained from a parked, out-of-order batch lands as the newest height while carrying an old leader round/epoch. Reading round/epoch off the tip silently regresses the frontier (this once wedged the proposer throttle). The old name (RecentBlocks) plus bare u32/B256 returns made this easy to stumble into and impossible for the compiler to catch.

What's changed

• Renamed RecentBlocks/RecentBlock → RecentlyExecutedBlocks/RecentlyExecutedBlock (+ accessor, field, module) to signal execution-arrival recency, not round recency.
• latest_block()/block_at_number() now return the RecentlyExecutedBlock wrapper; no Deref — safe fields (number/hash/num_hash) are delegated explicitly, and the raw header is reachable only via as_header()/into_header() (a greppable, deliberate exit).
• Provenance is exposed only through distinct newtypes — SubDagLeaderEpoch, SubDagLeaderRound, SubDagConsensusDigest — so comparing them against a frontier value fails to compile; unwrap via .get() deliberately.
• Extracted the (epoch<<32 | round) codec into one canonical home, rayls_infrastructure_types::nonce (pack_nonce/unpack_nonce + tests), and routed every previously hand-inlined encode/decode site through it.

No behavioral change — renames, one shared codec, compile-time-typed provenance.

[github-actions]

Claude finished @procdump's task in 3m 14s —— View job

---

PR Review

• Gather context and read changed files
• Review core new types (recently_executed_blocks.rs, nonce.rs)
• Review consensus bus changes
• Review call-sites (epoch manager, bridge, proposer)
• Summarize findings

---

Summary

This is a well-motivated refactoring that fixes a real production bug (proposer throttle wedge) through compile-time enforcement. The type design is sound, the canonical nonce codec is a clear improvement, and the documentation is unusually thorough. The PR introduces no behavioral change.

A handful of observations below — one minor correctness concern, a few design gaps, and some test coverage gaps.

---

Observations

1. block_capacity() returns u64 but capacity is semantically usize

File: crates/consensus/primary/src/recently_executed_blocks.rs:184

pub fn block_capacity(&self) -> u64 {
    self.num_blocks as u64  // internal type is usize
}

Then in consensus_bus.rs:reset_for_epoch (L325–330):

let recently_executed_blocks = self.tx_recently_executed_blocks.borrow().block_capacity();
// ... later:
RecentlyExecutedBlocks::new(recently_executed_blocks as usize)

The round-trip usize → u64 → usize is harmless in practice but semantically odd — a capacity is usize. The only reason to return u64 seems to be to match the watch channel's borrow type, but block_capacity is meant to be used as a size, not a number. Returning usize avoids the cast pair and is more idiomatic. Fix this →

---

2. reset_for_epoch pushes a default block when the window is empty

File: crates/consensus/primary/src/consensus_bus.rs:326–331

let latest = self.tx_recently_executed_blocks.borrow().latest_block();
let mut recently_executed_blocks =
    RecentlyExecutedBlocks::new(recently_executed_blocks as usize);
recently_executed_blocks.push_latest(latest.into_header());  // always pushes

When recently_executed_blocks is empty on a fresh genesis start, latest_block() returns a RecentlyExecutedBlock wrapping a default-constructed SealedHeader (nonce=0, hash=zero, number=0), and that synthetic block is pushed into the next epoch's window. This was the same behavior before this PR (the pre-existing unwrap_or_else(Default::default) in latest_block()), but into_header() now makes it a deliberate escape. A guard would make the intent explicit:

if !self.tx_recently_executed_blocks.borrow().is_empty() {
    recently_executed_blocks.push_latest(latest.into_header());
}

Fix this →

---

3. SubDagConsensusDigest derives PartialEq/Eq despite the "don't compare" doc

File: crates/consensus/primary/src/recently_executed_blocks.rs:62

#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
pub struct SubDagConsensusDigest(B256);

The doc says "use .get() only to look the header up, not to compare against a frontier digest." But the derived PartialEq allows anchor_digest == tip_digest directly, with no .get() unwrap required — two SubDagConsensusDigest values compare freely. Unlike SubDagLeaderEpoch vs SubDagLeaderRound (cross-type comparison is already blocked), this one relies entirely on documentation discipline.

For SubDagLeaderEpoch and SubDagLeaderRound, the PartialEq is intentional (same-dimension equality is fine: leader_epoch == leader_epoch). The question is whether two SubDagConsensusDigest values from different sources (e.g., an anchor's digest vs. the current tip's digest) should compare freely. If the design intent is "only use .get() to pass to a lookup", dropping PartialEq/Eq would enforce that. If two SubDagConsensusDigest values legitimately need equality (e.g., dedup), keep them — but then the doc's "never an equality check" becomes too broad.

---

4. latest_block() doesn't document its default-on-empty behavior

File: crates/consensus/primary/src/recently_executed_blocks.rs:236

latest_block_num_hash() (L202) documents: "This will return a default BlockNumHash if recent blocks are empty." But latest_block() (L236) returns a RecentlyExecutedBlock wrapping a default SealedHeader on an empty window without any mention of this. A caller reading latest_block().number() on an empty window gets 0 without indication that the value is synthetic. The doc should mirror latest_block_num_hash()'s wording. Fix this →

---

5. Missing unit test for RecentlyExecutedBlock epoch/round extraction

The nonce.rs roundtrip tests are thorough. But there are no tests in recently_executed_blocks.rs that exercise the full chain: build a SealedHeader with a specific nonce via pack_nonce, push it through push_latest, call latest_block(), and assert subdag_leader_epoch().get() / subdag_leader_round().get(). Given the previous prod bug, a test like this is warranted:

#[test]
fn subdag_leader_fields_decoded_from_nonce() {
    use rayls_infrastructure_types::nonce::pack_nonce;
    let mut blocks = RecentlyExecutedBlocks::new(10);
    let mut header = SealedHeader::default();
    header.nonce = pack_nonce(7, 498).into();  // adjust to the correct nonce type
    blocks.push_latest(header);
    let tip = blocks.latest_block();
    assert_eq!(tip.subdag_leader_epoch().get(), 7);
    assert_eq!(tip.subdag_leader_round().get(), 498);
}

Fix this →

---

6. SubDagLeaderEpoch/SubDagLeaderRound live in consensus-primary, not infrastructure-types

The canonical codec (pack_nonce/unpack_nonce) is correctly placed in rayls_infrastructure_types::nonce. But SubDagLeaderEpoch/SubDagLeaderRound are in rayls_consensus_primary, which creates an asymmetry: the execution crate (execution/evm) can use the codec but can't use the typed wrappers without a circular dependency. So execution.rs:deconstruct_nonce (L534) still returns (u32, u32):

pub fn deconstruct_nonce(nonce: u64) -> (u32, u32) {
    rayls_infrastructure_types::nonce::unpack_nonce(nonce)
}

This is an architectural trade-off, not a bug. The compile-time protection applies at the RecentlyExecutedBlock API boundary (where the misuse historically occurred), which is exactly where it's needed. Just noting the gap in case the team considers moving the wrappers to infrastructure-types in a follow-up.

---

Positive Notes

• executed_anchor as SSOT for frontier epoch/round — The proposer throttle (execution_lag()) correctly reads executed_anchor, not recently_executed_blocks().latest_block(). This directly fixes the production wedge.
• No Deref on RecentlyExecutedBlock — the raw SealedHeader is accessible only via the named as_header()/into_header() exits, which are grep-auditable.
• into_header() used correctly — reset_for_epoch and epoch_manager/state.rs are the only two call sites; both genuinely need the full header.
• nonce.rs tests — roundtrips and layout_is_epoch_high_round_low pin both the codec behavior and the bit layout, making future regressions obvious.
• Documentation — The warning in latest_block() is one of the best inline "incident report" comments I've seen in production Rust: specific example, exact numbers, named consequence.

[github-actions]

Claude finished @procdump's task in 1m 31s —— View job

---

PR Review

• Check recent commits since last review
• Read changed files
• Review observations from prior review — verify which were addressed
• Summarize findings

---

The branch now has a follow-up commit (a08c9ac) that addressed three of the six observations from the previous review. Here's the current state of each:

---

Addressed ✓

Observation #3 — SubDagConsensusDigest derived PartialEq/Eq despite the "don't compare" doc

Fixed. SubDagConsensusDigest now only derives Copy, Clone, Debug, removing PartialEq/Eq/Hash. The updated doc explains the intent explicitly:

It intentionally does NOT derive PartialEq/Eq: a frontier digest is a bare B256,
so that comparison already fails to compile, and there is no legitimate reason to
equality-check two of these against each other either.

This is the right call — the enforcement is now in the type, not the documentation.

---

Observation #4 — latest_block() didn't document its default-on-empty behavior

Fixed. The doc now mirrors latest_block_num_hash() and is actually more complete, explicitly directing callers to is_empty():

On an empty window this returns a RecentlyExecutedBlock wrapping a default SealedHeader
(number 0, zero hash, nonce 0) rather than None — same convention as latest_block_num_hash.
Call is_empty() first if a synthetic zero block would be mistaken for real execution history.

---

Observation #5 — Missing unit test for the epoch/round extraction chain

Fixed with three well-chosen tests:

• subdag_leader_fields_decoded_from_tip_nonce — the golden path through pack_nonce → push_latest → latest_block() → .get()
• drained_old_batch_regresses_tip_leader_round — the regression test for the production bug itself; this is the most valuable one and documents the exact semantics that caused the wedge
• block_at_number_exposes_guarded_accessors — coverage for block_at_number

The header_with_nonce helper using SealedHeader::seal_slow means tests use real seal computation, not synthetic hashes.

---

Still Open

Observation #1 — block_capacity() returns u64 but capacity is semantically usize

Not addressed. The round-trip usize → u64 → usize remains at consensus_bus.rs:325–329:

// recently_executed_blocks.rs:188
pub fn block_capacity(&self) -> u64 {
    self.num_blocks as u64
}

// consensus_bus.rs:325–329
let recently_executed_blocks = self.tx_recently_executed_blocks.borrow().block_capacity();
// ...
RecentlyExecutedBlocks::new(recently_executed_blocks as usize)

Harmless in practice (a capacity fits in both), but returning usize would be more idiomatic and remove both casts. Fix this →

---

Observation #2 — reset_for_epoch pushes a default block when the window is empty

Not addressed. consensus_bus.rs:327–330 still unconditionally pushes latest.into_header() into the new epoch's window even on genesis start, where latest_block() returns a default SealedHeader with number=0, hash=zero, nonce=0:

let latest = self.tx_recently_executed_blocks.borrow().latest_block();
let mut recently_executed_blocks =
    RecentlyExecutedBlocks::new(recently_executed_blocks as usize);
recently_executed_blocks.push_latest(latest.into_header());  // always pushes

The commit a08c9ac added documentation to latest_block() (observation #4) that explicitly calls this out — but reset_for_epoch remains the call site that acts on a synthetic block without checking. A simple guard makes the intent explicit:

let latest = self.tx_recently_executed_blocks.borrow().latest_block();
let mut recently_executed_blocks =
    RecentlyExecutedBlocks::new(recently_executed_blocks as usize);
if !self.tx_recently_executed_blocks.borrow().is_empty() {
    recently_executed_blocks.push_latest(latest.into_header());
}

Fix this →

---

Assessment

The follow-up commit addressed the three highest-value observations: the PartialEq enforcement gap on SubDagConsensusDigest is now in the type system rather than the docs, the latest_block() default-empty behavior is documented, and the regression test suite covers the production bug path. The two remaining open items (#1 and #2) are both minor: observation #1 is cosmetic, and observation #2 is a pre-existing behavior made more visible by the new API (the prior code had the same unwrap_or_else(Default::default) path). Neither blocks merge.

[github-actions]

Claude finished @procdump's task in 1m 35s —— View job

---

PR Review

• Gather context and check new commits
• Read changed files since last review
• Review CodeQL findings
• Check remaining open observations (chore(deps)(deps): bump thin-vec from 0.2.14 to 0.2.18 #1 and chore(deps)(deps): bump rpassword from 7.4.0 to 7.5.0 #2)
• Summarize findings

---

The branch has two commits on top of main. The second commit (1292b47) addressed three of the six observations from the previous reviews (observations #3, #4, #5). Here is the current state of each tracked item, plus an assessment of the CodeQL alerts.

---

CodeQL Alerts — False Positives

The security scanner flagged crates/infrastructure/types/src/nonce.rs at lines 14, 34, and 43 with "Hard-coded cryptographic value used as a nonce." These are false positives caused by a naming collision.

The field called nonce here is the Ethereum execution header's nonce field (an EIP-3675 leftover, 8 bytes), which this codebase repurposes as a (epoch << 32 | round) storage slot. It is not a cryptographic nonce (IV, challenge, or session token). The hard-coded values in the tests ((7, 498), (u32::MAX, u32::MAX), etc.) are purely arithmetic fixtures for verifying the bit-packing layout — there is no security-sensitive use here. CodeQL cannot distinguish the two meanings of "nonce" and fires on both.

No action needed. If the alerts are noisy, a // lgtm[...] or CodeQL query exclusion can suppress them — but that is cosmetic, not correctness.

---

Addressed ✓ (since previous review)

Observation #3 — SubDagConsensusDigest derived PartialEq/Eq despite the "don't compare" doc

Fixed. SubDagConsensusDigest now only derives Copy, Clone, Debug. The type comment explicitly explains the intent:

It intentionally does NOT derive PartialEq/Eq: a frontier digest is a bare B256, so that comparison already fails to compile, and there is no legitimate reason to equality-check two of these against each other either.

Enforcement is now in the type system, not the documentation.

---

Observation #4 — latest_block() didn't document its default-on-empty behavior

Fixed. The doc at recently_executed_blocks.rs:241–244 now matches latest_block_num_hash() and adds an explicit is_empty() callout:

On an empty window this returns a RecentlyExecutedBlock wrapping a default SealedHeader (number 0, zero hash, nonce 0) rather than None — same convention as latest_block_num_hash. Call is_empty() first if a synthetic zero block would be mistaken for real execution history.

---

Observation #5 — Missing unit test for the epoch/round extraction chain

Fixed with three tests in recently_executed_blocks.rs:289–322:

• subdag_leader_fields_decoded_from_tip_nonce — golden path through pack_nonce → push_latest → latest_block() → .get()
• drained_old_batch_regresses_tip_leader_round — regression test for the production bug itself; documents the exact semantics that caused the wedge
• block_at_number_exposes_guarded_accessors — coverage for block_at_number

Using SealedHeader::seal_slow means these tests cover real seal computation, not synthetic hashes.

---

Still Open

Observation #1 — block_capacity() returns u64 but capacity is semantically usize

Not addressed. recently_executed_blocks.rs:188 still casts to u64:

pub fn block_capacity(&self) -> u64 {
    self.num_blocks as u64
}

And consensus_bus.rs:325–329 round-trips it back:

let recently_executed_blocks = self.tx_recently_executed_blocks.borrow().block_capacity();
// ...
RecentlyExecutedBlocks::new(recently_executed_blocks as usize)

The usize → u64 → usize pair is harmless in practice (a capacity fits comfortably in u64) but semantically odd. Returning usize removes both casts and is more idiomatic for a capacity. Minor — does not block merge. Fix this →

---

Observation #2 — reset_for_epoch unconditionally pushes a default block on genesis start

Not addressed. consensus_bus.rs:327–330 still does:

let latest = self.tx_recently_executed_blocks.borrow().latest_block();
let mut recently_executed_blocks =
    RecentlyExecutedBlocks::new(recently_executed_blocks as usize);
recently_executed_blocks.push_latest(latest.into_header());  // always pushes

On a fresh genesis start (empty window), latest_block() returns a default SealedHeader (number=0, hash=zero, nonce=0), and that synthetic header is pushed into the next epoch's window. The new latest_block() doc (observation #4) explicitly warns about this — but reset_for_epoch is the one call site that acts on a synthetic block without a guard. A simple is_empty() check would make the intent explicit:

let latest = self.tx_recently_executed_blocks.borrow().latest_block();
let mut recently_executed_blocks =
    RecentlyExecutedBlocks::new(recently_executed_blocks as usize);
if !self.tx_recently_executed_blocks.borrow().is_empty() {
    recently_executed_blocks.push_latest(latest.into_header());
}

Minor — this is pre-existing behavior made more visible by the new API, and the is_empty() guard is defensive hygiene rather than a correctness fix. Does not block merge. Fix this →

---

Assessment

The follow-up commit addressed all three high-value observations from the previous reviews. The CodeQL alerts are false positives with no security impact. The two remaining open items (#1 and #2) are both minor: #1 is a cosmetic type choice, and #2 is a pre-existing behavior that the new API makes more visible but does not change. Neither blocks merge. The PR is in good shape.

[github-actions]

Claude finished @procdump's task in 2m 9s —— View job

---

PR Review

• Gather context from previous reviews
• Read key changed files
• Check remaining open observations (chore(deps)(deps): bump thin-vec from 0.2.14 to 0.2.18 #1 and chore(deps)(deps): bump rpassword from 7.4.0 to 7.5.0 #2)
• Evaluate CodeQL findings on nonce.rs
• Check for any new issues
• Summarize findings

---

Current State

The previous two review passes left two open items (#1 and #2). Both are still present. One new area to address: the CodeQL alerts on nonce.rs.

---

Still Open

Observation #1 — block_capacity() returns u64 but capacity is semantically usize

recently_executed_blocks.rs:188:

pub fn block_capacity(&self) -> u64 {
    self.num_blocks as u64
}

There are now two callers that do the round-trip cast:

consensus_bus.rs:325–329:

let recently_executed_blocks = self.tx_recently_executed_blocks.borrow().block_capacity();
RecentlyExecutedBlocks::new(recently_executed_blocks as usize)

epoch_manager/state.rs:291–300:

let block_capacity = self.consensus_bus.recently_executed_blocks().borrow().block_capacity();
self.consensus_bus.recently_executed_blocks()
    .send_replace(RecentlyExecutedBlocks::new(block_capacity as usize));
let restored_blocks = engine.last_executed_output_blocks(block_capacity).await?;

Note the second caller passes block_capacity as u64 to last_executed_output_blocks — which is why it was likely typed u64 in the first place. If last_executed_output_blocks takes a usize, changing the return type to usize and fixing both callers would be cleaner. If it takes a u64, the typing is intentional at that boundary and this observation is NAB.

Fix this →

---

Observation #2 — reset_for_epoch unconditionally pushes a default block on genesis start

consensus_bus.rs:325–331:

let recently_executed_blocks = self.tx_recently_executed_blocks.borrow().block_capacity();
let latest = self.tx_recently_executed_blocks.borrow().latest_block();
let mut recently_executed_blocks =
    RecentlyExecutedBlocks::new(recently_executed_blocks as usize);
recently_executed_blocks.push_latest(latest.into_header());  // always pushes

On a fresh genesis start the window is empty, latest_block() returns a default-zero SealedHeader, and that synthetic block is pushed into the new window. The latest_block() doc (fixed in the follow-up commit) now says explicitly "Call is_empty() first if a synthetic zero block would be mistaken for real execution history" — but this call site doesn't check. The fix is one line:

if !self.tx_recently_executed_blocks.borrow().is_empty() {
    recently_executed_blocks.push_latest(latest.into_header());
}

Note: epoch_manager/state.rs:314 already does the right thing — it checks is_empty() before logging and does not push a default block. The inconsistency between the two reset paths is the residual gap.

Fix this →

---

New: CodeQL "Hard-coded cryptographic value" alerts on nonce.rs

The scanner flagged multiple lines in nonce.rs — all inside #[cfg(test)] — as containing hardcoded cryptographic nonces:

nonce.rs:14   (7, 498)     — in roundtrips test
nonce.rs:34   pack_nonce(1, 0), pack_nonce(0, 1)   — in layout_is_epoch_high_round_low test
nonce.rs:43   (3u64 << 32) | 200   — same test

These are false positives. The Ethereum block header nonce field is a repurposed 8-byte field encoding (epoch << 32) | round — not a cryptographic nonce in the security sense. The constants are test vectors, not production randomness, and they live entirely inside #[cfg(test)]. No security issue exists here.

The alerts can be dismissed in the GitHub Security tab. Alternatively, add a top-level CodeQL paths-ignore or inline # lgtm[...] suppression comment if the scanner keeps flagging test vectors due to the file/function name collision with "nonce" in the cryptographic sense.

---

Assessment

The PR is in good shape. The three highest-value observations from earlier reviews were fixed:

• SubDagConsensusDigest no longer derives PartialEq/Eq (type-system enforcement, not just docs)
• latest_block() is documented for its default-on-empty behavior
• The production-bug regression test is in place

The two remaining items (#1 and #2) are both minor. Observation #1 may be intentional at the last_executed_output_blocks boundary. Observation #2 is a pre-existing behavior made more visible by the new API; it's low risk (the zero block from genesis reset wouldn't regress a running node's epoch/round) but inconsistent with how epoch_manager/state.rs handles the same case. Neither blocks merge.