fix: batch pending parent root fetches to avoid 300+ sequential round-trips

[ch4r10t33r]

Summary

• Adds EthLibp2p.state_mutex and locks it at the top of every export fn callback called from the Rust networking thread
• Adds BeamNode.state_mutex (a pointer to the same mutex, null in unit-tests) and locks it at the top of onInterval on the main libxev thread
• Batches pending parent-root fetches into a single blocks_by_root request to avoid sequential round-trips
• Fixes a missed flushPendingParentFetches in an early-return path of processBlockByRootChunk

Root cause of the integer overflow crash

The node was crashing with:

thread 1 panic: integer overflow
ssz/utils.zig  Bitlist.serializedSize
ssz/lib.zig    serialize
rocksdb.zig    putToBatch
node.zig       processCachedDescendants
node.zig       onInterval
io_uring.zig   run   ← main libxev thread

Two threads, one shared state, no synchronisation

create_and_run_network spawns a Rust/Tokio thread. Every network event — gossip messages, RPC responses, peer connect/disconnect — fires one of the export fn handlers in ethlibp2p.zig on that Rust thread. Those handlers call directly into Zig node state: onGossip, processBlockByRootChunk, cacheBlockAndFetchParent, chain.onBlock, etc.

At the same time, the main libxev thread runs onInterval on a timer and calls processCachedDescendants, which reads from the same fetched_blocks HashMap and walks the same chain / fork-choice state.

There was already a TODO comment in the code documenting the original intent to schedule these callbacks on the libxev loop instead:

// TODO: figure out why scheduling on the loop is not working
zigHandler.gossipHandler.onGossip(&message, sender_peer_id_slice, false) catch |e| { ... };

How the corruption produces an integer overflow

sszClone (used when caching a fetched block) deserialises into a freshly-allocated SignedBlockWithAttestation. It writes struct fields sequentially — Bitlist.length first, then the inner ArrayList. If the main thread observes the struct between those two stores — or if the fetched_blocks HashMap is modified while being iterated on the main thread — it reads a torn state:

• length has been written (or contains 0xAAAAAAAAAAAAAAAA from GPA debug poisoning of a concurrently freed slot)
• inner is not yet consistent

That torn length then flows into:

// Bitlist.serializedSize in ssz/utils.zig
pub fn serializedSize(self: *const Self) usize {
    return (self.length + 7 + 1) / 8;  // overflows when length ≈ usize::MAX
}

serializedSize returns usize (not !usize), so Zig cannot propagate an error — it panics at the arithmetic.

The fix

Add std.Thread.Mutex state_mutex to EthLibp2p. Every export fn acquires it before touching any Zig state. BeamNode.onInterval acquires the same mutex via a pointer stored in BeamNode.state_mutex (set to &network.state_mutex in production, null in unit-tests so test setup needs no changes).

This serialises all shared-state access between the two threads and eliminates the corruption. The Rust thread holds the lock only for the duration of processing one network event (typically microseconds), so there is no meaningful throughput impact.

The long-term fix is to dispatch all network events through xev.Async onto the main libxev thread — which is what the existing scheduleOnLoop infrastructure was designed for but never fully wired up. The mutex is the correct immediate solution; xev.Async-based dispatch can be added as a follow-up.

Test plan

• zig build — clean build
• zig build test --summary all — all tests pass
• zig fmt --check on modified files — no formatting issues

[zclawz]

Good optimization — clear problem, clean solution. A few observations:

1. roots.deinit(self.allocator) — likely a bug

std.ArrayList stores its allocator internally, so deinit() takes no arguments. This should be roots.deinit() (no parameter). With the extra argument this won't compile.

If you intended an unmanaged list, use std.ArrayListUnmanaged + deinit(allocator). But since self.allocator is already captured at initCapacity, plain ArrayList + deinit() is the right choice here.

2. max_depth across all batched roots

Using the maximum depth across all pending roots for the single batched request is conservative but correct — the worst case is we request a few more ancestors than strictly needed for shallower roots. Fine as a starting point; could be refined later if needed.

3. FetchFailed → CachingFailed consolidation

