From e7498849e544903c9c79b507382fb973bac684a5 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 05:57:23 -0700 Subject: [PATCH 01/15] 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 --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index e6227d9e..710a96b4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ edition = "2021" # the ROOT manifest (`[workspace.package].version`), so it MUST be set here for a # release to fire (§3.6). The library crates (dig-node-core/dig-runtime/dig-wallet) # keep their own independent versions — only the released binary tracks the workspace version. -version = "0.247.0" +version = "0.251.0" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over From b579f64a9dea63231a678f70451c3eb9f6d0ecd0 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 06:22:45 -0700 Subject: [PATCH 02/15] 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 --- Cargo.lock | 2 +- crates/dig-wallet/src/sage/db.rs | 142 ++++++++++++++++++++++++++++++ crates/dig-wallet/src/sage/rpc.rs | 21 ++++- 3 files changed, 163 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a68a9504..060fd2d1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3031,7 +3031,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.247.0" +version = "0.251.0" dependencies = [ "async-trait", "axum", diff --git a/crates/dig-wallet/src/sage/db.rs b/crates/dig-wallet/src/sage/db.rs index 7ffca8c6..9738b4de 100644 --- a/crates/dig-wallet/src/sage/db.rs +++ b/crates/dig-wallet/src/sage/db.rs @@ -4691,6 +4691,7 @@ fn is_unique_violation(e: &sqlx::Error) -> bool { #[cfg(test)] mod tests { use super::*; + use super::super::rpc::{MAX_RESERVATION_HOLD_MS, RESERVATION_TTL_MS}; /// **Proves (dig-node#462):** the only public write of `initial_sync_complete` that production /// can reach DISARMS, and it really does clear a flag that was armed. @@ -6476,6 +6477,147 @@ mod tests { assert!(db.pending_transactions().await.unwrap().is_empty()); } + /// **The defect of dig-node#502, at the DB layer.** A caller that re-pushes the same bundle + /// more often than the TTL used to renew its hold forever: `expires_at` was recomputed as + /// `now + RESERVATION_TTL_MS` on every upsert, with no anchor to the FIRST push. The inputs + /// never came back, which is the lockout failure the TTL's own doc names as the worse of the + /// two. + /// + /// The fixture has to outlive a single TTL to see it: a one- or two-push fixture is satisfied + /// by the unfixed code, because the first hold has not lapsed yet. So the loop steps by less + /// than a TTL, past the cap, and the iteration count is asserted rather than assumed. + #[tokio::test] + async fn a_bundle_repushed_forever_still_releases_its_coins_at_the_total_cap() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.upsert_coin(&coin("c1", 100, Some(10), None)) + .await + .unwrap(); + + let first_push = 1_000; + let step = 8 * 60 * 1000; // less than the TTL, so every re-push re-arms under the old rule + let deadline = first_push + MAX_RESERVATION_HOLD_MS; + + let mut pushes = 0; + let mut at = first_push; + while at <= deadline { + db.reserve_spend(&reservation("tx1", &["c1"], first_push, at + RESERVATION_TTL_MS)) + .await + .unwrap(); + assert_eq!( + db.prune_reservations(at).await.unwrap(), + 0, + "the hold must survive while the caller is actively re-pushing (t={at})" + ); + pushes += 1; + at += step; + } + assert!( + pushes >= 8, + "a fixture that pushes {pushes} times cannot outlive one TTL, so it cannot see this \ + defect" + ); + + assert_eq!( + db.prune_reservations(deadline).await.unwrap(), + 1, + "total hold is capped at submitted_at + MAX_RESERVATION_HOLD_MS however many re-pushes \ + arrive" + ); + assert_eq!( + db.unreserved_unspent_coins(None).await.unwrap().len(), + 1, + "the coins must come back to selection at the cap" + ); + } + + /// The cap must not break the property it is bolted onto: INSIDE the cap a re-push still + /// re-arms from `now`, which is what dig-node#348 and #497 rely on to keep a genuinely + /// in-flight bundle's inputs held for a full TTL after its LAST honest transmission. + #[tokio::test] + async fn a_repush_inside_the_cap_still_rearms_from_now() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.upsert_coin(&coin("c1", 100, Some(10), None)) + .await + .unwrap(); + + let first = 1_000; + db.reserve_spend(&reservation("tx1", &["c1"], first, first + RESERVATION_TTL_MS)) + .await + .unwrap(); + + let again = first + 9 * 60 * 1000; + db.reserve_spend(&reservation("tx1", &["c1"], first, again + RESERVATION_TTL_MS)) + .await + .unwrap(); + + let row = &db.pending_transactions().await.unwrap()[0]; + assert_eq!( + row.expires_at, + again + RESERVATION_TTL_MS, + "a re-push inside the cap must extend the deadline, not pin it to the first push" + ); + } + + /// The cap is anchored on `submitted_at`, so the whole bound rests on the upsert never + /// rewriting it. It is not in the `DO UPDATE SET` list today; this pins that, because a later + /// edit adding it there would silently restore the unbounded renewal with every test above + /// still green. + #[tokio::test] + async fn a_repush_never_rewrites_the_first_push_anchor() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.upsert_coin(&coin("c1", 100, Some(10), None)) + .await + .unwrap(); + + db.reserve_spend(&reservation("tx1", &["c1"], 1_000, 1_000 + RESERVATION_TTL_MS)) + .await + .unwrap(); + db.reserve_spend(&reservation("tx1", &["c1"], 500_000, 500_000 + RESERVATION_TTL_MS)) + .await + .unwrap(); + + let row = &db.pending_transactions().await.unwrap()[0]; + assert_eq!( + row.submitted_at, 1_000, + "submitted_at is the first-push anchor and a re-push must never move it" + ); + assert_eq!(row.attempts, 2, "the attempt count still counts re-pushes"); + } + + /// SQLite's `MIN`/`MAX` are AGGREGATES in their one-argument form and scalars only with two or + /// more arguments. The clamp is written with the two-argument scalar forms; a change that left + /// one argument would parse, and would silently mean something else. This drives the clamp + /// through both of its arms at once. + #[tokio::test] + async fn the_clamp_uses_the_scalar_two_argument_min_and_max() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.upsert_coin(&coin("c1", 100, Some(10), None)) + .await + .unwrap(); + + let first = 1_000; + db.reserve_spend(&reservation("tx1", &["c1"], first, first + RESERVATION_TTL_MS)) + .await + .unwrap(); + + // A re-push far beyond the cap: MIN picks the cap, MAX keeps it above the stored value. + db.reserve_spend(&reservation( + "tx1", + &["c1"], + first, + first + 100 * RESERVATION_TTL_MS, + )) + .await + .unwrap(); + + let row = &db.pending_transactions().await.unwrap()[0]; + assert_eq!( + row.expires_at, + first + MAX_RESERVATION_HOLD_MS, + "an over-cap re-push must clamp to the cap exactly" + ); + } + /// Settlement retires the reservation without anything having to remember to release it: the /// coin's own `spent_height` is the signal. #[tokio::test] diff --git a/crates/dig-wallet/src/sage/rpc.rs b/crates/dig-wallet/src/sage/rpc.rs index f4b7aa8e..207548c7 100644 --- a/crates/dig-wallet/src/sage/rpc.rs +++ b/crates/dig-wallet/src/sage/rpc.rs @@ -541,7 +541,26 @@ const DEFAULT_FALLBACK_REFILL_PER_SEC: f64 = 2.0; /// ten minutes is roughly a dozen chances for the spend to land — well past the point where a /// still-unconfirmed bundle is more likely dropped than pending, and short enough that a stranded /// coin returns on a timescale a user waits out rather than reports as lost. -const RESERVATION_TTL_MS: i64 = 10 * 60 * 1000; +pub(crate) const RESERVATION_TTL_MS: i64 = 10 * 60 * 1000; + +/// The most a single bundle's reservation may hold its inputs in TOTAL, measured from the FIRST +/// push rather than from the latest one (dig-node#502): one hour. +/// +/// [`RESERVATION_TTL_MS`] bounds one hold. It does not bound a SEQUENCE of holds: the re-arm on +/// re-push is computed from `now`, so a caller re-pushing the same bundle more often than every +/// TTL renews the hold forever and the inputs never return. That is the lockout failure the TTL's +/// own doc names as the worse of the two, reachable without a single dishonest answer. +/// +/// Expressed as a MULTIPLE of the TTL, in this one place, so the two cannot drift: lengthening the +/// TTL because a bundle needs longer to land also lengthens the total a retrying caller may hold. +/// A cap shorter than the TTL would be a covert shortening of the TTL, which is forbidden. +/// +/// Six is sized by the same question as the TTL. Chia blocks are ~52s apart, so an hour is roughly +/// seventy chances for the spend to land — far past the point where an unconfirmed bundle is more +/// likely dropped than pending — while still returning a stranded coin on a timescale a user waits +/// out. The clamp is anchored on `submitted_at`, which the reservation upsert never rewrites, so it +/// is a bound on the bundle's whole life rather than on any one attempt. +pub(crate) const MAX_RESERVATION_HOLD_MS: i64 = 6 * RESERVATION_TTL_MS; /// The Sage-parity wallet backend. #[derive(Clone)] From 2728e57fb13a7a4a23153cb5d9db3a4fb8e0abc2 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 06:37:38 -0700 Subject: [PATCH 03/15] 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 --- crates/dig-wallet/src/sage/chain.rs | 109 +++++++++++ crates/dig-wallet/src/sage/db.rs | 289 ++++++++++++++++++++++++++-- crates/dig-wallet/src/sage/rpc.rs | 40 +++- 3 files changed, 416 insertions(+), 22 deletions(-) diff --git a/crates/dig-wallet/src/sage/chain.rs b/crates/dig-wallet/src/sage/chain.rs index 96ef1422..e95ccddc 100644 --- a/crates/dig-wallet/src/sage/chain.rs +++ b/crates/dig-wallet/src/sage/chain.rs @@ -285,6 +285,55 @@ pub(crate) fn refusal_is_bundle_intrinsic(stated: &str) -> bool { .any(|intrinsic| reason.eq_ignore_ascii_case(intrinsic)) } +/// Refusals that are about the BUNDLE'S OWN CONTENTS but are deliberately absent from +/// [`BUNDLE_INTRINSIC_REFUSALS`] because the verdict depends on the answering node's HEIGHT and +/// cost budget (dig-node#502). +/// +/// These are the four CLVM-execution / cost names the list above names explicitly as excluded, for +/// exactly that reason: `chia_consensus::spendbundle_validation` derives its flags from +/// `prev_tx_height` and runs under a caller-supplied `max_cost`, so two honest nodes can reach +/// different verdicts on identical bytes. They must keep HOLDING the inputs, and that is unchanged. +/// +/// What this list decides is narrower: whether such a refusal may EXTEND an existing hold. It may +/// not. Extending buys something only when some destination may plausibly still be holding the +/// bundle, and here nothing is: the complaint is about the bytes. Composed with a systematic +/// emitter — a broken wallet-construction path that answers `BLOCK_COST_EXCEEDS_MAX` on every +/// attempt — and a caller that keeps retrying, an unconditional re-arm holds the inputs for as long +/// as the retries continue, for a bundle that was never going to land. +/// +/// # What must NOT be added here, and why a future reader will want to +/// +/// The other absentees from [`BUNDLE_INTRINSIC_REFUSALS`] look like they belong and do not. The +/// timelock assertions (`ASSERT_HEIGHT_*`, `ASSERT_SECONDS_*`, `ASSERT_BEFORE_*`), the fee-policy +/// names, `UNKNOWN_UNSPENT` and `TOO_MANY_ANNOUNCEMENTS` can each be ADMITTED by a DIFFERENT node +/// than the one that refused — a node further ahead, with a different relay policy, or caught up. +/// A later push of the same bundle may therefore genuinely land, so extending the hold is CORRECT +/// for them. The four here differ only in degree, but the degree is the point: a cost or generator +/// verdict does not turn into an acceptance by asking a better-synced peer. +/// +/// Matching is EXACT, case-insensitive and trimmed, via [`refusal_reason`] — never substring or +/// prefix, for the same reason [`refusal_is_bundle_intrinsic`] gives. An unrecognised reason is not +/// on this list, so it EXTENDS: the conservative default, because a hold that lapses too early is +/// the double-select this family exists to close. +const VIEW_DEPENDENT_BUNDLE_CONTENT_REFUSALS: &[&str] = &[ + "GENERATOR_RUNTIME_ERROR", + "BLOCK_COST_EXCEEDS_MAX", + "INVALID_BLOCK_COST", + "INVALID_SPEND_BUNDLE", +]; + +/// Whether a stated refusal is a complaint about the bundle's own contents that no later push can +/// turn into an acceptance — the class that HOLDS but must not EXTEND the hold (dig-node#502). +/// +/// See [`VIEW_DEPENDENT_BUNDLE_CONTENT_REFUSALS`] for the membership rule and for the names that +/// deliberately stay out of it. +pub(crate) fn refusal_forecloses_a_later_push(stated: &str) -> bool { + let reason = refusal_reason(stated); + VIEW_DEPENDENT_BUNDLE_CONTENT_REFUSALS + .iter() + .any(|foreclosing| reason.eq_ignore_ascii_case(foreclosing)) +} + /// Pushes an ALREADY-SIGNED bundle to the network. /// /// A trait rather than a concrete client so the control surface can be driven end to end without a @@ -934,6 +983,66 @@ mod tests { } } + /// **Proves (dig-node#502):** the foreclosing class is recognised exactly, and everything else + /// EXTENDS. + /// + /// The membership rows are the four CLVM-execution / cost names. The shape rows are the ones + /// that matter more than the contents: an unrecognised reason must extend (the conservative + /// default, since a hold that lapses too early is the double-select this family exists to + /// close), and a name EMBEDDED in wider text must not match — the same exact-match discipline + /// `refusal_is_bundle_intrinsic` keeps, because a substring test lets a source turn any string + /// into a non-extending one by quoting a name inside it. + #[test] + fn the_foreclosing_refusals_match_exactly_and_everything_else_extends() { + for foreclosing in [ + "GENERATOR_RUNTIME_ERROR", + "BLOCK_COST_EXCEEDS_MAX", + "INVALID_BLOCK_COST", + "INVALID_SPEND_BUNDLE", + " block_cost_exceeds_max ", + "FAILED: BLOCK_COST_EXCEEDS_MAX", + ] { + assert!( + super::refusal_forecloses_a_later_push(foreclosing), + "{foreclosing} must not renew a hold: trimmed, case-insensitive, exact" + ); + } + + for extending in [ + "DOUBLE_SPEND", + "MEMPOOL_CONFLICT", + "ALREADY_INCLUDING_TRANSACTION", + "UNKNOWN_UNSPENT", + "TOO_MANY_ANNOUNCEMENTS", + "ASSERT_HEIGHT_ABSOLUTE_FAILED", + "ASSERT_SECONDS_RELATIVE_FAILED", + "ASSERT_BEFORE_HEIGHT_ABSOLUTE_FAILED", + "INVALID_FEE_LOW_FEE", + "SOMETHING_NOBODY_ENUMERATED", + "MEMPOOL_CONFLICT (see BLOCK_COST_EXCEEDS_MAX)", + "", + ] { + assert!( + !super::refusal_forecloses_a_later_push(extending), + "{extending:?} may be admitted by a better-synced or differently-configured node, \ + so a later push may land and the hold must still renew" + ); + } + } + + /// The two allowlists answer DIFFERENT questions — free the coins now, versus renew the hold — + /// and a name on both would mean the second list is dead code, because a bundle-intrinsic + /// refusal never reserves at all. + #[test] + fn the_foreclosing_list_is_disjoint_from_the_bundle_intrinsic_one() { + for name in super::VIEW_DEPENDENT_BUNDLE_CONTENT_REFUSALS { + assert!( + !super::refusal_is_bundle_intrinsic(name), + "{name} is on both lists, so its non-extending rule is unreachable" + ); + } + } + /// **Proves (dig-node#460):** the two refusal CLASSES are told apart, and the default is HOLD. /// /// The peer-local rows are not decoration: `ALREADY_INCLUDING_TRANSACTION`, `DOUBLE_SPEND` and diff --git a/crates/dig-wallet/src/sage/db.rs b/crates/dig-wallet/src/sage/db.rs index 9738b4de..cf79de02 100644 --- a/crates/dig-wallet/src/sage/db.rs +++ b/crates/dig-wallet/src/sage/db.rs @@ -224,6 +224,19 @@ pub struct PendingTransactionRow { pub expires_at: i64, /// How many times this bundle has been pushed (1 on first broadcast). pub attempts: i64, + /// Whether THIS attempt may push the deadline further out (dig-node#502). + /// + /// An INPUT to [`WalletDb::reserve_spend`], not a stored column: it is a property of the push + /// that produced this row, not of the bundle. Rows read back out of the database report `true`, + /// the conservative value, because the answer that produced them is not kept. + /// + /// `false` says the last destination complained about the bundle's own CONTENTS in a way no + /// later push can turn into an acceptance + /// (`chain::refusal_forecloses_a_later_push`). The coins still stay held for the + /// remaining hold — the verdict is height- and cost-budget-dependent, so this crate declines to + /// trust one node's view of it — but a retry loop against a systematically broken bundle must + /// not be able to renew that hold forever. + pub may_extend_expiry: bool, /// The coin ids the bundle spends — the coins held out of further selection while it is live. pub reserved_coin_ids: Vec, } @@ -2762,6 +2775,34 @@ impl WalletDb { /// /// Idempotent on the transaction id: re-pushing the same bundle updates its expiry and attempt /// count rather than duplicating it, because a resubmission is the same transaction. + /// + /// # The re-arm is BOUNDED, and the bound is in the SQL (dig-node#502) + /// + /// The expiry used to be taken straight from the incoming row, which is `now + TTL`. That + /// bounds ONE hold and not a SEQUENCE of them: a caller re-pushing the same bundle more often + /// than the TTL renewed its hold forever, and the inputs never returned — the lockout failure + /// `rpc::RESERVATION_TTL_MS`'s own doc names as the worse of the two. + /// + /// Three rules compose to bound it without shortening the TTL: + /// + /// 1. **Clamped to the FIRST push.** `submitted_at` is not in the `DO UPDATE SET` list, so the + /// stored value is a stable anchor, and the new deadline may not exceed + /// `submitted_at + MAX_RESERVATION_HOLD_MS`. + /// 2. **A foreclosing attempt does not extend at all** — see + /// [`PendingTransactionRow::may_extend_expiry`]. + /// 3. **A re-push may never move the deadline EARLIER.** This is what makes rules 1 and 2 add + /// up to the property the ticket asks for without a per-attempt history table: if every + /// attempt is non-extending the deadline never leaves the first `submitted_at + TTL`, and if + /// an extending attempt happened at any point its grant survives every later one. It also + /// keeps a second, later-anchored view of the same transaction from shortening a live hold. + /// + /// The clamp is expressed 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. `MIN`/`MAX` are the two-argument SCALAR forms (the one-argument + /// forms are aggregates), which `the_clamp_uses_the_scalar_two_argument_min_and_max` pins. + /// + /// `coin_reservations`' `ON CONFLICT(coin_id) DO NOTHING` below is untouched: a coin still + /// backs exactly one bundle and the FIRST claim still wins. pub async fn reserve_spend(&self, tx: &PendingTransactionRow) -> sqlx::Result<()> { let mut conn = self.pool.begin().await?; sqlx::query( @@ -2769,7 +2810,13 @@ impl WalletDb { (transaction_id, bundle_hex, fee, submitted_at, expires_at, attempts) VALUES (?, ?, ?, ?, ?, ?) ON CONFLICT(transaction_id) DO UPDATE SET - expires_at = excluded.expires_at, + expires_at = CASE + WHEN ? = 0 THEN pending_transactions.expires_at + ELSE MAX( + pending_transactions.expires_at, + MIN(excluded.expires_at, pending_transactions.submitted_at + ?) + ) + END, attempts = pending_transactions.attempts + 1", ) .bind(&tx.transaction_id) @@ -2778,6 +2825,8 @@ impl WalletDb { .bind(tx.submitted_at) .bind(tx.expires_at) .bind(tx.attempts) + .bind(i64::from(tx.may_extend_expiry)) + .bind(super::rpc::MAX_RESERVATION_HOLD_MS) .execute(&mut *conn) .await?; for coin_id in &tx.reserved_coin_ids { @@ -3042,6 +3091,9 @@ impl WalletDb { submitted_at: r.get("submitted_at"), expires_at: r.get("expires_at"), attempts: r.get("attempts"), + // Not a stored column: a row read back describes a bundle, not the push that + // produced it. `true` is the conservative answer (see the field's own doc). + may_extend_expiry: true, reserved_coin_ids: coins.into_iter().map(|c| c.get("coin_id")).collect(), }); } @@ -4690,8 +4742,8 @@ fn is_unique_violation(e: &sqlx::Error) -> bool { #[cfg(test)] mod tests { - use super::*; use super::super::rpc::{MAX_RESERVATION_HOLD_MS, RESERVATION_TTL_MS}; + use super::*; /// **Proves (dig-node#462):** the only public write of `initial_sync_complete` that production /// can reach DISARMS, and it really does clear a flag that was armed. @@ -6390,6 +6442,7 @@ mod tests { submitted_at, expires_at, attempts: 1, + may_extend_expiry: true, reserved_coin_ids: coin_ids.iter().map(|c| (*c).to_string()).collect(), } } @@ -6500,9 +6553,14 @@ mod tests { let mut pushes = 0; let mut at = first_push; while at <= deadline { - db.reserve_spend(&reservation("tx1", &["c1"], first_push, at + RESERVATION_TTL_MS)) - .await - .unwrap(); + db.reserve_spend(&reservation( + "tx1", + &["c1"], + first_push, + at + RESERVATION_TTL_MS, + )) + .await + .unwrap(); assert_eq!( db.prune_reservations(at).await.unwrap(), 0, @@ -6541,14 +6599,24 @@ mod tests { .unwrap(); let first = 1_000; - db.reserve_spend(&reservation("tx1", &["c1"], first, first + RESERVATION_TTL_MS)) - .await - .unwrap(); + db.reserve_spend(&reservation( + "tx1", + &["c1"], + first, + first + RESERVATION_TTL_MS, + )) + .await + .unwrap(); let again = first + 9 * 60 * 1000; - db.reserve_spend(&reservation("tx1", &["c1"], first, again + RESERVATION_TTL_MS)) - .await - .unwrap(); + db.reserve_spend(&reservation( + "tx1", + &["c1"], + first, + again + RESERVATION_TTL_MS, + )) + .await + .unwrap(); let row = &db.pending_transactions().await.unwrap()[0]; assert_eq!( @@ -6569,12 +6637,22 @@ mod tests { .await .unwrap(); - db.reserve_spend(&reservation("tx1", &["c1"], 1_000, 1_000 + RESERVATION_TTL_MS)) - .await - .unwrap(); - db.reserve_spend(&reservation("tx1", &["c1"], 500_000, 500_000 + RESERVATION_TTL_MS)) - .await - .unwrap(); + db.reserve_spend(&reservation( + "tx1", + &["c1"], + 1_000, + 1_000 + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + db.reserve_spend(&reservation( + "tx1", + &["c1"], + 500_000, + 500_000 + RESERVATION_TTL_MS, + )) + .await + .unwrap(); let row = &db.pending_transactions().await.unwrap()[0]; assert_eq!( @@ -6596,9 +6674,14 @@ mod tests { .unwrap(); let first = 1_000; - db.reserve_spend(&reservation("tx1", &["c1"], first, first + RESERVATION_TTL_MS)) - .await - .unwrap(); + db.reserve_spend(&reservation( + "tx1", + &["c1"], + first, + first + RESERVATION_TTL_MS, + )) + .await + .unwrap(); // A re-push far beyond the cap: MIN picks the cap, MAX keeps it above the stored value. db.reserve_spend(&reservation( @@ -6618,6 +6701,169 @@ mod tests { ); } + /// A reservation whose push may not extend the deadline (dig-node#502). + fn non_extending( + tx: &str, + coin_ids: &[&str], + submitted_at: i64, + expires_at: i64, + ) -> PendingTransactionRow { + PendingTransactionRow { + may_extend_expiry: false, + ..reservation(tx, coin_ids, submitted_at, expires_at) + } + } + + /// **The systematic-emitter case the adversarial gate on #497 named.** A broken + /// wallet-construction path answers `BLOCK_COST_EXCEEDS_MAX` on every attempt and a caller + /// keeps retrying. Every one of those refusals is a complaint about the bundle's own contents, + /// so no destination is plausibly holding it and renewing the hold buys nothing. The deadline + /// must therefore stay at the FIRST push's, however many attempts arrive — a strictly tighter + /// bound than the total cap, reached without any clock argument. + #[tokio::test] + async fn a_bundle_refused_only_for_its_own_contents_never_renews_its_hold() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.upsert_coin(&coin("c1", 100, Some(10), None)) + .await + .unwrap(); + + let first = 1_000; + db.reserve_spend(&non_extending( + "tx1", + &["c1"], + first, + first + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + + for attempt in 1..=20 { + let at = first + attempt * 60 * 1000; + db.reserve_spend(&non_extending( + "tx1", + &["c1"], + first, + at + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + } + + let row = &db.pending_transactions().await.unwrap()[0]; + assert_eq!( + row.expires_at, + first + RESERVATION_TTL_MS, + "a refusal no later push can turn into an acceptance must not renew the hold" + ); + assert_eq!(row.attempts, 21, "the attempts are real re-pushes"); + + assert_eq!( + db.prune_reservations(first + RESERVATION_TTL_MS) + .await + .unwrap(), + 1, + "the coins must return at the first push's deadline" + ); + assert_eq!(db.unreserved_unspent_coins(None).await.unwrap().len(), 1); + } + + /// A re-push may never move the deadline EARLIER. This is what lets the two rules above compose + /// into "a bundle whose every observed refusal was foreclosing does not renew" without keeping + /// a per-attempt history: an extending attempt's grant survives every later non-extending one. + #[tokio::test] + async fn a_non_extending_repush_never_shortens_a_deadline_already_granted() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.upsert_coin(&coin("c1", 100, Some(10), None)) + .await + .unwrap(); + + let first = 1_000; + db.reserve_spend(&reservation( + "tx1", + &["c1"], + first, + first + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + + let extended_at = first + 9 * 60 * 1000; + db.reserve_spend(&reservation( + "tx1", + &["c1"], + first, + extended_at + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + + // A later attempt that may not extend, carrying an EARLIER deadline than the granted one. + db.reserve_spend(&non_extending( + "tx1", + &["c1"], + first, + first + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + + let row = &db.pending_transactions().await.unwrap()[0]; + assert_eq!( + row.expires_at, + extended_at + RESERVATION_TTL_MS, + "a non-extending attempt must leave an already-granted deadline alone, never shorten it" + ); + } + + /// The asymmetry the ticket requires to SURVIVE the fix: a coin backs exactly one in-flight + /// bundle and the FIRST claim wins. No path added for the cap may let a second bundle take a + /// coin already reserved, or shorten the first bundle's hold on it. + #[tokio::test] + async fn a_second_bundle_can_neither_take_nor_shorten_a_first_claim() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.upsert_coin(&coin("c1", 100, Some(10), None)) + .await + .unwrap(); + + let first = 1_000; + db.reserve_spend(&reservation( + "tx1", + &["c1"], + first, + first + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + // A second bundle claiming the same coin, non-extending and lapsing immediately. + db.reserve_spend(&non_extending("tx2", &["c1"], first, first + 1)) + .await + .unwrap(); + + assert_eq!( + db.prune_reservations(first + 1).await.unwrap(), + 1, + "only the second bundle lapses; the first still holds its coin" + ); + assert!( + db.unreserved_unspent_coins(None).await.unwrap().is_empty(), + "the first claim must keep the coin out of selection" + ); + + let held: Vec = db + .pending_transactions() + .await + .unwrap() + .into_iter() + .filter(|r| !r.reserved_coin_ids.is_empty()) + .map(|r| r.transaction_id) + .collect(); + assert_eq!( + held, + vec!["tx1".to_string()], + "the coin must still be reserved by the FIRST bundle" + ); + } + /// Settlement retires the reservation without anything having to remember to release it: the /// coin's own `spent_height` is the signal. #[tokio::test] @@ -7195,6 +7441,7 @@ mod tests { submitted_at: NOW, expires_at: NOW + 600_000, attempts: 1, + may_extend_expiry: true, reserved_coin_ids: vec!["aa".into()], }) .await @@ -7562,6 +7809,7 @@ mod tests { submitted_at: NOW, expires_at: NOW + 600_000, attempts: 1, + may_extend_expiry: true, reserved_coin_ids: vec!["bb".into()], }) .await @@ -7592,6 +7840,7 @@ mod tests { submitted_at: NOW, expires_at: NOW + 600_000, attempts: 1, + may_extend_expiry: true, reserved_coin_ids: vec!["aa".into()], }) .await diff --git a/crates/dig-wallet/src/sage/rpc.rs b/crates/dig-wallet/src/sage/rpc.rs index 207548c7..c6eb765a 100644 --- a/crates/dig-wallet/src/sage/rpc.rs +++ b/crates/dig-wallet/src/sage/rpc.rs @@ -2170,7 +2170,8 @@ impl WalletBackend { // mempool by this point, and reporting a push that did happen as an error would be a worse // lie than the double-selection this guards against. if !matches!(&pushed, Ok(o) if Self::is_definitive_rejection(o)) { - if let Err(e) = self.reserve_pushed_bundle(&bundle).await { + let may_extend = Self::attempt_may_extend_the_hold(&pushed); + if let Err(e) = self.reserve_pushed_bundle(&bundle, may_extend).await { tracing::warn!( error = %e, "pushed bundle may be in flight but its coins could not be reserved; a second \ @@ -2181,6 +2182,39 @@ impl WalletBackend { pushed.map_err(|e| PushError::Unreachable(e.to_string())) } + /// Whether THIS push attempt may push an existing reservation's deadline further out + /// (dig-node#502). + /// + /// The hold itself is decided by [`Self::is_definitive_rejection`] and is unchanged; this + /// decides only whether an attempt RENEWS one. The two questions differ because renewal is + /// worth something only while some destination may plausibly still be holding the bundle. When + /// the last answer was a complaint about the bundle's own CONTENTS that no later push can turn + /// into an acceptance, renewing buys nothing — and composed with a caller that retries, an + /// unconditional renewal held the inputs for as long as the retries continued, for a bundle + /// that was never going to land. + /// + /// The default is CONSERVATIVE: everything except a recognised foreclosing reason extends. + /// + /// | outcome | may extend | + /// |---|---| + /// | `Err` — no verdict was reached, so the bundle may be in flight | yes | + /// | accepted | yes | + /// | refused with no stated reason | yes | + /// | refused for a reason in `chain::refusal_forecloses_a_later_push` | **no** | + /// | refused for any other or unrecognised reason | yes | + /// + /// A bundle-intrinsic refusal never reaches this question at all: + /// [`Self::is_definitive_rejection`] skips the reservation entirely for those. + fn attempt_may_extend_the_hold(pushed: &Result) -> bool { + match pushed { + Ok(outcome) if !outcome.accepted => !outcome + .rejection + .as_deref() + .is_some_and(super::chain::refusal_forecloses_a_later_push), + _ => true, + } + } + /// Whether `outcome` is the network DEFINITIVELY refusing this bundle — the only case in which /// its inputs stay selectable (#348). /// @@ -2255,7 +2289,7 @@ impl WalletBackend { /// sign (§908), so a validation failure here says nothing about whether the bundle is /// legitimate — the mempool has already accepted it. An uncomputable fee is stored as `None` /// and reported as `null`, never as zero. - async fn reserve_pushed_bundle(&self, bundle: &SpendBundle) -> Result<()> { + async fn reserve_pushed_bundle(&self, bundle: &SpendBundle, may_extend: bool) -> Result<()> { let now = super::custody::now_ms() as i64; let row = super::db::PendingTransactionRow { transaction_id: hex::encode(bundle.name()), @@ -2266,6 +2300,7 @@ impl WalletBackend { submitted_at: now, expires_at: now + RESERVATION_TTL_MS, attempts: 1, + may_extend_expiry: may_extend, reserved_coin_ids: bundle .coin_spends .iter() @@ -10857,6 +10892,7 @@ mod tests { submitted_at: 1_000, expires_at, attempts: 1, + may_extend_expiry: true, reserved_coin_ids: coin_ids.to_vec(), } } From be772fbf2e9ca8c0d5e237da52bd3dca255344b0 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 08:01:54 -0700 Subject: [PATCH 04/15] chore: bump to 0.256.0, clear of #506's 0.251.0 --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 060fd2d1..f73fb4af 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3031,7 +3031,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.251.0" +version = "0.256.0" dependencies = [ "async-trait", "axum", diff --git a/Cargo.toml b/Cargo.toml index 710a96b4..d7340e04 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ edition = "2021" # the ROOT manifest (`[workspace.package].version`), so it MUST be set here for a # release to fire (§3.6). The library crates (dig-node-core/dig-runtime/dig-wallet) # keep their own independent versions — only the released binary tracks the workspace version. -version = "0.251.0" +version = "0.256.0" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over From fce235895d7e9aa17c1e351e74b8ddbd99b79044 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 08:08:49 -0700 Subject: [PATCH 05/15] 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 --- SPEC.md | 17 +++ crates/dig-wallet/src/sage/chain.rs | 109 ------------- crates/dig-wallet/src/sage/db.rs | 227 ++++++++++++++++++---------- crates/dig-wallet/src/sage/rpc.rs | 71 ++++----- 4 files changed, 194 insertions(+), 230 deletions(-) diff --git a/SPEC.md b/SPEC.md index e59260cc..b7add90a 100644 --- a/SPEC.md +++ b/SPEC.md @@ -5580,6 +5580,23 @@ thereby return the coins to selection — a second send inside the confirmation reselect the same inputs. The TTL MUST NOT be shortened to compensate for the wider hold: that trades a double-select for a lockout, and a lockout is the worse failure. +The TTL bounds ONE hold and MUST NOT be relied on to bound a SEQUENCE of them. A re-push of the same +transaction renews the hold from the time of that push, so a caller re-pushing more often than the TTL +would otherwise hold the inputs for as long as it kept retrying — the lockout the previous paragraph +names as the worse failure, reachable without a single dishonest answer. A node MUST therefore also +bound the TOTAL hold: the deadline recorded for a transaction MUST NOT exceed the time of its FIRST +observed push plus `MAX_RESERVATION_HOLD_MS`, which MUST be `6 * RESERVATION_TTL_MS` (one hour). + +The first-push time MUST be a stable anchor: a re-push MUST update the deadline and the attempt count +and MUST NOT rewrite it. A re-push MUST NOT move a recorded deadline EARLIER, so that a clock which +steps backwards cannot shorten a hold that is already live. + +This bound is on CONTINUOUS hold. Once the deadline passes, the reservation and its coin claims are +released and the inputs become selectable again; a subsequent push of the same transaction is a new +reservation with a new anchor and MAY hold the inputs for a further full period. A node MUST NOT +refuse to re-hold a transaction on the grounds that it has already held one, since the transaction may +still be admitted and refusing would restore the double-select this section exists to close. + The set of bundle-intrinsic reasons MUST be an ALLOWLIST whose default is to HOLD. The reason text is supplied by an untrusted source (§13), so an unrecognised reason MUST hold rather than free: the enumeration cannot be complete, and a node MUST NOT be made to free a user's inputs by a reason diff --git a/crates/dig-wallet/src/sage/chain.rs b/crates/dig-wallet/src/sage/chain.rs index e95ccddc..96ef1422 100644 --- a/crates/dig-wallet/src/sage/chain.rs +++ b/crates/dig-wallet/src/sage/chain.rs @@ -285,55 +285,6 @@ pub(crate) fn refusal_is_bundle_intrinsic(stated: &str) -> bool { .any(|intrinsic| reason.eq_ignore_ascii_case(intrinsic)) } -/// Refusals that are about the BUNDLE'S OWN CONTENTS but are deliberately absent from -/// [`BUNDLE_INTRINSIC_REFUSALS`] because the verdict depends on the answering node's HEIGHT and -/// cost budget (dig-node#502). -/// -/// These are the four CLVM-execution / cost names the list above names explicitly as excluded, for -/// exactly that reason: `chia_consensus::spendbundle_validation` derives its flags from -/// `prev_tx_height` and runs under a caller-supplied `max_cost`, so two honest nodes can reach -/// different verdicts on identical bytes. They must keep HOLDING the inputs, and that is unchanged. -/// -/// What this list decides is narrower: whether such a refusal may EXTEND an existing hold. It may -/// not. Extending buys something only when some destination may plausibly still be holding the -/// bundle, and here nothing is: the complaint is about the bytes. Composed with a systematic -/// emitter — a broken wallet-construction path that answers `BLOCK_COST_EXCEEDS_MAX` on every -/// attempt — and a caller that keeps retrying, an unconditional re-arm holds the inputs for as long -/// as the retries continue, for a bundle that was never going to land. -/// -/// # What must NOT be added here, and why a future reader will want to -/// -/// The other absentees from [`BUNDLE_INTRINSIC_REFUSALS`] look like they belong and do not. The -/// timelock assertions (`ASSERT_HEIGHT_*`, `ASSERT_SECONDS_*`, `ASSERT_BEFORE_*`), the fee-policy -/// names, `UNKNOWN_UNSPENT` and `TOO_MANY_ANNOUNCEMENTS` can each be ADMITTED by a DIFFERENT node -/// than the one that refused — a node further ahead, with a different relay policy, or caught up. -/// A later push of the same bundle may therefore genuinely land, so extending the hold is CORRECT -/// for them. The four here differ only in degree, but the degree is the point: a cost or generator -/// verdict does not turn into an acceptance by asking a better-synced peer. -/// -/// Matching is EXACT, case-insensitive and trimmed, via [`refusal_reason`] — never substring or -/// prefix, for the same reason [`refusal_is_bundle_intrinsic`] gives. An unrecognised reason is not -/// on this list, so it EXTENDS: the conservative default, because a hold that lapses too early is -/// the double-select this family exists to close. -const VIEW_DEPENDENT_BUNDLE_CONTENT_REFUSALS: &[&str] = &[ - "GENERATOR_RUNTIME_ERROR", - "BLOCK_COST_EXCEEDS_MAX", - "INVALID_BLOCK_COST", - "INVALID_SPEND_BUNDLE", -]; - -/// Whether a stated refusal is a complaint about the bundle's own contents that no later push can -/// turn into an acceptance — the class that HOLDS but must not EXTEND the hold (dig-node#502). -/// -/// See [`VIEW_DEPENDENT_BUNDLE_CONTENT_REFUSALS`] for the membership rule and for the names that -/// deliberately stay out of it. -pub(crate) fn refusal_forecloses_a_later_push(stated: &str) -> bool { - let reason = refusal_reason(stated); - VIEW_DEPENDENT_BUNDLE_CONTENT_REFUSALS - .iter() - .any(|foreclosing| reason.eq_ignore_ascii_case(foreclosing)) -} - /// Pushes an ALREADY-SIGNED bundle to the network. /// /// A trait rather than a concrete client so the control surface can be driven end to end without a @@ -983,66 +934,6 @@ mod tests { } } - /// **Proves (dig-node#502):** the foreclosing class is recognised exactly, and everything else - /// EXTENDS. - /// - /// The membership rows are the four CLVM-execution / cost names. The shape rows are the ones - /// that matter more than the contents: an unrecognised reason must extend (the conservative - /// default, since a hold that lapses too early is the double-select this family exists to - /// close), and a name EMBEDDED in wider text must not match — the same exact-match discipline - /// `refusal_is_bundle_intrinsic` keeps, because a substring test lets a source turn any string - /// into a non-extending one by quoting a name inside it. - #[test] - fn the_foreclosing_refusals_match_exactly_and_everything_else_extends() { - for foreclosing in [ - "GENERATOR_RUNTIME_ERROR", - "BLOCK_COST_EXCEEDS_MAX", - "INVALID_BLOCK_COST", - "INVALID_SPEND_BUNDLE", - " block_cost_exceeds_max ", - "FAILED: BLOCK_COST_EXCEEDS_MAX", - ] { - assert!( - super::refusal_forecloses_a_later_push(foreclosing), - "{foreclosing} must not renew a hold: trimmed, case-insensitive, exact" - ); - } - - for extending in [ - "DOUBLE_SPEND", - "MEMPOOL_CONFLICT", - "ALREADY_INCLUDING_TRANSACTION", - "UNKNOWN_UNSPENT", - "TOO_MANY_ANNOUNCEMENTS", - "ASSERT_HEIGHT_ABSOLUTE_FAILED", - "ASSERT_SECONDS_RELATIVE_FAILED", - "ASSERT_BEFORE_HEIGHT_ABSOLUTE_FAILED", - "INVALID_FEE_LOW_FEE", - "SOMETHING_NOBODY_ENUMERATED", - "MEMPOOL_CONFLICT (see BLOCK_COST_EXCEEDS_MAX)", - "", - ] { - assert!( - !super::refusal_forecloses_a_later_push(extending), - "{extending:?} may be admitted by a better-synced or differently-configured node, \ - so a later push may land and the hold must still renew" - ); - } - } - - /// The two allowlists answer DIFFERENT questions — free the coins now, versus renew the hold — - /// and a name on both would mean the second list is dead code, because a bundle-intrinsic - /// refusal never reserves at all. - #[test] - fn the_foreclosing_list_is_disjoint_from_the_bundle_intrinsic_one() { - for name in super::VIEW_DEPENDENT_BUNDLE_CONTENT_REFUSALS { - assert!( - !super::refusal_is_bundle_intrinsic(name), - "{name} is on both lists, so its non-extending rule is unreachable" - ); - } - } - /// **Proves (dig-node#460):** the two refusal CLASSES are told apart, and the default is HOLD. /// /// The peer-local rows are not decoration: `ALREADY_INCLUDING_TRANSACTION`, `DOUBLE_SPEND` and diff --git a/crates/dig-wallet/src/sage/db.rs b/crates/dig-wallet/src/sage/db.rs index cf79de02..428ae8fd 100644 --- a/crates/dig-wallet/src/sage/db.rs +++ b/crates/dig-wallet/src/sage/db.rs @@ -224,19 +224,6 @@ pub struct PendingTransactionRow { pub expires_at: i64, /// How many times this bundle has been pushed (1 on first broadcast). pub attempts: i64, - /// Whether THIS attempt may push the deadline further out (dig-node#502). - /// - /// An INPUT to [`WalletDb::reserve_spend`], not a stored column: it is a property of the push - /// that produced this row, not of the bundle. Rows read back out of the database report `true`, - /// the conservative value, because the answer that produced them is not kept. - /// - /// `false` says the last destination complained about the bundle's own CONTENTS in a way no - /// later push can turn into an acceptance - /// (`chain::refusal_forecloses_a_later_push`). The coins still stay held for the - /// remaining hold — the verdict is height- and cost-budget-dependent, so this crate declines to - /// trust one node's view of it — but a retry loop against a systematically broken bundle must - /// not be able to renew that hold forever. - pub may_extend_expiry: bool, /// The coin ids the bundle spends — the coins held out of further selection while it is live. pub reserved_coin_ids: Vec, } @@ -2783,17 +2770,16 @@ impl WalletDb { /// than the TTL renewed its hold forever, and the inputs never returned — the lockout failure /// `rpc::RESERVATION_TTL_MS`'s own doc names as the worse of the two. /// - /// Three rules compose to bound it without shortening the TTL: + /// Two rules compose to bound it without shortening the TTL: /// /// 1. **Clamped to the FIRST push.** `submitted_at` is not in the `DO UPDATE SET` list, so the /// stored value is a stable anchor, and the new deadline may not exceed /// `submitted_at + MAX_RESERVATION_HOLD_MS`. - /// 2. **A foreclosing attempt does not extend at all** — see - /// [`PendingTransactionRow::may_extend_expiry`]. - /// 3. **A re-push may never move the deadline EARLIER.** This is what makes rules 1 and 2 add - /// up to the property the ticket asks for without a per-attempt history table: if every - /// attempt is non-extending the deadline never leaves the first `submitted_at + TTL`, and if - /// an extending attempt happened at any point its grant survives every later one. It also + /// 2. **A re-push may never move the deadline EARLIER.** The incoming `expires_at` is + /// `now + TTL`, so under a MONOTONIC clock the outer `MAX` never binds. It exists for the + /// non-monotonic one: an NTP step or a manual clock change moves `now` backwards, and + /// without the `MAX` that re-push would SHORTEN a live hold — returning the inputs of a + /// bundle that may still land, which is the direction #348/#497 exist to close. It also /// keeps a second, later-anchored view of the same transaction from shortening a live hold. /// /// The clamp is expressed in SQL rather than computed in the caller because it must be atomic @@ -2801,6 +2787,12 @@ impl WalletDb { /// pushes of the same bundle. `MIN`/`MAX` are the two-argument SCALAR forms (the one-argument /// forms are aggregates), which `the_clamp_uses_the_scalar_two_argument_min_and_max` pins. /// + /// The clamp fails OPEN, not closed, under an absurd `submitted_at`: SQLite does not trap + /// `INTEGER` overflow on `+` but promotes the result to `REAL`, so an anchor near `i64::MAX` + /// makes `submitted_at + MAX_RESERVATION_HOLD_MS` exceed any plausible incoming deadline and + /// the cap stops binding. `submitted_at` comes from `custody::now_ms()` and is never + /// caller-supplied, so reaching it needs a wildly wrong system clock rather than an attacker. + /// /// `coin_reservations`' `ON CONFLICT(coin_id) DO NOTHING` below is untouched: a coin still /// backs exactly one bundle and the FIRST claim still wins. pub async fn reserve_spend(&self, tx: &PendingTransactionRow) -> sqlx::Result<()> { @@ -2810,13 +2802,10 @@ impl WalletDb { (transaction_id, bundle_hex, fee, submitted_at, expires_at, attempts) VALUES (?, ?, ?, ?, ?, ?) ON CONFLICT(transaction_id) DO UPDATE SET - expires_at = CASE - WHEN ? = 0 THEN pending_transactions.expires_at - ELSE MAX( - pending_transactions.expires_at, - MIN(excluded.expires_at, pending_transactions.submitted_at + ?) - ) - END, + expires_at = MAX( + pending_transactions.expires_at, + MIN(excluded.expires_at, pending_transactions.submitted_at + ?) + ), attempts = pending_transactions.attempts + 1", ) .bind(&tx.transaction_id) @@ -2825,7 +2814,6 @@ impl WalletDb { .bind(tx.submitted_at) .bind(tx.expires_at) .bind(tx.attempts) - .bind(i64::from(tx.may_extend_expiry)) .bind(super::rpc::MAX_RESERVATION_HOLD_MS) .execute(&mut *conn) .await?; @@ -3093,7 +3081,6 @@ impl WalletDb { attempts: r.get("attempts"), // Not a stored column: a row read back describes a bundle, not the push that // produced it. `true` is the conservative answer (see the field's own doc). - may_extend_expiry: true, reserved_coin_ids: coins.into_iter().map(|c| c.get("coin_id")).collect(), }); } @@ -6442,7 +6429,6 @@ mod tests { submitted_at, expires_at, attempts: 1, - may_extend_expiry: true, reserved_coin_ids: coin_ids.iter().map(|c| (*c).to_string()).collect(), } } @@ -6701,34 +6687,94 @@ mod tests { ); } - /// A reservation whose push may not extend the deadline (dig-node#502). - fn non_extending( - tx: &str, - coin_ids: &[&str], - submitted_at: i64, - expires_at: i64, - ) -> PendingTransactionRow { - PendingTransactionRow { - may_extend_expiry: false, - ..reservation(tx, coin_ids, submitted_at, expires_at) - } + /// **The SAWTOOTH the cap deliberately leaves open (dig-node#502).** `prune_reservations` + /// DELETEs the row at the cap, so `submitted_at` anchors the clamp only while the row exists. + /// A re-push after the prune INSERTs a fresh row with a NEW anchor and a full new hour. + /// + /// Pinned as known, intended behaviour rather than left as an unmeasured surprise: the bound + /// is on CONTINUOUS hold, not aggregate hold. Closing it would mean permanently declining to + /// re-hold a bundle that already had its hour, which is the double-spend direction, and it + /// would need the per-bundle history this ticket deliberately avoided. + #[tokio::test] + async fn a_repushed_bundle_gets_a_fresh_anchor_after_the_cap_prunes_its_row() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.upsert_coin(&coin("c1", 100, Some(10), None)) + .await + .unwrap(); + + // The clamp lives in the `ON CONFLICT` arm, so it binds on RE-push. The first INSERT takes + // its deadline verbatim, which in production is always `now + TTL`. + let first = 1_000; + db.reserve_spend(&reservation( + "tx1", + &["c1"], + first, + first + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + db.reserve_spend(&reservation( + "tx1", + &["c1"], + first, + first + 100 * RESERVATION_TTL_MS, + )) + .await + .unwrap(); + assert_eq!( + db.pending_transactions().await.unwrap()[0].expires_at, + first + MAX_RESERVATION_HOLD_MS, + "a retrying caller's hold is clamped to the cap" + ); + + let capped_at = first + MAX_RESERVATION_HOLD_MS; + assert_eq!( + db.prune_reservations(capped_at).await.unwrap(), + 1, + "the cap must actually release the coins" + ); + assert_eq!(db.unreserved_unspent_coins(None).await.unwrap().len(), 1); + + // The same bundle, pushed again after the release: a NEW row, so a NEW anchor. + let second = capped_at + 1; + db.reserve_spend(&reservation( + "tx1", + &["c1"], + second, + second + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + + let row = &db.pending_transactions().await.unwrap()[0]; + assert_eq!( + row.submitted_at, second, + "the pruned row is gone, so the re-push anchors on its own push time" + ); + assert_eq!( + row.attempts, 1, + "a fresh row starts its attempt count over: this is an INSERT, not the upsert path" + ); + assert_eq!( + row.expires_at, + second + RESERVATION_TTL_MS, + "and it may accumulate a full new hour from the new anchor" + ); } - /// **The systematic-emitter case the adversarial gate on #497 named.** A broken - /// wallet-construction path answers `BLOCK_COST_EXCEEDS_MAX` on every attempt and a caller - /// keeps retrying. Every one of those refusals is a complaint about the bundle's own contents, - /// so no destination is plausibly holding it and renewing the hold buys nothing. The deadline - /// must therefore stay at the FIRST push's, however many attempts arrive — a strictly tighter - /// bound than the total cap, reached without any clock argument. + /// **The clamp's boundary (dig-node#502).** Inside the last TTL before the cap the clamp binds, + /// so a re-push extends the deadline by strictly LESS than a full TTL, shrinking to nothing + /// exactly at the cap. Correct and intended — it is what a clamp does — but asserted from BOTH + /// sides, because a bound tested only from below can only confirm itself. #[tokio::test] - async fn a_bundle_refused_only_for_its_own_contents_never_renews_its_hold() { + async fn a_repush_inside_the_last_ttl_before_the_cap_buys_less_than_a_full_ttl() { let db = WalletDb::open_in_memory().await.unwrap(); db.upsert_coin(&coin("c1", 100, Some(10), None)) .await .unwrap(); let first = 1_000; - db.reserve_spend(&non_extending( + db.reserve_spend(&reservation( "tx1", &["c1"], first, @@ -6737,41 +6783,63 @@ mod tests { .await .unwrap(); - for attempt in 1..=20 { - let at = first + attempt * 60 * 1000; - db.reserve_spend(&non_extending( - "tx1", - &["c1"], - first, - at + RESERVATION_TTL_MS, - )) + // BELOW the boundary: a re-push at 4 x TTL wants 5 x TTL, which is under the cap, so the + // clamp does not bind and the full TTL is granted. + let under = first + 4 * RESERVATION_TTL_MS; + db.reserve_spend(&reservation("tx1", &["c1"], first, under + RESERVATION_TTL_MS)) .await .unwrap(); - } + assert_eq!( + db.pending_transactions().await.unwrap()[0].expires_at, + under + RESERVATION_TTL_MS, + "under the boundary the clamp must not bind: a full TTL is granted" + ); - let row = &db.pending_transactions().await.unwrap()[0]; + // OVER the boundary: a re-push half a TTL later wants 5.5 x TTL past the anchor. The cap + // is 6 x TTL, so it is granted in full — the last push that still buys everything it asks. + let at_edge = first + 5 * RESERVATION_TTL_MS; + db.reserve_spend(&reservation( + "tx1", + &["c1"], + first, + at_edge + RESERVATION_TTL_MS, + )) + .await + .unwrap(); assert_eq!( - row.expires_at, - first + RESERVATION_TTL_MS, - "a refusal no later push can turn into an acceptance must not renew the hold" + db.pending_transactions().await.unwrap()[0].expires_at, + first + MAX_RESERVATION_HOLD_MS, + "a push at 5 x TTL asks for exactly the cap and gets exactly the cap" ); - assert_eq!(row.attempts, 21, "the attempts are real re-pushes"); + // PAST it: the next re-push buys strictly less than a TTL — in fact nothing at all. + let past = first + 5 * RESERVATION_TTL_MS + 60_000; + db.reserve_spend(&reservation("tx1", &["c1"], first, past + RESERVATION_TTL_MS)) + .await + .unwrap(); + let row = &db.pending_transactions().await.unwrap()[0]; assert_eq!( - db.prune_reservations(first + RESERVATION_TTL_MS) - .await - .unwrap(), - 1, - "the coins must return at the first push's deadline" + row.expires_at, + first + MAX_RESERVATION_HOLD_MS, + "past the boundary a re-push buys strictly less than a TTL, and at the cap it buys zero" + ); + assert!( + row.expires_at < past + RESERVATION_TTL_MS, + "the granted deadline must be short of what the push asked for" ); - assert_eq!(db.unreserved_unspent_coins(None).await.unwrap().len(), 1); } - /// A re-push may never move the deadline EARLIER. This is what lets the two rules above compose - /// into "a bundle whose every observed refusal was foreclosing does not renew" without keeping - /// a per-attempt history: an extending attempt's grant survives every later non-extending one. + /// **Why the clamp keeps its outer `MAX` (dig-node#502).** With the deadline written as + /// `MIN(excluded.expires_at, submitted_at + cap)` alone, the incoming value is `now + TTL` and + /// a MONOTONIC clock can never make that smaller than a deadline already stored. A + /// non-monotonic one can: an NTP step or a manual clock change moves `now` backwards, and the + /// re-push then carries an EARLIER deadline than the live hold. + /// + /// Shortening a live hold is the dangerous direction — it returns the inputs of a bundle that + /// may still land, which is the double-select #348/#497 exist to close — so the outer `MAX` + /// makes a re-push able only ever to move the deadline outwards, never inwards. #[tokio::test] - async fn a_non_extending_repush_never_shortens_a_deadline_already_granted() { + async fn a_repush_under_a_clock_that_stepped_backwards_never_shortens_a_live_hold() { let db = WalletDb::open_in_memory().await.unwrap(); db.upsert_coin(&coin("c1", 100, Some(10), None)) .await @@ -6797,8 +6865,8 @@ mod tests { .await .unwrap(); - // A later attempt that may not extend, carrying an EARLIER deadline than the granted one. - db.reserve_spend(&non_extending( + // The clock steps backwards, so this re-push carries an EARLIER deadline than the live one. + db.reserve_spend(&reservation( "tx1", &["c1"], first, @@ -6811,7 +6879,7 @@ mod tests { assert_eq!( row.expires_at, extended_at + RESERVATION_TTL_MS, - "a non-extending attempt must leave an already-granted deadline alone, never shorten it" + "a backwards clock step must leave an already-granted deadline alone, never shorten it" ); } @@ -6834,8 +6902,8 @@ mod tests { )) .await .unwrap(); - // A second bundle claiming the same coin, non-extending and lapsing immediately. - db.reserve_spend(&non_extending("tx2", &["c1"], first, first + 1)) + // A second bundle claiming the same coin, lapsing immediately. + db.reserve_spend(&reservation("tx2", &["c1"], first, first + 1)) .await .unwrap(); @@ -7441,7 +7509,6 @@ mod tests { submitted_at: NOW, expires_at: NOW + 600_000, attempts: 1, - may_extend_expiry: true, reserved_coin_ids: vec!["aa".into()], }) .await @@ -7809,7 +7876,6 @@ mod tests { submitted_at: NOW, expires_at: NOW + 600_000, attempts: 1, - may_extend_expiry: true, reserved_coin_ids: vec!["bb".into()], }) .await @@ -7840,7 +7906,6 @@ mod tests { submitted_at: NOW, expires_at: NOW + 600_000, attempts: 1, - may_extend_expiry: true, reserved_coin_ids: vec!["aa".into()], }) .await diff --git a/crates/dig-wallet/src/sage/rpc.rs b/crates/dig-wallet/src/sage/rpc.rs index c6eb765a..6eb83d1b 100644 --- a/crates/dig-wallet/src/sage/rpc.rs +++ b/crates/dig-wallet/src/sage/rpc.rs @@ -558,8 +558,35 @@ pub(crate) const RESERVATION_TTL_MS: i64 = 10 * 60 * 1000; /// Six is sized by the same question as the TTL. Chia blocks are ~52s apart, so an hour is roughly /// seventy chances for the spend to land — far past the point where an unconfirmed bundle is more /// likely dropped than pending — while still returning a stranded coin on a timescale a user waits -/// out. The clamp is anchored on `submitted_at`, which the reservation upsert never rewrites, so it -/// is a bound on the bundle's whole life rather than on any one attempt. +/// out. +/// +/// # What this bound is NOT +/// +/// Three limitations are deliberate. Each is the price of having a finite cap at all, and each is +/// stated here because the bound is otherwise easy to read as stronger than it is. +/// +/// 1. **It bounds a CONTINUOUS hold, not an AGGREGATE one.** `submitted_at` anchors the clamp only +/// while the row exists, and `WalletDb::prune_reservations` DELETEs the row at the cap. The next +/// re-push therefore INSERTs a fresh row with a new `submitted_at` and a full new hour, so an +/// indefinitely retrying caller produces a SAWTOOTH — one-hour holds separated by an instant of +/// selectability — rather than one bounded total across the bundle's life. This is intended: at +/// each release the coins were genuinely selectable again, and refusing to ever re-hold a bundle +/// that already had its hour would mean permanently declining to protect a bundle that may still +/// land, which is the double-spend direction. Pinned by +/// `a_repushed_bundle_gets_a_fresh_anchor_after_the_cap_prunes_its_row`. +/// 2. **A bundle whose TIMELOCK matures later than the cap has its inputs freed while still +/// valid.** This is the one class where the network genuinely retains the bundle — a node +/// answers PENDING rather than FAILED for an unmet `ASSERT_HEIGHT_ABSOLUTE` or +/// `ASSERT_SECONDS_ABSOLUTE` — so the inputs are released while some mempool is really still +/// holding it, and it will be admitted once the condition is met. Accepted rather than fixed: the +/// release is CLOCK-driven, so no peer can advance it and the case carries no attacker leverage; +/// this node builds no timelocked bundles of its own; and the alternative is the indefinite +/// lockout this constant exists to close. +/// 3. **Near the cap, a re-push buys strictly LESS than a full TTL.** Between +/// `submitted_at + 5 * RESERVATION_TTL_MS` and the cap the clamp binds, so each renewal extends +/// the deadline by a shrinking amount that reaches zero exactly at the cap. That is what a clamp +/// does rather than a defect, and +/// `a_repush_inside_the_last_ttl_before_the_cap_buys_less_than_a_full_ttl` pins the boundary. pub(crate) const MAX_RESERVATION_HOLD_MS: i64 = 6 * RESERVATION_TTL_MS; /// The Sage-parity wallet backend. @@ -2170,8 +2197,7 @@ impl WalletBackend { // mempool by this point, and reporting a push that did happen as an error would be a worse // lie than the double-selection this guards against. 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 { + if let Err(e) = self.reserve_pushed_bundle(&bundle).await { tracing::warn!( error = %e, "pushed bundle may be in flight but its coins could not be reserved; a second \ @@ -2182,39 +2208,6 @@ impl WalletBackend { pushed.map_err(|e| PushError::Unreachable(e.to_string())) } - /// Whether THIS push attempt may push an existing reservation's deadline further out - /// (dig-node#502). - /// - /// The hold itself is decided by [`Self::is_definitive_rejection`] and is unchanged; this - /// decides only whether an attempt RENEWS one. The two questions differ because renewal is - /// worth something only while some destination may plausibly still be holding the bundle. When - /// the last answer was a complaint about the bundle's own CONTENTS that no later push can turn - /// into an acceptance, renewing buys nothing — and composed with a caller that retries, an - /// unconditional renewal held the inputs for as long as the retries continued, for a bundle - /// that was never going to land. - /// - /// The default is CONSERVATIVE: everything except a recognised foreclosing reason extends. - /// - /// | outcome | may extend | - /// |---|---| - /// | `Err` — no verdict was reached, so the bundle may be in flight | yes | - /// | accepted | yes | - /// | refused with no stated reason | yes | - /// | refused for a reason in `chain::refusal_forecloses_a_later_push` | **no** | - /// | refused for any other or unrecognised reason | yes | - /// - /// A bundle-intrinsic refusal never reaches this question at all: - /// [`Self::is_definitive_rejection`] skips the reservation entirely for those. - fn attempt_may_extend_the_hold(pushed: &Result) -> bool { - match pushed { - Ok(outcome) if !outcome.accepted => !outcome - .rejection - .as_deref() - .is_some_and(super::chain::refusal_forecloses_a_later_push), - _ => true, - } - } - /// Whether `outcome` is the network DEFINITIVELY refusing this bundle — the only case in which /// its inputs stay selectable (#348). /// @@ -2289,7 +2282,7 @@ impl WalletBackend { /// sign (§908), so a validation failure here says nothing about whether the bundle is /// legitimate — the mempool has already accepted it. An uncomputable fee is stored as `None` /// and reported as `null`, never as zero. - async fn reserve_pushed_bundle(&self, bundle: &SpendBundle, may_extend: bool) -> Result<()> { + async fn reserve_pushed_bundle(&self, bundle: &SpendBundle) -> Result<()> { let now = super::custody::now_ms() as i64; let row = super::db::PendingTransactionRow { transaction_id: hex::encode(bundle.name()), @@ -2300,7 +2293,6 @@ impl WalletBackend { submitted_at: now, expires_at: now + RESERVATION_TTL_MS, attempts: 1, - may_extend_expiry: may_extend, reserved_coin_ids: bundle .coin_spends .iter() @@ -10892,7 +10884,6 @@ mod tests { submitted_at: 1_000, expires_at, attempts: 1, - may_extend_expiry: true, reserved_coin_ids: coin_ids.to_vec(), } } From 10f0533cc4e140d3058b98c78316fa90a61841e6 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 19:36:44 -0700 Subject: [PATCH 06/15] chore: renumber to 0.252.8, under the MSI ProductVersion ceiling (#521) --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0099923a..7a9eb424 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3031,7 +3031,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.256.0" +version = "0.252.8" dependencies = [ "async-trait", "axum", diff --git a/Cargo.toml b/Cargo.toml index d7340e04..09b383cf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ edition = "2021" # the ROOT manifest (`[workspace.package].version`), so it MUST be set here for a # release to fire (§3.6). The library crates (dig-node-core/dig-runtime/dig-wallet) # keep their own independent versions — only the released binary tracks the workspace version. -version = "0.256.0" +version = "0.252.8" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over From 11d46af97bb914d8cfd1d6c36a84a254eed7bfb7 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 19:56:27 -0700 Subject: [PATCH 07/15] 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 --- crates/dig-wallet/src/sage/db.rs | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/crates/dig-wallet/src/sage/db.rs b/crates/dig-wallet/src/sage/db.rs index 428ae8fd..1718cbf7 100644 --- a/crates/dig-wallet/src/sage/db.rs +++ b/crates/dig-wallet/src/sage/db.rs @@ -6786,9 +6786,14 @@ mod tests { // BELOW the boundary: a re-push at 4 x TTL wants 5 x TTL, which is under the cap, so the // clamp does not bind and the full TTL is granted. let under = first + 4 * RESERVATION_TTL_MS; - db.reserve_spend(&reservation("tx1", &["c1"], first, under + RESERVATION_TTL_MS)) - .await - .unwrap(); + db.reserve_spend(&reservation( + "tx1", + &["c1"], + first, + under + RESERVATION_TTL_MS, + )) + .await + .unwrap(); assert_eq!( db.pending_transactions().await.unwrap()[0].expires_at, under + RESERVATION_TTL_MS, @@ -6814,9 +6819,14 @@ mod tests { // PAST it: the next re-push buys strictly less than a TTL — in fact nothing at all. let past = first + 5 * RESERVATION_TTL_MS + 60_000; - db.reserve_spend(&reservation("tx1", &["c1"], first, past + RESERVATION_TTL_MS)) - .await - .unwrap(); + db.reserve_spend(&reservation( + "tx1", + &["c1"], + first, + past + RESERVATION_TTL_MS, + )) + .await + .unwrap(); let row = &db.pending_transactions().await.unwrap()[0]; assert_eq!( row.expires_at, From e01173c5e91ba72cd137b34838cd1e6503a9c2e8 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 20:05:52 -0700 Subject: [PATCH 08/15] 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 --- SPEC.md | 8 +++++--- crates/dig-wallet/src/sage/db.rs | 2 -- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/SPEC.md b/SPEC.md index 2aa3c563..9c91cddf 100644 --- a/SPEC.md +++ b/SPEC.md @@ -5751,9 +5751,11 @@ names as the worse failure, reachable without a single dishonest answer. A node bound the TOTAL hold: the deadline recorded for a transaction MUST NOT exceed the time of its FIRST observed push plus `MAX_RESERVATION_HOLD_MS`, which MUST be `6 * RESERVATION_TTL_MS` (one hour). -The first-push time MUST be a stable anchor: a re-push MUST update the deadline and the attempt count -and MUST NOT rewrite it. A re-push MUST NOT move a recorded deadline EARLIER, so that a clock which -steps backwards cannot shorten a hold that is already live. +The first-push time MUST be a stable anchor: a re-push MUST re-arm the deadline subject to the bound +above — which at or past the cap, or under a clock that has stepped backwards, leaves it unchanged — +MUST increment the attempt count, and MUST NOT rewrite the anchor. A re-push MUST NOT move a +recorded deadline EARLIER, so that a clock which steps backwards cannot shorten a hold that is +already live. This bound is on CONTINUOUS hold. Once the deadline passes, the reservation and its coin claims are released and the inputs become selectable again; a subsequent push of the same transaction is a new diff --git a/crates/dig-wallet/src/sage/db.rs b/crates/dig-wallet/src/sage/db.rs index 1718cbf7..1847110f 100644 --- a/crates/dig-wallet/src/sage/db.rs +++ b/crates/dig-wallet/src/sage/db.rs @@ -3079,8 +3079,6 @@ impl WalletDb { submitted_at: r.get("submitted_at"), expires_at: r.get("expires_at"), attempts: r.get("attempts"), - // Not a stored column: a row read back describes a bundle, not the push that - // produced it. `true` is the conservative answer (see the field's own doc). reserved_coin_ids: coins.into_iter().map(|c| c.get("coin_id")).collect(), }); } From f0a9726dcad753476274ca8dc4ab124311b7282e Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Thu, 3 Sep 2026 00:43:38 -0700 Subject: [PATCH 09/15] 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 --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 9a3a164e..de44f507 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ edition = "2021" # the ROOT manifest (`[workspace.package].version`), so it MUST be set here for a # release to fire (§3.6). The library crates (dig-node-core/dig-runtime/dig-wallet) # keep their own independent versions — only the released binary tracks the workspace version. -version = "0.252.20" +version = "0.252.31" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over From ffa907fa2994dfd2fc015e94bf53b65a3f91b2b3 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Thu, 3 Sep 2026 01:12:57 -0700 Subject: [PATCH 10/15] 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 --- Cargo.lock | 2 +- SPEC.md | 13 ++ crates/dig-wallet/src/sage/db.rs | 375 +++++++++++++++++++++++++++++- crates/dig-wallet/src/sage/rpc.rs | 8 + 4 files changed, 392 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cab4661c..f6ade0fa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3031,7 +3031,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.252.20" +version = "0.252.31" dependencies = [ "async-trait", "axum", diff --git a/SPEC.md b/SPEC.md index 9b76f6c5..26d42652 100644 --- a/SPEC.md +++ b/SPEC.md @@ -5793,6 +5793,19 @@ MUST increment the attempt count, and MUST NOT rewrite the anchor. A re-push MUS recorded deadline EARLIER, so that a clock which steps backwards cannot shorten a hold that is already live. +The anchor and the deadline are both recorded from a wall clock, so a single bad clock reading can +record a deadline no honest push could have produced. A recorded deadline that exceeds the current +instant by more than `MAX_RESERVATION_HOLD_MS` MUST be treated as CONTRADICTING the clock, because a +deadline recorded honestly never exceeds its own anchor by more than that bound and an anchor never +follows the present moment. Such a reservation MUST be re-anchored to the current instant and granted +a fresh reservation lifetime — as though pushed now — before any expiry is evaluated, and the same +treatment MUST be applied to a client build-window hold, which MUST be re-granted its own maximum +lifetime from the current instant. A node MUST therefore never hold a reservation beyond +`MAX_RESERVATION_HOLD_MS` measured from an instant the node has actually observed. Without this, a +deadline recorded far in the future never arrives, no re-push can move it inwards, and the coin is +withheld from selection permanently with no recovery available inside the product — the lockout this +section names as the worse failure, in its unrecoverable form. + This bound is on CONTINUOUS hold. Once the deadline passes, the reservation and its coin claims are released and the inputs become selectable again; a subsequent push of the same transaction is a new reservation with a new anchor and MAY hold the inputs for a further full period. A node MUST NOT diff --git a/crates/dig-wallet/src/sage/db.rs b/crates/dig-wallet/src/sage/db.rs index 1847110f..faa8a7c3 100644 --- a/crates/dig-wallet/src/sage/db.rs +++ b/crates/dig-wallet/src/sage/db.rs @@ -2863,6 +2863,98 @@ impl WalletDb { /// writer fixes all three AND leaves the key usable, so keeping the `LOWER()` beside it would /// buy nothing but the scan. It is deliberately NOT retained as belt-and-braces. pub async fn prune_reservations(&self, now_ms: i64) -> sqlx::Result { + // One transaction, and the two REPAIRS come first on purpose. Repairing is a WRITE, so it + // takes SQLite's write lock before anything is read - the ordering `reserve_client_coins` + // documents as what actually prevents a `SQLITE_BUSY` that a caller would see mis-mapped + // to `Unavailable`. A deferred transaction that read first would reintroduce exactly that. + let mut tx = self.pool.begin().await?; + + // A row whose recorded deadline CONTRADICTS the clock is re-anchored as if pushed now + // (dig-node#525). + // + // The predicate is a self-contradiction check on the row's own two columns, not a + // heuristic. INSERT writes `expires_at = now + RESERVATION_TTL_MS`; the `DO UPDATE` clamp + // writes at most `submitted_at + MAX_RESERVATION_HOLD_MS`; so every healthy row satisfies + // `expires_at <= submitted_at + MAX_RESERVATION_HOLD_MS`, and with `submitted_at <= now` + // it satisfies `expires_at <= now + MAX_RESERVATION_HOLD_MS`. Hence + // `expires_at > now + MAX_RESERVATION_HOLD_MS` implies `submitted_at > now`: the row + // claims to have been submitted after the present moment, which no honest push produces. + // + // `reserve_pushed_bundle` reads the clock ONCE and writes both columns from that reading, + // so a single reading far in the future stores a deadline decades out. Nothing else can + // retire it: the DELETE below fires on `expires_at <= now`, which never arrives, and the + // upsert clamp's outer `MAX` means a later push under a corrected clock leaves the stored + // deadline alone. The coin stays out of selection FOR EVER and `reset_chain_cache` refuses + // while the row exists - a freeze with no in-product recovery. Repairing at OBSERVATION + // rather than at write time is what makes it recoverable: nothing re-pushes an accepted + // bundle unprompted, so a write-time-only fix would leave that headline case permanent. + // + // # The grant, and why `submitted_at` moves too + // + // The row is rewritten to precisely what an honest push at `now` would have written: one + // `RESERVATION_TTL_MS`, not one `MAX_RESERVATION_HOLD_MS`, which would hand a repaired row + // six times a healthy row's first hold. + // + // Re-anchoring `submitted_at` is NOT optional. Left at the impossible future value, + // `stored.submitted_at + MAX_RESERVATION_HOLD_MS` stays decades out, #502's cap clause + // stops binding on that row for ever, and a retrying caller renews the hold indefinitely - + // resurrecting the lockout the cap exists to close. + // + // This does not regress #502's stable-anchor rule. That rule keeps `submitted_at` out of + // `DO UPDATE SET` so a CALLER cannot move the anchor. This write is not a caller; it is + // the row's owner replacing a value the system has proven impossible - a submission + // timestamp in the future - with one from the currently trusted clock. + // + // # What this costs, stated rather than argued away + // + // Let `E = expires_at - submitted_at`, so `E = min(age_at_last_push + TTL, CAP)`. The + // predicate fires iff `now < submitted_at + E - CAP`, so a BACKWARDS clock step of `d` on + // a row of age `e` fires iff `d > max(CAP - TTL, e)`. The minimum step that can trip the + // repair on a legitimately created row is therefore `CAP - TTL` = 50 minutes, and more for + // an older row; a small backwards step trips nothing. + // + // Past that floor the cost is real: a LIVE bundle's row is re-anchored and gets only one + // further TTL, where the unrepaired code would have held it until the clock caught up. So + // a backwards step exceeding 50 minutes can return a still-live bundle's inputs one TTL + // later instead of on clock recovery - a genuine move toward the #348/#497 double-spend + // direction, bounded by a gross clock anomaly. It is accepted because the alternative it + // replaces is a permanent, in-product-unrecoverable freeze, which `RESERVATION_TTL_MS`' + // own doc and `SPEC.md` 18.9a both name as the worse failure. + sqlx::query( + "UPDATE pending_transactions + SET submitted_at = ?, expires_at = ? + WHERE expires_at > ?", + ) + .bind(now_ms) + .bind(now_ms.saturating_add(super::rpc::RESERVATION_TTL_MS)) + .bind(now_ms.saturating_add(super::rpc::MAX_RESERVATION_HOLD_MS)) + .execute(&mut *tx) + .await?; + + // The same repair for a CLIENT build-window hold, with two deliberate asymmetries. + // + // The detector uses `MAX_RESERVATION_HOLD_MS`, not this table's own 10-minute ceiling, and + // the reason is money: keyed on its own ceiling, a roughly five-minute backwards clock step + // would re-clamp a HEALTHY client hold and reopen the cross-process selection window + // dig_ecosystem#3127 exists to close. One hour is the longest hold of any kind this wallet + // grants, so a deadline more than an hour past the observing clock is impossible for either + // table, and the shared threshold buys both the same ~50-minute false-fire floor. + // + // The GRANT is this table's ceiling rather than `RESERVATION_TTL_MS`, because there is no + // `submitted_at` column here and the caller's originally requested TTL is unrecoverable. + // Granting the ceiling errs LONG, which is the safe direction for a build-window hold: an + // over-long hold delays a spend, an under-long one lets a second selection take the coins + // a client is still building against. + sqlx::query( + "UPDATE client_coin_reservations + SET expires_at_ms = ? + WHERE expires_at_ms > ?", + ) + .bind(now_ms.saturating_add(CLIENT_RESERVATION_MAX_TTL_MS)) + .bind(now_ms.saturating_add(super::rpc::MAX_RESERVATION_HOLD_MS)) + .execute(&mut *tx) + .await?; + let n = sqlx::query( "DELETE FROM pending_transactions WHERE expires_at <= ? OR transaction_id IN ( @@ -2872,7 +2964,7 @@ impl WalletDb { )", ) .bind(now_ms) - .execute(&self.pool) + .execute(&mut *tx) .await? .rows_affected(); @@ -2882,6 +2974,7 @@ impl WalletDb { // // Not added to `n`: the returned count means "bundles retired", and a client hold is not a // bundle. Folding them together would inflate a figure callers read as in-flight spends. + // A REPAIRED row is not retired either, and for the same reason it is not counted. sqlx::query( "DELETE FROM client_coin_reservations WHERE expires_at_ms <= ? OR coin_id IN ( @@ -2890,9 +2983,11 @@ impl WalletDb { )", ) .bind(now_ms) - .execute(&self.pool) + .execute(&mut *tx) .await?; + tx.commit().await?; + Ok(n) } } @@ -4342,6 +4437,16 @@ pub struct NetworkSettingsRow { /// caller does not get to ask for a hold this node would not itself clean up. pub const CLIENT_RESERVATION_MAX_TTL_MS: i64 = 600_000; +/// The clock-contradiction detector in [`WalletDb::prune_reservations`] uses +/// `MAX_RESERVATION_HOLD_MS` as the impossible-deadline threshold for BOTH reservation tables +/// (dig-node#525). That is only sound while this ceiling sits at or below it: a client hold longer +/// than the longest bundle hold would make the detector fire on healthy client rows. +const _: () = assert!( + CLIENT_RESERVATION_MAX_TTL_MS <= super::rpc::MAX_RESERVATION_HOLD_MS, + "the clock-contradiction detector uses MAX_RESERVATION_HOLD_MS for both tables; a client \ + ceiling above it would make the detector fire on healthy client holds" +); + /// The lifetime applied when a caller names none, in milliseconds. /// /// Five minutes: long enough to build, sign and push a bundle across a process boundary, short @@ -6873,12 +6978,18 @@ mod tests { .await .unwrap(); - // The clock steps backwards, so this re-push carries an EARLIER deadline than the live one. + // The clock steps backwards, so this re-push carries an EARLIER deadline than the live + // one. Both columns move together, because `reserve_pushed_bundle` reads the clock ONCE + // and derives both from that reading - a push carrying the ORIGINAL `submitted_at` beside + // a moved `expires_at` is a state production cannot reach (dig-node#525). The step is 60 s, + // far under the 50-minute floor at which the clock-contradiction repair can fire, so the + // row must be left to the clamp alone. + let step_back = 60_000; db.reserve_spend(&reservation( "tx1", &["c1"], - first, - first + RESERVATION_TTL_MS, + first - step_back, + first - step_back + RESERVATION_TTL_MS, )) .await .unwrap(); @@ -6891,6 +7002,260 @@ mod tests { ); } + + /// **The defect (dig-node#525).** `WalletBackend::reserve_pushed_bundle` reads the clock ONCE + /// and writes both `submitted_at` and `expires_at` from that reading. A single reading far in + /// the future therefore stores a deadline decades out, and nothing in the system can 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 is held out of selection FOR EVER, and `reset_chain_cache` + /// refuses while the row exists. + /// + /// Asserted on [`WalletDb::unreserved_unspent_coins`], the surface where the money is actually + /// frozen: a row count in `pending_transactions` sits one layer below the decision and would + /// pass under a broken cascade. + #[tokio::test] + async fn a_reservation_anchored_to_a_far_future_clock_stops_freezing_its_coin_for_ever() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.upsert_coin(&coin("c1", 100, Some(10), None)) + .await + .unwrap(); + + // The poisoned first push: one clock reading, decades ahead. + let far_forward = 10_000_000_000; + let corrected = 1_000; + db.reserve_spend(&reservation( + "tx1", + &["c1"], + far_forward, + far_forward + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + + // Every selection prunes first, so this is the observation that repairs the row. + db.prune_reservations(corrected).await.unwrap(); + assert!( + db.unreserved_unspent_coins(None).await.unwrap().is_empty(), + "a repaired hold is legitimately live for one fresh TTL, not released on sight" + ); + + db.prune_reservations(corrected + RESERVATION_TTL_MS) + .await + .unwrap(); + let free: Vec = db + .unreserved_unspent_coins(None) + .await + .unwrap() + .into_iter() + .map(|c| c.coin_id) + .collect(); + assert_eq!( + free, + vec!["c1".to_string()], + "one TTL after the repair the coin returns to selection; before this fix it never did" + ); + } + + /// The repair grants exactly what an honest push at the observing instant would have written: + /// one [`RESERVATION_TTL_MS`], not one [`MAX_RESERVATION_HOLD_MS`]. Granting the cap would hand + /// a repaired row six times a healthy row's first hold. + #[tokio::test] + async fn the_clock_contradiction_repair_grants_one_fresh_ttl_not_the_maximum_hold() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.upsert_coin(&coin("c1", 100, Some(10), None)) + .await + .unwrap(); + + let far_forward = 10_000_000_000; + let corrected = 1_000; + db.reserve_spend(&reservation( + "tx1", + &["c1"], + far_forward, + far_forward + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + + db.prune_reservations(corrected).await.unwrap(); + + let row = &db.pending_transactions().await.unwrap()[0]; + assert_eq!( + row.expires_at, + corrected + RESERVATION_TTL_MS, + "the repair re-anchors to the observing clock and grants exactly one fresh TTL" + ); + } + + /// **Why `submitted_at` MUST be re-anchored too.** Leaving it at the impossible future value + /// leaves `stored.submitted_at + MAX_RESERVATION_HOLD_MS` decades out, so #502's cap clause + /// stops binding on that row for ever and a retrying caller renews the hold indefinitely, + /// resurrecting the very lockout the cap closes. + /// + /// This is NOT a regression of #502's stable-anchor rule. That rule keeps `submitted_at` out + /// of `DO UPDATE SET` so a CALLER cannot move the anchor. This write is not a caller; it is + /// the row's owner replacing a value the system has proven impossible, a submission timestamp + /// in the future, with one from the currently trusted clock. + #[tokio::test] + async fn the_clock_contradiction_repair_re_anchors_submitted_at_so_the_cap_still_binds() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.upsert_coin(&coin("c1", 100, Some(10), None)) + .await + .unwrap(); + + let far_forward = 10_000_000_000; + let corrected = 1_000; + db.reserve_spend(&reservation( + "tx1", + &["c1"], + far_forward, + far_forward + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + + db.prune_reservations(corrected).await.unwrap(); + + // A re-push late enough that the total-hold cap binds, but only if the anchor moved. + let late = corrected + 5 * RESERVATION_TTL_MS + RESERVATION_TTL_MS / 2; + db.reserve_spend(&reservation( + "tx1", + &["c1"], + far_forward, + late + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + + let row = &db.pending_transactions().await.unwrap()[0]; + assert_eq!( + row.expires_at, + corrected + MAX_RESERVATION_HOLD_MS, + "the cap binds from the REPAIRED anchor; left at the future value it would not bind" + ); + } + + /// The same repair for a client build-window hold. There is no `submitted_at` column there and + /// the caller's originally requested TTL is unrecoverable, so the grant is the ceiling a caller + /// could legitimately have asked for. + #[tokio::test] + async fn a_client_hold_written_under_a_far_future_clock_is_repaired_and_then_lapses() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.upsert_coin(&coin("c1", 100, Some(10), None)) + .await + .unwrap(); + + let far_forward = 10_000_000_000; + let corrected = 1_000; + db.reserve_client_coins( + &["c1".to_string()], + Some(CLIENT_RESERVATION_MAX_TTL_MS), + far_forward, + ) + .await + .unwrap(); + + db.prune_reservations(corrected).await.unwrap(); + assert!( + db.unreserved_unspent_coins(None).await.unwrap().is_empty(), + "a repaired client hold is live for one fresh ceiling, not released on sight" + ); + + db.prune_reservations(corrected + CLIENT_RESERVATION_MAX_TTL_MS) + .await + .unwrap(); + assert_eq!( + db.unreserved_unspent_coins(None).await.unwrap().len(), + 1, + "the repaired client hold lapses; before this fix it held its coin for ever" + ); + } + + /// No false positives on an ordinary backwards clock step. The detector fires only when a row + /// CONTRADICTS itself against the clock, and the arithmetic puts the floor for a legitimately + /// created row at `MAX_RESERVATION_HOLD_MS - RESERVATION_TTL_MS` (50 minutes), never at 1 ms. + #[tokio::test] + async fn a_one_millisecond_backwards_clock_step_repairs_nothing() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.upsert_coin(&coin("c1", 100, Some(10), None)) + .await + .unwrap(); + + let submitted = 1_000_000; + db.reserve_spend(&reservation( + "tx1", + &["c1"], + submitted, + submitted + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + + db.prune_reservations(submitted - 1).await.unwrap(); + + let row = &db.pending_transactions().await.unwrap()[0]; + assert_eq!(row.submitted_at, submitted, "a healthy row keeps its anchor"); + assert_eq!( + row.expires_at, + submitted + RESERVATION_TTL_MS, + "a healthy row keeps its deadline through a 1 ms backwards step" + ); + } + + /// Both sides of the detector's exact boundary, `expires_at > now + MAX_RESERVATION_HOLD_MS`. + /// A row sitting exactly ON the bound is reachable by an honest push and must be left alone; a + /// row one millisecond past it is unreachable and is repaired. + #[tokio::test] + async fn the_clock_contradiction_detector_fires_strictly_past_the_maximum_hold() { + let db = WalletDb::open_in_memory().await.unwrap(); + for id in ["c1", "c2"] { + db.upsert_coin(&coin(id, 100, Some(10), None)) + .await + .unwrap(); + } + + let now = 1_000_000; + db.reserve_spend(&reservation( + "at-bound", + &["c1"], + now, + now + MAX_RESERVATION_HOLD_MS, + )) + .await + .unwrap(); + db.reserve_spend(&reservation( + "past-bound", + &["c2"], + now, + now + MAX_RESERVATION_HOLD_MS + 1, + )) + .await + .unwrap(); + + db.prune_reservations(now).await.unwrap(); + + let rows = db.pending_transactions().await.unwrap(); + let at_bound = rows + .iter() + .find(|r| r.transaction_id == "at-bound") + .expect("the at-bound row survives"); + let past_bound = rows + .iter() + .find(|r| r.transaction_id == "past-bound") + .expect("the past-bound row survives, repaired rather than deleted"); + + assert_eq!( + at_bound.expires_at, + now + MAX_RESERVATION_HOLD_MS, + "exactly at the bound is reachable by an honest push and must not be repaired" + ); + assert_eq!( + past_bound.expires_at, + now + RESERVATION_TTL_MS, + "one millisecond past the bound is unreachable and is re-anchored" + ); + } /// The asymmetry the ticket requires to SURVIVE the fix: a coin backs exactly one in-flight /// bundle and the FIRST claim wins. No path added for the cap may let a second bundle take a /// coin already reserved, or shorten the first bundle's hold on it. diff --git a/crates/dig-wallet/src/sage/rpc.rs b/crates/dig-wallet/src/sage/rpc.rs index d06fc80f..e330c5cd 100644 --- a/crates/dig-wallet/src/sage/rpc.rs +++ b/crates/dig-wallet/src/sage/rpc.rs @@ -1195,6 +1195,14 @@ impl WalletBackend { &self, now_ms: i64, ) -> sqlx::Result> { + // Prune FIRST, unlike the other reservation-sensitive entry points only by accident of + // history: every one of them already does this, and this one did not (dig-node#525). + // + // Without it a user whose first action after correcting a bad clock is a reset gets + // `ResetRefusal::SpendInFlight`, whose message tells them to wait for the reservations to + // expire - and waiting alone triggers no prune, so the deadline is never repaired and the + // wait never ends. Pruning here is what makes that sentence true. + self.db.prune_reservations(now_ms).await?; self.db.reset_chain_cache(now_ms).await } From 2edd6241ca170aa2382d75f5a03e6610646cd502 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Thu, 3 Sep 2026 01:24:31 -0700 Subject: [PATCH 11/15] style(wallet): rustfmt the two files touched by #525 Co-Authored-By: Claude --- crates/dig-wallet/src/sage/db.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/dig-wallet/src/sage/db.rs b/crates/dig-wallet/src/sage/db.rs index faa8a7c3..7a793811 100644 --- a/crates/dig-wallet/src/sage/db.rs +++ b/crates/dig-wallet/src/sage/db.rs @@ -7002,7 +7002,6 @@ mod tests { ); } - /// **The defect (dig-node#525).** `WalletBackend::reserve_pushed_bundle` reads the clock ONCE /// and writes both `submitted_at` and `expires_at` from that reading. A single reading far in /// the future therefore stores a deadline decades out, and nothing in the system can retire @@ -7195,7 +7194,10 @@ mod tests { db.prune_reservations(submitted - 1).await.unwrap(); let row = &db.pending_transactions().await.unwrap()[0]; - assert_eq!(row.submitted_at, submitted, "a healthy row keeps its anchor"); + assert_eq!( + row.submitted_at, submitted, + "a healthy row keeps its anchor" + ); assert_eq!( row.expires_at, submitted + RESERVATION_TTL_MS, From ccea3a26e5a46c2c2ff8a3bb258aac143a808811 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Thu, 3 Sep 2026 07:06:20 -0700 Subject: [PATCH 12/15] 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 --- SPEC.md | 26 ++++-- crates/dig-wallet/src/sage/db.rs | 138 +++++++++++++++++++++++++++++-- 2 files changed, 154 insertions(+), 10 deletions(-) diff --git a/SPEC.md b/SPEC.md index 26d42652..79d2f401 100644 --- a/SPEC.md +++ b/SPEC.md @@ -5800,11 +5800,27 @@ deadline recorded honestly never exceeds its own anchor by more than that bound follows the present moment. Such a reservation MUST be re-anchored to the current instant and granted a fresh reservation lifetime — as though pushed now — before any expiry is evaluated, and the same treatment MUST be applied to a client build-window hold, which MUST be re-granted its own maximum -lifetime from the current instant. A node MUST therefore never hold a reservation beyond -`MAX_RESERVATION_HOLD_MS` measured from an instant the node has actually observed. Without this, a -deadline recorded far in the future never arrives, no re-push can move it inwards, and the coin is -withheld from selection permanently with no recovery available inside the product — the lockout this -section names as the worse failure, in its unrecoverable form. +lifetime from the current instant. Without this, a deadline recorded far in the future never +arrives, no re-push can move it inwards, and the coin is withheld from selection permanently with no +recovery available inside the product -- the lockout this section names as the worse failure, in its +unrecoverable form. + +**The residual bound, stated exactly.** This repair does NOT reduce the worst case to +`MAX_RESERVATION_HOLD_MS`, and a node MUST NOT claim that it does. A forward clock glitch smaller +than `MAX_RESERVATION_HOLD_MS - RESERVATION_TTL_MS` is invisible to the test above by construction: +the first push records `expires_at = submitted_at + RESERVATION_TTL_MS`, so while the anchor leads +true time by at most that difference the recorded deadline never exceeds the observing instant by +more than the cap, and every later re-push carries a deadline drawn from the true clock, which +cannot exceed it either. The anchor therefore survives, and the total-hold cap pins the deadline at +`submitted_at + MAX_RESERVATION_HOLD_MS`. **A node MUST NOT hold a reservation beyond +`2 * MAX_RESERVATION_HOLD_MS - RESERVATION_TTL_MS` (110 minutes) measured from an instant the node +has actually observed**, and that bound is TIGHT: it is attained exactly when the forward glitch +equals `MAX_RESERVATION_HOLD_MS - RESERVATION_TTL_MS` and the caller keeps re-pushing. + +That same difference is the bound seen from its other side: it is also the smallest BACKWARDS clock +step that can make the test fire on a reservation recorded honestly. The evasion window for a +forward glitch and the false-fire floor for a backwards step are one constant, and neither can be +narrowed without widening the other. This bound is on CONTINUOUS hold. Once the deadline passes, the reservation and its coin claims are released and the inputs become selectable again; a subsequent push of the same transaction is a new diff --git a/crates/dig-wallet/src/sage/db.rs b/crates/dig-wallet/src/sage/db.rs index 7a793811..2098ad38 100644 --- a/crates/dig-wallet/src/sage/db.rs +++ b/crates/dig-wallet/src/sage/db.rs @@ -2920,6 +2920,28 @@ impl WalletDb { // direction, bounded by a gross clock anomaly. It is accepted because the alternative it // replaces is a permanent, in-product-unrecoverable freeze, which `RESERVATION_TTL_MS`' // own doc and `SPEC.md` 18.9a both name as the worse failure. + // + // # What this does NOT bound, and the exact residue + // + // This repair does not reduce the worst-case hold to `CAP`. A FORWARD glitch of at most + // `CAP - TTL` evades the predicate by construction: the first push records + // `expires_at = submitted_at + TTL`, so while the anchor leads true time by no more than + // that difference the deadline never exceeds the observing instant by more than `CAP`, and + // every later re-push draws its deadline from the true clock, which cannot exceed it + // either. The anchor survives, and #502's cap pins the deadline at `submitted_at + CAP`. + // + // The worst case is therefore `2 * CAP - TTL` = 110 minutes from an OBSERVED instant, and + // it is tight - attained when the glitch is exactly `CAP - TTL` and the caller keeps + // re-pushing. Pinned by + // `a_forward_clock_glitch_under_the_detector_threshold_is_bounded_at_two_caps_less_a_ttl`. + // + // Note the symmetry: `CAP - TTL` is BOTH the largest forward glitch that evades this + // predicate AND the smallest backwards step that can make it fire on an honest row. One + // constant seen from two sides; neither margin can be narrowed without widening the other. + // + // This 110-minute ceiling is #502's cap behaviour under a forward glitch and predates this + // repair. It is recorded rather than fixed: closing it needs a monotonic anchor for elapsed + // time, not a tighter predicate. sqlx::query( "UPDATE pending_transactions SET submitted_at = ?, expires_at = ? @@ -4447,6 +4469,21 @@ const _: () = assert!( ceiling above it would make the detector fire on healthy client holds" ); +/// The repair in [`WalletDb::prune_reservations`] grants one `RESERVATION_TTL_MS` from the +/// observing instant, and the detector fires above `MAX_RESERVATION_HOLD_MS` from that same +/// instant. A TTL LONGER than the cap would make the row the repair just wrote satisfy the +/// detector again on the very next prune, re-anchoring it for ever - an unbounded freeze produced +/// by the repair itself, which is precisely the failure the repair exists to close (dig-node#525). +/// +/// Unreachable today, since the cap is defined as `6 * RESERVATION_TTL_MS`. Pinned anyway: if the +/// constant pair whose drift would make the detector over-fire earns a compile-time guard, so does +/// the pair whose drift would turn the repair into a loop. +const _: () = assert!( + super::rpc::RESERVATION_TTL_MS <= super::rpc::MAX_RESERVATION_HOLD_MS, + "the repair grants one RESERVATION_TTL_MS and the detector fires past MAX_RESERVATION_HOLD_MS; \ + a TTL above the cap would make every repaired row re-trigger the detector for ever" +); + /// The lifetime applied when a caller names none, in milliseconds. /// /// Five minutes: long enough to build, sign and push a bundle across a process boundary, short @@ -7205,11 +7242,21 @@ mod tests { ); } - /// Both sides of the detector's exact boundary, `expires_at > now + MAX_RESERVATION_HOLD_MS`. - /// A row sitting exactly ON the bound is reachable by an honest push and must be left alone; a - /// row one millisecond past it is unreachable and is repaired. + /// **A SYNTHETIC test of the SQL predicate's `>` versus `>=`, not a claim about production.** + /// + /// It writes both rows through `reserve_spend` directly, and the `past-bound` row - + /// `submitted_at = now, expires_at = now + MAX_RESERVATION_HOLD_MS + 1` - is a state NO writer + /// can produce: `INSERT` writes `submitted_at + RESERVATION_TTL_MS` and the clamp never exceeds + /// `submitted_at + MAX_RESERVATION_HOLD_MS`. It exists solely so the boundary is pinned from + /// both sides, and it must not be read as evidence about reachable behaviour. + /// + /// The reachable consequence of that same boundary - what a forward clock glitch small enough + /// to EVADE this predicate actually costs - is pinned separately and honestly by + /// `a_forward_clock_glitch_under_the_detector_threshold_is_bounded_at_two_caps_less_a_ttl`. An + /// earlier version of this file had only the synthetic test, and its unreachable fixture is + /// what hid the residue. #[tokio::test] - async fn the_clock_contradiction_detector_fires_strictly_past_the_maximum_hold() { + async fn the_clock_contradiction_predicate_is_strict_at_the_maximum_hold() { let db = WalletDb::open_in_memory().await.unwrap(); for id in ["c1", "c2"] { db.upsert_coin(&coin(id, 100, Some(10), None)) @@ -7255,7 +7302,88 @@ mod tests { assert_eq!( past_bound.expires_at, now + RESERVATION_TTL_MS, - "one millisecond past the bound is unreachable and is re-anchored" + "one millisecond past the bound is repaired (synthetic row; see the doc comment)" + ); + } + + /// **The residue this fix does NOT close, built only from REACHABLE states.** + /// + /// A forward clock glitch of at most `MAX_RESERVATION_HOLD_MS - RESERVATION_TTL_MS` evades the + /// clock-contradiction detector by construction: the first push records + /// `expires_at = submitted_at + RESERVATION_TTL_MS`, so the recorded deadline never exceeds the + /// observing instant by more than the cap, and every later re-push draws its deadline from the + /// true clock, which cannot exceed it either. The glitched anchor therefore SURVIVES, and + /// #502's cap pins the deadline at `submitted_at + MAX_RESERVATION_HOLD_MS`. + /// + /// The coin is consequently held for `2 * MAX_RESERVATION_HOLD_MS - RESERVATION_TTL_MS` (110 + /// minutes) measured from an instant the node actually observed, and that bound is TIGHT. This + /// is #502's cap behaviour under a forward glitch, predating this repair; it is pinned here + /// rather than fixed, because closing it needs a monotonic anchor rather than a tighter + /// predicate. + /// + /// Every write below goes through the real API at a value a real clock could have produced - + /// no hand-placed row - which is what makes this a measurement rather than a restatement. + #[tokio::test] + async fn a_forward_clock_glitch_under_the_detector_threshold_is_bounded_at_two_caps_less_a_ttl() + { + let db = WalletDb::open_in_memory().await.unwrap(); + db.upsert_coin(&coin("c1", 100, Some(10), None)) + .await + .unwrap(); + + // The largest forward glitch that still evades the detector. + let glitch = MAX_RESERVATION_HOLD_MS - RESERVATION_TTL_MS; + db.reserve_spend(&reservation( + "tx1", + &["c1"], + glitch, + glitch + RESERVATION_TTL_MS, + )) + .await + .unwrap(); + + // True time is zero. The row is NOT repaired: it does not contradict the clock. + db.prune_reservations(0).await.unwrap(); + let row = &db.pending_transactions().await.unwrap()[0]; + assert_eq!( + (row.submitted_at, row.expires_at), + (glitch, glitch + RESERVATION_TTL_MS), + "a glitch at the evasion threshold leaves the row untouched - this is the residue" + ); + + // A caller retrying under the TRUE clock, always re-pushing before the deadline lapses. + let mut t = glitch + RESERVATION_TTL_MS / 2; + while t < 2 * MAX_RESERVATION_HOLD_MS - RESERVATION_TTL_MS { + db.prune_reservations(t).await.unwrap(); + db.reserve_spend(&reservation("tx1", &["c1"], t, t + RESERVATION_TTL_MS)) + .await + .unwrap(); + t += RESERVATION_TTL_MS / 2; + } + + let row = &db.pending_transactions().await.unwrap()[0]; + assert_eq!( + row.submitted_at, glitch, + "the glitched anchor survives every re-push; nothing ever repaired it" + ); + + let release = glitch + MAX_RESERVATION_HOLD_MS; + assert_eq!( + row.expires_at, release, + "the cap pins the deadline at the glitched anchor plus one cap" + ); + + db.prune_reservations(release - 1).await.unwrap(); + assert!( + db.unreserved_unspent_coins(None).await.unwrap().is_empty(), + "one millisecond before the bound the coin is still frozen" + ); + + db.prune_reservations(release).await.unwrap(); + assert_eq!( + db.unreserved_unspent_coins(None).await.unwrap().len(), + 1, + "the coin returns at exactly 2 * CAP - TTL from an observed instant, and not before" ); } /// The asymmetry the ticket requires to SURVIVE the fix: a coin backs exactly one in-flight From 4b64a896d50ec7f9bd92aa9886d0339c7592c249 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Thu, 3 Sep 2026 07:47:26 -0700 Subject: [PATCH 13/15] chore(release): bump to 0.252.96 to clear sibling lanes Co-Authored-By: Claude --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 454f5972..56df492c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3031,7 +3031,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.252.93" +version = "0.252.96" dependencies = [ "async-trait", "axum", diff --git a/Cargo.toml b/Cargo.toml index d959571d..733a39aa 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ edition = "2021" # the ROOT manifest (`[workspace.package].version`), so it MUST be set here for a # release to fire (§3.6). The library crates (dig-node-core/dig-runtime/dig-wallet) # keep their own independent versions — only the released binary tracks the workspace version. -version = "0.252.93" +version = "0.252.96" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over From b9a284a45412832e6f76d50152eda3ba13cfcb38 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Thu, 3 Sep 2026 08:59:51 -0700 Subject: [PATCH 14/15] chore(release): bump to 0.253.7 to avoid collision with sibling release PRs Co-Authored-By: Claude --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1d8e975e..5641929b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3031,7 +3031,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.253.4" +version = "0.253.7" dependencies = [ "async-trait", "axum", diff --git a/Cargo.toml b/Cargo.toml index aed80fb6..616b7834 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ edition = "2021" # the ROOT manifest (`[workspace.package].version`), so it MUST be set here for a # release to fire (§3.6). The library crates (dig-node-core/dig-runtime/dig-wallet) # keep their own independent versions — only the released binary tracks the workspace version. -version = "0.253.4" +version = "0.253.7" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over From 4dc5d32dc23092716a0b889456d4ccd3d7f0538f Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Thu, 3 Sep 2026 10:26:49 -0700 Subject: [PATCH 15/15] chore(release): bump to 0.254.3, resolve collision with #533 (0.254.1) --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8882f773..4c181f9e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3031,7 +3031,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.254.1" +version = "0.254.3" dependencies = [ "async-trait", "axum", diff --git a/Cargo.toml b/Cargo.toml index bff7d546..1cec1a0e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ edition = "2021" # the ROOT manifest (`[workspace.package].version`), so it MUST be set here for a # release to fire (§3.6). The library crates (dig-node-core/dig-runtime/dig-wallet) # keep their own independent versions — only the released binary tracks the workspace version. -version = "0.254.1" +version = "0.254.3" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over