[MegaRedHand]

🗒️ Description / Motivation

Fixes from testing the v1.0.0 release (ethlambda beacon on mainnet, Docker and the release binary). Four commits, one per finding:

Finding	Fix
docker stop force-kills after 10 s, but a clean shutdown during catch-up took 14.3 s, so the state was lost	README: --stop-timeout 60 on both docker run lines
The release binary's --version says -rc.2- while the image says -stable-, and only RELEASE.md explains why	README: one note next to the binary instructions, linking RELEASE.md#version-string (the release notes link this README)
Every clean Ctrl+C logged ERROR ... P2P networking is no longer functional	The swarm loop's exit is logged at info when the P2P actor shut down; error only when the swarm itself ends
/eth/v1/node/identity metadata was a placeholder (seq 0, no subnets), disagreeing with the ENR's attnets	The endpoint reports the live MetaData peers get over req/resp
/eth/v1/beacon/headers/head returned canonical: false while the head was still the checkpoint anchor	The beacon bootstrap indexes the anchor's slot in BlockRoots, as lean's init_store already does

Not fixed here: is_syncing and /eth/v1/node/health misreport during catch-up (health 200 while 80+ slots behind). The cause is that the beacon sync tracker's "freshest block seen" counts only imported blocks, so pending gossip blocks during catch-up look like a network stall. Changing that input has a security trade-off (leanSpec counts only signature-valid blocks so a peer cannot fake freshness), so this PR only widens the README caveat.

What Changed

• README.md: --stop-timeout 60, the version-string note, and a wider is_syncing/health caveat.
• crates/net/p2p/src/swarm_adapter.rs: swarm_loop breaks with a LoopExit reason. A channel closed by the P2P actor is a graceful shutdown (info). An actor panic looks the same from here, but spawned-concurrency logs it as an error before stopping the actor.
• Identity MetaData:
  • ethlambda-network-api gains MetaDataController/AdvertisedMetaData, a shared handle in the SyncStatusController mould.
  • BeaconWire publishes to it at startup and on every sequence-number bump, all of which now go through BeaconWire::bump_metadata_seq_number (fork digest switch, sync committee subnet join and leave).
  • P2P::local_metadata() hands it to main.rs, which puts it in BeaconIdentity; get_identity reads it per request.
