test(network): prove block-sync retained-memory accounting#225
Draft
p0mvn wants to merge 3 commits into
Draft
Conversation
`RetainedPipelineBytes` and its `wire_bytes()` helper were removed when block-sync moved to exact retained-memory accounting, and the reactor inlined the equivalent sum. The offline bench trace path still referenced the deleted struct, so `--features internal-bench` failed to compile. Inline the same sum the reactor uses, so the bench emits an identical `retained_pipeline_wire_bytes` value. This was not caught because the crate's checks run under default features, where `bench.rs` is not compiled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BMbDnFH5FmsfPCENm6JP5j
The existing coverage bounds sampled trace peaks and asserts a final retained total of zero. That infers correct accounting from the absence of a visible plateau breach, so a charge that is transferred incorrectly, released twice, or briefly stranded can still pass. Add three layers that state the accounting laws directly: - Tracker property tests. A contender spinning `try_charge` at a saturated limit witnesses any headroom the tracker publishes, so growth exposing nothing is provable rather than sampled. Also pins racing resizes applying growth exactly once, clones sharing one charge, and all-or-none reservations never oversubscribing. - A test-only accounting probe over the live ledgers, built from the same `RoutineWiring` handles the production path threads to each peer routine. It outlives teardown, so the fuzz harness now asserts every ledger drains to exactly zero, with no slack. - A model-based transition test over `WorkQueue` + `RetainedBodyMemoryTracker`, asserting conservation after *every* generated operation, including the unmatched-body claim path and its RAII rollback. The model exists because teardown assertions are structurally blind to a transient leak: a reservation stranded by `reset_above` is swept by a later `advance_floor`, so the end state is clean. The model fails at the transition that stranded it. Each assertion was validated by mutation: reconcile as release-then-recharge, `reset_above` and `claim_received` stranding a reservation, `reconcile_exact` ignoring the exact size, `mark_issued` over-counting, and an uncommitted claim never rolling back are all caught, each by a different assertion. The probe supersedes `retained_memory_probe`, which is removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BMbDnFH5FmsfPCENm6JP5j
|
Note Complete: Audit complete. V12 did not find any issues that need review. Open the full results here. Analyzed 10 files, diff |
p0mvn
marked this pull request as draft
July 16, 2026 21:56
The plateau tests bounded peak retained memory with a hand-chosen `max_body x 128` arrival margin, unlike the sibling window term, which is derived from `MIN_BS_CHECKPOINT_SUBMITTED_BLOCK_APPLIES`. A hand-chosen margin cannot say whether a peak it admits is correct. Derive the ceiling from the admission gate instead. The retained-memory tracker's limit is `max_reorder_lookahead_bytes` (`L`), and above-window requests reserve against it atomically, so the gated lane alone can never exceed `L`. Exactly two paths charge without consulting the limit: - commit-window bodies, which take no reservation at issue and charge unconditionally at receipt, bounded by one exempt span; and - reconciliation growth, where an above-window reservation resizes from its estimate to the body's exact retained size without re-consulting the limit, bounded by the bodies that can be in flight at once. The old margin turns out to be the second term: `sum(peers.max_inflight_requests) x max_blocks_per_response` is exactly 128 for the single-block scenarios, so the derived ceiling reproduces the previous bound for two of the three tests. For the multi-block scenario the same quantity is 2048, so its previous margin was 16x too small and passed only because reconciliation growth stays far below worst case. Measured peaks show the commit-window term dominates by two orders of magnitude: the plateau test retains ~227 MB against an 8 MiB look-ahead budget, of which ~227.7 MB is the exempt span. The configured soft cap is not what sizes block-sync resident memory. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BMbDnFH5FmsfPCENm6JP5j
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
PR #217 makes block-sync retained-memory accounting exact. Its coverage bounds sampled trace peaks and asserts a final retained total of zero. Both are useful, but neither states the accounting laws:
ByteBudget::audit()is one-sided (actual >= expected), so excess charges pass;max_body_bytes × 128arrival margin — a hand-chosen heuristic, unlike the siblingwindow_slackterm, which is derived fromMIN_BS_CHECKPOINT_SUBMITTED_BLOCK_APPLIES;final_retained_memory_bytes == 0is exact and valuable, but end-state only.Together these infer correct accounting from the absence of a visible plateau breach. A charge that is transferred incorrectly, released twice, or briefly stranded can still pass every one of them.
Solution
Three layers that state the laws directly. All test-only; no production logic changes.
Tracker property tests (
retained_memory.rs). A contender spinningtry_chargeat a saturated limit witnesses any headroom the tracker publishes — every byte it wins is headroom that really existed, so "growth exposed nothing" is provable rather than sampled. Also pins racing resizes applying growth exactly once, clones sharing one charge, and all-or-none reservations never oversubscribing.Accounting probe (
accounting_probe.rs, new). Reads the live ledgers via the sameRoutineWiringhandles the production path threads to each peer routine. It owns its handles, so it outlives teardown — the state in which a zero-slack assertion is meaningful. The fuzz harness now asserts every ledger drains to exactly zero, with no slack. It supersedesretained_memory_probe, which is removed.Model-based transition test (
work_queue_model.rs, new). Generated operation sequences overWorkQueue+RetainedBodyMemoryTracker, asserting conservation after every operation, including the unmatched-body claim path and its RAII rollback.The model exists because teardown assertions are structurally blind to a transient leak: a reservation stranded by
reset_aboveis swept by a lateradvance_floor, so the end state is clean. The model fails at the transition that stranded it — see Testing.The model tracks accounting, not scheduling. Which heights
take_in_range_budgetedselects is driven from the call's own result rather than re-derived; its contiguity and byte-cap rules already have unit tests, and reimplementing them in the model would only test the copy.Testing
cargo test -p zakura-network --lib block_sync— 260 passedcargo test -p zakura-network --lib blocksync_fuzz— 21 passedcargo clippy -p zakura-network --all-targets— 0 warningscargo clippy -p zakura-network --lib --all-features— 0 warnings (see the bench commit)cargo fmt --all -- --checkEvery assertion was validated by mutation testing rather than trusted because it was green. Each mutation is caught, each by a different assertion:
reconcile_exactas release-then-rechargereset_abovestrands a memory reservationclaim_receivedstrands a memory reservationreconcile_exactignores the exact sizemark_issuedover-counts reserved bytesThe
reset_abovecase is the load-bearing one: it passes the zero-slack teardown assertion, becauseadvance_floorsweeps the stranded reservation before teardown. The model catches it at step 2 and shrinks to a 3-step counterexample. End-state and per-transition assertions cover genuinely different classes of leak.Two tests were discarded or rewritten because mutation showed they passed under the exact bug they targeted:
Arcpointer equality was ABA-unsafe (the allocator reuses the freed address, so release-then-recharge passed) — deleted, since the contender covers it soundly;live_bodiesback out of the implementation under test, making it circular — the model now tracks the expected size independently.No accounting bugs were found in #217. Every mutation above is artificial. The accounting held under all of them, which is the intended result: this PR converts "we believe it is correct" into "these properties are proven".
Derived the retained-memory bound. The plateau tests used a hand-chosen
max_body × 128arrival margin. Deriving it from the admission gate instead:Lismax_reorder_lookahead_bytes. Above-window requests reserve against it with an atomictry_reserve_many, so the gated lane alone can never exceed it. Exactly two paths charge without consulting the limit: commit-window bodies (exempt, no reservation at issue, charge unconditionally at receipt) and reconciliation growth (an above-window reservation resizes from estimate to exact retained size without re-consulting the limit).The old margin turns out to be the second term:
Σ peers(max_inflight_requests) × max_blocks_per_response= 2 × 64 × 1 = exactly 128 for the single-block scenarios. The derived ceiling reproduces the previous bound identically for two of the three tests, which is a good sign the derivation matches the original reasoning. For the multi-block scenario the same quantity is 2048, so its margin was 16× too small and passed only because reconciliation growth stays far below worst case.The headline finding is what the measurement showed, not the margin. Peaks against the derived terms:
Lfuzz_commit_stall..._resident_plateauThe peak tracks the commit-window term to within 0.02% and is unrelated to
L.max_reorder_lookahead_bytesis not what sizes block-sync resident memory in these scenarios — the commit-window exemption is, and it is not configurable. Nothing amplifiesLhere: the exempt bodies never take a reservation against it, so loweringLcannot lower the peak.Two independent multipliers produce that 227.8 MB, and only the first is structural:
MIN_BS_CHECKPOINT_SUBMITTED_BLOCK_APPLIES).attributed_memory_size_bytescharges a fixedinline_size_bytes::<Transaction>()per transaction, so tiny transactions make that overhead dominate. Measured against real mainnet vectors, where transactions are ~1.2–1.7 kB:MAINNET_1MAINNET_982681MAINNET_434873So the plateau tests run ~5–7× pessimistic on decoded expansion. That is the safe direction for a ceiling, but the absolute MB figures in these tests should not be read as representative of mainnet.
Specifications & References
Stacked on
feat/exact-block-sync-retained-memory(#217); please merge that first.AI disclosure: authored with Claude Code.
Follow-up Work
Lis irrelevant to it.BS_CHECKPOINT_RANGE_BYTE_FLOORalready encodes the wire half of this (401 × 2 MB= ~802 MB), so the floor is known and intentional — checkpoint ranges must be co-resident to commit. What the tests add is that it is the dominant term, and that loweringmax_reorder_lookahead_bytescannot bring resident memory below it. Sizing it with the measured real-block expansion (~2.3–3.1×) rather than the synthetic 16×, a worst-case 2 MB-block range costs roughly401 × ~8 MB≈ 3 GB, on top of the 1.5 GiB defaultL. Typical mainnet blocks (a few kB) make the same term ~20 MB and irrelevant. So the exposure is real but confined to sustained large-block stretches. Worth a design conversation for the OOM work; not a test gap.prepare_submitclones the charge,retain_for_driver_in_placeresizes it, detach/unsubmit/resubmit move it). That is the largest remaining transient-leak surface.fuzz_reorgremains ignored: a faithful mid-sync reset needs the high-fidelityCommitter<MockVerifier>tier.