fix(wallet): bound the total hold a repushed bundle can keep on its inputs - #505
Conversation
Salvage anchor for the dig-node#502 lane. Version assigned 0.251.0 (origin/main is 0.247.0; 0.248-0.250 are held by sibling lanes). Co-Authored-By: Claude <noreply@anthropic.com>
Four failing db-layer tests for dig-node#502, plus the constant they measure against. `MAX_RESERVATION_HOLD_MS` is defined as a multiple of `RESERVATION_TTL_MS` in one place so the two cannot drift; the TTL itself is unchanged. The acceptance test steps by less than a TTL past the cap and asserts its own iteration count: a one- or two-push fixture is satisfied by the unfixed code, because the first hold has not lapsed yet. Co-Authored-By: Claude <noreply@anthropic.com>
…nputs `reserve_spend` re-armed `expires_at` to `now + RESERVATION_TTL_MS` on every push of a given transaction id, so a caller re-pushing the same signed bundle more often than the TTL renewed its hold forever and the inputs never returned. That is the lockout failure the TTL's own doc names as the worse of the two, reachable without a single dishonest answer. Two composed bounds, neither of which shortens the TTL: * A TOTAL cap anchored on the FIRST push. `MAX_RESERVATION_HOLD_MS` is defined as `6 * RESERVATION_TTL_MS` in one place, so lengthening the TTL scales the cap and the two cannot drift. `submitted_at` is not in the upsert's `DO UPDATE SET` list, so the stored value is a stable anchor; a test pins it. * A reason-conditional re-arm. `chain::refusal_forecloses_a_later_push` names the four CLVM-execution / cost refusals that complain about the bundle's own contents and that no better-synced node can turn into an acceptance. Those still HOLD -- the verdict is height-dependent, so this crate declines to trust one node's view of it -- but they may not RENEW the hold. Everything else extends, including an unrecognised reason, an `Err`, and a bare verdict. The two compose into the gate's "every observed refusal was foreclosing" case without a per-attempt history, because a re-push may never move the deadline EARLIER: if every attempt is non-extending the deadline never leaves the first `submitted_at + TTL`, and an extending attempt's grant survives every later one. The clamp lives in the SQL so it is atomic against the stored anchor; a read-then-write above this layer would race two concurrent pushes. `coin_reservations`' `ON CONFLICT(coin_id) DO NOTHING` first-claim-wins rule is untouched and pinned by a test. Closes #502 Co-Authored-By: Claude <noreply@anthropic.com>
|
IN PROGRESS — not the verdict. Independent correctness gate, fresh context, reviewing head Confirmed so far (evidence, not the PR body):
Still open: my own run of the suite + revert-proof, and two findings I am writing up. |
loop-security — IN PROGRESS, not the verdictHead audited: Tooling note: gitnexus was not used — its Posting these as they resolve. Nothing below is the verdict. Q1 — does the new caution list grant the attacker capability they did not have? No. The free-set is UNCHANGED, and I can show it is identical rather than merely smaller.I did not take the "strictly less power" framing on trust. The reason it holds is a structural fact about the skip, not about the size of the list:
if !matches!(&pushed, Ok(o) if Self::is_definitive_rejection(o)) {
let may_extend = Self::attempt_may_extend_the_hold(&pushed);
if let Err(e) = self.reserve_pushed_bundle(&bundle, may_extend).await {So on a re-push, an attacker emitting one of the 11 Therefore the "prevent this re-push from extending the hold" capability already existed before this PR, reachable with The asymmetry that keeps it that way is that Q2 — earliest wall-clock moment an attacker can make a coin reselectable: identical old vs new,
|
Adversarial gate (dig-node#502 / PR #505) — REFUTEDHead Test counts I personally observed — The defect is not in the counts. It is that the tests measure the two halves separately and never R1 — primary refutation: the non-extending list holds the exact class the same file says must extend
And the PR's own rule for what must stay OUT of the new list (
All four members of the new list satisfy that exclusion criterion verbatim. The doc says the four The concrete sequence, with the line permitting each step.
Inputs freed for a bundle that LANDS, 550 s earlier than main would have freed them. That is the Step 2 needs no attacker: the honest three-destination race is the case Claim 1 is therefore false as stated. "The first 600 s after any push is unchanged" holds only for Claim 6 answered: the population is wrong. Claim 2 mostly survives but is overstated. An intrinsic name skips the reserve entirely R2 — claim 4 is false: the anchor restarts, so this is a sawtooth, not a bound
The lockout #502 set out to fix is not fixed for the retry loop it names — it becomes a free window Whether the restart is right is a genuine design question, but it is undecided here, and the docs on R3 — claim 5: the timelock case is reachable and needs no hostile node
This one is arguably inherent to any total cap and so may be an accepted cost — but it is currently The coverage gap that let all three through
That composition is exactly what #497 DOES test for the intrinsic list — RecommendationDo not merge as-is.
What would change my answer on R1: evidence that |
MichaelTaylor3d
left a comment
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
CHANGES-REQUIRED — independent correctness gate
Head reviewed: 2728e57fb13a7a4a23153cb5d9db3a4fb8e0abc2 (resolved from the remote, not taken from the dispatch brief). Read in my own detached worktree; the lane's dn-502 and the primary checkout were not touched.
The core fix is correct. Verified independently, not from the PR body: the bind order matches the eight ? positions; submitted_at is written by exactly one statement in the crate and is absent from DO UPDATE SET; prune_reservations is untouched and still expires_at <= ?; the four new names are disjoint from BUNDLE_INTRINSIC_REFUSALS and matched exactly through the same refusal_reason splitter; RESERVATION_TTL_MS is unchanged; coin_reservations' first-claim-wins is untouched.
Tests I ran myself, unpiped, exit status read directly:
- clean head:
running 779 tests-> 778 passed; 0 failed; 1 ignored; 0 filtered out. - with the SQL clamp reverted (CASE removed, both new binds dropped): 773 passed; 5 failed. Four are the clamp tests, each failing on its own assertion rather than a compile error - e.g.
a_bundle_repushed_forever_still_releases_its_coins_at_the_total_cappanicking atdb.rs:6570,left: 0 / right: 1. The fifth,sync_supervisor::stall_evidence_survives_the_end_of_a_session, is an unrelated 120s timeout under load; it passed on the clean run. a_repush_inside_the_cap_still_rearms_from_nowstayed GREEN under the revert, exactly as the lane reported. That reading is correct: it pins the #348/#497 property the fix must not break, and rightly does not depend on the clamp.
The acceptance loop genuinely executes 8 pushes at 8-minute steps across the 1-hour cap and asserts its own iteration count, so it does outlive a single TTL.
Two findings block, three do not. Each is an inline thread.
The refutation is accepted. Part B is being removed; the clamp stays.Verdict from the adversarial gate: REFUTED (#505 (comment)). R1 is correct and decisiveThe four names I put in the new caution list --
The sharpest part: my own brief stated the correct exclusion rule -- "each of those can be Measured cost had it shipped: freed 550 s earlier than R2 is correct, and it is a real limitation of the cap rather than a bug in it
I am not building a tombstone table for this. Refusing to ever re-hold a bundle that already had its R3 is accepted as the inherent cost of having a cap at allA bundle whose timelock matures more than an hour after its first push has its inputs freed while it What shipsPart A only -- |
loop-security — VERDICT: PASSHead audited: Test run I personally executed, in my own worktree at this head, unpiped: 778 tests ran — not an exit status, and Tooling: gitnexus not used (its dig-node index points at the primary checkout, hundreds of commits behind, so The central question: does the new bound free inputs for a bundle that may still land?It can — but only on the clock, never at an attacker's choosing, and never earlier than the pre-existing floor. Detail in Q2 and Q3. Q1 — attacker capability: UNCHANGED. Identical, not merely "strictly less".I did not accept the framing that the caution list is strictly less power; I tested it, and the answer is stronger than the claim. The reason is a structural property of the skip, not of list size — if !matches!(&pushed, Ok(o) if Self::is_definitive_rejection(o)) {
let may_extend = Self::attempt_may_extend_the_hold(&pushed);
if let Err(e) = self.reserve_pushed_bundle(&bundle, may_extend).await {A definitive rejection does not merely decline to extend — it skips The reverse direction is closed by construction: Q2 — earliest moment an attacker can make a coin reselectable:
|
| path | result | vs stored |
|---|---|---|
| INSERT (no conflict) | excluded.expires_at |
n/a, first value |
DO UPDATE, may_extend = 0 |
pending_transactions.expires_at |
identity |
DO UPDATE, may_extend = 1 |
MAX(stored, MIN(excluded, submitted_at + CAP)) |
>= stored for any inner value |
Old code was expires_at = excluded.expires_at = now + TTL, monotonically increasing across re-pushes. So the floor is t1 + TTL in both, and prune_reservations fires on expires_at <= now_ms (db.rs:2879). The new code is never earlier than the old code for any input. a_non_extending_repush_never_shortens_a_deadline_already_granted pins the monotonicity.
The two free-early primitives that would break this are both closed, and I checked each:
- No caller-supplied clock. All five
prune_reservationscall sites readcustody::now_ms()internally (rpc.rs:848,866,2929,3196,3663).reservations_held's own doc states the reason: a caller-suppliednowwould be a lapse oracle. - No RPC releases a bundle-backed reservation.
control.wallet.reservations.release->release_reservation(rpc.rs:880) ->release_client_reservation, which only ever touchesclient_coin_reservations. It cannot reachcoin_reservations.
Q3 — the 1-hour cap against a genuinely live bundle: a real, bounded, non-exploitable residue. Named below as DID-1, not gating.
Q4 — first-claim-wins: intact.
coin_reservations.coin_id is the PRIMARY KEY with ON CONFLICT(coin_id) DO NOTHING (db.rs:2836-2838); the PR adds no path that touches it. A second bundle sharing an input gets its own pending_transactions row but no coin_reservations row for the contested coin, so its lapse cascades nothing for that coin. PRAGMA foreign_keys(true) is set on both the file and in-memory pools (db.rs:924,931), so ON DELETE CASCADE genuinely fires rather than orphaning silently. The new CASE cannot cross transaction ids: ON CONFLICT(transaction_id) targets the PK, so pending_transactions.submitted_at inside DO UPDATE resolves to the conflicting row's own anchor.
Q5 — submitted_at as a security anchor: sound.
Exactly one production caller of reserve_spend in the entire tree (rpc.rs:2310), fed solely by reserve_pushed_bundle, which sets submitted_at from custody::now_ms() (custody.rs:441). Never a caller argument, never from the wire — and may_extend is derived from the push OUTCOME rather than from any request field, since control.wallet.broadcast accepts only params.signed_bundle_hex. submitted_at is absent from the DO UPDATE SET list, pinned by a_repush_never_rewrites_the_first_push_anchor.
Q6 — bind order and SQL shape: correct.
SQLite numbers unnumbered ? by order of appearance in the statement text: six in VALUES, then WHEN ? = 0, then submitted_at + ?. Binds at db.rs:2821-2829 are transaction_id, bundle_hex, fee, submitted_at, expires_at, attempts, i64::from(may_extend_expiry), MAX_RESERVATION_HOLD_MS — 8 binds, 8 placeholders, aligned. Both MIN/MAX are the two-argument scalar forms; the_clamp_uses_the_scalar_two_argument_min_and_max drives both arms. No string interpolation anywhere, so no injection surface; i64::from(bool) yields 0/1 against an integer comparison, no coercion surface. The clamp is evaluated inside the single upsert statement against the STORED anchor, so two concurrent pushes cannot race a read-then-write.
Q7 — i64 overflow: unreachable, and the failure direction is safe regardless.
submitted_at is ~1.7e12 ms, MAX_RESERVATION_HOLD_MS is 3.6e6, headroom to i64::MAX ~9.2e18. SQLite promotes integer-addition overflow to REAL rather than wrapping. And a garbage-small anchor fails safe: MIN picks the small value, MAX(stored, small) returns stored, so the hold ends at the first TTL and never earlier.
Q8 — exact-match discipline: correct, and both directions are bounded.
refusal_reason (chain.rs:265-270) splits on the first ": ". So the only strings that foreclose are the bare name or <verdict>: <NAME>. MEMPOOL_CONFLICT (see BLOCK_COST_EXCEEDS_MAX) does not match (asserted in the new test), and neither does BLOCK_COST_EXCEEDS_MAX: MEMPOOL_CONFLICT (the reason resolves to the tail) nor X: Y: BLOCK_COST_EXCEEDS_MAX. eq_ignore_ascii_case over ASCII-only entries means a non-ASCII input simply fails to match. A non-match falls into EXTEND, the hold direction — and per Q1 a match grants nothing new. Both directions are safe. the_foreclosing_list_is_disjoint_from_the_bundle_intrinsic_one additionally prevents the new list becoming dead code.
Q9 — log / PII surface: clean.
Zero tracing! / println! / log! / dbg! statements added anywhere in the diff, verified by grepping only the added lines. No seed, key, or bundle secret reaches a log line on the new paths. The only nearby log is the pre-existing rpc.rs:2175 warn carrying a sqlx::Error, untouched.
Reachability — who can invoke this
push_signed_bundle's only production caller is control.wallet.broadcast (control.rs:2936), which is token-gated, asserted by an existing test at control.rs:4578 (!is_open_control_read("control.wallet.broadcast")). No new method, parameter, caller, or peer-reachable surface is introduced. There is no internal retry loop — reserve_spend runs twice for one transaction id only when an authenticated local client re-pushes the same bundle, so the extension machinery is never driven by a stranger.
Non-gating findings — recommend follow-up tickets, do NOT hold the merge
DID-1 (defense-in-depth) — the cap silently overrides the timelock reasoning it sits next to. chain.rs:307-313 vs rpc.rs:562
VIEW_DEPENDENT_BUNDLE_CONTENT_REFUSALS' own doc argues carefully that the timelock assertions must stay OUT of the non-extending class because "a later push of the same bundle may therefore genuinely land, so extending the hold is CORRECT for them." That reasoning is right — and then MAX_RESERVATION_HOLD_MS clamps the correct extension at one hour anyway, for the one refusal class where the bundle is most likely to be genuinely retained. Chia returns PENDING rather than FAILED for ASSERT_HEIGHT_* / ASSERT_SECONDS_* and holds such bundles in its potential cache, so this is not hypothetical.
Sequence: a client pushes bundle B spending coin C with a timelock maturing at t1+2h; every node answers ASSERT_SECONDS_ABSOLUTE_FAILED; the client re-pushes inside each TTL; at t1+1h the cap frees C; between t1+1h and t1+2h coin selection can pick C for a second bundle B2; at t1+2h B matures and both bundles target C.
Why this is not gating. No money is lost — a coin spends once and the loser is refused by the mempool. No attacker leverage — the cap fires on the node's own clock, cannot be advanced, and cannot free a coin earlier than t1 + TTL (Q2). It requires an external client to push a bundle with a timelock longer than an hour and keep re-pushing for over an hour; dig-node constructs no timelocked bundles itself (the only ASSERT_SECONDS/ASSERT_HEIGHT occurrences in the tree are refusal-name fixtures at chain.rs:1017,1018,1095,1096) and relays only what somebody else signed (§908). And the alternative is the measured indefinite lockout this ticket exists to close, which the TTL's own doc ranks as the worse failure. This is the trade #502 chose, and it chose correctly. What is missing is only that the docs do not acknowledge the tension. Suggested follow-up: state it in MAX_RESERVATION_HOLD_MS' doc, or exempt a bundle whose last refusal was a timelock assertion from the cap.
DID-2 (latent, unreachable at this head) — db.rs:3091 hard-codes may_extend_expiry: true on read-back
pending_transactions() reports true unconditionally, which is the conservative choice and is correctly documented. It becomes a laundering path — turning a non-extending push back into an extending one — the moment any caller reads a row and feeds it to reserve_spend. Today none does; reserve_spend has exactly one production caller. Worth a pin or a comment on pending_transactions() so a future read-modify-write does not quietly restore unbounded renewal.
DID-3 (documentation precision, no code change needed)
The residual cost of the cap has a sharper statement than the docs give it: after t1 + 5*TTL, each further push buys strictly less than a full TTL of protection, shrinking to zero at t1 + 6*TTL. A bundle re-pushed at t1+59m is protected for one minute, not ten. That is correct and intended, but a_repush_inside_the_cap_still_rearms_from_now exercises only the far-from-cap case, so the boundary behaviour is neither documented nor tested.
Areas checked, and why each is clear
| area | verdict |
|---|---|
| Secrets / credentials | No key, token, seed or credential introduced, logged or committed. Zero new log statements in the diff. |
| Custody / privilege | §908 boundary untouched — the node still signs nothing. may_extend derives from the push outcome, never from a request field. No elevation, no FFI, no filesystem or permission change. |
| Input / boundary | The only untrusted input is the refusal string: exact-match, trimmed, case-insensitive, split on the first ": ", non-match falls to HOLD. No unwrap on adversarial input, no path/shell/deserialization surface. |
| Crypto / protocol | No crypto touched. No downgrade, replay or nonce surface. Verify-then-use unchanged. |
| AuthZ / exposure | Sole caller control.wallet.broadcast stays token-gated (asserted, control.rs:4578). No new method, parameter or peer-reachable path; the new capability hangs off no peer-facing handler. |
| Amplification / cost asymmetry | No new work elicited per request — one extra CASE in an existing single-row upsert. No new allocation, IO or egress. |
| Guard specificity | Tested the one-off variants against the exact-match splitter: embedded, prefixed, suffixed and multi-colon forms all fall to the safe side. |
| Persisted state as untrusted | submitted_at is now load-bearing and has exactly one production writer, from the local clock. A corrupt or hostile value can only cause a bounded early lapse at t1 + TTL, never an earlier free. |
| SQL injection / binds | 8 binds, 8 placeholders, correct order; scalar two-arg MIN/MAX; no interpolation. |
| Dependencies | None added, changed, or loosened. |
| Double-spend direction | The new bound cannot free any input earlier than the pre-existing floor. Proven by total case analysis over the two-branch CASE. |
| Lockout direction | Bounded at submitted_at + 1h from a stable anchor a re-push cannot move. |
Version gate: 0.247.0 -> 0.251.0, base not stale (main is still 476055fb at 0.247.0). dig-wallet itself stays at 0.47.0, which matches the model stated in the root manifest — library crates keep independent versions and only the released binary tracks the workspace version — and dig-wallet carries git deps so it cannot publish, so the new pub field breaks no external consumer.
PASS. No security defect in this diff. The three items above are defense-in-depth: file them as follow-ups, do not gate on them.
Reconciling the three gate verdicts — they are not in conflict, but two of them read as if they areFor the record, because a later reader landing on the security PASS alone would draw the wrong
The apparent contradictionThe security gate's headline answer is "the new code is never earlier than the old code for any Both are correct. They answer different questions:
The second question is the one the double-spend risk turns on. "Never moves the stored value backwards" This is the same shape as the other instrument failures recorded in this repo: a reassuring number What this means for the mergeThe refutation governs. Part B is removed; the clamp stays. All three gates' non-moot findings are in
Re-gate scope after the fix: the diff SHRINKS to a strict subset of what all three already examined, |
…clamp The adversarial gate on #505 refuted the reason-conditional half and it is removed in full: VIEW_DEPENDENT_BUNDLE_CONTENT_REFUSALS, refusal_forecloses_a_later_push, attempt_may_extend_the_hold, the PendingTransactionRow::may_extend_expiry field and the CASE arm in reserve_spend. The four names it listed -- GENERATOR_RUNTIME_ERROR, BLOCK_COST_EXCEEDS_MAX, INVALID_BLOCK_COST, INVALID_SPEND_BUNDLE -- do not identify a bundle no destination is holding. push_tx relays to up to three destinations and only the LAST answer returns, so such a refusal from the last says nothing about the first, which may have admitted and gossiped the bundle. The removed code freed inputs up to 550s earlier than main for a bundle that lands, with no attacker: the double-spend direction #497 exists to close. What ships is the clamp alone: expires_at is bounded by submitted_at + 6 * RESERVATION_TTL_MS. The outer MAX is retained because a non-monotonic clock is the one case that can still drive an incoming deadline below a live one, and shortening a live hold is the dangerous direction. SPEC.md 18.9a gains the total-hold bound as a normative clause: without it a reimplementation built from the spec as written reproduces the unbounded re-arm this change fixes. Three limitations are now stated in MAX_RESERVATION_HOLD_MS' doc and two are pinned by tests: the bound is on CONTINUOUS hold and a re-push after the prune gets a fresh anchor; a bundle whose timelock matures past the cap has its inputs freed while the network genuinely still holds it; and inside the last TTL before the cap a re-push buys strictly less than a full TTL. The clamp also fails OPEN under an absurd clock, since SQLite promotes integer overflow to REAL. Refs #502 Co-Authored-By: Claude <noreply@anthropic.com>
…total-hold # Conflicts: # Cargo.lock # Cargo.toml
…total-hold # Conflicts: # Cargo.lock # Cargo.toml
cargo fmt wanted the multi-line call form at both boundary tests. Formatted those two files only; the workspace-wide check is now clean at zero diffs. Co-Authored-By: Claude <noreply@anthropic.com>
The comment sat on `reserved_coin_ids`, which IS assembled from a stored table and has no `true`. It described `may_extend_expiry`, the bool this branch removed in fce2358, and pointed at a field doc that no longer exists. Also tighten SPEC 18.9a: a re-push does not unconditionally 'update the deadline' -- at or past the cap, and under a backwards clock, it correctly leaves the deadline unchanged. State the re-arm as subject to the bound. Co-Authored-By: Claude <noreply@anthropic.com>
…total-hold # Conflicts: # Cargo.lock # Cargo.toml
…total-hold # Conflicts: # Cargo.lock # Cargo.toml
… only by the first push's (#528) * chore(wallet): open the #502 lane -- bound total reservation hold Salvage anchor for the dig-node#502 lane. Version assigned 0.251.0 (origin/main is 0.247.0; 0.248-0.250 are held by sibling lanes). Co-Authored-By: Claude <noreply@anthropic.com> * test(wallet): pin the total reservation hold a repushed bundle may take Four failing db-layer tests for dig-node#502, plus the constant they measure against. `MAX_RESERVATION_HOLD_MS` is defined as a multiple of `RESERVATION_TTL_MS` in one place so the two cannot drift; the TTL itself is unchanged. The acceptance test steps by less than a TTL past the cap and asserts its own iteration count: a one- or two-push fixture is satisfied by the unfixed code, because the first hold has not lapsed yet. Co-Authored-By: Claude <noreply@anthropic.com> * fix(wallet): bound the total hold a repushed bundle may take on its inputs `reserve_spend` re-armed `expires_at` to `now + RESERVATION_TTL_MS` on every push of a given transaction id, so a caller re-pushing the same signed bundle more often than the TTL renewed its hold forever and the inputs never returned. That is the lockout failure the TTL's own doc names as the worse of the two, reachable without a single dishonest answer. Two composed bounds, neither of which shortens the TTL: * A TOTAL cap anchored on the FIRST push. `MAX_RESERVATION_HOLD_MS` is defined as `6 * RESERVATION_TTL_MS` in one place, so lengthening the TTL scales the cap and the two cannot drift. `submitted_at` is not in the upsert's `DO UPDATE SET` list, so the stored value is a stable anchor; a test pins it. * A reason-conditional re-arm. `chain::refusal_forecloses_a_later_push` names the four CLVM-execution / cost refusals that complain about the bundle's own contents and that no better-synced node can turn into an acceptance. Those still HOLD -- the verdict is height-dependent, so this crate declines to trust one node's view of it -- but they may not RENEW the hold. Everything else extends, including an unrecognised reason, an `Err`, and a bare verdict. The two compose into the gate's "every observed refusal was foreclosing" case without a per-attempt history, because a re-push may never move the deadline EARLIER: if every attempt is non-extending the deadline never leaves the first `submitted_at + TTL`, and an extending attempt's grant survives every later one. The clamp lives in the SQL so it is atomic against the stored anchor; a read-then-write above this layer would race two concurrent pushes. `coin_reservations`' `ON CONFLICT(coin_id) DO NOTHING` first-claim-wins rule is untouched and pinned by a test. Closes #502 Co-Authored-By: Claude <noreply@anthropic.com> * chore: bump to 0.256.0, clear of #506's 0.251.0 * fix(wallet): drop the reason-conditional re-arm, keep the total-hold clamp The adversarial gate on #505 refuted the reason-conditional half and it is removed in full: VIEW_DEPENDENT_BUNDLE_CONTENT_REFUSALS, refusal_forecloses_a_later_push, attempt_may_extend_the_hold, the PendingTransactionRow::may_extend_expiry field and the CASE arm in reserve_spend. The four names it listed -- GENERATOR_RUNTIME_ERROR, BLOCK_COST_EXCEEDS_MAX, INVALID_BLOCK_COST, INVALID_SPEND_BUNDLE -- do not identify a bundle no destination is holding. push_tx relays to up to three destinations and only the LAST answer returns, so such a refusal from the last says nothing about the first, which may have admitted and gossiped the bundle. The removed code freed inputs up to 550s earlier than main for a bundle that lands, with no attacker: the double-spend direction #497 exists to close. What ships is the clamp alone: expires_at is bounded by submitted_at + 6 * RESERVATION_TTL_MS. The outer MAX is retained because a non-monotonic clock is the one case that can still drive an incoming deadline below a live one, and shortening a live hold is the dangerous direction. SPEC.md 18.9a gains the total-hold bound as a normative clause: without it a reimplementation built from the spec as written reproduces the unbounded re-arm this change fixes. Three limitations are now stated in MAX_RESERVATION_HOLD_MS' doc and two are pinned by tests: the bound is on CONTINUOUS hold and a re-push after the prune gets a fresh anchor; a bundle whose timelock matures past the cap has its inputs freed while the network genuinely still holds it; and inside the last TTL before the cap a re-push buys strictly less than a full TTL. The clamp also fails OPEN under an absurd clock, since SQLite promotes integer overflow to REAL. Refs #502 Co-Authored-By: Claude <noreply@anthropic.com> * chore: renumber to 0.252.8, under the MSI ProductVersion ceiling (#521) * style(wallet): rustfmt the two reserve_spend test call sites cargo fmt wanted the multi-line call form at both boundary tests. Formatted those two files only; the workspace-wide check is now clean at zero diffs. Co-Authored-By: Claude <noreply@anthropic.com> * docs(wallet): drop a comment left behind by the reverted re-arm field The comment sat on `reserved_coin_ids`, which IS assembled from a stored table and has no `true`. It described `may_extend_expiry`, the bool this branch removed in fce2358, and pointed at a field doc that no longer exists. Also tighten SPEC 18.9a: a re-push does not unconditionally 'update the deadline' -- at or past the cap, and under a backwards clock, it correctly leaves the deadline unchanged. State the re-arm as subject to the bound. Co-Authored-By: Claude <noreply@anthropic.com> * chore(wallet): open the lane for dig-node#525 (clock-anchored reservation freeze) Version anchor only. The fix follows: a far-forward clock at a bundle's FIRST push writes a `submitted_at` far in the future, and #505's outer `MAX` then pins `expires_at` there permanently, so no later correct-clock push and no prune can ever release the coins. Refs #525 Co-Authored-By: Claude <noreply@anthropic.com> * fix(wallet): repair a reservation whose deadline contradicts the clock (#525) `reserve_pushed_bundle` reads the clock once and writes both `submitted_at` and `expires_at` from that reading, so a single reading far in the future stores a deadline decades out. Nothing could retire it: `prune_reservations` deletes on `expires_at <= now`, which never arrives, and #502's upsert clamp is `MAX(stored, ...)`, so a later push under a corrected clock leaves the stored deadline alone. The coin was withheld from selection for ever and `reset_chain_cache` refused while the row existed. `prune_reservations` now repairs at OBSERVATION: a row whose deadline exceeds `now + MAX_RESERVATION_HOLD_MS` contradicts its own columns against the clock (an honest row satisfies `expires_at <= submitted_at + CAP` and `submitted_at <= now`), so it is re-anchored to `now` and granted one fresh `RESERVATION_TTL_MS`. `submitted_at` moves too, or #502's cap clause would stop binding on that row for ever. The client hold table gets the same repair, keyed on the SAME threshold so a five-minute backwards step cannot re-clamp a healthy hold, and granted its own ceiling since its requested TTL is unrecoverable. All four statements now share one write-first transaction. `reset_coin_db` prunes first, like every other reservation-sensitive entry point, so the refusal message telling a user to wait becomes true. Co-Authored-By: Claude <noreply@anthropic.com> * style(wallet): rustfmt the two files touched by #525 Co-Authored-By: Claude <noreply@anthropic.com> * fix(wallet): correct the clock-anchor SPEC claim and pin the 110-minute forward-glitch residue The adversarial gate on #525 found the normative sentence this PR added to SPEC.md was false: it claimed a reservation is never held beyond MAX_RESERVATION_HOLD_MS (60 min) from an observed instant. A forward clock glitch of up to CAP - TTL (50 min) at the first push evades the clock-contradiction detector by construction, and the true worst case is 2*CAP - TTL = 110 minutes, tight. - SPEC.md 18.9a now states the 110-minute bound explicitly, and names the CAP-TTL constant as both the forward-glitch evasion window and the backwards-step false-fire floor -- one constant, two sides. - db.rs: doc comments on the repair explain the residue instead of overclaiming past it. - A new compile-time assert pins RESERVATION_TTL_MS <= MAX_RESERVATION_HOLD_MS -- unreachable today (CAP = 6*TTL) but load-bearing if that ratio is ever narrowed, since a TTL above the cap would make every repaired row re-trigger the detector forever. - The existing boundary test is renamed and its doc comment states plainly that its past-bound row is synthetic and unreachable by any writer -- it pins the SQL predicate's `>` only, not production behaviour. - A new regression test, built entirely from real reserve_spend/prune_reservations calls (no hand-placed rows), measures the actual 110-minute residue under a real retry loop. No behaviour change: the repair itself is unchanged. This corrects a normative claim born false in the commit that wrote it, and pins the honest bound in its place. Co-Authored-By: Claude <noreply@anthropic.com> * chore(release): bump to 0.252.96 to clear sibling lanes Co-Authored-By: Claude <noreply@anthropic.com> * chore(release): bump to 0.253.7 to avoid collision with sibling release PRs Co-Authored-By: Claude <noreply@anthropic.com> * chore(release): bump to 0.254.3, resolve collision with #533 (0.254.1) --------- Co-authored-by: Claude <noreply@anthropic.com>
1 participant
Closes #502
Closes #511
The defect
WalletDb::reserve_spend's upsert re-armedexpires_attoexcluded.expires_at— whichreserve_pushed_bundlecomputes asnow + RESERVATION_TTL_MS— on every push of a giventransaction_id. That bounds ONE hold and not a SEQUENCE of them: a caller re-pushing the samesigned bundle more often than every 600 s renewed the hold forever, and the user's inputs never came
back.
attemptswas incremented and never read as a cap.That is the lockout failure
RESERVATION_TTL_MS's own doc names as the worse of the two, and it isreachable without a single dishonest answer from anyone.
origin/mainstill carries the bareexpires_at = excluded.expires_at, so this PR is the cap.The fix — a total cap in SQL, and the TTL is NOT shortened
Two rules compose, and each guards a different direction:
submitted_atis deliberately absent from theDO UPDATE SETlist, so the stored value is a stable anchor and the new deadline may not exceed
submitted_at + MAX_RESERVATION_HOLD_MS—6 * RESERVATION_TTL_MS, one hour, defined oncebeside the TTL in
rpc.rsso lengthening the TTL scales the cap and the two cannot drift.MAXneverbinds; it exists for the non-monotonic one, where an NTP step would otherwise SHORTEN a live hold
and return the inputs of a bundle that may still land — the sec(reservation): gated on an untrusted 'accepted' — the under-claim direction fails OPEN into the double-select window #348/fix(wallet): a peer-local refusal must not free inputs another destination may hold #497 direction.
The clamp is in SQL rather than computed in the caller because it must be atomic against the STORED
anchor: a read-then-write above this layer would race two concurrent pushes of the same bundle.
coin_reservations'ON CONFLICT(coin_id) DO NOTHINGis untouched, so first-claim-wins survives.What this bound is NOT. It bounds CONTINUOUS hold, not aggregate hold. At the cap
prune_reservationsDELETEs the row,coin_reservationscascades, and the coins become selectableagain; a later push is then a new reservation with a new anchor and a full new hour. That sawtooth is
intended — at each release the coins were genuinely selectable, and refusing to ever re-hold a bundle
that already had its hour would permanently decline to protect a bundle that may still land, which is
the double-spend direction #497 deliberately chose against. The three limitations are enumerated on
MAX_RESERVATION_HOLD_MS' own doc rather than left for a reader to discover.Why this also closes #511
#511 reports that the cap bounds continuous rather than aggregate hold. That premise exists only on
this branch — on
origin/mainthere is no cap and no sawtooth to refine. An aggregate bound wasconsidered and rejected on the merits: it needs a tombstone, so it would permanently decline to
protect a bundle that may still land. #511 offers acceptance-with-the-reasoning-recorded as a closing
arm, and this PR takes it: the decision is recorded on
MAX_RESERVATION_HOLD_MS' doc and thecontinuous-hold limitation is now normative in
SPEC.md.SPEC.md
Changed (§18.9a, +17 lines). The section previously stated only the per-hold TTL, so an
independent reimplementation built from the normative text would reproduce the unbounded re-arm this
PR fixes — the defect was derivable from the spec. It now states the total bound as a bound, the
stable anchor, the never-move-earlier rule, and that the bound is on CONTINUOUS hold.
Blast radius
gitnexus's
dig-nodeindex is stale and points at the primary checkout, so per §2.0 it cannotanswer — a zero from it would be false-safe. Done by grep and direct read instead, and stated rather
than implied.
reserve_spend,reserve_pushed_bundleandPendingTransactionRowappear nowhere outsidecrates/dig-wallet/src/sage/in this repo.reserve_spendhas exactly one production caller,reserve_pushed_bundle; that has one,push_signed_bundle; whose one production caller isdig-node-service'scontrol.wallet.broadcast. No signature changed and no field was added. Risk:LOW.
How verified
cargo test -p dig-wallet --libat5c778702(main merged in, version 0.252.12) — 794 passed, 0 failed, 1 ignored, 0 filtered out. Workspacecargo fmt --all --check: 0 diffs.The clamp is asserted at the decision and revert-proven in both directions independently, by
mutating each half separately rather than reverting the feature as a whole. Baseline
sage::db::testsat11d46af9: 94 passed, 0 failed, 694 filtered out. The clamp SQL is unchanged since.submitted_at + ? * 1000000) — the unbounded re-arm restoreda_bundle_repushed_forever_still_releases_its_coins_at_the_total_cap,the_clamp_uses_the_scalar_two_argument_min_and_max,a_repushed_bundle_gets_a_fresh_anchor_after_the_cap_prunes_its_row,a_repush_inside_the_last_ttl_before_the_cap_buys_less_than_a_full_ttlMAXremoved — a backwards clock may shorten a live holda_repush_under_a_clock_that_stepped_backwards_never_shortens_a_live_holdInverting either half localises to the test that names it. The counts are real matches, not a filter
that matched nothing and exited 0. Mutations ran in a separate worktree; the reviewed tree is
byte-identical to the head.
The acceptance test asserts its own iteration count (
pushes >= 8, 8-minute steps across a one-hourcap): a one- or two-push fixture would be satisfied by the unfixed code, because the first hold has
not lapsed yet.
Scope note
An earlier revision of this branch also made the re-arm conditional on the refusal reason. Commit
fce23589dropped that in favour of the clamp alone;refusal_forecloses_a_later_push,VIEW_DEPENDENT_BUNDLE_CONTENT_REFUSALS,attempt_may_extend_the_holdandmay_extend_expiryhavezero hits crate-wide, and
chain.rsis byte-identical toorigin/main. This body describes theshipped diff only.
Dependency freshness (reported, NOT acted on)
Sibling lanes are live in this repo, so no cascade was run.
dig-wallet'sdig-*deps are at thelatest published versions. The
chia-*set is pinned to the 0.36.1 line, unified on thechia-wallet-sdk0.36.0 ceiling (dig_ecosystem#3161) — a deliberate pin, not drift.