Skip to content

fix(rpc): give every slot its own root in the test state builders - #793

Merged
pablodeymo merged 1 commit into
mainfrom
fix/rpc-test-u8-overflow
Nov 12, 2026
Merged

pablodeymo merged 1 commit into
mainfrom
fix/rpc-test-u8-overflow

Conversation

@MegaRedHand

@MegaRedHand MegaRedHand commented Nov 12, 2026 •

Copy link
Copy Markdown
Collaborator

Motivation

Closes #786.

cargo test -p ethlambda-rpc --lib in the default (debug) profile fails 60 tests with attempt to add with overflow. Two test state builders fill block_roots with H256::repeat_byte(slot as u8 + 1), and the window is SLOTS_PER_HISTORICAL_ROOT long (8192 on the mainnet preset), so 255u8 + 1 overflows. CI runs under release-fast, where overflow-checks = false, so there it wraps silently instead. That still undoes what the builders meant: past slot 255 the "distinct roots per slot" repeat.

The issue names ptc.rs:574, but validator.rs's fulu_state() has the same loop. The failures are the ptc and validator tests that build a state through either one.

Changes

  • test_utils::slot_root(slot) in crates/net/rpc/src/lib.rs writes slot + 1 big-endian into a root's last eight bytes, so every slot gets its own root and none is the zero hash.
  • ptc.rs: gloas_state() fills block_roots with it, and the PTC-duties test's expected dependent_root uses it too. That assertion recomputed the old formula, so changing only the builder would have broken it.
  • validator.rs: fulu_state() fills block_roots with it. Its assertions already read the root back through get_block_root_at_slot, so they need no change.

Test code only; no production code changes.

Verification

  • cargo test -p ethlambda-rpc --lib (debug): 335 passed, 0 failed (60 failed before this change)
  • cargo test -p ethlambda-rpc --lib --profile release-fast (what CI runs): 335 passed, 0 failed
  • make fmt, make lint

Not in this PR

The issue also suggests running unit tests with overflow checks in CI. release-fast sets overflow-checks = false on purpose (see the comment in Cargo.toml), so arithmetic matches release. Changing that is a separate decision and is left out here.

gloas_state() in ptc.rs and fulu_state() in validator.rs filled
block_roots with H256::repeat_byte(slot as u8 + 1). The window is
SLOTS_PER_HISTORICAL_ROOT long, so 255u8 + 1 overflowed: 60 ethlambda-rpc
tests panicked in debug builds, and under release-fast (overflow checks
off) the roots wrapped and repeated after slot 255.

Both builders now use test_utils::slot_root, which writes slot + 1
big-endian into a root's last eight bytes, and the PTC-duties test's
expected dependent_root uses it too.

Closes #786
@github-actions

Copy link
Copy Markdown

🤖 Grok Code Review

I'll review the test helper and its call sites for correctness, especially the historical-root overflow the comment describes.


Automated review by Grok (xAI) · grok-4.7 · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

Review: PR 793, slot_root test helper

This is a test-only change, and it fixes a real latent bug. The old H256::repeat_byte(slot as u8 + 1) failed once the block_roots window passed 255 entries:

  • Debug builds: at slot 255, 255u8 + 1 overflows and panics.
  • Release builds: the roots wrap. Slot 255 gets the zero hash, and slot 256 collides with slot 0. That weakens the "root read from the wrong slot shows up" property the tests depend on.

The new helper is correct. slot + 1 written big-endian into bytes 24..32 gives a distinct, non-zero root for every slot up to u64::MAX - 1, which is far beyond SLOTS_PER_HISTORICAL_ROOT.

Minor suggestions

  1. crates/net/rpc/src/lib.rs, slot_root. If H256 is the fixed-hash/ethereum-types type, the body can be one line with the same layout:

    pub(crate) fn slot_root(slot: u64) -> H256 {
        H256::from_low_u64_be(slot + 1)
    }

    The current version is fine if H256 is a local newtype without that constructor.

  2. Doc comment. The comment is accurate. You could shorten it to the invariant ("distinct, non-zero root per slot") plus a one-line reason not to use repeat_byte. The release-mode detail doesn't need to live in the code.

  3. Other call sites. Run rg 'repeat_byte\(.*as u8' across the workspace to catch any other per-slot or per-index root fillers with the same wraparound, for example in the state-transition or fork-choice test suites. Those may not live in the rpc crate's test_utils. If they exist, consider moving this helper into a shared test-utils crate.

  4. ptc.rs around line 742. slot_root(dependent_slot) requires dependent_slot: u64. If that is a Slot newtype rather than a u64 alias, this won't compile. CI will catch it, so I'm only flagging it.

No correctness, security, or performance concerns. LGTM.


Automated review by Claude · claude-opus-5-5 · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Gemini Code Review

Looks good to me. Replacing H256::repeat_byte(slot as u8 + 1) with slot_root(slot) cleanly avoids the u8 overflow at slot 255 when SLOTS_PER_HISTORICAL_ROOT >= 256 (e.g., under the mainnet preset) while preserving distinct non-zero roots per slot.


Automated review by Gemini · gemini-4-argon · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

No findings. This test-only change fixes debug overflow at slot 255 and provides distinct, nonzero roots across the historical window. No production or consensus behavior changes.

Tests could not run because rustup attempted to write to a read-only directory.


Automated review by OpenAI Codex · gpt-6.1-sol · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

No issues found. The new slot_root helper in crates/net/rpc/src/lib.rs correctly avoids the u8 truncation, debug overflow at slot 255, and repeating roots in release builds. Both test setups and the dependent_root expectation were updated consistently.

Optional nit: the “never the zero hash” documentation technically excludes only u64::MAX through debug overflow or release wrapping in slot + 1. That is unreachable for realistic beacon-state windows and not worth complicating this test helper.

Looks good to merge.


Automated review by Kimi (Moonshot AI) · kimi-k3.1-pro · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 DeepSeek Code Review

This PR is test-only and fixes a real test-hygiene bug: H256::repeat_byte(slot as u8 + 1) would panic in debug at slot 255 and repeat roots afterward. The new slot_root helper correctly gives distinct roots for the historical-root window.

Minor issue:

  • In crates/net/rpc/src/lib.rs:755-765, slot_root does slot + 1, which can overflow at u64::MAX (panic in debug, wrap to zero in release), contradicting the “never the zero hash” comment. Current callers only pass small slot values, so this is not blocking, but a hardening tweak would be:
    let v = slot.checked_add(1).expect("slot_root: slot must not be u64::MAX");
    root[24..].copy_from_slice(&v.to_be_bytes());
    or use a clear debug assert.

Optional style nit:

  • H256(root) is fine if the tuple field is public; if H256 implements From<[u8; 32]>, H256::from(root) would be slightly more idiomatic.

The replacements in crates/net/rpc/src/beacon/ptc.rs and crates/net/rpc/src/beacon/validator.rs are consistent and remove the old u8 truncation bug. No production code or consensus logic is affected.


Automated review by DeepSeek · deepseek-v4-pro · custom prompt

@pablodeymo
pablodeymo added this pull request to the merge queue Nov 12, 2026
Merged via the queue into main with commit 881d6ce Nov 12, 2026
22 checks passed
@pablodeymo
pablodeymo deleted the fix/rpc-test-u8-overflow branch November 12, 2026 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ethlambda-rpc tests fail in debug builds: u8 overflow in ptc.rs test state builder

2 participants