• Anchor index:
  • Store::index_beacon_anchor writes the anchor's BlockRoots row, keyed by the anchor block's slot (a checkpoint state sits after its latest block when the epoch's first slot was empty).
  • get_forkchoice_store calls it right after inserting the anchor block, and so does the RPC test fixture that mirrors it.
  • Docs in block_id.rs, docs/data_storage.md and docs/rpc.md updated.

Correctness / Behavior Guarantees

• metadata.seq_number is the MetaData counter, not the ENR's seq: the p2p specification counts the two separately, so they can still differ. attnets now matches the ENR.
• With the anchor indexed, BeaconBlocksByRange serves the anchor block too (BlocksByRoot already did). Its data columns were never fetched, so a columns-by-range answer still has none for that slot.
• Directories bootstrapped before this change have no anchor row. Slot lookups there keep falling back to anchored_root_at_slot (the head, justified and finalized roots). No DB_VERSION bump.
• Lean is unaffected: the anchor index is beacon-only and lean has no MetaData.

Tests Added / Run

• canonical_is_checked_against_the_index now expects the anchor to be true (it pinned the old gap) and adds a non-canonical sibling that must be false.
• New: index_beacon_anchor_keys_the_row_by_the_blocks_slot (storage), the_beacon_api_copy_follows_every_change (p2p), identity_reports_the_metadata_peers_are_told (rpc).
• cargo test --profile release-fast --lib for ethlambda-storage, -rpc, -p2p, -network-api, -state-transition, -blockchain: all pass. leanSpec fixture tests not run.

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran make test (test-consensus plus test-node, at release-fast) — only the --lib targets above

[github-actions]

🤖 Claude Code Review

Review of PR 727

I read the diff and did not build or run it. The changes are small, well scoped and well documented, and I found no blocking issues.

Anchor index (Store::index_beacon_anchor)

• The row is keyed by the block's own slot, read back from the block row. This is correct, and the test pins the empty-first-slot checkpoint case (block at slot 30, state at slot 32).
• The call sits right after insert_signed_block in get_forkchoice_store, so the block_entry lookup can't miss.
• Calling it again on any re-entry just rewrites the same row, so it is idempotent.
• The change is consensus-neutral. It only adds a BlockRoots row that update_checkpoints never wrote, and canonical_root_at_slot now answers for the anchor.
• BeaconBlocksByRange now serves the anchor block. That is reasonable, and the PR description says so.
• Old directories without the row keep the anchored_root_at_slot fallback, so skipping a DB_VERSION bump is fine.
• Nit: index_beacon_anchor takes &mut self but only writes through the backend. If the other writers use &mut self too, this matches them.
• The new canonical_is_checked_against_the_index test now covers both the true and false cases.

Identity metadata

• MetaDataController follows the SyncStatusController pattern. Routing every metadata_seq_number += 1 through bump_metadata_seq_number keeps the published copy from drifting. I confirmed that all three bump sites changed: fork-digest switch, sync-committee join and leave.
• build_swarm publishes at startup, so the first read isn't a default zero.
• advertised_metadata is built from the same attnets/syncnets helpers that build_metadata uses, so the two can't disagree. The test asserts this.
• std::sync::RwLock is fine here. The critical sections are a clone or a write with no .await, so there is no async-lock concern. The expect on poisoning is acceptable.
• Nit: BeaconWire::metadata_seq_number is still a pub field, so a future += 1 could bypass the bump helper. The doc comment points at the helper, which is probably enough, but a debug assertion or a private field would enforce it.
• The None fallback in get_identity still returns the placeholder zeros. That is acceptable when there is no beacon p2p layer.
• Hex encoding of the SSZ bytes matches the Beacon API format. attnets is 8 bytes and syncnets is 1 byte, as in the test.

Swarm loop exit logging

• LoopExit cleanly separates the routine actor-shutdown exit from the swarm ending. The reasoning in the doc comment holds.
• Treating a closed event_tx as ActorStopped is a slight assumption, but the doc comment states it.

README and docs

• --stop-timeout 60, the version-string note and the widened is_syncing/health caveat are accurate and helpful. The docs for data_storage.md, rpc.md and block_id.rs are updated consistently.

Testing gap

• The checklist says only the --lib targets ran, and the full make test did not. Nothing here touches consensus logic, but let CI cover test-node and test-consensus before merging.

Verdict: looks good to merge once CI is green.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR focusing on the areas you specified. This is a substantial change touching P2P metadata handling, fork choice indexing, and RPC endpoints.

Overall Assessment

The PR introduces live MetaData reporting for the Beacon API's /eth/v1/node/identity endpoint and fixes an indexing gap for beacon anchors in BlockRoots. The architecture is sound, but there are several issues to address.

---

Critical Issues

1. Potential Deadlock in MetaDataController (crates/net/api/src/lib.rs)

Lines 471-513: MetaDataController uses std::sync::RwLock with .write().expect() and .read().expect(). While the operations are brief, this introduces a poisoning risk. More critically, if any caller holds the read lock and attempts to acquire a write lock (or vice versa in a nested call), this will deadlock.

Specific concern: The get() method clones the entire AdvertisedMetaData under the lock. The attnets and syncnets fields are Vec<u8> — small, but this allocates while holding the lock.

Recommendation: Consider using parking_lot::RwLock (already common in this codebase's dependencies) which doesn't poison and has better performance characteristics. At minimum, document the lock ordering if this grows more complex.

// Current (lines 505-511):
pub fn get(&self) -> AdvertisedMetaData {
    self.metadata
        .read()
        .expect("metadata lock poisoned")
        .clone()
}

2. BeaconIdentity Default Behavior Change (crates/net/rpc/src/lib.rs)

Lines 202-210: Adding metadata: Option<MetaDataController> to BeaconIdentity with ..Default::default() in beacon_identity() (bin/ethlambda/src/main.rs:145-152) means existing code paths constructing BeaconIdentity directly will have metadata: None.

Line 148 in bin/ethlambda/src/main.rs: The beacon_identity function now returns metadata: None, then the caller overrides it. This split construction is fragile — if someone calls beacon_identity directly elsewhere, they'll get stale metadata.

Recommendation: Either make beacon_identity take the MetaDataController as a parameter, or document that this function returns an incomplete struct.

---

Security & Consensus Concerns

3. Fork Choice Anchor Indexing (crates/blockchain/state_transition/src/beacon/fork_choice.rs)

Lines 1282-1288: The new index_beacon_anchor call is correct for fixing the missing BlockRoots entry, but verify this doesn't interact dangerously with repair_head or state regeneration paths.

Question: What happens if get_forkchoice_store is called on a database that already has a head moved past the anchor? The comment says "The anchor heads the canonical chain" — is this always true for recovery scenarios?

4. index_beacon_anchor Slot Keying (crates/storage/src/store.rs)

Lines 3142-3166: The function reads the block's slot from storage, then writes to BlockRoots. However, block_entry returns (slot, parent_root) — verify that block_entry doesn't itself depend on BlockRoots being populated.

Looking at the implementation: block_entry reads Table::SignedBeaconBlocks directly, so this is safe. ✓

Edge case: The error type UnexpectedMissingBlockHeader(root) at line 3154 — if this panics in a production path, it's a liveness failure. Ensure this is only called when the anchor block is known to exist.

---

Correctness Issues

5. Test Fixture Sibling Block Construction (crates/net/rpc/src/beacon/headers.rs)

Lines 155-168: The test modifies sibling.message.proposer_index = 1 to create a distinct block hash. This is correct but fragile — if phase0_beacon_block ever starts using proposer_index in its hash, this breaks.

Line 157-160: The unreachable! pattern for SignedBeaconBlock::Phase0 — this will panic if the helper is extended. Use assert_matches! or handle the enum properly.

6. LoopExit Logging Asymmetry (crates/net/p2p/src/swarm_adapter.rs)

Lines 301-320: The ActorStopped case logs at info! level, but per the comment, this could mask a panic. Consider warn! if the actor didn't shut down gracefully — though detecting this requires additional machinery.

More importantly: the SwarmEnded case logs error! but doesn't propagate or trigger any recovery. Is P2P permanently down at this point? Should this signal the main task?

---

Performance & Rust Idioms

7. Unnecessary Clone in local_metadata() (crates/net/p2p/src/lib.rs)

Line 1301-1303:

pub fn local_metadata(&self) -> Option<MetaDataController> {
    self.local_metadata.clone()
}

MetaDataController already contains an Arc<RwLock<...>>, so this clones the Arc (cheap). But the return type forces allocation of the Option. Consider Option<&MetaDataController> if callers can borrow, or verify this is only called infrequently.

8. MetaDataController in BeaconWire (crates/net/p2p/src/beacon/mod.rs)

Lines 90-94: BeaconWire is a large struct already, and MetaDataController adds another Arc. This is fine, but note that BeaconWire derives no explicit Clone — verify that the new field doesn't break any derived impls.

Looking at the struct: no #[derive(Clone)], so this is manual. ✓

9. publish_metadata Called After bump_metadata_seq_number (crates/net/p2p/src/beacon/mod.rs)

Lines 104-108:

pub fn bump_metadata_seq_number(&mut self) {
    self.metadata_seq_number += 1;
    self.publish_metadata();
}

This is correct, but note that publish_metadata takes &self while bump_metadata_seq_number takes &mut self. The &mut ensures exclusive access for the sequence number bump, then the &self call to publish_metadata re-borrows immutably. This is fine but slightly unusual — consider inlining or documenting why the split.

---

Documentation & Maintainability

10. Comment/Implementation Drift in get_identity (crates/net/rpc/src/beacon/node.rs)

Lines 105-110: The doc comment says metadata is "read at request time" and "the p2p specification counts the two separately" (referring to seq_number vs ENR seq).

Line 118: The implementation uses metadata.seq_number.to_string() — verify this matches the spec's string encoding. Ethereum consensus specs typically use decimal strings for seq_number in JSON, which this does. ✓

Lines 122-126: The None branch hardcodes attnets: "0x0000000000000000" (8 bytes) and syncnets: "0x00" (1 byte). These are the Phase0 and Altair defaults respectively. For a lean chain or future fork, these sizes may be wrong — document this assumption or make it dynamic.

11. SSZ Encoding Assumption (crates/net/p2p/src/beacon/handler.rs)

Lines 153-160:

pub(crate) fn advertised_metadata(wire: &BeaconWire) -> AdvertisedMetaData {
    AdvertisedMetaData {
        seq_number: wire.metadata_seq_number,
        attnets: attnets(wire).to_ssz(),
        syncnets: syncnets(wire).to_ssz(),
    }
}

The attnets and syncnets bitfields are SSZ-encoded here, then hex-encoded in the RPC layer. Verify that to_ssz() for bitvectors produces the correct packed representation per the spec. The libssz crate's BitVector encoding should be correct, but this is consensus-critical.

---

Minor Issues

12. Default::default() in Test (crates/net/p2p/src/beacon/handler.rs)

Line 268: metadata: Default::default() in test construction. Prefer explicit MetaDataController::default() for clarity, or verify this is consistent with the codebase style.

13. local_metadata Naming (crates/net/p2p/src/lib.rs)

Lines 1135-1138, 1189, 1295-1303: The field is local_metadata but the type is MetaDataController (not MetaData). The naming is slightly inconsistent — metadata_controller or live_metadata would be clearer.

---

Positive Observations

1. The index_beacon_anchor fix is well-tested — the test at crates/storage/src/store.rs:6239-6286 explicitly verifies slot vs. state slot keying.

2. The LoopExit enum in swarm_adapter.rs (lines 323-330) is a good pattern for distinguishing graceful shutdown from errors.

3. The test in crates/net/rpc/src/beacon/node.rs lines 280-310 verifies live metadata updates end-to-end.

4. Documentation updates are thorough — README.md, data_storage.md, and rpc.md all reflect the changes.

---

Summary of Required Changes

Priority	File	Line	Issue
Medium	crates/net/api/src/lib.rs	471-513	Consider parking_lot::RwLock; document lock ordering
Medium	bin/ethlambda/src/main.rs	145-161	Refactor beacon_identity to avoid split construction
Low	crates/net/p2p/src/lib.rs	1301	Return Option<&MetaDataController> if possible
Low	crates/net/rpc/src/beacon/node.rs	122-126	Document hardcoded bitfield sizes or make dynamic
Low	crates/net/rpc/src/beacon/headers.rs	157	Replace unreachable! with proper enum handling

No critical security vulnerabilities found. The consensus changes (anchor indexing) appear correct and well-tested.

---

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

[github-actions]

🤖 Codex Code Review

Looks good overall. I did not spot a consensus-critical regression in fork choice, attestation handling, justification/finalization, state transition, XMSS, or SSZ logic from this diff. The two substantive changes both look correct and well-contained.

• crates/blockchain/state_transition/src/beacon/fork_choice.rs:1288

  • index_beacon_anchor(anchor_root) fixes a real correctness bug: the anchor was canonical but absent from BlockRoots, which could make /eth/v1/beacon/headers/... misreport canonical.
  • crates/storage/src/store.rs:3159 correctly keys the row by the anchor block’s slot, not the checkpoint state slot. That matters for empty epoch-boundary slots and avoids off-by-one-style canonicality errors.

• crates/net/p2p/src/beacon/mod.rs:104

  • Centralizing metadata updates in bump_metadata_seq_number() is a good safety improvement. It reduces the risk that future code increments metadata_seq_number without republishing the RPC-visible copy.
  • The initialization path also looks right: crates/net/p2p/src/lib.rs:1102 constructs the wire, then publish_metadata() seeds the shared copy before exposure.

• bin/ethlambda/src/main.rs:772

  • Plumbing live metadata into BeaconIdentity fixes the stale /eth/v1/node/identity behavior cleanly.
  • crates/net/rpc/src/beacon/node.rs:112 reads the controller at request time, which matches the intended semantics.

• crates/net/p2p/src/beacon/sync_committee.rs:153 and crates/net/p2p/src/beacon/transition.rs:225

  • Both call sites now use the centralized helper, which keeps syncnets and fork-digest-driven metadata changes consistent.

• crates/net/p2p/src/swarm_adapter.rs

  • Logging only SwarmEnded as an error is an improvement in signal/noise and does not affect behavior.

No obvious security issues jump out from the new code:

• No unsafe code added.
• No unbounded memory growth or attacker-controlled allocation introduced.
• The RwLock use is straightforward and scoped to small cloned metadata payloads.

Small note:

• I couldn’t run cargo test in this environment because rustup tried to write under a read-only home directory, so this is a source review rather than an executed validation.

---

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