Migrate spawned-concurrency from 0.4 to 0.5.0-rc

[pablodeymo]

Motivation

The spawned-concurrency crate is releasing v0.5.0-rc with a new protocol/actor macro API that replaces the GenServer pattern. This migration adopts the new API, which provides:

• Type-safe message dispatch via #[protocol] traits instead of manual enum matching
• Simpler handler signatures with #[send_handler] instead of handle_cast + CastResponse::NoReply
• Synchronous fire-and-forget sends — ActorRef::send() returns Result<(), ActorError> immediately (no .await)
• Cleaner timer API — send_after takes Context<A> instead of cloning the handle

Description

Dependency changes

File	Change
Cargo.toml (workspace)	spawned-concurrency and spawned-rt: "0.4" → git dep at lambdaclass/spawned.git tag v0.5.0-rc
bin/ethlambda/Cargo.toml	Removed spawned-concurrency and spawned-rt (unused — binary only uses BlockChain from ethlambda_blockchain)

Actor migration (crates/blockchain/src/lib.rs)

Before (0.4 — GenServer pattern):

• CastMessage enum with 4 variants (NewBlock, NewAttestation, NewAggregatedAttestation, Tick)
• impl GenServer for BlockChainServer with handle_cast matching on the enum
• GenServerHandle<BlockChainServer> for the wrapper
• handle.cast(CastMessage::X).await for sending messages
• send_after(duration, handle.clone(), CastMessage::Tick) for timer scheduling

After (0.5 — Protocol/Actor pattern):

• #[protocol] trait BlockChainProtocol defining 4 send-only methods
• #[actor(protocol = BlockChainProtocol)] impl BlockChainServer with individual #[send_handler] methods
• ActorRef<BlockChainServer> for the wrapper
• handle.new_block(block) (sync, returns Result<(), ActorError>)
• send_after(duration, ctx.clone(), block_chain_protocol::Tick) for timer scheduling

The #[protocol] macro generates a block_chain_protocol module with message structs (Tick, NewBlock, NewAttestation, NewAggregatedAttestation) and implements the protocol methods directly on ActorRef<BlockChainServer>.

Wrapper simplification

notify_* methods on BlockChain changed from async fn(&mut self) to fn(&self):

• No .await needed — sends are non-blocking
• &self instead of &mut self — ActorRef::send() takes &self

P2P call site updates

Removed .await from all 4 call sites in the P2P layer:

File	Method
crates/net/p2p/src/gossipsub/handler.rs	notify_new_block
crates/net/p2p/src/gossipsub/handler.rs	notify_new_aggregated_attestation
crates/net/p2p/src/gossipsub/handler.rs	notify_new_attestation
crates/net/p2p/src/req_resp/handlers.rs	notify_new_block

Migration guide improvements

While migrating, we identified several gaps in the upstream spawned-concurrency migration documentation. These are suggestions for the spawned maintainers:

1. send_after / send_interval API changes (undocumented)

The migration guide doesn't mention that send_after changed from taking GenServerHandle<A> to Context<A>. Every user of timers will hit this.

// 0.4: send_after takes GenServerHandle
send_after(duration, handle.clone(), MyMessage::Tick);

// 0.5: send_after takes Context
// Inside a handler (ctx available):
send_after(duration, ctx.clone(), Tick);
// Outside a handler (from ActorRef):
send_after(duration, actor_ref.context(), Tick);

2. Self-rescheduling tick pattern (undocumented)

A very common pattern is an actor that reschedules itself with variable delays. The guide should show this before/after:

// 0.4:
CastMessage::Tick => {
    self.do_work();
    send_after(next_delay, handle.clone(), CastMessage::Tick);
}

// 0.5:
#[send_handler]
async fn handle_tick(&mut self, _msg: Tick, ctx: &Context<Self>) {
    self.do_work();
    send_after(next_delay, ctx.clone(), Tick);
}

3. ActorRef::context() method (undocumented)

The guide shows ActorRef but doesn't document that actor_ref.context() converts an ActorRef<A> to a Context<A>. This is needed when scheduling timers from outside a handler (e.g., in spawn()).

