Summary
QBFT wiring has a construction cycle: Consensus::new needs a broadcaster; the real broadcaster is the qbft p2p Handle; Behaviour::new needs Arc<Consensus>. It is broken with an Arc<OnceLock<Handle>> filled ~60 lines later, self-flagged with TODO: the Arc<OnceLock<Handle>> pattern is awkward; explore alternatives.
Fragility:
- Between construction and
set, the Consensus is live and any broadcast panics on .expect("qbft p2p handle initialized before broadcast") — the invariant is enforced only by source ordering and a comment.
- A duplicate
set is mapped to the unrelated Error::BehaviourClosed.
- The pattern is duplicated verbatim in the consensus crate's test harness (
p2p.rs#L1227-L1256), which the app comment cites as precedent — a refactor must land in both.
For context, the only other set-later OnceLock in production (P2PContext::local_peer_id) degrades gracefully (getter returns Option), so this is the one true contortion.
Proposed change
Break the cycle with channel indirection instead of late binding: the broadcaster closure owns an mpsc::Sender<BroadcastRequest> created before either component; the p2p behaviour (or a small forwarding task started once the Handle exists) drains the receiver. Messages sent before the behaviour is up queue instead of panicking, the OnceLock, the expect, and the misleading error all disappear, and the test harness uses the same shape.
An alternative — splitting Consensus construction from transport attachment — is already hinted at by the start must be called exactly once doc note; acceptable if the channel shape doesn't fit.
Summary
QBFT wiring has a construction cycle:
Consensus::newneeds a broadcaster; the real broadcaster is the qbft p2pHandle;Behaviour::newneedsArc<Consensus>. It is broken with anArc<OnceLock<Handle>>filled ~60 lines later, self-flagged withTODO: the Arc<OnceLock<Handle>> pattern is awkward; explore alternatives.Fragility:
set, theConsensusis live and any broadcast panics on.expect("qbft p2p handle initialized before broadcast")— the invariant is enforced only by source ordering and a comment.setis mapped to the unrelatedError::BehaviourClosed.p2p.rs#L1227-L1256), which the app comment cites as precedent — a refactor must land in both.For context, the only other set-later
OnceLockin production (P2PContext::local_peer_id) degrades gracefully (getter returnsOption), so this is the one true contortion.Proposed change
Break the cycle with channel indirection instead of late binding: the broadcaster closure owns an
mpsc::Sender<BroadcastRequest>created before either component; the p2p behaviour (or a small forwarding task started once theHandleexists) drains the receiver. Messages sent before the behaviour is up queue instead of panicking, theOnceLock, theexpect, and the misleading error all disappear, and the test harness uses the same shape.An alternative — splitting
Consensusconstruction from transport attachment — is already hinted at by thestart must be called exactly oncedoc note; acceptable if the channel shape doesn't fit.