refactor(gossip): make gossip into an actor - #833
Conversation
ee237c6 to
0ce4cae
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The connected-peers API currently returns nondeterministically ordered results due to HashSet iteration, which should be stabilized before merging.
Pull request overview
This PR refactors the sequencer’s libp2p gossip implementation into a dedicated kameo actor crate (sequencer_gossip_actor), wiring it into the service and RPC server while preserving the “gossip outage degrades to L1-only” invariant.
Changes:
- Moves gossip implementation out of
sequencer_core::gossipinto a newsequencer_gossip_actorcrate with anactorfeature gate. - Replaces the previous drive-task + channels plumbing with a
GossipActormailbox API (PublishTransactiontell,GetConnectedPeersask) and adds an outage watchdog. - Updates service/RPC wiring and features (
mdns) to depend on the new actor crate.
File summaries
| File | Description |
|---|---|
| lez/sequencer/service/src/lib.rs | Spawns GossipActor, stores bootstrap addrs, shuts gossip down explicitly, and keeps gossip out of health aggregation. |
| lez/sequencer/service/Cargo.toml | Adds sequencer_gossip_actor dependency and forwards mdns feature to it. |
| lez/sequencer/core/src/lib.rs | Removes gossip module export from sequencer_core. |
| lez/sequencer/core/src/gossip/mod.rs | Deletes the old core gossip module facade. |
| lez/sequencer/core/src/gossip/message.rs | Deletes unused/uncompiled wire-format file. |
| lez/sequencer/core/Cargo.toml | Removes the mdns feature from sequencer_core. |
| lez/sequencer/actors/rpc_server/src/actor/service.rs | Switches gossip publisher import to sequencer_gossip_actor. |
| lez/sequencer/actors/rpc_server/src/actor.rs | Switches gossip publisher import to sequencer_gossip_actor. |
| lez/sequencer/actors/rpc_server/Cargo.toml | Adds sequencer_gossip_actor dependency (with actor feature). |
| lez/sequencer/actors/gossip/src/validation.rs | Updates references to use sequencer_core config/helpers after move. |
| lez/sequencer/actors/gossip/src/tests.rs | Updates integration-style tests to run against the spawned GossipActor. |
| lez/sequencer/actors/gossip/src/seen_cache.rs | Introduces bounded FIFO dedup cache for seen transaction hashes. |
| lez/sequencer/actors/gossip/src/lib.rs | New crate root with actor-gated exports and module layout. |
| lez/sequencer/actors/gossip/src/actor.rs | Implements GossipActor, mailbox messages, publisher, and outage watchdog. |
| lez/sequencer/actors/gossip/src/accreditation/mod.rs | Updates config import path after move. |
| lez/sequencer/actors/gossip/Cargo.toml | Adds new sequencer_gossip_actor crate and feature definitions. |
| Cargo.toml | Adds the gossip actor crate to the workspace members and dependencies. |
| Cargo.lock | Adds lock entries for the new sequencer_gossip_actor crate and dependents. |
Review details
Suppressed comments (1)
lez/sequencer/actors/gossip/src/actor.rs:301
connected_pubkeys()iterates aHashSet, so the returned peer list has nondeterministic ordering. This can lead to unstable RPC responses (and flaky tests/metrics if any consumer compares lists). Sort the collected pubkeys before returning to keep the output stable.
- Files reviewed: 16/18 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| /// One sequencer's signature over an offence, for peers to collect. | ||
| #[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize)] | ||
| pub struct SlashApprovalMessage { |
There was a problem hiding this comment.
@schouhy I'm guessing these were going to be used for slashing k/n right? i removed these so that it is more direct for your slashing PR to add later, if that is ok
There was a problem hiding this comment.
reason for removal: they were dead code at the moment
Arjentix
left a comment
There was a problem hiding this comment.
Glad to see it's being moved to a separate actor, nice work!
…r `sequencer_core`
…t, and some ordering changes
87e3089 to
0f4fd8f
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The change is a significant runtime/architecture refactor of the sequencer’s p2p gossip subsystem (task → actor + new crate boundaries), which warrants final human validation beyond static review.
Review details
- Files reviewed: 20/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
| logos-blockchain-core.workspace = true | ||
| logos-blockchain-key-management-system-service.workspace = true | ||
| logos-blockchain-zone-sdk.workspace = true |
There was a problem hiding this comment.
Shouldn't it be gated by actor feature as well?
There was a problem hiding this comment.
It is used by accreditation, #833 (comment)
Depending on the resolution of that comment we may remove accreditation module all together
There was a problem hiding this comment.
accreditation has no consumers anywhere in the tree, so it is the only thing keeping a featureless build from being protocol-only; removing it drops these three deps rather than gating them.
| #[cfg(feature = "actor")] | ||
| pub use libp2p::Multiaddr; | ||
|
|
||
| pub mod accreditation; |
There was a problem hiding this comment.
So this is not just actor implementation detail? Where it's used then?
There was a problem hiding this comment.
It was to be kept for slashing & gathering signatures for it, but looking at https://github.com/logos-blockchain/logos-execution-zone/pull/818/changes#r3968326121 I can see that this is not needed anymore
let me check in with @schouhy on this to be sure
moudyellaz
left a comment
There was a problem hiding this comment.
lgtm, thanks! Left some comments.
moudyellaz
left a comment
There was a problem hiding this comment.
lgtm, thanks for addressing my comments!
Arjentix
left a comment
There was a problem hiding this comment.
LGTM, waiting for resolving older comments related to accreditation
🎯 Purpose
Turns the sequencer's p2p gossip module into a
kameoactor of its own, resolving the// TODO: Should be a separate actorseam in the service'srun()(suggested by @Arjentix alongside the actor-encapsulation work).It now lives as
sequencer_gossip_actorunderlez/sequencer/actors/, following the same crate layout andactorfeature convention as the executor, storage, and RPC server actors (#798).⚙️ Approach
The former
GossipNetworkdrive task is the actor now:GossipActorowns the libp2p swarm and drives it from anActor::next()override (mirroringRpcServerActor::next), selecting over the mailbox, swarm events, and the bootstrap-retry tick. The mailbox replaces the hand-rolled plumbing wholesale:sequencer_core::gossipmodule to a newsequencer_gossip_actorcrate atlez/sequencer/actors/gossip; rewritenetwork.rsasactor.rs.CancellationToken+ publishmpscchannel +watchchannel with the actor mailbox:PublishTransaction(tell),GetConnectedPeers(ask), andRetryBootstrap(tell, sent by the service scheduler every 30s, same shape asProduceBlock). The rpc_server publishes through a type-erasedRecipient<PublishTransaction>withtell(..).try_send(), keeping the old non-blocking, drop-on-overflow publish semantics (bounded mailbox, same 1024 depth).on_stop: the swarm isOption-held and dropped there, so the QUIC sockets are freed the moment the actor reports stopped rather than when the struct is dropped — a shutdown observer can rebind immediately.Ed25519PublicKey(map +GetConnectedPeersreply, sorted deterministically viato_bytes), and the pending-publish queue is a newcommon::bounded_vec_deque::BoundedVecDequewhose eviction is an invariant of the type.ActorHandleis deliberately excluded fromSequencerHandle::failed()/is_healthy()(documented on the field). The old driver watchdog becomesspawn_gossip_outage_watchdog: it warns every 5 minutes only on an abnormal actor stop (panic), stays silent on graceful stop/kill, and its guard aborts the warner on node shutdown.GossipActor::newbinds and waits for the listen address before the actor is spawned; bootstrap addrs are captured pre-spawn soSequencerHandle::gossip_bootstrap_addrs()stays sync for the multi-node fixtures. Graceful gossip shutdown added toSequencerHandle::shutdown().StorageActor#798actorfeature convention: without it the crate is protocol-only (protocolmessages +accreditation;validationandseen_cacheare private actor implementation details);actorpulls in kameo/libp2p/tokio/borsh and the implementation, with tests declared inside theactormodule like the other actor crates. Only the service enables it — rpc_server consumes the crate featureless, exactly as it consumes the executor. Themdnsfeature moves here fromsequencer_core(service forwards it; it impliesactor).gossip/message.rswire-format file — it was never declared as a module (uncompiled), andsequencer_stake_core::SlashApprovalis the real slash-approval type.GossipConfigstays insequencer_core::config(it is embedded inSequencerConfig), which is the one reason core keeps its libp2p dependency. The ingest path is unchanged: gossiped transactions still enter through the executor's admission door via theIngestSubmithook wired in the service.🧪 How to Test
🔗 Dependencies
None
🔜 Future Work
Recipient<PublishTransaction>, so a test can hand it a recipient of any lightweight actor.link/supervisioninstead of the warn-only outage watchdog — needs defined restart semantics (rebind + publisher re-wiring);on_stopalready frees the sockets so a supervised restart is possible when we get there.is_healthy()intentionally staystruewhen gossip is down, so the 5-minute log is currently the only operator signal.📋 PR Completion Checklist