4. &mut self → &self for send callers (undocumented)

Switching from .cast().await (async) to direct protocol method calls (sync Result<(), ActorError>) means callers no longer need &mut self. This can simplify wrapper structs and their call sites significantly.

5. Generated module naming convention (incomplete)

The guide mentions generated structs but doesn't explicitly state the naming rule:

• #[protocol] on trait FooBarProtocol generates module foo_bar_protocol (snake_case)
• Message structs: foo_bar_protocol::MethodName (PascalCase of method name)
• Type-erased ref: FooBarProtocolRef = Arc<dyn FooBarProtocol>

6. Mixed send + request protocols (missing example)

The guide shows send-only and request-only examples separately. A before/after for a protocol with both send and request methods would be helpful.

7. #[handler] vs #[send_handler] vs #[request_handler] (unclear)

The "Escape Hatches" section mentions Handler<M> but doesn't explain the difference between the three handler attributes:

• #[request_handler] — for request-response methods
• #[send_handler] — for fire-and-forget methods
• #[handler] — for manual Message types not in a protocol (?)

8. Wrapper pattern migration (missing)

A common pattern is wrapping ActorRef in a public struct to hide internals. The guide should show this before/after:

// 0.4:
pub struct MyService { handle: GenServerHandle<MyServer> }
impl MyService {
    pub async fn do_thing(&mut self, data: String) {
        let _ = self.handle.cast(MyMsg::DoThing(data)).await;
    }
}

// 0.5:
pub struct MyService { handle: ActorRef<MyServer> }
impl MyService {
    pub fn do_thing(&self, data: String) {
        let _ = self.handle.do_thing(data);
    }
}

9. spawned-rt changes (undocumented)

The migration guide focuses on spawned-concurrency but doesn't mention whether spawned-rt has breaking changes.

10. #[started] panic behavior (unclear)

In 0.4, .start() returned the handle directly. In 0.5, ActorStart::start() also returns ActorRef<A> directly, but what happens if the #[started] hook panics? The guide should clarify whether subsequent sends get ActorError::ActorStopped.

How to test

make fmt    # cargo fmt --all
make lint   # clippy with -D warnings
make test   # all workspace tests + spec tests

All pass locally. For end-to-end verification, run a local devnet:

make run-devnet

Verify that:

• Blocks are produced and imported
• Attestations are gossiped and processed
• Justification and finalization advance
• Tick scheduling works correctly (800ms intervals)

[github-actions]

🤖 Kimi Code Review

Review Summary

The PR migrates the codebase from spawned v0.4 to v0.5.0-rc, switching from the GenServer trait to the new actor protocol system. The changes are mostly mechanical and follow the new API patterns correctly.

Issues Found

1. Race Condition in Tick Scheduling (Critical)

File: crates/blockchain/src/lib.rs, lines 72-77 and 510-517

The new send_after API uses ctx.clone() which creates a new context reference, but the scheduling logic has a potential race condition. If the system is under heavy load, the 800ms interval boundary calculation could drift because:

• The calculation happens after processing the tick
• Network delays or CPU contention could cause the actual send time to be later than calculated
• This could lead to missed slot boundaries in consensus

Fix: Calculate the next tick time at the beginning of the tick handler, before processing, to maintain more accurate timing.

2. Unnecessary Async in Notify Methods (Minor)

File: crates/blockchain/src/lib.rs, lines 79-104

The notify methods (notify_new_block, notify_new_attestation, notify_new_aggregated_attestation) are no longer async but still have async in their signatures in the calling code. This creates unnecessary .await calls in the P2P handlers.

Fix: Remove the .await calls in:

• crates/net/p2p/src/gossipsub/handler.rs lines 52, 77, 103
• crates/net/p2p/src/req_resp/handlers.rs line 172

3. Error Handling Inconsistency (Medium)

File: crates/blockchain/src/lib.rs, lines 82-104

The error handling pattern changed from .await.inspect_err(...) to .inspect_err(...) but the error messages still reference "Failed to notify..." which might be misleading since these are now synchronous calls that could fail immediately.

