refactor(network): remove unneeded Zakura block-sync memory knobs and dedupe accounting#413
Merged
Merged
Conversation
… dedupe accounting Hard-remove two config fields that never needed operator tuning (configs carrying the keys no longer parse): - max_reorder_lookahead_blocks: the defense-in-depth block-count cap is now the fixed admission::LOOKAHEAD_BLOCK_HARD_CAP (262,144) constant. The knob's validate/clamp plumbing and default constant go with it; the gate itself stays, since it still binds ahead of the byte gate for tiny bodies (average wire size under ~6.1 KB) whose per-entry bookkeeping overhead the flat resident factor does not model. - fanout: a legacy reservation multiplier no scheduler ever used; it only inflated the floor_request_byte_reservation validation floor. The mock harness plumbing (ZAKURA_MOCK_BS_FANOUT) goes with it. Dedupe and trim the surrounding accounting surface: - One formula for retained pipeline bytes: the new RetainedPipelineBytes helper backs estimated_resident_pipeline_bytes, the reactor's retained_pipeline_wire_bytes emission, and the bench emitter — which previously omitted the sequencer-input term, so bench traces underreported retention to the fuzz invariant reader (bench-only bugfix). - Delete DEFAULT_BS_MAX_SUBMITTED_BLOCK_APPLIES (exact alias of MIN_BS_CHECKPOINT_SUBMITTED_BLOCK_APPLIES). - Reduce DESERIALIZED_MEM_FACTOR and MIN_BS_CHECKPOINT_SUBMITTED_BLOCK_APPLIES to pub(crate): no out-of-crate consumers. - Drop the stale #[allow(dead_code)] on ByteBudget::subscribe_capacity (it is consumed by the peer-routine run loop). Behavior-preserving at shipped defaults: admission decisions are bit-identical for any config that left the removed knobs at their defaults, and the resident-plateau fuzz anchors pass unchanged.
|
Note Complete: Audit complete. No review-worthy issues remain after automatic triage. Two findings were auto-invalidated. Open the full results here. Analyzed nine files, diff |
p0mvn
marked this pull request as draft
July 2, 2026 18:49
The tests module still imported the deleted DEFAULT_BS_MAX_REORDER_LOOKAHEAD_BLOCKS constant, and the pub(crate) re-exports of DESERIALIZED_MEM_FACTOR / MIN_BS_CHECKPOINT_SUBMITTED_BLOCK_APPLIES tripped -D unused-imports on non-test builds (their only consumers are the fuzz-testkit test modules). Gate both re-exports on cfg(test).
Author
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 7cb54d7. Configure here.
p0mvn
commented
Jul 2, 2026
p0mvn
commented
Jul 2, 2026
p0mvn
commented
Jul 2, 2026
Same CI race fixed for the genesis-convergence e2e: connect_all fires PeerConnected at both reactors concurrently, and a status emitted before the receiver has processed its own PeerConnected is rejected as unknown-peer misbehavior. The source's frontier never changes in this test, so nothing re-sends the status and the tip wait runs out its 30s deadline on loaded CI runners. Wait for both peers to register, then deliver the source's status deterministically; a duplicate of the natural status is harmless.
p0mvn
marked this pull request as ready for review
July 2, 2026 20:43
p0mvn
commented
Jul 2, 2026
evan-forbes
reviewed
Jul 2, 2026
| /// `effective_budget / (DESERIALIZED_MEM_FACTOR × 262_144)` wire bytes | ||
| /// (~6.1 KB at the default budget), i.e. for tiny early-chain bodies whose | ||
| /// per-entry bookkeeping overhead the flat resident factor does not model. | ||
| pub(super) const LOOKAHEAD_BLOCK_HARD_CAP: u64 = 262_144; |
There was a problem hiding this comment.
why even have a hardcap though
would it be simpler if we simply only use the byte budget and make that the only thing we care about?
Author
There was a problem hiding this comment.
It's mostly for initial safety while we make our byte accounting more reliable
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
2 participants
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
The #386 fix left the memory-management surface with parameters and duplication that carry no operator value: a block-count cap knob that is pure defense-in-depth on bookkeeping size, a legacy
fanoutmultiplier no scheduler ever used, an exact-alias default constant, three near-duplicate pool-sum formulas (one of which underreported retention in bench traces), and internal calibration constants exported crate-publicly with zero out-of-crate consumers.Solution
Removed config fields (hard removal — configs carrying the keys no longer parse; stored-config fixtures updated):
max_reorder_lookahead_blocks→ the gate survives as the fixedadmission::LOOKAHEAD_BLOCK_HARD_CAP(262,144). The gate is kept (not deleted) deliberately: it binds ahead of the byte gate for tiny bodies averaging under ~6.1 KB wire, where per-entry bookkeeping overhead isn't modeled by the flat ×4 resident factor. Its validate zero-check, clamp branch, and default constant are gone.fanout→ deleted along with itsfloor_request_byte_reservationmultiplier and the mock-harness plumbing. At the default (1) the reservation math is value-identical; forfanout > 1configs the validation floor only loosens.Deduped accounting:
RetainedPipelineByteshelper is now the single formula behindestimated_resident_pipeline_bytes, the reactor'sretained_pipeline_wire_bytestrace emission, and the bench emitter. The bench emitter previously omitted the sequencer-input term — its trace rows underreported retention to the fuzz invariant reader. That is the one intended value change (bench-only bugfix); the production reactor emission is algebraically identical.DEFAULT_BS_MAX_SUBMITTED_BLOCK_APPLIESdeleted (exact alias ofMIN_BS_CHECKPOINT_SUBMITTED_BLOCK_APPLIES).DESERIALIZED_MEM_FACTORandMIN_BS_CHECKPOINT_SUBMITTED_BLOCK_APPLIESreduced topub(crate)(no out-of-crate consumers; testkit is same-crate).#[allow(dead_code)]removed fromByteBudget::subscribe_capacity(it is live in the peer-routine run loop — the compiler now proves it).Behavior-preserving at shipped defaults:
admit()decisions are bit-identical for any config that left the removed knobs at default.