wip: txn forwarder liveness: mode-gate the sink and route by sender-affinity rendezvous - #136
wip: txn forwarder liveness: mode-gate the sink and route by sender-affinity rendezvous#136bronxyz wants to merge 5 commits into
Conversation
bronxyz
changed the title
bc7dd54 to
ceedb6d
Compare
bronxyz
changed the title
ceedb6d to
0545fe4
Compare
|
Claude finished @bronxyz's task in 5m 41s —— View job PR Review: feat/txn-forwarder-livenessTasks
This is a well-engineered PR. The breaker state machine, rendezvous routing, and storage read-path changes are all solid. A few things worth addressing before merge: Potential bugs / correctness1. Latent panic in let peer = self.last_acked_by(sender).unwrap_or_else(|| committee[owner as usize]);If let peer = self.last_acked_by(sender)
.or_else(|| committee.get(owner as usize).copied())?; // return early if no peer to blame2. Self::Closed(closed) => {
closed.transport_failures = 0;
false
}A validator that recovers from a send-failure trip (acked) still carries its previous 3. Two separate let candidates = self.health.lock().candidates(owner, &self.committee, connected, now);
// ...
if self.health.lock().on_failure(&peer, &e, Instant::now()) { // fresh Instant, not `now`
Performance4. O(n²) connectivity filter in .filter(|peer| connected.contains(peer)) // O(n) linear scan per peer
let connected_set: std::collections::HashSet<_> = connected.iter().collect();
.filter(|peer| connected_set.contains(peer))5. The Code quality / clarity6. let mut key = Vec::with_capacity(Address::len_bytes() + 8 + 96);Called once in 7. by_owner[group.owner as usize].extend(...)Safe by construction (rendezvous_owner always returns Highlights (what works well)
None of the issues are blocking correctness for normal operation. Items 1–2 are the ones worth fixing before merge; items 3–7 are polish/future-hardening. |
Security Scan - CodeSeverity: HIGH, CRITICAL No vulnerabilities found View scan results |
bronxyz
changed the title
- a non-batch-producing node (CvvInactive catching up, or an observer) pooled
forwarded transactions and stranded them until rejoin; the worker now refuses
the submit before the fan-out permit or any rayon work
- refusals reuse the existing error reply, so there is no wire change: the
Display strings are the contract, kept distinct so a sender can tell a mode
reject from a load shed, and un-upgraded senders already map either to walk-on
- submit_txns parses the reply once, beside the Display it inverts, into
SubmitError { Rejected(SubmitRejection), Network }, so a sender matches an enum
instead of re-classifying strings
- the mode reaches the worker as a fresh receiver of the app-scoped node_mode
watch at each epoch spawn, so a mid-epoch demotion gates the next submit
- move NodeMode to infrastructure types so the worker and forwarder share one
definition
…dator health - route each sender's chain to a committee owner by rendezvous over the connected, closed-breaker validators, so every observer with the same view routes a sender identically: a load balancer can split a sender across observers but not across validators - the only ranking signal is this node's own submit outcomes: no bus field, gossip hook, or app-scoped store, and no reputation or leader-schedule input that would starve slow but honest geo-remote validators - exclude a tripped validator from the rendezvous eligibility set, so a re-send re-selects a live successor instead of retrying a censored owner; within one send, fail over along the validator ring from the owner slot (held breakers last) when the chosen owner is momentarily unreachable - open a breaker on three consecutive transport failures or backlog sheds, immediately on a peer's mode reject, or on repeated non-inclusion (counted independently of send failures); close it on an ack or, for a non-inclusion trip, only on an inclusion read off the node's own pool - once a hold lapses admit one probe group at a time (half-open) and double the hold on each failed probe up to a cap, so a persistent censor is probed ever more rarely while an honest one closes on its first probe's inclusion - the breaker is a closed/open sum type whose open state only a trip can construct, so a hold cannot be forged and half-open is read off the clock; timing is deliberate wall-clock, a node-local heuristic, never a replicated decision - key the owner window on the executed consensus number (monotonic per committed output), the same watermark the resend policy anchors on, rather than the laggy recently-executed EVM tip - drop the AckedStale terminal mark: a stale ack is stamped as a send, so a hash still pending after the window re-sends and an ack cannot silence a transaction - resend a forwarded tx once local execution passes its send head by a 2-block margin within a 3s WAN latency budget, with backoff capped at one doubling so a frontier the owner acked but has not included is not throttled out of the budget - release in-flight marks by looking up the node's executed hashes against the pool on a 1s tick, not on the commit notification, so the builder never re-seals an executed transaction and pool-lock time does not grow with blocks per second - carry the re-send flag off the existing ForwardProbe.forwarded bit rather than a separate count, and own blame-on-non-inclusion inside ValidatorHealth
- read the executed consensus tip and the worker max-seq scan through read txns, whose iterators are lazy, so each touches only the rows it needs instead of the store-level iterator cloning or materializing the whole mem-resident table - project the tip's block/epoch/round from the raw ConsensusBlocks row instead of decoding the sub-dag, which would reconstruct every certificate's BLS signature on each gossiped certificate - finalize a sealed cold segment without a writer barrier, since the finalize already re-waits every queued consensus write in FIFO order - merge the two hot-layer tips lazily in last_record and record_prior_to, scanning the merged iterators only when the persisted tip is tombstoned or both layers are empty - open explicit write txns for one-off store writes (kad, the batch/payload/proposer/vote stores, epoch state) instead of the implicit per-call txn - name vote tasks by header digest computed once above the per-peer loop, instead of rendering the whole header's Debug on every per-peer task spawn - gate storage test rigs behind a new test-utils feature and add a transaction-scoped iterator-ordering test
- move the ConsensusHeaders high-water store write to spawn_blocking and await the join handle, so an epoch-teardown abort drops this future at the await while the synchronous write finishes on its own thread - the write paces every insert under the mem-store lock behind a writer backlog; run inline, a worker parked inside it pins the future on its stack where the abort can never reach it - await the manager's forwards in the processing test, since the store write now completes on the blocking pool before the forward rather than inline
- the validator that acked a stream's previous chunk leads the next one, so a stream that failed over stays whole on one pool - a per-chunk walk otherwise splits one sender's nonce chain between a flapping owner and its successor, each half gapped and queued until the other executes
0545fe4 to
2477780
Compare
|
Claude finished @bronxyz's task in 4m 13s —— View job PR Review: feat/txn-forwarder-liveness (re-review after sync)Tasks
This is a well-engineered set of changes. The breaker state machine, rendezvous routing, and storage read-path work are all solid. The prior review identified the right blocking items; they remain unfixed in the current state. A few new observations follow. Still unfixed from prior review1. Latent panic in let peer = self.last_acked_by(sender).unwrap_or_else(|| committee[owner as usize]);
2. Self::Closed(closed) => {
closed.transport_failures = 0; // not_included untouched
false
}Confirmed intentional ("only an inclusion does either"), but a reader hitting this cold will assume an ack clears the whole record. The New findings3. (Self::Open(open), _) => Self::Open(Open { since: now, ..open }),This arm fires when the breaker is holding AND a failed send arrives (last-resort path). Setting 4. Self::Open(open) => matches!(open.cause, Cause::Send),For a 5. let mut candidates = self.health.lock().candidates(owner, &self.committee, connected, now);
// ... (inside the failover loop)
self.marks.mark_forwarded(hashes, Instant::now(), anchor); // line 469 — ack time, correct
self.health.lock().on_failure(&peer, &e, Instant::now()); // line 476 — failure time, correct
6. by_owner[group.owner as usize].extend(Self::nonce_sorted(group.txns));Safe by construction: 7. fn record_prior_to<T: Table>(&self, key: &T::Key) -> Option<(T::Key, T::Value)> {
self.read_txn().ok()?.record_prior_to::<T>(key)
}
8. if self.last_progress_at.elapsed() >= window {
Corrections to prior review
Highlights (confirmed correct)
Items 1–2 are worth fixing before merge. Items 3–8 are clarity/future-hardening and are non-blocking. |
1 participant
Summary
Work in progress, not part of the 1/9-9/9 txpool series: based on #135 only for the code it needs, and not to be merged with the stack. Four commits that keep the transaction forwarder live and trim hot-path costs.
ae70095f): a non-batch-producing node (CvvInactive catching up, or an observer) pooled forwarded transactions and stranded them until rejoin. The worker now refuses the submit before the fan-out permit or any rayon work, reusing the existing error reply so there is no wire change.NodeModemoves to infrastructure types so worker and forwarder share one definition.5224badd): each sender's chain routes to a committee owner by rendezvous over the connected, closed-breaker validators, so every observer with the same view routes a sender identically (a load balancer can split a sender across observers but not across validators). A per-validator breaker opens on three consecutive transport failures or backlog sheds, immediately on a peer's mode reject, or on repeated non-inclusion (counted independently of send failures and closed only by an inclusion read off the node's own pool); a lapsed breaker admits one probe group at a time with a doubling hold, so a persistent censor is probed ever more rarely while an honest one recovers on its first probe. Re-sends re-select a live successor via rendezvous when a censor's breaker trips; the ring-walk remains the send-time failover. TheAckedStalemark is dropped (a stale ack is stamped as a send, so it cannot silence a still-pending hash), the resend policy is latency-tuned (2-block anchor margin, 3s WAN budget, one backoff doubling), and in-flight marks are released by a 1s executed-hash pool lookup rather than the commit notification. The owner window keys on the executed consensus number (the canonical monotonic watermark), not the laggy recently-executed EVM tip.4866118c): read the executed consensus tip and the worker max-seq scan through lazy read txns, touching only the rows needed instead of cloning/materializing the mem-resident table; project the tip's block/epoch/round from the rawConsensusBlocksrow instead of decoding every certificate's BLS signature; finalize a sealed cold segment without a writer barrier (the FIFO queue already applies index, prune, and high-water mark in order); open explicit write txns for one-off store writes; name vote tasks by header digest instead of full-headerDebug.CertificateStore::write_allnow borrows its certificates (impl Borrow<Certificate>), removing a fullVecDeque<Certificate>clone on the cert-manager persist path and in the subscriber. Test doubles consolidate into onestorage::test_utils::TestDb(behind atest-utilsfeature) so no crate re-implements theDatabasetrait for tests.ceedb6da): theConsensusHeadershigh-water store write moves tospawn_blockingwith the join handle awaited, so an epoch-teardown abort drops the future at the await while the synchronous, backlog-paced write finishes on its own thread.Surface areas touched
eth_*,rayls_*, faucet)rayls-contracts/)etc/, scripts, Docker, compose).github/workflows/,Makefile)doc/, in-crate READMEs, root docs)Breaking / compatibility
None on the wire. The mode gate reuses the existing
WorkerResponse::Errorreply (theDisplaystrings are the contract, kept distinct so a sender tells a mode reject from a load shed); un-upgraded senders map either case to walk-on. The forwarder routing, breaker, and resend policy are entirely sender-side and node-local; no message, codec, or gossip shape changed, so the range is rolling-upgrade safe.write_all's move to a borrowed parameter is an internal API change (all callers accept it, owned or borrowed). The owner window keys on the executed consensus number, a deterministic value every observer agrees on.Testing
make checkclean;make fmtclean; clippy adds no new warnings on the touched crates.cargo nextest run -p rayls-middleware-orchestratorforwarder suite passes, covering the breaker state machine (transport/shed/mode-reject trips, non-inclusion trip closed only by inclusion), the half-open single-probe with doubling hold, the censoring-owner escape end-to-end, rendezvous stability/spread under eligibility changes, the slow-acking-owner-does-not-stall-others concurrency, and the latency-tuned resend gating.cargo nextest run -p rayls-infrastructure-storage -p rayls-consensus-primarypasses, including the barrier-free coldfinalizeoracle, theTestDb-driven cert-manager teardown-reap test, and the cert-store tests over the borrowedwrite_all(noVecDequeclone).