Fix: Update error messages to reflect synchronous nature, e.g., "Failed to send message to blockchain actor".

4. Protocol Trait Visibility (Minor)

File: crates/blockchain/src/lib.rs, line 488

The BlockChainProtocol trait is marked pub(crate) but the generated protocol might need different visibility for the actor system to work correctly.

Fix: Consider if pub visibility is needed for the protocol trait.

Positive Changes

1. Cleaner API: The new actor protocol system provides a cleaner, more type-safe interface
2. Better Separation: Protocol definition is now separate from implementation
3. Consistent Dependencies: All spawned crates now use the same git reference

Security Considerations

No security vulnerabilities introduced. The changes are purely mechanical API updates without affecting consensus logic or cryptographic operations.

Performance Impact

Minimal performance impact. The change from async to sync notify methods might slightly reduce overhead, but the difference is negligible for the message volumes expected.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

1. Medium - Supply-chain pin is mutable (tag-only git dependency).
   Using tag = "v0.5.0-rc" without rev means a retag upstream could change build inputs for the same manifest, which is risky for consensus software reproducibility.
   Reference: Cargo.toml:47, Cargo.toml:48
   Suggested change: pin immutable commits (rev = "...") for both spawned-concurrency and spawned-rt (or vendor/fork internally).

2. Low - Duplicate spawned-* sources are now in lockfile (git + crates.io).
   Cargo.lock contains both registry and git variants of spawned-concurrency/spawned-rt, which increases compile time/binary size and can become a type-fragmentation hazard if APIs cross crate boundaries later.
   Reference: Cargo.lock:2332, Cargo.lock:2333, Cargo.lock:7060, Cargo.lock:7073, Cargo.lock:7096, Cargo.lock:7110
   Suggested change: unify to one source via workspace-level dependency policy (or [patch.crates-io]), then regenerate lockfile.

No direct correctness regressions found in fork-choice/attestation/state-transition logic in this diff; the Rust changes in crates/blockchain/src/lib.rs look behavior-preserving for message handling/tick scheduling.

I couldn’t run cargo check in this environment due rustup temp-file permission errors, so this review is static-analysis only.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Now I have enough context to write a thorough review.

---

Review: Migrate spawned-concurrency from 0.4 to 0.5.0-rc

Overall this is a clean, mechanical migration. The behavior is preserved and the new API is used correctly. A few items worth noting:

Cargo / Dependencies

Dual spawned-concurrency versions in the binary

Cargo.lock now contains two copies of spawned-concurrency 0.4.5: one from crates.io (pulled in transitively by the external ethrex-p2p dependency) and one from the git tag. Because these types don't cross a crate boundary, there is no runtime incompatibility — but the binary compiles both, adding modest bloat. This is a known and unavoidable consequence of the external dependency; worth a comment in the PR description or a follow-up to update ethrex-p2p when v0.5.0 ships to crates.io.

Git tag version mismatch

The workspace pins tag = "v0.5.0-rc", yet the resolved package version in Cargo.lock is 0.4.5. The commit hash is locked (8b4dbf73), so builds are reproducible, but the tag/version mismatch is confusing for auditors and dependabot-style tooling. The lock file comment git+...tag=v0.5.0-rc doesn't communicate an actual semver bump. This should be cleaned up when the stable release lands.

crates/blockchain/src/lib.rs

Redundant variable (handle_tick, lines ~494–502)

timestamp.as_millis() as u64 is computed, passed into on_tick, then immediately re-bound to ms_since_epoch for the interval calculation:

let timestamp = SystemTime::UNIX_EPOCH.elapsed()...;
self.on_tick(timestamp.as_millis() as u64);
let ms_since_epoch = timestamp.as_millis() as u64;  // same value, computed again

Prefer:

let ms_since_epoch = timestamp.as_millis() as u64;
self.on_tick(ms_since_epoch);
let ms_to_next_interval = MILLISECONDS_PER_INTERVAL - (ms_since_epoch % MILLISECONDS_PER_INTERVAL);

#[allow(dead_code)] scope

