fix(replication): scope round-2 audit openings and apply the audit wire ceiling on encode - #194
Open
grumbach wants to merge 3 commits into
Open
fix(replication): scope round-2 audit openings and apply the audit wire ceiling on encode#194grumbach wants to merge 3 commits into
grumbach wants to merge 3 commits into
Conversation
Round 2 of the storage-commitment audit authorised an opening if its key was anywhere in the pinned commitment, rather than in the subtree that round 1 actually proved. Round 1 proves one subtree of roughly the square root of the commitment's key count, so a caller could complete a cheap round 1 over small records and then request openings against large records elsewhere in the same commitment. The work round 2 costs was therefore not bounded by the work the caller had already paid for in round 1, and the per-peer cooldown that would otherwise limit repetition is escapable by presenting a different peer identity. Openings are now checked against the round-1 subtree, and a challenge naming any key outside it is refused whole before a single chunk is read. This matches how an over-broad challenge is already handled a few lines earlier. The authorised set is recomputed from the pinned tree and the audit nonce rather than stored in the round-1 session. The subtree is a pure function of those two, and both are already pinned, so recomputing costs one tree walk with no chunk reads. Storing the key list instead would have cost up to a full subtree of keys for every live session, which is the wrong trade when sessions are bounded by count rather than by size. An honest auditor cannot trip the new check: it samples only the leaves round 1 returned, and without that leaf's committed block-tree root and content length it has nothing to verify an answer against, so an opening outside the subtree could tell it nothing even if it were served. A failure to rebuild the subtree is reported as transient rather than as a protocol error, because the tree belongs to this node and round 1 already walked it successfully, so an inconsistency there is local and should not brand the node with a confirmed audit failure. The accompanying test drives one live round 1 and then asserts both halves from its result, so they cannot drift apart: a leaf inside the proved subtree is still served, and a key that is genuinely committed and whose bytes are present on disk is refused solely for being outside the subtree. Verified to fail without the change, where the responder serves a complete block opening for a key round 1 never proved.
…wn variants Two gaps in how the replication wire-size ceiling was selected, both of them places where the rule "a body that cannot be classified must not be treated generously" was not being applied. The audit message families take a much tighter ceiling than core replication, because a core body legitimately carries a whole chunk while an audit body never does. That ceiling was enforced when decoding but not when encoding, where only the core limit applied. An audit body that outgrew the audit ceiling would therefore serialise successfully and then be dropped, before decoding, by every peer that received it. Because that drop is silent, the sender would be scored for the resulting non-answer instead of being told its message was unsendable. Encoding now measures against the same family ceiling the decoder uses, so the failure surfaces locally, at the point the oversized body was built. No legitimate body reaches the limit today: the largest an honest audit can produce is a round-1 proof at the commitment key-count cap, which an existing test pins under the ceiling with headroom. Separately, the family lookup answered "core" for any index outside the declared variants, by way of a catch-all arm. An index no variant declares cannot be a legitimate message of any family, so treating it as core handed it the larger allowance on both the encode and decode paths. The lookup now returns no family for such an index, and every caller gives that the strict ceiling, matching how a prefix too malformed to parse is already handled. Postcard rejects an unknown outer discriminant before decoding any trailing collection, so this closes the invariant rather than a reachable hole; the value is that a variant added later without a family entry fails loudly instead of quietly inheriting the larger limit. Two existing tests built oversized audit payloads by calling the encoder, which now refuses them. They exercise the receive path, so they construct their bytes by serialising directly instead. That is also a more faithful reproduction: a hostile peer does not run this encoder, and a test of what arrives should not depend on the sender being willing to produce it.
The per-peer cooldown limits how often one identity may demand a round-1 proof, but peer identities are free to mint, so a caller rotating them faced no sustained bound at all. Honest auditors were then refused for capacity, and because a refused audit becomes a graced timeout, that failure is silent: coverage falls with no peer penalised and nothing in the logs. Round-1 challenges are now served only to peers in this node's routing table, checked before any concurrency permit, cooldown entry or work budget is touched. Sustained work becomes the product of two bounds that already existed individually: how many peers may ask, and how often each may ask. Even at saorsa-core's theoretical maximum routing table that is under three proofs per second, and a realistic table puts it an order of magnitude lower. The gate is not a new policy. Inbound replication hints and commitment gossip are already accepted only from routing-table peers, for the same reason: off-table identities are free, so anything they can reach is unbounded. It is also why the gate cannot refuse an honest auditor — a peer audits this node because it saw this node's gossiped commitment, and that gossip is itself accepted only from routing-table peers, so an auditor able to derive a challenge from our gossip is one we exchange gossip with. An aggregate work budget was implemented for this twice, and abandoned both times. The reason is recorded at the dispatch and at the cooldown constant, since the idea is the obvious one and will occur to the next reader: any shared limit sheds audits when exhausted, a shed audit is graced, and identities are free — so an aggregate limit is a cheaper way to suppress every audit of a node than the resource exhaustion it would be added to prevent. Bounding who may ask is safe in a way that bounding the total is not. What it does not claim: a determined operator running real DHT nodes can still place identities in a routing table, subject to that layer's per-IP and per-subnet diversity limits. This removes clients, drive-by packet sources and anything not participating in the DHT, and turns an unbounded population into a bounded one, which is what makes the cooldown a real limit rather than a formality. The cost is stated rather than hidden: a node whose routing table has not filled refuses audits it would otherwise serve. Those land in the graced timeout lane so nobody is penalised, and a young node has correspondingly little to prove, but it is audited less until its table fills.
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
1 participant
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.
Linear issue
Risk tier
T2 rather than T3, and the boundary is worth stating since the parent PR was T3. Nothing here changes the wire format, the proof shape, stored data, economics or the upgrade mechanism. What does change is responder behaviour: a round-2 challenge that was previously served is now refused if it names a key outside the proved subtree, and an oversized audit body now fails at the sender instead of at every receiver. That is behavioural surface, hence T2 rather than T1.
Compatibility
family_of_variant(crate-internal) now returnsOption<BodyFamily>; aceiling_forhelper is added beside it.Semver impact
Both changes are defect fixes in behaviour, but one alters a crate-internal signature (
family_of_variantnow returns anOption). On a0.xcrate that is a minor bump, which is whatfeaturedelivers.Test evidence
cfdclean: clippy-D warningsacross all targets and features, fmt,cargo doc --deny=warnings.poc_audit_handler_live16/16: the production responder against real LMDB, including an out-of-subtree key refused with its bytes present, the same refused with its bytes absent (so the refusal is shown not to depend on storage contents), and a challenge mixing an authorised with an unauthorised opening refused whole.poc_commitment_audit_attacks19/19.Both fixes were verified to fail without the change they cover. Each production line was reverted, the test observed to break, and the line restored. Reverting the scope check produces a served
Presentopening for a key round 1 never proved; reverting the encode ceiling lets a 6.9 MB audit body encode.Not run: the e2e testnet suite. Nothing here touches the proof or the wire, and the audit e2e set passed on this work before the rebase onto current
main.New dependency
None.
ADR
n/a — no architectural decision changes. ADR-0009 already describes the audit proof shape and protocol families, and nothing here alters it.
Mitigation / rollback
Revert the branch. No stored data changes and no migration. The two changes are independent, so either commit can be reverted on its own. The round-1 pool size is already config-driven, so admission pressure can be relieved without a code change.
What this is
Two of the three findings reviewers raised against #181 and agreed were follow-ups rather than merge blockers. Both are responder-side hardening. The third is deliberately excluded; see below.
Round 2 authorised any key in the pinned commitment, not just the subtree round 1 proved. A cheap round 1 over small records could therefore authorise openings against large records elsewhere, so round 2's cost was not bounded by what the caller had already paid for.
Stated precisely, because the one-line version reads worse than the fact: the keys were always ones the responder had committed to, and nothing outside the commitment was ever served.
mainhas the identical whole-commitment gate today and serves full chunk bytes for it. An honest auditor cannot reach the case either, since verifying an opening needs the round-1 leaf and it has none outside the subtree.The audit wire ceiling applied on decode but not on encode, so an oversized audit body would serialise and then be silently dropped by every receiver — and the sender scored for the resulting non-answer. Both directions now share one selector rather than two hand-synced matches. Separately, an undeclared discriminant used to classify as "core" and inherit the larger allowance; it now classifies as nothing, and nothing takes the strict ceiling.
The third finding is deliberately NOT here
Reviewers also raised that round-1 work is bounded by bytes but not by rate: nothing bounds a chunk from below, so a commitment of small records runs a full subtree of lookups per audit while charging almost nothing, and a caller rotating peer identities never trips the per-peer cooldown.
I implemented a responder-wide rate bucket for that and withdrew it before opening this PR, because review showed it made audit suppression cheaper rather than harder. Keeping both heavy workers busy needs a sustained stream of full proofs; holding a global rate bucket empty needs roughly one small proof per second, and the caller picks the audit nonce, which picks the proved subtree. Since a refused audit becomes a graced timeout, a globally shed refusal suppresses every audit of that node for free.
Answering a spent budget with a rejection, to make suppression cost something, was also tried and reverted: the rejection grades as a timeout, which is graced, so it never penalises a deleter, and holder credit expires on its own regardless — while an intermittent flood could use it to zero an honest node's credit cheaply.
The conclusion, reached independently by several reviewers, is that a global limit cannot be added safely without deciding who may spend it. That needs an eligibility or fair-share admission rule, which is a design change deserving its own PR and its own review rather than being bolted on here. Tracked on the Linear issue.
Review
This went through several review rounds with independent reviewers, before and after the rebase onto current
main. They corrected the analysis more than once, found a concurrency race the exactness tests structurally could not see, and showed that two tests proved nothing. The final round is why the rate budget is not in this PR.Two claims of mine were also walked back rather than left standing: that the absent-bytes test proves storage was never read (it proves the refusal does not depend on storage contents, which is weaker and is what the comment now says), and that an unclassified body would "fail loudly" at encode (only if it exceeds the strict ceiling).