feat(quote): refuse quotes to clients that cannot settle correctly - #204
feat(quote): refuse quotes to clients that cannot settle correctly#204grumbach wants to merge 11 commits into
Conversation
A merkle batch pays on-chain before any storer sees a PUT, so checking the settlement rule at PUT time checks it after the money is gone. Merkle receipts are not refundable, so every such rejection destroys a user's payment. Production is currently rejecting a steady trickle of uploads for an exact 3x underpayment, which is the signature of a client that predates the ADR-0008 multiplier and applies none at all. Handle the settlement-version quote requests and refuse any client below MIN_SUPPORTED_SETTLEMENT_VERSION with ClientUpdateRequired. No quote means no pool commitment, which means no payment, so a refused client has spent nothing. A version NEWER than this build understands is deliberately served: the storer still verifies whatever payment arrives, so nothing is weakened, while refusing would let a node that has not been upgraded veto a rule set the network has already moved to. Unversioned requests are still served. A node cannot distinguish a client that settles correctly but predates the version field from one that does not, so refusing both would break clients that are behaving. A running count of unversioned quotes is logged every 1000 per path, which is the evidence needed to decide when that policy can be flipped. Also extend the underpayment rejection itself. An exact multiplier shortfall is an outdated client rather than a pricing dispute, so the message now says so and tells the reader how to upgrade, noting the payment already made cannot be recovered. This is the only signal that reaches clients losing money today, because a client too old to settle correctly is also too old to declare a settlement version at quote time. The advice is keyed on an exact shortfall so a merely-cheap payment is not misreported as a stale client. Pins ant-protocol to the branch carrying the wire types while WithAutonomi/ant-protocol#23 is in review.
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
Reviewed exact head 2f7a05ff6e7575e732c6b3ae09c9f3ada66beb97 with the coordinated protocol/client PRs.
The local gate implementation is straightforward and the focused settlement tests pass 11/11. I am requesting changes on the coordinated behaviour rather than the handler mechanics:
handle_quote_v2/handle_merkle_candidate_quote_v2inheritsettlement_version_is_supported(version >= MIN). An older node consequently quotes to an arbitrarily newer settlement version it cannot understand. That forfeits the pre-payment compatibility guarantee; actual PUT-time verification is too late once payment has settled.- Legacy requests remain unconditionally served (
src/storage/handler.rs:288-296). Combined with the client PR retrying V2 after anyNetwork/Timeout, a transient failure or dropped V2 response can downgrade an upgraded peer to the ungated legacy path. This becomes payment-burning again whenMIN_SUPPORTED_SETTLEMENT_VERSIONis next raised.
Please land the superseding ADR and specify a downgrade-resistant rollout/capability policy, then add a mixed-fleet test covering legacy node, upgraded node, structured refusal, lost refusal/timeout, and send failure before merging.
CI is not fully green: Windows tests failed because two testnets could not initialise the dual-stack transport (94 passed, 2 failed), apparently unrelated but still unresolved. The manifest also points at a contributor branch and must be changed to the published ant-protocol = "2.4.0" before merge.
Review raised that inheriting "any version at or above the minimum" lets an older node quote an arbitrarily newer client, which forfeits the pre-payment guarantee the gate exists for. PUT-time verification is too late: by then the client has settled on-chain and cannot be refunded. Refuse both directions, using the bounded settlement_compatibility check. A version above this node's own is answered with StorerUpdateRequired rather than ClientUpdateRequired, because the client is fine and this node is the one behind. The client should route to another storer and tell its user nothing. Telling an up-to-date user to upgrade would be wrong, and during the client-first rollout ADR-0008 prescribes it would be wrong for most of the fleet at once. The test that pinned the previous behaviour is inverted rather than deleted, so the corrected policy is the one under regression cover.
Captures the decision the coordinated protocol/node/client change implements, as requested at review: the inclusive MIN..=CURRENT range and why the upper bound is load-bearing, the two refusal directions and why they must stay distinct during a client-first rollout, the unversioned retry as a bounded downgrade path with a compile-time cutover rule, and the residual that merkle storers are not exactly the peers a client quotes. Also records what is not fixed: the clients burning money today are too old to declare a version, so only the reworded error reaches them.
Keeps the lockfile at the commit CI resolves for the branch pin. No source change; picks up the ruint advisory bump made on the protocol branch.
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
Follow-up review of exact head 26b34e35f94c08c4ff513c4ad9ff1aa06bb67861 across the coordinated protocol/node/client set.
The node-side code blocker from my previous review is resolved: settlement_gate() now maps ClientTooOld to terminal ClientUpdateRequired and NodeTooOld to skippable StorerUpdateRequired. Local focused settlement tests passed 11/11, and completed CI checks are green.
The coordinated merge gates are not complete yet:
Cargo.tomlstill uses the mutable personal-fork branch pin. Merge/publish ant-protocol 2.4.0 first, then repin this PR to the registry release and regenerate the lockfile.- ADR-0010 remains Proposed and explicitly records the mixed-version dev-testnet validation as outstanding. For this T3 pre-payment gate, that run should cover legacy/upgraded nodes, structured refusal, lost response/timeout and send failure before this PR merges.
- Three platform test jobs are still running at review time.
- The PR body still says future settlement versions are served and cites the removed
a_newer_settlement_version_is_not_treated_as_an_errortest; the implementation now deliberately refuses them. Please update the body before sign-off.
No additional handler defect found, but the stated release/validation gates remain open.
Versioning is the release train's call, so the comment now points at 'a published version pin' rather than naming one that has not been decided. Lockfile follows the protocol branch, which no longer carries a bump. Also records in ADR-0010 that the semver impact is declared, not taken.
…rule The document said the unversioned retry is build-enforced before the minimum can rise. That held for the merkle path only; the independent single-node retry was unguarded. Both now reference one shared constant, and the text says so. Also records why a refusal must not depend on which peers answered first: the collector drains every launched peer rather than stopping at the quote target, and the verdict is kept outside the collection timeout whose elapsed arm deliberately falls through.
The new-client-against-old-fleet case is no longer outstanding. ant-client's merkle E2E spawns a 35-node testnet from the published ant-node, which predates the versioned requests, so the suite is a live mixed-version run. It passed functionally and failed on cost: the suite went from a 24-38 minute baseline to exceeding the 60-minute CI cap with 4 of 7 tests done, because a peer that cannot decode the versioned request never answers and the client waited a full quote timeout before falling back, on every request rather than once per peer. That is the kind of defect a unit test cannot surface, which is why the reviewer was right to ask for this. Records the two client-side bounds that came out of it, the misjudgement tradeoff the probe ceiling accepts, and what remains unproven: the reverse direction needs a testnet built from this branch's ant-node, which is not available until the coordinated set lands.
…venly The upgrade advice on an underpayment was gated on `expected == paid * required_multiplier`. That assumes the expectation is linear in the multiplier, and it is not: `merkle_expected_per_node` floors after multiplying, so at median 901 and depth 7 the parity expectation is 49_426 while three times the bare expectation is 49_425. The check therefore asked `49_426 == 49_425` and stayed silent on a settlement that really was unmultiplied. That silence fell on exactly the population the advice exists for, at every depth whose leaf count is not divisible by the depth. Compare against the bare expectation instead, which is the same arithmetic the expectation was built with and holds everywhere. The predicate is extracted so the case can be pinned directly rather than needing a depth-7 proof fixture. Also corrects a comment in the quote gate that still described the previous policy, claiming a newer settlement version is "deliberately allowed through" immediately above the code that refuses it as StorerUpdateRequired. Records in ADR-0010 the guarantees the tests do and do not provide: no test drives a real collection to timeout, merkle storers are not exactly the quoted peers, a deployed client binary cannot be reached by a source-level guard, and a refusal in a later sub-batch arrives after earlier sub-batches have already paid.
…rule Three corrections from a second review pass. The decision text said a client-first rollout must stay possible while the implemented upper bound makes an old node refuse a newer client. Both are true but the first was stated too strongly: the rollout stays usable because the refusal is not a client fault and a newer client routes elsewhere, but node rollout is now a prerequisite for the next settlement bump rather than merely desirable. Says so plainly. The cutover rule keyed deletion of the unversioned retry on MIN alone, while the compile guard requires both MIN and CURRENT. Raising CURRENT creates the node-behind refusal as soon as any node lags, and the retry routes around that too. Records why a refusal needs corroboration before it is believed, and why the verdict is then held client-wide, plus the two trade-offs that come with it: a two-peer denial of service replaces a one-peer one, and a later sub-batch still returns its already-paid proofs rather than stranding that spend.
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 1a8ff66408cae9915605ef39627dafac4528644f. The prior code findings are resolved: compatibility is bounded in both directions, ADR-0010 now describes both downgrade paths accurately, and the uneven-depth stale-client diagnostic uses the correct bare expectation. Focused local settlement tests and the new depth-7 regression pass; all 15 GitHub checks are green.\n\nOne merge blocker remains: Cargo.toml still uses the mutable personal-fork branch github.com/grumbach/ant-protocol#settlement-version-quote-gate. Please merge/release ant-protocol #23, repin this PR to the published registry version, and regenerate Cargo.lock. Also refresh the PR body: its Windows-failure note is stale because the current Windows unit/build jobs pass.
A shorter probe ceiling was tried to bring the slower CI runner under its job cap, and independent review showed it would silently re-enable the loss this ADR exists to prevent: the probe wait is the only window in which a peer can refuse, and the fallback re-asks under a new request id, so a refusal arriving after the ceiling is answering a request nobody is listening to. Records the resulting rule, that the ceiling stays at or above the largest production quote timeout, and that the remaining suite cost is an artifact of the temporary fork-branch pin rather than of the design.
Merging puts this in the next release, so the bar is fleet-ready rather than code-complete, and green CI is evidence for the code gate alone. Enumerates what is still open: the mutable protocol branch pin, mixed-version proof over a real connection, deployment ordering, observability of the adoption counter, NAT/canary, rollback rehearsal and fleet safety. Calls out that deployment ordering is the one gate that is not inert. The refusal machinery cannot fire while MIN and CURRENT are both the first declarable version, but against a fleet that cannot answer a versioned request every first contact still costs a probe wait, so releasing the client ahead of the nodes adds real latency to cold uploads.
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
Re-reviewed exact head a4aa14e0c1133b677347033a7549a8daea509946. The incremental changes are ADR-only and introduce no executable regression. The revised release-gate and probe-ceiling analysis is materially accurate. Local formatting and focused settlement tests pass 11/11; all 15 GitHub checks are green. No node-code blocker remains.
One merge blocker remains unchanged: Cargo.toml still resolves ant-protocol through the mutable personal-fork branch grumbach/ant-protocol#settlement-version-quote-gate. Protocol #23 is still open and the latest registry release remains 2.3.2, so a stable repin is not yet possible. Please merge/publish the protocol release, replace this with the registry version and regenerate Cargo.lock; I will then perform the final sign-off.
Fleet validation, observability, canary and rollback items in ADR-0010 are production-release gates rather than additional code findings on this head.
2 participants
Linear issue
V2-975 — https://linear.app/autonominetwork/issue/V2-975/refuse-to-quote-clients-that-cannot-settle-correctly-instead-of
Risk tier
New wire handling on the payment-admission path.
What this fixes
Production nodes reject a steady trickle of merkle uploads for underpayment at an exact 3x ratio. The node is right and the client is wrong: those clients settle under the pre-ADR-0008 rule and apply no multiplier.
The problem is when we refuse. A merkle batch pays on-chain before any storer sees a PUT, so checking the settlement rule at PUT time checks it after the money is gone, and merkle receipts are not refundable. ADR-0008 already names this as a re-open trigger: "a rise in refused batch uploads after the boundary, indicating clients that never upgraded." That trigger has fired.
Change
Refuse at quote time instead. No quote means no pool commitment, which means no payment, so a refused client has spent nothing.
MIN_SUPPORTED_SETTLEMENT_VERSIONgetsClientUpdateRequiredwith an upgrade instruction.CURRENT_SETTLEMENT_VERSIONgetsStorerUpdateRequired. This node is the old one; the client should use a different storer and tell its user nothing.ant_node::quote::settlement, which is the evidence for flipping that later.Changes since first review
Compatibility
CHUNK_PROTOCOL_IDbump. Every existing message shape is handled as before. Deployment ordering matters in one direction: nodes before clients, since a node on the current publishedant-protocolcannot decode versioned requests. The client PR covers the mixed fleet with a per-peer fallback, so this is a preference rather than a hard gate.Semver impact
No version bump is taken in this PR; the release train owns that.
Test evidence
cargo test --lib— 930 passed, 0 failed.cfd(fmt + clippy with-D clippy::panic -D clippy::unwrap_used -D clippy::expect_used -D warnings) clean.Gate behaviour (
storage::handler):v2_merkle_quote_is_served_at_the_current_settlement_version— the gate is invisible to clients that can pay.v2_merkle_quote_is_refused_below_the_minimum_settlement_version— asserts the refusal type, both versions, and that the rendered message containsant update.v2_single_node_quote_is_refused_below_the_minimum_settlement_version— so a refused merkle client cannot fall back to per-chunk quotes and burn money that way instead.a_newer_settlement_version_is_refused_as_this_nodes_fault— the corrected upper bound, assertingStorerUpdateRequiredspecifically.unversioned_requests_are_still_served— the decision that would break correctly-paying clients if it regressed.Message behaviour (
payment::verifier):merkle_legacy_1x_settlement_rejected_after_the_parity_boundaryextended to assert the upgrade advice on an exact 1x settlement.a_partial_shortfall_is_not_blamed_on_an_outdated_client— a one-wei shortfall rejects without upgrade advice, so the advice keeps meaning something.Mixed-version validation
No longer outstanding for the direction that could be tested.
ant-client's merkle E2E spawns a 35-node testnet from the publishedant-node, so it is a live new-client-against-old-fleet run: every node logs a decode failure for each versioned probe and answers only the unversioned retry.It passed functionally and failed on cost, which is what makes it worth having run: the suite went from a 24–38 minute baseline on
mainto exceeding the 60-minute CI cap with 4 of 7 tests done. A peer that cannot decode never answers, so the client waited a full quote timeout before falling back, on every request rather than once per peer. Two bounds on the client PR fix it. Full write-up in ADR-0010.Still outstanding: the reverse direction, old client against an upgraded node, and a genuinely mixed fleet. Both need a testnet built from this branch's
ant-node, which does not exist until the coordinated set lands.CI
All 15 checks green on the current head,
Test (windows-latest)included.That job failed on an earlier head, in testnet setup ("Failed to create dual-stack network nodes") rather than on an assertion, and the same job fails on
mainin both of its recent failing runs. It passes here on re-run, which settles it as the known hosted-runner flake rather than anything in this change.New dependency
none.
ant-protocolis temporarily repointed at the review branch for WithAutonomi/ant-protocol#23, and reverts to a published version pin once that merges and the release train publishes it. That repin is a merge-order dependency, not something this PR can take: publishing is the train's step, and per repo policy a feature PR does not bump or name crate versions.ADR
ADR-0010: Settlement version and pre-payment compatibility (Proposed) — added on this branch in response to review.
https://github.com/WithAutonomi/ant-node/blob/settlement-version-quote-gate/docs/adr/ADR-0010-settlement-version-and-pre-payment-compatibility.md
Records the inclusive
MIN..=CURRENTrange and why the upper bound is load-bearing, the two refusal directions and why they must stay distinct during a client-first rollout, the unversioned retry as a bounded downgrade path with a shared compile-time cutover guard, the rule that a refusal must not depend on which peers answered first, and the residual that merkle storers are not exactly the peers a client quotes.Builds on ADR-0008, whose re-open trigger is what fired here.
Release readiness: NOT production ready
Merging puts this in the next release, so the bar is fleet-ready rather than code-complete. CI being green is evidence for the code gate only. Do not merge while any row below is open.
Cargo.tomlpins a mutable fork branchWhat is genuinely low risk today
The gate is inert on arrival.
MIN_SUPPORTED_SETTLEMENT_VERSIONandCURRENT_SETTLEMENT_VERSIONare both the first declarable version, so no client in existence can be refused for being too old and no node can be refused for being behind. The refusal machinery, the corroboration quorum and the latch cannot fire until a future settlement bump.That is a reason the risk is low. It is not evidence the gates are closed, and it does not make the set fleet-ready.
Mitigation / rollback
Revert.
MIN_SUPPORTED_SETTLEMENT_VERSIONequals the first version any client can declare, so no client in existence can trip the lower gate, and no node has a lowerCURRENT, so none can trip the upper one. The change is inert on arrival and becomes load-bearing at the next settlement bump. The underpayment message change is text only.