The attribute is placed on the whole trait. The comment is accurate (tick() is dispatched via send_after, never called directly), but the suppression is broad — if a protocol method were accidentally removed in future, the compiler would stay silent. If the macro allows it, annotating only fn tick(...) would be tighter. If that's a macro limitation, the existing comment is sufficient.

pub(crate) vs pub on BlockChainProtocol

pub(crate) is correct here — BlockChainProtocol is an implementation detail; callers use the BlockChain wrapper. No issue.

Call-site changes (gossipsub/handler.rs, req_resp/handlers.rs)

The .await removals are correct and consistent. The notify_* wrappers now take &self instead of &mut self, which correctly reflects that ActorRef::send() only needs a shared reference. All four call sites updated.

Behavior Parity

• Tick rescheduling: old code reused the CastMessage::Tick enum value; new code constructs a fresh block_chain_protocol::Tick. Since Tick carries no data, the behavior is identical.
• Timer initialization: handle.context() is used for the genesis-delayed first tick; inside the handler ctx.clone() is used for subsequent ticks. Both are correct per the new API.
• Error handling: inspect_err with error!(...) is consistent with the rest of the codebase pattern.

Summary

The migration is functionally correct. The two actionable items are the redundant ms_since_epoch binding in handle_tick and tracking the RC dependency for promotion to a stable release. Everything else is minor or expected for a mid-migration state.

---

Automated review by Claude (Anthropic) · custom prompt

[greptile-apps]

Greptile Summary

This PR migrates spawned-concurrency from the stable crates.io release 0.4 to a git pre-release at tag v0.5.0-rc, replacing the GenServer/CastMessage enum pattern in crates/blockchain/src/lib.rs with the new #[protocol]/#[actor]/#[send_handler] macro API. The P2P call sites are updated accordingly by removing .await from the now-synchronous notify_* wrapper methods.

The actor migration is correct: all four message variants (NewBlock, NewAttestation, NewAggregatedAttestation, Tick) are properly ported to #[send_handler] methods, and the self-rescheduling tick pattern uses ctx.clone() correctly. The BlockChain wrapper simplification (async fn &mut self → fn &self) is sound and consistently applied across all four P2P call sites. The workspace dependency update is clean, with git+tag pinned in Cargo.lock for reproducibility.

One stale documentation reference remains in the BlockChainServer struct doc comment (refers to "GenServer" pattern when code uses the new Actor pattern).

Confidence Score: 5/5

• Safe to merge. Functional logic is correct; only minor doc comment clarification needed.
• The blockchain actor migration is mechanically correct with all four message handlers properly ported. The P2P call-site updates are consistent. The dependency update is properly pinned. The one identified issue is purely stylistic (outdated doc comment reference).
• No files require special attention.

Sequence Diagram

sequenceDiagram
    participant P2P as P2P Layer<br/>(gossipsub/req_resp)
    participant BC as BlockChain<br/>(ActorRef wrapper)
    participant BCS as BlockChainServer<br/>(Actor)
    participant Timer as send_after / Timer

    Note over P2P,BCS: New block / attestation flow (sync, no .await)
    P2P->>BC: notify_new_block(block) [fn &self]
    BC->>BCS: handle.new_block(block) → Result<(), ActorError>
    BCS-->>BC: Ok(())
    BC-->>P2P: (returns immediately)

    Note over BCS,Timer: Self-rescheduling tick pattern
    Timer->>BCS: handle_tick(_msg: Tick, ctx)
    BCS->>BCS: on_tick(timestamp_ms)
    BCS->>Timer: send_after(ms_to_next_interval, ctx.clone(), Tick)

    Note over BC,BCS: Spawn / initial tick scheduling
    BC->>BCS: BlockChainServer { ... }.start() → ActorRef
    BC->>Timer: send_after(time_until_genesis, handle.context(), Tick)

Loading

Comments Outside Diff (1)

1. crates/blockchain/src/lib.rs, line 107-108 (link)

   The doc comment still references "GenServer" but the code has been migrated to the new Actor/Protocol pattern. Update for accuracy:

_{Last reviewed commit: 714b5f8}