fix(replication): drain priority sync queue and recover from routing-event lag - #165
fix(replication): drain priority sync queue and recover from routing-event lag#165mickvandijke wants to merge 69 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves replication convergence under churn by (a) ensuring neighbor sync drains queued priority peers without waiting for periodic ticks and (b) recovering from lagged DHT routing-table broadcast events. It also expands verification/fetch pipeline behavior (batching caps, retry/defer semantics, paid-list edge-voter quorum handling) and adds an e2e scenario covering paid-list-authorized repair below storage quorum.
Changes:
- Neighbor-sync loop now drains the priority queue back-to-back and resyncs close peers on
RecvError::Lagged. - Verification/fetch pipeline enhancements: bounded verification batches, fetch→verification retry metadata, and deferred re-verification scheduling.
- Paid-list quorum evaluation updated with “edge voter” handling; adds a deterministic paid-list repair e2e scenario and bumps crate version.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/poc_d1_bounded_queues.rs | Updates test VerificationEntry construction for new timing fields. |
| tests/poc_bootstrap_stall.rs | Updates test VerificationEntry construction for new timing fields. |
| tests/e2e/replication.rs | Adds deterministic e2e scenario for paid-list-majority repair under low storage quorum. |
| src/replication/types.rs | Adds next_verify_at, fetch retry metadata, and NeighborSyncState::has_priority_peers + test. |
| src/replication/scheduling.rs | Adds deferred pending scheduling, fetch retry→verification requeue, and returns evicted keys. |
| src/replication/quorum.rs | Adds edge-aware paid-list vote summary and splits verification requests into capped batches. |
| src/replication/mod.rs | Implements lag recovery, neighbor-sync drain-before-park, verification request caps, and retry/defer integration. |
| src/replication/config.rs | Introduces PAID_LIST_FLEX_EDGE_COUNT and MAX_VERIFICATION_KEYS_PER_REQUEST. |
| src/replication/bootstrap.rs | Updates test VerificationEntry construction for new timing fields. |
| src/replication/admission.rs | Adjusts cross-set dedup/admission so paid hints can survive replica rejection under churn. |
| Cargo.toml | Bumps version to 0.14.3. |
| Cargo.lock | Updates locked crate version to 0.14.3. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| results.push(protocol::KeyVerificationResult { | ||
| key: *key, | ||
| present, | ||
| present: cached.present.unwrap_or(false), | ||
| paid, | ||
| }); |
| let handles = | ||
| spawn_verification_batch_tasks(targets, p2p_node, config.verification_request_timeout); | ||
| collect_verification_batch_results(handles, targets, &mut evidence).await; |
1a6fe63 to
fc8d724
Compare
74c990e to
b8d490d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/replication/storage_commitment_audit.rs:163
run_subtree_auditalways forwardsSubtreeAuditOrigin::Manual, but its docstring says this is a “gossip-triggered” audit pinned to a gossiped commitment. That’s misleading for readers and also makes the newaudit_originlogging attribute inaccurate unless callers userun_subtree_audit_with_origindirectly.
/// Run one gossip-triggered subtree audit against `challenged_peer`, pinned to
/// the commitment hash the peer just gossiped (`expected_commitment_hash`).
///
/// ADR-0002 two-round audit. The auditor sends a fresh random nonce and runs:
///
src/replication/audit.rs:114
replication::auditis a public module; removing theaudit_tickwrapper changes the crate’s public API and forces all downstream callers onto the new, more complex signature. Consider keeping a compatibilityaudit_tickwrapper that supplies emptyRepairProofsand a coordinator (callers that need shared coordination can still useaudit_tick_with_repair_proofs).
/// Execute one repair-proof-gated audit tick.
///
/// This is the production path used by the replication engine. Direct
/// callers that have not adopted repair proofs remain conservative and do not
/// audit peers for unproven keys.
#[allow(
clippy::implicit_hasher,
clippy::too_many_arguments,
clippy::too_many_lines
)]
pub async fn audit_tick_with_repair_proofs(
src/replication/bootstrap.rs:198
expire_capacity_rejectedexpires sources based on the stored timestamp incapacity_rejected_sources, which is documented elsewhere as “first-seen and never refreshed”. The doc comment here says “most recent rejection”, which is inconsistent with the actual semantics and could cause future misuse.
/// Expire capacity-rejection records whose most recent rejection is older
/// than `max_age`, returning how many sources were expired.
///
dirvine
left a comment
There was a problem hiding this comment.
Reviewed exact head 160567bbe5cb5c6ade72d5b5cc7486f0dde07ed3 with a six-seat review panel, followed by independent source and test verification.
Verdict: hold. Four blocking issues remain.
Blocking
-
The inbound serial queue is count-bounded, not byte-bounded, and can retain roughly 2.5 GiB of remote payload.
src/replication/config.rs:329-332permits 10 MiB replication messages.src/replication/mod.rs:2519-2520creates a 256-entry channel of owned, decoded messages; decode happens before queue admission at:2563-2586, and dropping starts only after the queue is full at:2608-2625.- A valid
NeighborSyncRequestcan approach the wire cap, with no per-request hint-count limit (src/replication/mod.rs:5536-5540). 256 × 10 MiB = 2.5 GiBbefore allocator/container overhead, transport buffers and the message currently being processed. The later routing-table check cannot protect this allocation because it runs after queueing.- Please add aggregate byte accounting/admission (or a substantially smaller class-specific bound) before decode-owned messages can accumulate.
-
An unvalidated fresh offer can pre-claim a key, suppress the valid offer, and frame the receiver for non-possession.
- Only missing-proof/oversize checks precede the key claim (
src/replication/mod.rs:4737-4764). - The key is claimed at
:4851-4858; a concurrent valid duplicate is refused at:4859-4872. - Content-address validation occurs later at
:5051-5081, and payment validation later still at:5140-5165. - The sender uses one-way
send_messageand does not consume the rejection (src/replication/fresh.rs:113-145), while the delayed possession path applies audit-severity failure weight when the receiver lacks the key (src/replication/possession.rs:183-194,215-236). - A malicious uploader knows the key and targets, so it can race a malformed offer before the legitimate one. Validate authoritative content/payment before exclusive key ownership, or coalesce duplicates and propagate the first handler's actual result instead of refusing them.
- Only missing-proof/oversize checks precede the key claim (
-
The public Rust API is broken while ADR-0005 says “SemVer: patch” and “No … public-API change”.
- The crate publicly exposes
replication(src/lib.rs:54). - The public
audit_tickwrapper was removed;audit_tick_with_repair_proofsgained a required coordinator argument (src/replication/audit.rs:104-123). AuditTickResult::Failedgained a mandatory field and is not#[non_exhaustive](src/replication/audit.rs:47-63). Other publicly constructible replication structs/constants also changed.- ADR-0005 currently makes the opposite compatibility claim at
docs/adr/ADR-0005-replication-repair-hardening.md:682-684, whileCargo.tomlremains0.15.0. - Restore compatibility shims/surface, or explicitly treat this as a breaking
0.16.0API change and correct the ADR/release metadata.
- The crate publicly exposes
-
Audit coordination can accumulate unbounded waiters and make shutdown drain proportional to the entire backlog.
- Possession events use an unbounded channel (
src/replication/mod.rs:1600-1603) and each event becomes a separately tracked delayed task (:2121-2152). AuditChallengeCoordinator::acquirehas no deadline, queue bound or cancellation branch (src/replication/audit_coordinator.rs:75-95).- Once a possession task enters
run_possession_check, cancellation is checked before each peer but not while parked inacquire(src/replication/possession.rs:169-180,358-360). - Shutdown waits without a timeout for every detached task (
src/replication/mod.rs:1990-2003), so accepted write volume concentrated on overlapping peers can leave an arbitrarily long serial drain. This is economically gated, but normal paid load can also trigger it. - Bound/coalesce the scheduler backlog and make coordinator acquisition cancellation-aware.
- Possession events use an unbounded channel (
Material fixes recommended in the same pass
storage.exists()failure can still become definitivepresent: falsewhen the paid-list lookup succeeds (src/replication/mod.rs:5680-5725), which the requester maps toPresenceEvidence::Absent(src/replication/quorum.rs:737-744). Omit the result or encode presence as unresolved when the local read failed. Panel severity was split because the trigger is a local LMDB fault rather than a direct remote input.AuditChallengePermit::dropremoves the final coordinator reference before the local_permitis actually dropped (src/replication/audit_coordinator.rs:132-136), allowing a narrow semaphore-recreation race.drop(self.permit.take())beforerelease_referencecloses it. Panel severity was split; the bug is real and the fix is small.- A coordinator-acquire failure in prune audit is mapped to
MalformedResponseand then peer failure (src/replication/pruning.rs:1749-1755,1662-1687). It is currently defensive/unreachable in production, but local lifecycle failure should not become remote blame.
Verification
Local exact-head checks passed:
cargo fmt --all -- --checkcargo clippy --lib --all-features -- -D warnings- focused overload/bootstrap/audit-coordinator/quorum tests
poc_d1_bounded_queues: 7/7poc_bootstrap_stall: 3/3poc_shutdown_lmdb_drain: 1/1
GitHub platform builds/tests, Clippy, format, docs, audit, no-logging and ADR validation are green on this SHA. The remaining required process checks are red: linear-link (no linked Linear issue) and pr-template (missing required template sections).
I submitted this as an advisory COMMENT, not an approval/request-changes review, and did not merge.
…ceiling INBOUND_REPLICATION_SERIAL_QUEUE_CAPACITY bounded a message COUNT, but a queued item is an owned decoded ReplicationMessage of up to MAX_REPLICATION_MESSAGE_SIZE (10 MiB) — decode runs on the receiver, before admission, to keep deserialization off the serial loop. At 256 the resident worst case was 2.5 GiB; try_enqueue_serial_message accounts for neither bytes nor originating peer, its only admission test being try_send. Drop the bound to 64, putting the worst case at 640 MiB. This is a mitigation, not a fix: the lane is bimodal — small control messages share it with multi-MiB bulk responses that burst precisely during repair — so no single count suits both, and going lower would shed the traffic the queue exists to absorb. ADR-0005 decision 12 records the trade, both rejected alternatives, byte accounting as the deferred real fix, and a re-open trigger noting that 64 is not backed by field data since queue_depth is only sampled on drop. Also correct the constant's doc comment, which still claimed the receiver handles a message inline when the queue is full; e6b15f7 changed that to a drop. No wire-format or behavioural change beyond the bound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ADR-0005 asserted "SemVer: patch. No wire-format or public-API change". The wire-format half is true and verified: REPLICATION_PROTOCOL_ID stays at v2 and no ReplicationMessageBody variant or field changes (the only protocol.rs edit in the branch widens a private const to pub(crate)). The public-API half is false. src/lib.rs exposes `pub mod replication`, and the branch breaks it: audit_tick and run_prune_pass removed, AuditTickResult::Failed and PrunePassContext and VerificationTargets gained required fields, VerificationEntry and FetchCandidate restructured (FetchCandidate also losing its Ord/PartialOrd/Eq impls to the new FetchOrder), MAX_PENDING_VERIFY_PER_PEER and MAX_PRUNE_AUDIT_CHALLENGES_PER_PASS removed, plus pending_count_for_sender and evict_stale changes. Replace the claim with a Compatibility bullet that states both halves accurately and enumerates every break against the decision that forced it, and record the absence of compatibility shims as a decision: the changed items are the data structures this ADR restructures, so a parallel deprecated surface would pin the old representation in place. Carries no versioning or release semantics — the required version position is the release manager's call, not the ADR's. This also drops the same stale "patch-level" claim from the Constraints section, which stated it a second time. Raised as blocking issue 3 in review of PR #165. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The per-chunk possession check raced the shutdown token against the settle sleep only — `run_possession_check` was called from inside the sleep branch's BODY, so once the sleep won, the `select!` was already resolved and nothing downstream was cancellable. That mattered because a started check parks on the per-target audit challenge coordinator, which has no deadline, no queue bound, and no cancellation branch. Waiters therefore drained at one full probe timeout per `MAX_CONCURRENT_AUDIT_CHALLENGES_PER_TARGET` slots — the loop-top cancellation check in `run_possession_check` cannot fire until the acquired probe completes. Meanwhile `detached_task_tracker.wait()` is deliberately unbounded (the LMDB contract forbids a timeout there), so shutdown blocked for the whole drain. Worst case is a shutdown during a partition or against a dead peer, where every probe burns its full deadline. Wrap the sleep and the check in a single future so shutdown races both. This is the shape the neighbor-sync round already uses, and it is why the prune path — same coordinator, no shutdown token of its own — has no equivalent stall: its whole round future is dropped. Dropping the future mid-probe is safe: a parked coordinator acquire releases its counted reference through `ReferenceGuard` (covered by `cancelled_wait_releases_reference`), and a dropped LMDB `spawn_blocking` is already covered by the storage-quiescence wait in `shutdown`. Fixes the drain only. Closing the coordinator semaphore was rejected as an alternative: `acquire` returning `None` is mapped to `MalformedResponse` in the prune path, which would report misbehaviour against an innocent peer on every shutdown. This change does not touch `acquire`, so that path stays dormant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r key `dispatch_fresh_offer` took the exclusive per-key in-flight claim after only two checks — proof-of-payment present, payload under MAX_CHUNK_SIZE — both satisfiable with junk. The content-address check ran later, on the worker. An offer that had not proved it carried the key's bytes could therefore reserve the key, and the genuine offer arriving behind it was refused as a "duplicate already in flight". The forged offer then died at the content-address check and stored nothing, so the chunk landed on fewer nodes than the protocol promises and the absence was charged to the receiver at audit severity by the delayed possession check. Knowing a key is not confined to the peers that receive the chunk: PaidNotify carries it to PaidCloseGroup(K) (20) while the chunk goes to CLOSE_GROUP_SIZE (7), over a small message that is not gated by MAX_CONCURRENT_REPLICATION_SENDS and can outrun the multi-MiB push it describes. Fold the check into `fresh_offer_structural_rejection` so it runs on the serial loop before the claim, ordered after the size check so an oversized payload is never hashed. Hashing there is affordable: BLAKE3 runs at GB/s against offers arriving at link speed, so a flooder cannot make the loop hash faster than it can deliver the bytes. `handle_fresh_offer`'s standalone copy of the check is now covered by its existing defence-in-depth call and is removed. Also correct the four comments claiming the delayed possession check re-offers a key it could not place. It does not — `run_possession_check` only penalises, and repair comes later from neighbor sync (10-20 min) than the penalty (5-15 min). This includes the stale-shed rationale, which is a memory-pressure trade rather than the redundancy argument it claimed; the threshold itself is left unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
grumbach
left a comment
There was a problem hiding this comment.
Checked both of these against the branch, they hold up.
Serial handoff has no byte budget. INBOUND_REPLICATION_SERIAL_QUEUE_CAPACITY = 256 counts messages, and each one is a fully decoded ReplicationMessage held in the channel. MAX_REPLICATION_MESSAGE_SIZE is 10 MiB and FreshReplicationOffer carries up to a 4 MiB chunk inline, so a peer keeping the queue full parks ~1 GiB of offers before any admission code runs. That undercuts the ceiling FRESH_OFFER_MAX_OUTSTANDING documents ("at 4 MiB each, sixteen is a 64 MiB ceiling"), because the 16-slot gate only fires after dequeue. Before this PR the loop handled one message at a time, so this is new resident memory. One caveat on the fix: the upstream broadcast buffer is also bounded in events rather than bytes, so byte-weighting the mpsc alone still won't give you a real ceiling.
While you're in there, the doc comment on that constant says "If this fills, the receiver handles the message inline instead of dropping it, preserving delivery while bounding memory." The code warns and drops, and the comment at the drop site says never run a handler there. That stale comment is a lot of why the ceiling looks enforced.
Hint source cap is only half applied. MAX_HINT_SOURCES_PER_KEY is checked in the pending_verify branch of add_pending_verify and nowhere else. The fetch_payloads and in_flight_fetch branches do retry.hint_sources.extend(...) and payload.sources.push(...) with no bound, then requeue_fetch_for_verification passes the entry to insert_pending_owned_unchecked, which writes every source into pending_keys_by_source without rechecking. So park a key in fetch state, have N peers advertise it, fail the fetch, and it comes back to pending_verify carrying N sources. That is the N * MAX_PENDING_VERIFY association growth the constant's own doc comment says the cap exists to prevent. payload.sources is an unbounded Vec on its own too, no truncate anywhere in the file. Applying the same retention policy in all three branches closes both.
Rest of the PR reads well, the admission and shutdown work especially. Just these two before merge.
The per-key in-flight claim was exclusive and abandoned on failure: the first offer to arrive became the only offer. A sender whose proof did not verify took the key, refused every other offer as a duplicate, stored nothing, and left the absence charged to this node at audit severity by the delayed possession check — with repair waiting on neighbor sync (10-20 min), later than the penalty (5-15 min). Verifying the content address before admission (previous commit) established that every offer for a key carries identical bytes, and that is what this builds on: only the proof_of_payment and the sender differ. So the in-flight map now holds the bytes once per key and queues each sender's proof, and the handler works down that queue. A rejected proof disqualifies its sender and rotates to the next; a verdict about the key itself — not responsible, no capacity, shutting down, write failed — abandons it, since every queued proof would meet the same wall. Duplicates are routine, not adversarial: a client PUT is confirmed by CLOSE_GROUP_MAJORITY nodes and each fans out to the close group, so a receiver sees ~4 offers per chunk. Queueing proofs keeps that at one permit, one worker slot, one payload, and one on-chain verification per key. Charging each duplicate a permit instead would have cut concurrent PUT capacity fourfold and pushed offers into the capacity refusal that this node is then penalised for. Bound it in three ways: MAX_FRESH_OFFER_ATTEMPTS_PER_KEY (= CLOSE_GROUP_MAJORITY) proofs per key, one per source peer, and MIN/MAX_PAYMENT_PROOF_SIZE_BYTES enforced on the serial loop. The last is new and load-bearing: the verifier applies those limits only once an offer reaches a worker, and a proof is capped on the wire only by MAX_REPLICATION_MESSAGE_SIZE, so retaining proofs without it would admit a 640 MiB vector against a 64 MiB payload ceiling. Rotation also makes each proof independently attributable, so structural defects are now penalised: missing, undersized, oversized, or non-matching payloads, none producible by an honest sender. Payment outcomes stay unpenalised — PaymentRequired means "no payment found", not "definitively unpaid", so a lagging or reorganising chain view would charge the whole close group at once — and a verification error is usually our own EVM endpoint. Residual, recorded in the ADR: CLOSE_GROUP_MAJORITY sybil identities can still fill one key's queue with failing proofs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of PR #165 asked for the possession-check scheduler backlog to be bounded or coalesced. Bounding it is the wrong move and the ADR now says why, so the decision survives the next reader who notices the unbounded channel. The queue is fed from exactly one place: a FreshWriteEvent this node accepted and fanned out. Every entry is therefore a chunk this node took a client PUT for and was paid for — nobody inflates it without paying per chunk and having this node accept it. That is categorically unlike the inbound serial queue of decision 12, where a stranger's bytes size the queue, which is why a bound is right there and wrong here. A cap would also discard what the queue is for: a dropped possession check is a peer that failed to store going unpunished. Memory does not motivate one either — ~224 bytes plus task overhead per event, steady state being arrival rate times the ~10 minute settle window, so ~6k parked tasks at a sustained 40 MB/s of accepted ingest. What is genuinely unbounded is lateness. A single-key probe against a per-target limit of 2 yields ~40 probes/s from a healthy peer but ~0.45/s from one burning the full 4.4s deadline, so a backlog forms only when peers are already timing out — and a late verdict is a wrong verdict, since neighbor sync may have delivered the chunk by then. A count cap does not address that; it discards a different arbitrary subset. Records the cancellability fix that did matter, the two rejected alternatives (capping the queue; closing the coordinator semaphore, which would report MalformedResponse against innocent peers on every shutdown), and coalescing per target as the deferred real fix — AuditChallenge.keys is already a Vec answered by per-key digests, and audit_response_timeout(key_count) already gives the batch a principled ceiling. Re-open trigger is probe lateness, not queue depth. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/replication/storage_commitment_audit.rs:160
run_subtree_auditnow hard-codesSubtreeAuditOrigin::Manual, but its doc comment still describes it as a “gossip-triggered” audit pinned to a commitment the peer “just gossiped”. This is misleading (and makes it look like the origin-tracking is wired when it isn’t). Update the comment to describe the wrapper’s current behavior.
/// Run one gossip-triggered subtree audit against `challenged_peer`, pinned to
/// the commitment hash the peer just gossiped (`expected_commitment_hash`).
src/replication/bootstrap.rs:197
- The docstring says expiry is based on the “most recent rejection”, but
capacity_rejected_sourcesstores the first-seen rejection timestamp andexpire_capacity_rejectedcomparesnow.duration_since(*rejected_at)against that first-seen time. This mismatch is confusing for future maintenance and makes it harder to reason about bootstrap-drain guarantees.
/// Expire capacity-rejection records whose most recent rejection is older
/// than `max_age`, returning how many sources were expired.
`FreshOfferAdmission::surplus_reason` is read only by a `debug!`, which compiles to nothing without the `logging` feature, so the no-default-features build tripped `dead_code` under the workflow's `RUSTFLAGS: -D warnings` and failed Build on all three platforms plus Test (no logging). Gate the impl on the feature, matching `SerialQueueDropReason::as_str` directly above it. Verified with the commands CI runs: `cargo build --release --no-default-features` and `cargo test --lib --no-default-features` (854 passed) under `-D warnings`, plus `cargo clippy --all-targets --all-features` and `cargo test --lib --features test-utils`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/replication/audit_coordinator.rs:136
AuditChallengePermitreleases the coordinator reference before dropping the semaphore permit. There is a small race window whererelease_reference()can remove the per-peer limiter entry while the permit is still held, allowing a concurrentacquire()to create a fresh semaphore at full capacity and temporarily exceedMAX_CONCURRENT_AUDIT_CHALLENGES_PER_TARGETfor that peer. Drop theOwnedSemaphorePermitbefore callingrelease_reference()to keep the limit strict under concurrency.
impl Drop for AuditChallengePermit {
fn drop(&mut self) {
let _permit = self.permit.take();
self.coordinator.release_reference(self.peer);
}
src/replication/storage_commitment_audit.rs:196
run_subtree_auditis documented as “gossip-triggered”, but the wrapper hard-codesSubtreeAuditOrigin::Manual. In this crate it’s invoked by the first-monetized audit scheduler and the test-onlyaudit_peer_nowpath (seesrc/replication/mod.rs:2508and:1987), so logs will mislabel those audits asmanual. Consider either (1) making the origin an explicit parameter (e.g. exposerun_subtree_audit_with_origin), or (2) provide distinct wrappers forFirstMonetizedvsManualand update the docs accordingly.
expected_commitment_hash,
key_count,
credit,
SubtreeAuditOrigin::Manual,
)
src/replication/storage_commitment_audit.rs:249
- This module uses the
audit_originlog field for the local subsystem origin (gossip/first_monetized/manual), but other audit requester logs useaudit_originto mean the audit type (e.g.AuditType::ResponsibleChunk/Prune/Possession). Reusing the same field name for different semantics makes dashboards/queries ambiguous. Consider keepingaudit_originstable (e.g. constant"subtree"here) and adding a separate field likesubtree_audit_originfor the local scheduler origin.
info!(
target: "ant_node::replication::audit_requester",
event = "started",
audit_origin = origin.as_str(),
audit_round = "subtree",
dirvine
left a comment
There was a problem hiding this comment.
Reviewed exact head 8d66ee08fe8092f8982399eae122e12808ce739d with a second six-seat panel, followed by independent source/test verification and a temporary regression probe.
Verdict: hold. Three blocking issues remain.
Blocking
-
The inbound serial queue is still count-bounded rather than byte-bounded.
- Full remote payloads are decoded before admission (
src/replication/mod.rs:2772-2795). - The queue is 64 owned decoded messages (
:2728-2729) and admission is onlytry_send(:4133-4154). - With the 10 MiB replication-message ceiling, this remains a remotely reachable 640 MiB retained-payload spike before allocator/transport overhead (
:979-987). - Reducing 256 to 64 is useful mitigation, but it does not close the original memory-exhaustion blocker. Please add byte-weighted admission (preferably charged from encoded length before decode/enqueue), while retaining a count bound as a secondary guard.
- Full remote payloads are decoded before admission (
-
The new per-key fresh-offer proof cap is replenishable and therefore not a lifetime bound.
- Admission checks only
entry.pending.len()(src/replication/mod.rs:1291-1301). next_attempt()pops the current proof before awaiting verification (:1334-1342,:5358-5372), freeing a pending slot.- A new distinct source can refill that slot; every source remains in
entry.sources, and joined offers bypass the global/per-peer responder permit because only the opener takes one (:5185-5243). - This permits unbounded sequential payment-verifier/EVM/DHT work, indefinite worker/key occupancy, and unbounded source-set growth, contradicting ADR-0005's four-proof/bounded-work claim.
- I reproduced this on the exact head with a temporary unit probe: 21 distinct sources were accepted sequentially for one live key while each prior attempt was popped. The probe passed, then was removed; the worktree is clean.
- Please enforce a monotonic lifetime attempt count per entry, and add a pop→refill regression test.
- Admission checks only
-
Fresh-offer capacity refusal still becomes audit-severity trust damage against the refusing receiver.
- Senders use one-way
send_messageand do not consumeFreshReplicationResponse(src/replication/fresh.rs:96-145). - A locally saturated receiver refuses the offer (
src/replication/mod.rs:5236-5279), but the sender still schedules that target for possession checking; absence/timeout is then charged at audit severity (src/replication/possession.rs:169-258). - ADR-0005 itself records that this can occur under ordinary bursts and feed eviction (
docs/adr/ADR-0005-replication-repair-hardening.md:823-836). - Panel severity was split here: three reviewers classified it blocking, two treated it as a follow-up, and one did not assess it. I retain it as blocking because local overload is not attributable peer misbehaviour. Capacity rejection needs an authenticated/consumed outcome and neutral retry/inconclusive handling, not a possession-failure penalty.
- Senders use one-way
Resolved from the previous review
- Fresh-offer key pre-claim: resolved. Payload/proof shape and
key == BLAKE3(data)are now checked before entering the key (src/replication/mod.rs:4963-5043,:5169-5202), and later valid proofs can rotate after a failed one. - Rust API/ADR contradiction: resolved as raised. ADR-0005 now explicitly distinguishes unchanged wire format from the deliberately breaking public Rust API and enumerates the breaks.
- Shutdown drain through possession waiters: resolved narrowly. Shutdown now races the entire sleep-plus-probe future (
src/replication/mod.rs:2327-2360), so parked coordinator waits are cancellable rather than draining through network deadlines.
Material fixes still open (not independently merge-gating here)
storage.exists()error can still be serialised aspresent: falsewhen paid status succeeds (src/replication/mod.rs:6091-6138), turning local uncertainty intoAbsentevidence. Omit the result when presence is unknown.AuditChallengePermit::dropstores the taken permit in a local, then removes the coordinator reference before that local drops (src/replication/audit_coordinator.rs:132-136). Usedrop(self.permit.take())beforerelease_reference.- Prune coordinator-local acquire failure still maps to malformed remote response (
src/replication/pruning.rs:1749-1755); currently dormant, but semantically wrong. - ADR tunables inventory says PaidNotify
8/2, while code and the substantive decision use64/16(docs/adr/ADR-0005-replication-repair-hardening.md:879-882;src/replication/mod.rs:1087-1095).
Verification
All current GitHub checks are green on this SHA. Local exact-head checks passed:
cargo fmt --all -- --check- fresh-offer tests: 8/8
- audit-coordinator tests: 3/3
- possession tests: 11/11
poc_shutdown_lmdb_drain: 1/1- verification-response roundtrip: 1/1
- temporary lifetime-cap reproducer: 1/1 (removed afterwards)
Review-team result: 6/6 retained the queue-memory blocker; every seat that independently analysed the replenishable proof queue confirmed it, and the local probe reproduced it. The trust-attribution item had the severity split recorded above.
Submitted as an advisory COMMENT; I did not approve or merge.
MAX_FRESH_OFFER_ATTEMPTS_PER_KEY gated on `pending.len()`, the queue's instantaneous depth. The handler pops a proof before verifying it, so every pop returned a slot that a fresh source could refill. Since the per-source set only bars repeats, a stream of distinct peers kept one entry alive indefinitely: unbounded sequential payment verifications — EVM and DHT work — while holding an admission permit and one of only four fresh-offer worker slots. Four such keys idle the whole pool. The staleness shed is no backstop; it runs once before the loop, not inside it. Count admissions instead, and never decrement. A popped proof has spent its slot rather than returned it, so a key costs at most CLOSE_GROUP_MAJORITY verifications no matter how many peers offer it. This also bounds the entry's source set, which previously grew one PeerId per sybil for the life of the entry. Regression test drives pop-then-admit with a fresh source each time and asserts the lifetime count holds; it accepted 16 proofs against a budget of 4 before the fix. Reported by AI review of PR #165, independently reproduced here before fixing. Verified with the commands CI runs, including the no-default-features build and test that the previous commit tripped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/replication/audit_coordinator.rs:136
AuditChallengePermit::dropreleases the coordinator reference before theOwnedSemaphorePermitis dropped. That creates a race where another task canacquire()the same peer, observe no existing limiter entry, create a fresh semaphore at full capacity, and temporarily exceed the intended per-target concurrency cap.
Drop the semaphore permit first, then release the reference, so the old limiter can’t be removed/recreated while a permit is still held.
impl Drop for AuditChallengePermit {
fn drop(&mut self) {
let _permit = self.permit.take();
self.coordinator.release_reference(self.peer);
}
src/replication/storage_commitment_audit.rs:161
- The doc comment for
run_subtree_auditsays it is “gossip-triggered”, but the wrapper hard-codesSubtreeAuditOrigin::Manual. This makes the docs misleading and also makes it easy for non-gossip callers to end up with the wrong origin in logs.
Either update the docs to reflect that this is a legacy/manual-origin wrapper (and that callers should prefer run_subtree_audit_with_origin), or change the wrapper to accept/derive the correct origin.
/// Run one gossip-triggered subtree audit against `challenged_peer`, pinned to
/// the commitment hash the peer just gossiped (`expected_commitment_hash`).
///
…roughout
The decision-2 bullet was corrected when the cap was fixed, but two other
places still described it as a queue depth: the tunables list, and the
residual bullet ("fill a key's queue"). Both now say what the code does —
admissions counted per entry, never decremented.
The residual is also restated rather than just reworded. A lifetime budget
makes the sybil case sharper on purpose, since working through the queue no
longer replenishes it, but not longer-lived: the entry closes when its queue
drains and the next genuine offer opens a fresh entry with a fresh budget,
so the suppression is a bounded window, not a lasting hold on the key.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/replication/storage_commitment_audit.rs:163
- The doc comment says this is a “gossip-triggered” subtree audit pinned to a commitment the peer “just gossiped”, but
run_subtree_audithard-codesSubtreeAuditOrigin::Manual. That makes the docs inaccurate and also means callers that use this wrapper (e.g. non-manual schedulers) will have their origin mislabeled unless they callrun_subtree_audit_with_origindirectly.
/// Run one gossip-triggered subtree audit against `challenged_peer`, pinned to
/// the commitment hash the peer just gossiped (`expected_commitment_hash`).
///
/// ADR-0002 two-round audit. The auditor sends a fresh random nonce and runs:
///
src/replication/bootstrap.rs:197
expire_capacity_rejectedexpires entries based on the first-seen rejection timestamp stored inBootstrapState::capacity_rejected_sources, but the doc comment says “most recent rejection”. This is misleading and can cause incorrect assumptions about how the TTL behaves (refresh vs. first-seen).
/// Expire capacity-rejection records whose most recent rejection is older
/// than `max_age`, returning how many sources were expired.
dirvine
left a comment
There was a problem hiding this comment.
Follow-up review of exact head cee207d24d7276ffa2a84dbdf326da6320e74873 (new commits 4963130, cee207d), with six completed review seats plus independent source/history/test verification. One timed-out seat was discarded and replaced.
Verdict: hold. The proof-cap blocker is resolved; two blockers remain.
Resolved: replenishable proof cap
4963130 fixes the bug correctly:
FreshOfferEntry.admittedis a monotonic lifetime count (src/replication/mod.rs:1220-1233).- Existing entries gate on
entry.admitted >= MAX_FRESH_OFFER_ATTEMPTS_PER_KEY, then increment under the same mutex (:1307-1320). - New entries start at
admitted: 1(:1323-1333). next_attempt()pops pending work without returning budget (:1355-1371).- The pop→refill regression (
:9268-9306) discriminates the old bug and passes.
ADR-0005 now consistently describes a lifetime budget and the bounded residual window. No off-by-one, lock race, staleness-budget, or retained-byte regression was found in this fix.
Provenance correction
I checked both operative upstream baselines mechanically:
- merge-base:
f4c8cd56a85790031666254c333aa053fe56e6cd - current
origin/main:c8f0ce15c8d146ccbedcbec251f0b101c43b225d
Neither contains the decoded serial handoff queue or the fresh-offer admission/refusal mechanism. Both are introduced by this PR branch. They are pre-existing only relative to earlier revisions of this same PR, not relative to main.
1. 640 MiB decoded serial queue — still blocking
c61d432introducedINBOUND_REPLICATION_SERIAL_QUEUE_CAPACITY = 256,InboundReplicationMessage, and the serialmpschandoff.d513554reduced it from 256 to 64 and documented the resulting 640 MiB ceiling.- Neither introducing commit is on
main. - At head, full attacker-controlled payloads are decoded before queue admission (
src/replication/mod.rs:2798-2821); admission is count-onlytry_send(:4159-4171); the replication ceiling is 10 MiB.
So 2.5 GiB → 640 MiB is a fourfold improvement within the PR branch, not relative to main. Relative to main, this PR introduces a new queue capable of retaining 640 MiB of decoded remote payload before overhead. The ADR is candid, but documentation and a normal-load re-open trigger do not bound adversarial memory.
Required before re-review: byte-weighted admission charged from encoded length before retaining decoded payloads, with the count cap kept as a secondary guard (or another defensible substantially smaller byte ceiling).
2. Capacity refusal → audit-severity damage — still blocking, with panel dissent recorded
The broad mismatch is inherited: replicate_fresh already used one-way send_message, returned selected targets, and later possession checking could penalise undelivered/unstored targets.
The deterministic capacity-refusal trigger is not inherited:
7a86d1eintroducedFRESH_OFFER_MAX_OUTSTANDING = 16, the admission semaphore, and the explicit behaviour change “past the admission bound an offer is refused”.- Merge-base and current
mainhave noFRESH_OFFER_MAX_OUTSTANDINGor corresponding capacity-refusal path. - At head, admission failure deliberately refuses without storing (
src/replication/mod.rs:5262-5305), but the sender cannot consume that rejection (src/replication/fresh.rs:100-145), and later absence can become an audit-weight trust failure (src/replication/possession.rs:125-259).
This PR may reduce older broadcast-lag losses overall; static inspection cannot quantify that net effect. It nevertheless adds a predictable local-overload condition that is converted into remote misbehaviour evidence. Correct trust attribution should not depend on the event being rare.
Panel severity was split: two reconciliation reviewers retained this as blocking; one accepted it as a bounded residual/follow-up. I retain it as blocking because the trust signal is knowingly non-attributable and the production-shaped saturation case is not covered by the cached-payment/64 KiB E2E.
Required before re-review: make capacity refusal actionable to the sender (request/response + retry/reassignment), exclude a known local refusal from adverse possession evidence, or convert refusal into durable repair/hint state such that local load shedding cannot become an unearned audit penalty.
Verification
Current GitHub checks are all green on cee207d, including Ubuntu/macOS/Windows tests, Clippy, format, docs, no-logging, audit and ADR validation.
Local exact-head checks passed:
cargo fmt --all -- --check- lifetime pop→refill regression: 1/1
- fresh-offer tests: 9/9
- audit-coordinator tests: 3/3
- possession tests: 11/11
- stale-focused tests: 10/10
- bounded-responder tests: 2/2
The attempted shutdown name filter matched zero tests and is not counted as coverage.
Submitted as an advisory COMMENT; I did not approve or merge.
dirvine
left a comment
There was a problem hiding this comment.
Maintainer follow-up on exact head cee207d24d7276ffa2a84dbdf326da6320e74873:
- The 640 MiB worst-case decoded serial-queue ceiling is explicitly accepted as an ADR-0005 design trade-off/deferral, not treated as a merge blocker.
- Fresh-offer capacity refusal becoming possession/audit trust damage is also explicitly intentional: a peer that cannot keep up with accepted fresh-offer traffic is expected to incur that signal. The path is considered sufficiently unlikely and is documented in ADR-0005.
- The replenishable per-key proof-budget defect was the implementation bug in this review cycle; it is fixed by
4963130, covered by the discriminating pop→refill regression, and the ADR wording is reconciled bycee207d.
Given that clarified policy, I withdraw my merge-blocking classification for the first two items. The underlying technical facts and provenance remain as recorded, but they are consciously accepted design decisions rather than unanswered findings.
All current required checks are green on this SHA. This is an advisory follow-up comment; I did not approve or merge.
dirvine
left a comment
There was a problem hiding this comment.
Approved exact head cee207d24d7276ffa2a84dbdf326da6320e74873 after focused re-review.
Verified:
- the replenishable fresh-offer proof budget is now a monotonic per-entry lifetime cap (
4963130), with a discriminating pop→refill regression; - ADR-0005 consistently documents the lifetime budget and the explicitly accepted queue/capacity trade-offs (
cee207d); - six-seat review completed, including a replacement for one timed-out reviewer;
- all required GitHub checks are green across Linux, macOS and Windows, plus Clippy, format, docs, no-logging, security audit and ADR validation;
- focused local fresh-offer, possession, stale and bounded-admission tests passed.
No implementation blockers remain under the maintainer-confirmed ADR policy. Approval only; not merged.
Linear issue
V2-694 — Improve chunk/paid-list replication repair speed and stability
Risk tier
Proposed; a human confirms at review. Node logic with a broad behavioural
surface, but no wire, storage-format, or economics change — same shape as #187.
Compatibility
REPLICATION_PROTOCOL_IDstaysv2, and noReplicationMessageBodyvariant or field is added, removed, or reordered — the branch's onlyprotocol.rschange widens a private const topub(crate). A node running this branch replicates with a0.15.0node unchanged, so there is no mixed-version window.ant_node::replication.audit::audit_tickandpruning::run_prune_passremoved;AuditTickResult::Failed,PrunePassContext, andquorum::VerificationTargetsgain required fields;types::VerificationEntryandFetchCandidaterestructured (FetchCandidatealso loses itsOrd/PartialOrd/Eqimpls to the newFetchOrder);MAX_PENDING_VERIFY_PER_PEERandMAX_PRUNE_AUDIT_CHALLENGES_PER_PASSremoved. Enumerated with rationale in the ADR's Compatibility section. No shims: the changed items are the data structures the ADR restructures, and every consumer is first-party.Semver impact
Test evidence
Run locally on
fd4a97e:cargo fmt --all -- --check— clean.cargo clippy --all-targets --all-features -- -D warnings -D clippy::panic -D clippy::unwrap_used -D clippy::expect_used— clean.cargo test --lib --all-features— 854 passed, 0 failed.poc_d1_bounded_queues— 7/7;poc_shutdown_lmdb_drain— 1/1.cargo test --test e2e fresh_offer— 3/3, includingnormal_upload_never_reaches_fresh_offer_capacity, which guards the case where an ordinary upload would otherwise be pushed into the capacity refusal this node is then penalised for.Reviewer verification at
160567b(six-seat panel, PR review of 2026-07-29) additionally confirmed fmt, clippy, and the focused overload/bootstrap/audit-coordinator/quorum suites green on that SHA.Outstanding for this tier: no dev-testnet run has been performed for the commits after
160567b. T2 asks for it and it is not yet done.New dependency
None. The only
Cargo.tomlchange on the branch adds thepoc_shutdown_lmdb_draintest target.ADR
https://github.com/WithAutonomi/ant-node/blob/fix/neighbor-sync-drain-window/docs/adr/ADR-0005-replication-repair-hardening.md
Mitigation / rollback
Revert the branch. Because neither the wire protocol nor the storage format
changes, a reverted node interoperates with the deployed fleet exactly as before,
and no data written under it needs migrating. The tunables introduced here
(inbound queue depth, fresh-offer proof-queue depth, admission bounds) are plain
constants and can be adjusted without a protocol change.
Review Response (dirvine, 2026-07-29)
Four blocking issues were raised. Status:
Mitigated, not fully fixed.
INBOUND_REPLICATION_SERIAL_QUEUE_CAPACITY256 → 64, moving the worst case 2.5 GiB → 640 MiB (
d513554). Byte accountingis the real fix and is deferred deliberately, with both rejected alternatives
and a re-open trigger recorded in ADR-0005 decision 12. This is short of the
aggregate byte admission the review asked for, and is flagged as such rather
than claimed closed.
Fixed, in two steps.
b17887amoves the content-address check onto theserial loop ahead of the claim, so a key can no longer be seized by a peer
that merely knows it — reachable more widely than the close group, since
PaidNotifycarries the key to 20 peers while the chunk goes to 7.61bb67bthen removes the remaining suppression: the in-flight entry holdsthe bytes once and queues each sender's proof, so a proof that fails to verify
disqualifies its sender and rotates to the next rather than costing the record.
Residual, recorded in the ADR:
CLOSE_GROUP_MAJORITYsybil identities canstill fill one key's queue.
change". Fixed (
9734808). The ADR now states both halves accurately —wire format verified unchanged, public API broken — and enumerates every break
against the decision that forced it. A second, stale "patch-level" claim in the
ADR's Constraints section was also removed. Per repo convention the version
bump itself is the release manager's and is not in this PR.
proportional to the backlog. Drain fixed (
bcf0d17): the settle sleep andthe check are now wrapped in one future so shutdown races both — previously
the check ran inside the sleep branch's body, after the
select!had alreadyresolved, so nothing downstream was cancellable. Backlog deliberately left
unbounded, with the reasoning recorded in ADR-0005 decision 13: the queue is
fed only by writes this node accepted and was paid for, so it is not
remote-amplifiable, and a cap would drop possession checks — letting a peer
that failed to store go unpunished — to buy memory that was never scarce. What
is genuinely unbounded is probe lateness against unresponsive peers; the
deferred fix is coalescing per target, which the deployed multi-key
AuditChallengealready supports.The three "material fixes recommended in the same pass" are not yet addressed:
the
storage.exists()failure that still becomes a definitivepresent: false,the
AuditChallengePermit::dropordering, and the prune coordinator-acquirefailure mapped to
MalformedResponse.Summary
This PR hardens replication repair under churn, load, queue pressure, event-stream failure, and shutdown. It expands the original neighbor-sync drain fix into a broader set of paid-list repair, verification, bootstrap accounting, audit concurrency, fresh-offer dispatch, admission gating, and async lifecycle fixes so legitimate repair work is not silently dropped, converted into false audit failures, delayed behind stale peers, left spinning on closed broadcasts, serialized behind a saturated worker pool and dropped through broadcast lag, used to conscript out-of-range nodes into storing arbitrary keys, downloaded and stored under responsibility decisions that topology churn had already invalidated, left holding LMDB/P2P resources — including detached LMDB blocking transactions — after engine shutdown, or wedged in permanent bootstrap by a peer-removal race that orphans capacity-rejection accounting.
Problems Addressed
Paid-list repair could become terminal too early. Duplicate replica/paid hints were deduplicated before their actual admission outcome was known, and verified repair work could lose its retry context after transient no-holder or no-source rounds.
Paid-list edge peers were too strict under churn. Boundary disagreement could reject a valid majority formed by the stable core of the paid close group.
Neighbor sync could stall after topology bursts.
Notifycoalescing allowed queued priority peers to wait for later 10-20 minute periodic ticks, while lagged DHT broadcasts could hide entrants entirely or leave departed peers ahead of current neighbors, each consuming a request timeout.Verification retry capacity could be stolen. Promoted verified keys stopped counting against pending capacity, allowing unrelated hints to consume the capacity required to requeue them.
Verification work lacked source-aware prioritisation. Duplicate hints discarded useful corroborating-source information, and large ready queues could schedule weak singleton claims ahead of better-supported repair work.
Bootstrap hint batches were published incrementally. Verification could race a partially admitted neighbor-sync batch and miss the complete source picture.
Verification fan-out was unnecessarily fragmented. Per-peer key sets were split into many requests despite already having a bounded verification cycle.
Bootstrap could remain blocked by departed peers. A peer that caused an admission-capacity rejection could leave while its rejection marker continued preventing drain.
Local audit bursts could look like honest-peer timeouts. Independent audit issuers could exceed the responder's per-source admission limit and interpret the resulting drops as remote failure.
Cancelled fresh offers could detach LMDB writes. Dropping an async
spawn_blockingwaiter does not cancel the blocking transaction, so shutdown could report drained while LMDB still owned the environment.Detached responder/audit work was outside engine lifecycle tracking. Digest, subtree, byte, possession-check, and audit-launch tasks could outlive engine shutdown while retaining storage or P2P state.
Closed event streams could spin the replication loop. Closed Tokio broadcast receivers remain immediately ready forever; continuing after
RecvError::Closedcould consume a core, flood P2P warnings, and prevent the replication pipeline from shutting down cleanly.Sole-source replica hints could avoid trust penalties. A definitive close-group rejection was only penalized when the advertising peer also explicitly denied possession, allowing unsupported free-replication claims to escape punishment.
Fresh-offer dispatch could serialize on the non-audit loop. Once all four worker permits were held, dispatch handled the next offer — an on-chain payment verification plus a multi-MiB LMDB write — inline on the serial message loop. Per-key shard locks (
key[0] % 64) collapsed the close-prefix accepted-key set onto a single shard, making the inline path the steady state, backing up the 256-slot inbound queue, and dropping replication messages wholesale once the P2P event loop lagged its broadcast receiver.Replica hints could conscript out-of-range nodes into storage.
HintPipeline::Replicaskipped theis_responsible(storage_admission_width)gate that paid keys had to pass, and a stored pipeline tag let a second replica message escalate a queued paid entry through thealready_pendingfast path, so a peer could name any key and force nodes ranked 10-20 for it to fetch and store it.Duplicate hint merges rebuilt the whole fetch heap. Merging a re-advertised key's advertiser only touches a field the heap never orders on, but the old path took the whole heap, scanned it linearly, and re-heapified — O(n) per duplicate and O(m·n) for a batch under the global queue write lock, which a neighbor could trigger deliberately by re-hinting queued keys.
Shutdown could return while detached LMDB transactions were still running. Several paths race a
select!on the shutdown token against futures awaiting aspawn_blockingLMDB transaction — the fetch worker'sstorage.put, the prune pass'sstorage.delete/paid_list.remove_batch, the verification worker'spaid_list.insert. Dropping the losing future does not cancel the blocking closure, which keeps running with a clonedEnv; per-fetch tasks were also bare-spawned outside both trackers, so a droppedin_flightset could leakArc<LmdbStorage>pastshutdown(). Reopening the same environment afterwards was undefined behavior.Bootstrap drain could stall permanently on a peer-removal race. A capacity-rejection record for source P was cleared only by P's next clean admission cycle or by P's
PeerRemovedcleanup — but the note sites and the removal handler run on different tokio tasks, with await points between the last "P is live" observation and the insert. A removal fully processed inside that window made its clear a no-op, and the subsequent note recorded an entry no future event could retire:check_bootstrap_drainedreturned false forever, audits stayed disabled (Invariant 19), and the node advertisedbootstrapping: trueindefinitely, drawing network-wide bootstrap-claim trust penalties — reachable deliberately by overflowingpending_verifyduring a victim's bootstrap and disconnecting. Adjacent liveness gap: every drain check was event-driven and a clean-cycle clear never re-checked, so a quiet node could satisfy the drain condition with nothing left to observe it.Fetch decisions could go stale between promotion and download. Storage responsibility was checked once, at verification-completion time, and never again before the chunk was downloaded and stored. The fetch queue holds up to 131,072 entries and dequeues nearest-first, so a far candidate can wait unboundedly long while closer keys jump ahead, and every per-source retry reused the same stale answer — topology churn after promotion still ended in a download, a disk write, and fetch→store→prune churn, violating the contract documented in
types.rsandadmission.rsthat responsibility is decided against live routing state at the point of download.Raw LMDB reads could race concurrent map resize.
all_keys()andget_raw()opened read transactions without the shared environment lock, allowing an exclusiveenv.resize()to change the memory map while those transactions were active.One routing peer could monopolize verification capacity. The global-only 131,072-entry admission bound let one peer fill the queue and continuously consume the 8,192-key verification cycle, rejecting honest hints before source-count prioritization could help; restoring a fixed 8,192-per-peer quota would instead truncate legitimate bootstrap batches containing tens of thousands of hints.
Bulk replication responders could monopolize the serial message loop. Fetch responses perform large LMDB reads and uploads, while verification and neighbor-sync responses perform synchronous point-lookups and queue publication; handling them directly let one source delay unrelated protocol traffic.
Serial-queue backpressure could defeat its own bound. When the bounded handoff filled or closed, the fallback executed the rejected handler inline on the P2P event receiver, allowing overload to propagate into broadcast lag and wider message loss.
Responder load shedding and audit latency lacked actionable attribution. Admission drops, stale dequeues, logical audit issuers, remote sources, and stage timings were not available as bounded counters and structured summaries.
Responsive possession-proof failures lost their precise cause. Validation branches collapsed into a generic failed verdict, making malformed proofs, missing records, and commitment mismatches indistinguishable in logs even though trust behavior correctly remained the same.
Mature prune backlogs could not drain at scale. The prune-confirmation audit budget was counted per candidate-to-peer edge, so a close group of 7 capped a pass at roughly nine records regardless of how many mature out-of-range records existed; far-out copies (ranked beyond the width-20 admission neighbourhood) were retained indefinitely because every deletion required a remote possession round. A first attempt to raise audit throughput then string-parsed a deployed responder's oversized-batch rejection to split-and-retry in-band — brittle wire-wording coupling whose failure mode was a trust penalty against an innocent capacity-limited peer.
Fresh-offer fairness was sized as a request quota, and its refusals were not actionable. The two-slot per-source share throttled ordinary single-sender upload fan-out while the global pool still had capacity. A refusal later becomes an audit-severity penalty against the refusing node, yet the counters omitted fresh offers and paid notifications and did not identify whether the global pool or per-peer share bound.
Changes
Paid-list verification and repair
pending_verify, fetch queue, and in-flight fetch state.Paid-list edge churn
Neighbor sync and bootstrap lifecycle
HashMap<PeerId, Instant>instead of a bare set) and expires records only after a runtime-derived full-cycle window covering every neighbor batch, per-peer request deadlines, cooldown, and one slow-cadence interval of slack (125 minutes with defaults), forfeiting abandoned/departed-source debt consistently with peer-removal cleanup.pending_peer_requestsearly-returns: pending requests legitimately block the drain check itself but no longer block expiry, and a drain condition that became true without a triggering event is now observed within one tick.Source-aware bounded verification
Fresh-offer dispatch, admission gating, and fetch-queue merges
handle_fresh_offerhas a single caller and no inline verification/LMDB path on the serial loop.key[0] % 64shard locks with an exact per-key in-flight set behind an RAII guard, so unrelated keys never contend and concurrent duplicates collapse onto the first claimant instead of repeating its verification.replica_hint_sourcesinstead of storing the tag, deleting the paid→replica escalation and both demotion sites.is_responsible(storage_admission_width)at both fetch sites (local paid-list fast path and post-verification path), matching pruning width; fresh PUTs keep their wider accept window.paid_list_close_group_size(20), so mislabelling a hint gains nothing and the two-gate rescue dance (rejected_replicarescued byadmitted_paid) is removed; the admissible key set is unchanged.FetchCandidateintoFetchOrder(key + distance, the only fields theOrdimpl reads) held in the heap andFetchPayload(sources + retry metadata) held in a key-indexed map that also serves as the membership index, making a duplicate-source merge an O(1) map lookup and rebuilding the heap only when a departed peer actually orphans a candidate.is_responsible(storage_admission_width)on every fetch attempt insideexecute_single_fetch— before spending bandwidth (per-source retries re-enter there, so they are covered) and once more beforestorage.put, so bytes arriving after responsibility lapsed mid-round-trip are not written. A lapsed attempt resolves asFetchResult::NoLongerResponsible, which sharesStored's terminal path (retry-slot release plus bootstrap accounting) and reports no trust event; the verification-time check remains as a cheap pre-filter that keeps never-responsible keys out of the fetch queue.apply_fetch_result, so eachFetchResultvariant's queue transition is unit-testable without a live network.Audit concurrency and observability
AuditChallengeCoordinatoracross responsible, prune-confirmation, and possession audits.Bounded responder isolation and diagnostics
possession_failure_reasonfield without changing evidence, trust, or penalty semantics.Storage resize safety
env_lock.read()inside the tracked blocking closures forall_keys()andget_raw(), matching the other raw storage operations.env.resize()operation while preserving cancellation and shutdown tracking semantics.Event-stream and detached-task lifecycle
LmdbStorageandPaidListroute everyspawn_blockingthrough per-instanceTaskTrackers and exposewait_idle(), so a dropped async awaiter no longer untracks a live transaction (constructor-time opens stay untracked — they cannot outlive the constructor).shutdown()awaits timed-out long-lived tasks after requesting abort, then awaitswait_idle()on both environments after the detached-task drain; when it returns, producer tasks have actually exited, no LMDB blocking operation is still running, and no engine-spawned task holdsArc<LmdbStorage>/Arc<PaidList>.tokio::spawn; thein_flightplumbing and prompt network-I/O cancellation are unchanged — only the bounded in-flight LMDB transaction is awaited.Prune backlog draining
sqrt(local_stored_keys)sender limit, so a pass is bounded by actual batched requests and complete candidate proof sets rather than ~9 records.Rejectedas an attributable failure (loggedsize_reject) rather than renegotiating it in-band. The responder'smax_incoming_audit_keysalready carries a 2x margin, so an honestly-sized peer only rejects past a 4x close-group store spread — double the assumed ~2x — and aRejectedis never a possession signal (a peer lacking a key answersDigestswith an absent marker). This removed the earlier string-parsing split-and-retry, its request-budget atomics, and the innocent-peer trust-penalty risk. No wire-format change; the deployed multi-key challenge/response representation is unchanged.Coverage Added Or Updated
Bounded responder guards release global and per-source slots on ordinary drop and cancelled admission waits.
Neighbor-sync admission serializes one source while allowing a different source to progress concurrently.
Expired responder work is shed at dequeue while fresh requests are served.
A full serial handoff drops the message instead of invoking its handler inline.
Bulk-responder counters remain separated by class and admission-versus-staleness outcome.
Audit summaries order top logical origins and preserve per-stage timings.
Possession validation branches map to stable structured failure labels.
LMDB resize waits for an in-flight
get_raw()transaction holding the shared environment lock.Elastic max-min admission under attacker-first and honest-first arrival orders, including large sole-source bootstrap batches and the unchanged global capacity bound.
Fair per-owner verification service with slack redistribution, retry protection, peer-removal ownership transfer, bounded duplicate bookkeeping, and displaced-bootstrap redelivery accounting.
Paid-hint admission after duplicate replica rejection.
Verification retry, reservation transfer, discard, exhaustion, and per-sender capacity behavior.
Paid-list edge vote behavior for negative, positive, unresolved, self-inclusive, undersized, and non-default runtime-configured group widths.
Neighbor-sync priority drain/termination and lag recovery.
Bootstrap completion after a capacity-rejected peer departs.
Capacity-rejection TTL derivation across the full configured sync cycle and runtime settings, plus semantics: a within-TTL rejection still blocks drain, an expired record unblocks it, expiry is per-source (a stale source's expiry does not forfeit a fresh source's owed re-delivery), and a repeat rejection refreshes the timestamp.
The peer-removal race ordering itself: removal cleanup runs first as a no-op, the rejection is recorded after for the now-departed peer, drain is blocked, then TTL expiry retires the orphaned record and drain completes.
The verification-tick self-heal helper: an orphaned record survives a within-TTL tick and a past-TTL tick expires it and completes bootstrap.
Duplicate hint source aggregation and source-aware scheduling.
Singleton replica-hint penalties for definitive close-group rejection and explicit source denial, including neutral inconclusive, paid-only, and corroborated cases.
Atomic bootstrap batch publication and full-cycle verification request bounds.
Fresh-offer admission bounding, per-key in-flight collapse of concurrent duplicates, and the refusal-past-bound possession penalty.
Replica-download responsibility gating at both fetch sites, including rejection of out-of-range keys and the removed paid→replica escalation path.
Single-gate hint admission parity across replica and paid labels for the unchanged admissible key set.
O(1) duplicate-source merge and heap rebuild only on genuine candidate orphaning.
Audit coordinator per-target serialization, cross-peer parallelism, and cancellation cleanup.
Closed P2P event handling now explicitly verifies terminal control flow; lagged P2P event handling still verifies continuation and metric accounting.
E2E paid-list majority repair below storage quorum.
Storage- and paid-list-level
wait_idleregression tests: a write parked inside its blocking closure with a dropped awaiter keepswait_idleblocked, commits after release, and leaves the store usable.Engine-level shutdown drain (
poc_shutdown_lmdb_drain):shutdown()blocks until a detached LMDB write commits, after which both environments reopen cleanly.Per-attempt responsibility recheck: worker disposition of
NoLongerResponsible(terminal exit, retry-slot release, no verification requeue), terminal-path parity withStored, and preserved source-failure retry/requeue transitions.E2E stale-fetch-candidate driver on a live 12-node network: a seam-enqueued candidate for an out-of-responsibility key is never stored and exits the pipeline terminally, with an in-responsibility positive control through the same seam and holder; the test fails when the rechecks are disabled.
Existing prune, fetch-retry, and replication tests updated for shared coordination and reservation-aware APIs.
Prune backlog draining: width-9 hysteresis classification; complete-width-20 fast deletion versus incomplete-width-20 audited fallback; fast-path and audited TOCTOU revalidation; dynamic square-root peer batches; request-budget admission of complete candidate proof sets; rotating fairness; and every
Rejectedreason grading to the non-recoveringRejectedstatus without parsing its wording (a challenge-id mismatch stays a plain failure).Fresh-offer reserve invariants, per-ceiling admission attribution, and separation of responder-class counters.
E2E normal 48-chunk single-source upload with a propagation positive control and zero fresh-offer or paid-notify refusal deltas.
Expected Effect
Fetch, verification, and neighbor-sync floods cannot monopolize the serial replication lane; bounded, per-source-fair workers isolate their LMDB, queue, and upload costs.
Serial-queue saturation remains local to the rejected message instead of propagating inline work and lag into the P2P receiver.
Operators can distinguish admission pressure, expired queued work, audit-origin hotspots, slow response stages, and exact responsive possession-proof failures.
Raw LMDB reads cannot overlap an unsafe memory-map resize, closing the remaining resize-safety gap for audit, pruning, and commitment scans.
A routing peer can use idle verification capacity but cannot monopolize admission or service once honest peers contend; large genuine bootstrap batches remain admissible when capacity is available.
Partition heals and mass joins drain priority neighbor-sync work in seconds-scale rounds instead of waiting through periodic ticks or stale-peer timeouts.
Repair remains live through transient routing disagreement, no-holder/no-source rounds, queue pressure, and fetch exhaustion.
Better-corroborated hints are verified first without allowing verification rounds or request fan-out to grow without bound.
Sole peers cannot advertise unacknowledged replicas to offload storage for free without incurring bounded trust penalties.
Fresh-offer verification and storage never run inline on the serial loop, so worker saturation no longer backs up the inbound queue or drops replication messages through broadcast lag.
Ordinary single-source fresh-offer fan-out can use 12 of 16 slots while four remain reserved for other sources; any refusal is attributed to the binding ceiling and surfaced as an operator-visible health signal.
A peer cannot conscript out-of-range nodes into fetching and storing arbitrary keys by labelling hints as replicas.
Duplicate-hint bursts no longer scale the fetch-queue write-lock hold time with queue depth.
Topology churn between fetch promotion and download no longer costs a download, a disk write, and a prune cycle — stale candidates are declined at download time, terminally and without stalling bootstrap drain or penalizing an innocent source.
Bootstrap cannot be held indefinitely by partial batch publication or departed capacity-rejected peers — including one whose removal races the rejection record — and a drain condition that becomes true without a triggering event is observed within one worker tick, so audits cannot be disabled permanently nor the node trust-penalized for a perpetual bootstrap claim.
Local audit concurrency no longer manufactures false remote timeout verdicts.
Mature out-of-range records drain at scale: far-out copies beyond a complete width-20 view are fast-deleted in bounded local batches without remote traffic, and the audited rank-10-20 band is denser (multi-key peer batches bounded by actual requests) instead of ~9 records per pass. An oversized-batch rejection is graded as an attributable failure rather than parsed and retried, so no honest capacity-limited peer is trust-penalized under the assumed store spread.
Closed replication event streams terminate cleanly without CPU spin or repeated warnings.
Graceful shutdown does not return while tracked detached storage or P2P work is still active.
When
shutdown()returns, timed-out long-lived tasks have been aborted and joined, no LMDB blocking operation is still running on either environment, and no engine-spawned task holds the storage, so the same LMDB files can be reopened safely.Review Response (grumbach, 2026-07-27)
Investigated all thirteen findings from the review at
4c9c681. Twelveconfirmed; one did not hold (see below). Nine were fixed, three deliberately
deferred with the reasoning recorded in ADR-0005 rather than left implicit.
Must fix
d3a1d56). The record wasstamped with the most recent rejection and aged from that stamp, so a source
that keeps overflowing the queue kept its own record permanently fresh:
check_bootstrap_drainednever returned true, the node advertisedbootstrapping: trueindefinitely, and auditing stayed off for the wholeduration of the pressure (Invariant 19). No attacker needed — an ordinarily
busy peer wedges the node open. Now recorded first-seen and never refreshed.
A fairness displacement also no longer stamps its victim; charging our own
reclaim decision to the displaced owner let a flooder block our drain through
an unrelated honest peer.
PaidNotifyverified inline on the serial loop (a84c42b). Not a boundedlocal computation: on the merkle path it performs an iterative Kademlia lookup
capped only by the verifier's
CLOSENESS_LOOKUP_TIMEOUT(240s). One messagecould park every other non-audit message for minutes while the bounded inbound
queue behind it overflowed and dropped unrelated replication traffic wholesale.
Moved onto the detached-responder pattern.
a84c42b), unlike everyother responder class. One peer could hold all sixteen slots, and since a
refusal is later read as absence by the sender's delayed possession check, the
resulting refusals land as audit-severity penalties on the refuser — making
an unbounded global pool a targeted eviction primitive. Structural checks
(missing proof, oversized payload) now run before the permit, so malformed work
cannot occupy a slot for the length of a verification.
Should fix
shutdown()could hang indefinitely (a84c42b,d79752e). The detacheddrain is deliberately unbounded — a timeout could return with an
Arc<LmdbStorage>still held — so it was made finite instead: workersemaphores are closed so queued work takes the (previously unreachable) error
arm and exits, superseded fresh offers are shed at dequeue, and payment
verification races the shutdown token. The cancellation boundary is drawn at
network I/O only;
storage.putawaitsspawn_blockingand is never cancelled.809f509).MAX_PENDING_VERIFYcounts keys, not the peers remembered against each one, and re-advertising an
already-pending key is the cheapest path through admission. Capped at eight,
with replica claimants displacing paid-only advertisers.
809f509), so an owner with manyin-flight retries read as under-loaded and won more than its share.
(
1094543). A fixed four-peer edge is a fifth of the production width but fourfifths of a configured width of five, leaving a voting core of one and
authorizing on a single
Confirmedvote. Now scaled to the group with anabsolute confirmation floor. Production behaviour at width 20 is unchanged.
MAX_PENDING_VERIFY_PER_PEERwas undocumented (1632ba7,809f509). ADR-0005 described the replacement but never recorded that itremoved a named defence, or that one sender's worst case rises 8,192 → 131,072.
Now stated with the adversarial numbers, plus a test pinning the convergence
property that replaced the hard cap.
Smaller
cca744c) —including why clearing timestamps before the commit would be strictly worse.
0a03c71); each built a throwawayAuditChallengeCoordinatorthat would have defeated the shared one.eeb068b): it used akey out of range for both gates, so every version of the code passes it, and
had no positive control. Rewritten against a key in the symmetric difference of
the two gates, with a control proving the harness observes a store at all.
Not fixed, and why
scheduling.rsrequeue_fetch_for_verificationleaving a stalecapacity_owner_by_keyentry — this one does not hold. The no-owner branchis only reachable when the lookup already returned
None(the.or_elserunsonly on
None), so there is no entry to leave behind.the sender to read the
Rejectedresponse — the receiver already returns aprecise reason, it is simply discarded — and to grade the possession outcome
accordingly. The per-source share bounds how large the effect can be made
deliberately, but does not remove it. Recorded in ADR-0005 Trade-offs.
by_sourcetop-k deferred.Three regressions caught and fixed during this work
Both were found by the e2e suite and confirmed against a stashed baseline rather
than assumed:
summarize_paid_list_voteswithout scaling edgemembership in
build_verification_targetsletcore + confirmed_edgeexceedthe real group size and demand more confirmations than existed.
PaidNotifylike a request/response responder (8 outstanding / 2 perpeer / 15s shed) discards durable paid-list evidence rather than shedding load,
because the message is one-way with no retry. A joining node silently lost
entries for a fraction of its responsible keys. Now sized as a memory ceiling.
Integration
Merged
origin/mainrather than rebasing (d86e3f1). The branch and maindiverged independently inside the same functions — main has none of this
branch's detached-task tracking and replaced the first-audit scheduler wholesale
— so replaying fifty-three commits would have meant roughly forty semantic
re-applications onto rewritten code. The merge resolved ten conflicts once. Its
commit message records each resolution, notably taking main's rewritten
first-audit scheduler over this branch's superseded
LruCacheimplementation,and keeping both the cumulative and windowed audit-drop counters, which are
complementary rather than duplicates.
d79752ethen re-tracks main's new first-audit launch, the one detached taskits rewrite spawned bare.
Verification
On the merged tree: 828 lib tests, 92 e2e (3 pre-existing ignored), 38 PoC tests,
clippy clean at
-D clippy::panic -D clippy::unwrap_used -D clippy::expect_used,rustfmt clean, release and
--no-default-featuresbuilds green.