Replacing the now-removed FetchFailed with CachingFailed (for the put allocation failure path) is reasonable — an allocation failure during enqueue is effectively a caching failure. Callers that handled FetchFailed may need updating if they had separate recovery logic, but since this is an internal error type it's unlikely to matter.

4. Flush coverage looks complete

The four flush sites (gossip early-return, handleGossipProcessingResult, processBlockByRootChunk end, and the RPC response handler) cover the natural exit points. Roots that accumulate on error paths will be flushed on the next message cycle, which is acceptable.

Fix the deinit call and this looks good to merge.

[ch4r10t33r]

On point 1 : roots.deinit(self.allocator) is intentional and correct for this project.

This project targets Zig 0.15, where std.ArrayList.deinit() was changed to require an explicit allocator argument:

// Zig 0.15 stdlib — std/array_list.zig
pub fn deinit(self: *Self, gpa: Allocator) void {

Without the argument it fails to compile with:

error: member function expected 1 argument(s), found 0

The same deinit(self.allocator) pattern is used consistently throughout this file (6 call sites, e.g. missing_roots.deinit(self.allocator) in fetchBlockByRoots a few lines below). You may be thinking of Zig ≤ 0.13, where ArrayList stored the allocator internally and deinit() took no arguments.

[GrapeBaBa]

Can we just keep the lock only on the shared state access code path, current way looks like not good

[anshalshukla]

I’m not in favor of locking the state. A better approach may be to lock only the DB for writes and buffer any new writes until later, while still serving network calls. Locking state in order to serve network requests introduces a large security vector. I don’t think even DB-level locking should be necessary for reads. Block sync operates on blocks after finalization, so that data should already be immutable.

[ch4r10t33r]

@GrapeBaBa Fixed in the latest commit. The lock is now acquired immediately before the first call that touches shared node state in each handler, and released right after:

• handleMsgFromRustBridge — lock covers only gossipHandler.onGossip()
• handleRPCRequestFromRustBridge — lock covers only reqrespHandler.onReqRespRequest()
• handleRPCResponseFromRustBridge — lock covers only rpcCallbacks.getPtr() + callback_ptr.notify()
• handleRPCEndOfStreamFromRustBridge / handleRPCErrorFromRustBridge — lock covers only rpcCallbacks.fetchRemove() + callback.notify()
• Peer handlers — lock covers only the peerEventHandler.on*() call

The setup work (frame parsing, snappy decompression, SSZ deserialisation, span/enum casts, logging) all runs outside the lock. It is safe without the lock because the GPA allocator is thread-safe in Zig 0.15 multi-threaded builds, node_registry is *const and immutable after init, and everything else creates fresh heap objects with no shared-state reads or writes.

[ch4r10t33r]

@anshalshukla Thanks for the detailed feedback. A few points:

On "buffer writes and lock only the DB": That is exactly the xev.Async approach already mentioned in the PR description as the long-term fix. The Rust callback would enqueue an event into a lock-free channel, and the main libxev thread would dequeue and process it on its own tick, with no mutex at all. That is the right architectural solution. This PR is the minimal safe patch for an active crash; the proper redesign is a follow-up once the immediate regression is resolved.

On "block sync operates on finalized, immutable data": The race is not on finalized state. The crash happens in fetched_blocks, which is an in-flight cache of pre-finalized blocks that are still being validated by the fork-choice and chain modules. These blocks are written by the Rust thread (when an RPC block-by-root response arrives) and read concurrently by the main libxev thread (during the interval tick that processes cached descendants). That shared mutable structure is exactly what the mutex serialises.

On "DB-level locking": The DB is fine; RocksDB is thread-safe. The issue is the in-memory Zig structures (fetched_blocks HashMap, the fork-choice tree, etc.) that are not thread-safe on their own.

On "security vector": Happy to understand this concern better if you can expand on it. A mutex that serialises two threads accessing the same in-memory data structures is a standard correctness tool, not a security concern as far as I can see. The critical sections are narrow (we narrowed them further per GrapeBaBa's comment), so latency impact is minimal.