diff --git a/Cargo.lock b/Cargo.lock index 0fa4b3b2..1c4f2ae5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3031,7 +3031,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.240.0" +version = "0.245.0" dependencies = [ "async-trait", "axum", diff --git a/Cargo.toml b/Cargo.toml index 129c10f8..f13046c9 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.240.0" +version = "0.245.0" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over diff --git a/SPEC.md b/SPEC.md index b28111a2..c901abac 100644 --- a/SPEC.md +++ b/SPEC.md @@ -5513,15 +5513,52 @@ runs from stranding a coin permanently. Failing to record a reservation MUST NOT mempool already accepted. A push MUST reserve its inputs unless the network DEFINITIVELY refused the bundle. A refusal is -definitive only when the mempool stated its reason (`accepted:false` WITH a `rejection`); a bare -denial carrying no reason, and any transport failure, MUST be treated as POSSIBLY IN FLIGHT and hold -the inputs to the TTL. The node cannot distinguish "never relayed" from "relayed, and the -acknowledgement was lost", and under §13 every dialled peer is untrusted, so a source that denies a -relay it performed MUST NOT thereby return the coins to selection — a second send inside the -confirmation window could otherwise 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. Requiring a STATED reason is what keeps a genuine mempool rejection from holding a user's -coins for the full TTL. +definitive only when the mempool stated its reason (`accepted:false` WITH a `rejection`) AND that +reason is a property of the BUNDLE rather than of the answering node's own view; a bare denial +carrying no reason, a refusal whose stated reason is view-dependent, a refusal whose reason the node +does not recognise, and any transport failure MUST all be treated as POSSIBLY IN FLIGHT and hold the +inputs to the TTL. + +A single push is not a single transmission: the chain client relays to UP TO THREE destinations in +turn and only the LAST answer is observed, and the earlier attempts fail in ways that do not +distinguish "never transmitted" from "transmitted, admitted, and the acknowledgement was lost". A +refusal MUST therefore NOT be read as the network's verdict merely because a destination stated one. +A reason that reports the answering node's OWN mempool or chain view — a conflict with a bundle it +already holds, a coin it has not yet seen, a relay-fee policy, a timelock evaluated against its own +peak — MUST NOT free the inputs, because the destination that answered may be refusing precisely +BECAUSE an earlier destination admitted the bundle. + +The node cannot distinguish "never relayed" from "relayed, and the acknowledgement was lost", and +under §13 every dialled peer is untrusted, so a source that denies a relay it performed MUST NOT +thereby return the coins to selection — a second send inside the confirmation window could otherwise +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 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 +nobody foresaw. A node MUST match an allowlisted reason EXACTLY — never as a substring, prefix or +suffix — after trimming, and case-insensitively. + +The allowlist is exactly these eleven Chia error names, and an independent implementation MUST use +this set: `BAD_AGGREGATE_SIGNATURE`, `COIN_AMOUNT_NEGATIVE`, `COIN_AMOUNT_EXCEEDS_MAXIMUM`, +`DUPLICATE_OUTPUT`, `MINTING_COIN`, `RESERVE_FEE_CONDITION_FAILED`, `WRONG_PUZZLE_HASH`, +`ASSERT_MY_COIN_ID_FAILED`, `ASSERT_MY_PARENT_ID_FAILED`, `ASSERT_MY_PUZZLEHASH_FAILED`, +`ASSERT_MY_AMOUNT_FAILED`. + +A name MUST NOT be admitted to that set unless every node refuses it identically REGARDLESS of the +node's peak height, activated consensus flags, cost budget and mempool contents. The CLVM-execution +names — `GENERATOR_RUNTIME_ERROR`, `BLOCK_COST_EXCEEDS_MAX`, `INVALID_BLOCK_COST` and +`INVALID_SPEND_BUNDLE` — MUST NOT be admitted, even though they appear to be properties of the bytes: +bundle validation runs under flags derived from the answering node's height and under a +caller-supplied cost budget, so two honest nodes can disagree on identical bytes. `ASSERT_MY_BIRTH_*` +is view-dependent and MUST NOT be admitted either. + +Requiring a STATED and BUNDLE-INTRINSIC reason is what keeps a mempool rejection the whole network +agrees on — a bad signature, say — from holding a user's coins for the full TTL. It does NOT keep a +view-dependent refusal from doing so, and MUST NOT be described as though it did: a bundle every node +will refuse for a reason outside the allowlist is held for the TTL, and that is the intended and safe +outcome. 18.8. **Method surface — reads (served).** `login`, `logout`, `get_version`, `get_sync_status`, `check_address`, `get_derivations`, `get_are_coins_spendable`, diff --git a/crates/dig-wallet/src/sage/chain.rs b/crates/dig-wallet/src/sage/chain.rs index f81c2554..96ef1422 100644 --- a/crates/dig-wallet/src/sage/chain.rs +++ b/crates/dig-wallet/src/sage/chain.rs @@ -158,6 +158,10 @@ pub struct PushOutcome { /// /// `None` for a bare verdict, and that absence is load-bearing: dig-node#348's reservation /// hold keys on it, because a refusal the mempool did not explain may still be in flight. + /// + /// Presence alone does not free the inputs. Since dig-node#460 the hold also asks what CLASS + /// of reason this is — [`refusal_is_bundle_intrinsic`] — because a push reaches up to three + /// destinations and a peer-local refusal from the last one says nothing about the first. pub rejection: Option, /// The source's label for the answer (`SUCCESS`, `PENDING`, `FAILED`, `UNKNOWN`) — always /// present, whether or not a reason came with it. @@ -168,6 +172,119 @@ pub struct PushOutcome { pub verdict: String, } +/// Chia error names that are a property of the BUNDLE, not of the answering node's own view. +/// +/// A node refusing for one of these has looked at the bundle's own contents and found them +/// invalid: the signature does not verify, the outputs exceed the inputs, the puzzle reveal does +/// not hash to the coin. Every honest node reaches the same verdict from the same bytes, so no +/// other destination can be holding the bundle and its inputs are safe to return to selection. +/// +/// # This is an ALLOWLIST, and that is the whole design (dig-node#460) +/// +/// The reason text is supplied by an untrusted source (§13 / NC-12), so the guard cannot trust it +/// to make a POSITIVE safety claim. It does not have to. Freeing early is the dangerous direction +/// and holding is the safe one, so the default is HOLD and this list is the only exception to it. +/// Three consequences follow, and each is a property the code would lose as a denylist: +/// +/// - **An incomplete list is safe.** A Chia error name added after this was written, a source with +/// its own vocabulary, a peer inventing text — all land in the hold class and cost at most one +/// bounded `RESERVATION_TTL_MS`. The same names written as "free unless one of these" would free +/// on every string nobody foresaw. +/// - **The free set strictly SHRANK**, which removes the ACCIDENTAL free: a source that denies a +/// relay it performed, or answers with its own conflict, no longer frees. It does NOT raise the bar +/// against a DELIBERATE attacker in the answering position — these names are public constants, so +/// emitting one is a lookup rather than a feat. This guard fixes the honest-race defect; it is not +/// a defence against a hostile last destination, and must not be described as one. +/// - **The other direction is unchanged.** A source wanting the inputs HELD could already achieve +/// that by stating no reason at all, which dig-node#348 made a hold. This adds no new lockout +/// capability, and the TTL that bounds it MUST NOT be shortened to compensate. +/// +/// # What is deliberately absent +/// +/// Everything whose answer depends on WHO was asked. `DOUBLE_SPEND`, `MEMPOOL_CONFLICT` and +/// `ALREADY_INCLUDING_TRANSACTION` are a node's report of its OWN mempool, and on the multi- +/// destination push path they are what a peer says when it has already seen the bundle another +/// destination admitted — the exact refusal that must never free. `UNKNOWN_UNSPENT` is a node that +/// has not caught up. The fee names (`INVALID_FEE_LOW_FEE`, `INVALID_FEE_TOO_CLOSE_TO_ZERO`) are +/// per-node relay POLICY. The timelock assertions (`ASSERT_HEIGHT_*`, `ASSERT_SECONDS_*`, +/// `ASSERT_BEFORE_*`) are evaluated against the asked node's PEAK, so a node behind the tip refuses +/// what a node at the tip admits. +/// +/// `TOO_MANY_ANNOUNCEMENTS` is the subtle one and the reason this paragraph names it explicitly. It +/// reads as a pure property of the bundle — a bundle either carries too many announcements or it does +/// not — and it is NOT: `chia_consensus::conditions` decrements the per-spend announcement countdown +/// only `if (flags & COST_CONDITIONS) == 0`, and `COST_CONDITIONS` is derived from the answering +/// node's height. So a node below `hard_fork2_height` refuses an announcement-heavy bundle that a node +/// above it admits. It is absent, so it holds, and it is written down HERE because it is the entry a +/// future reader is most likely to add believing it intrinsic. +/// +/// **The CLVM-EXECUTION names are absent for the SAME reason, which is not obvious and was got +/// wrong once.** `GENERATOR_RUNTIME_ERROR`, `BLOCK_COST_EXCEEDS_MAX`, `INVALID_BLOCK_COST` and +/// `INVALID_SPEND_BUNDLE` look like pure properties of the bytes and are not. Bundle validation is +/// parameterised by the answering node's HEIGHT and by a caller-supplied cost budget: +/// `chia_consensus::spendbundle_validation::get_flags_for_height_and_constants` derives +/// `COST_CONDITIONS` / `ENABLE_KECCAK_OPS_OUTSIDE_GUARD` / `SIMPLE_GENERATOR` from `prev_tx_height`, +/// and `run_spendbundle(.., max_cost, flags, ..)` runs under both. So a node above a hard fork and a +/// node below it can reach DIFFERENT verdicts on identical bytes — the same property that excludes +/// the timelocks. Do not re-add them. +/// +/// The list is also kept SHORT on purpose: a name is added only when every node is certain to +/// refuse it identically. The announcement-consumption names are omitted for that reason, not +/// because they are believed view-dependent. Omission costs a bounded hold; a wrong inclusion costs +/// a double-select window. +/// +/// **The one acknowledged residue.** `BAD_AGGREGATE_SIGNATURE` is verified against messages built +/// with the node's own `AGG_SIG_ME_ADDITIONAL_DATA`, so it is a property of the bundle only for +/// nodes on the same network. The peer handshake's `network_id` check is what makes that hold in +/// practice; it is stated rather than left implicit, because it is the assumption this entry rests +/// on. +const BUNDLE_INTRINSIC_REFUSALS: &[&str] = &[ + "BAD_AGGREGATE_SIGNATURE", + "COIN_AMOUNT_NEGATIVE", + "COIN_AMOUNT_EXCEEDS_MAXIMUM", + "DUPLICATE_OUTPUT", + "MINTING_COIN", + "RESERVE_FEE_CONDITION_FAILED", + "WRONG_PUZZLE_HASH", + "ASSERT_MY_COIN_ID_FAILED", + "ASSERT_MY_PARENT_ID_FAILED", + "ASSERT_MY_PUZZLEHASH_FAILED", + "ASSERT_MY_AMOUNT_FAILED", +]; + +/// The bare reason out of a composed [`PushOutcome::rejection`]. +/// +/// `ChainTransport::stated_rejection` composes `"{verdict}: {reason}"` so an operator reading one +/// field sees both. The dig-node#460 classifier needs the reason ALONE, and re-deriving that split +/// at the call site would let the two drift apart in silence — a composition change would not fail +/// anything, it would just start mis-classifying every refusal in the safe-looking direction. +/// `the_stated_form_round_trips_back_to_the_bare_reason` pins the pair, so a change to one that +/// this no longer inverts is a failing test rather than a quiet regression. +/// +/// A string carrying no `": "` is returned whole: a source that stated a bare reason still stated a +/// reason. +fn refusal_reason(stated: &str) -> &str { + match stated.split_once(": ") { + Some((_verdict, reason)) => reason.trim(), + None => stated.trim(), + } +} + +/// Whether a stated refusal is a property of the BUNDLE rather than of one node's view +/// (dig-node#460). +/// +/// This is what [`super::rpc::WalletBackend::push_signed_bundle`] keys its reservation release on. +/// The match is EXACT against [`BUNDLE_INTRINSIC_REFUSALS`], case-insensitively and after trimming +/// — never a substring or prefix test. A source that embeds an allowlisted name in wider text +/// (`"MEMPOOL_CONFLICT (see BAD_AGGREGATE_SIGNATURE)"`) does not match, and lands in the hold +/// class, which is the direction an unparseable answer belongs in. +pub(crate) fn refusal_is_bundle_intrinsic(stated: &str) -> bool { + let reason = refusal_reason(stated); + BUNDLE_INTRINSIC_REFUSALS + .iter() + .any(|intrinsic| reason.eq_ignore_ascii_case(intrinsic)) +} + /// 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 @@ -510,6 +627,12 @@ impl ChainTransport { /// still have put it in flight, and reselecting those coins opens the double-select window §13 and /// SPEC §18.7 exist to close. /// + /// A stated reason is NECESSARY and not sufficient: dig-node#460 added the CLASS test + /// ([`refusal_is_bundle_intrinsic`]) on top, because the reason a SECOND push destination states + /// is typically its own mempool conflict with the bundle the FIRST one admitted. The text this + /// composes is what the operator reads; [`refusal_reason`] is what the classifier reads back + /// out of it. + /// /// An earlier version manufactured a reason here from `status.status`, so `rejection` was /// `Some(..)` on EVERY non-admitted answer, `is_definitive_rejection` was true every time, and the /// hold could never fire. The guard read as shipped and was vacuous. @@ -556,10 +679,11 @@ impl ChainTransport { // The node's OWN words, and ONLY its own words. `None` when it sent none. // // This is load-bearing for #348, not cosmetic. `is_definitive_rejection` - // (`rpc.rs`) frees the inputs only for a rejection the mempool STATED, and holds - // them otherwise — because a peer that relayed the bundle and then answered with a - // bare verdict may still have put it in flight, and reselecting those coins opens - // the double-select window. + // (`rpc.rs`) frees the inputs only for a rejection the mempool STATED — and, since + // #460, only when that reason is a property of the BUNDLE — and holds them + // otherwise, because a peer that relayed the bundle and then answered with a bare + // verdict may still have put it in flight, and reselecting those coins opens the + // double-select window. // // Manufacturing a reason here from `status.status` defeated exactly that: it made // `rejection` `Some(..)` on EVERY non-admitted answer, so the guard was true every @@ -783,6 +907,168 @@ mod tests { ); } + /// **Proves (dig-node#460):** the composed operator string round-trips back to the bare reason. + /// + /// `stated_rejection` composes `"{verdict}: {reason}"`; `refusal_reason` is its declared + /// inverse and the classifier reads only what it returns. Nothing else pins the two together, + /// so a change to the composition would otherwise leave the classifier silently reading the + /// wrong substring — and it would fail in the direction that LOOKS fine, because an + /// unrecognised reason simply holds. + /// + /// **Catches:** changing the separator, prefixing the verdict differently, or dropping the + /// verdict from the composed form without updating the split. + #[test] + fn the_stated_form_round_trips_back_to_the_bare_reason() { + for reason in [ + "BAD_AGGREGATE_SIGNATURE", + "ALREADY_INCLUDING_TRANSACTION", + "SOMETHING_NOBODY_ENUMERATED", + ] { + let composed = super::ChainTransport::stated_rejection(&status("FAILED", Some(reason))) + .expect("a stated reason must survive composition"); + assert_eq!( + super::refusal_reason(&composed), + reason, + "the classifier reads a different substring than the one the source stated" + ); + } + } + + /// **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 + /// `MEMPOOL_CONFLICT` are exactly what a second push destination answers once a FIRST + /// destination has admitted the bundle and gossiped it, which is the #460 path. Classifying + /// any of them as definitive frees the inputs of a bundle sitting in a public mempool. + /// + /// The unrecognised row is the one that proves the SHAPE rather than the contents. The + /// enumeration cannot be complete — Chia adds error names and a hostile source writes whatever + /// it likes — so the property that matters is that everything outside the list holds. Written + /// as a denylist the same names would read almost identically and fail the opposite way. + /// + /// The `TOO_MANY_ANNOUNCEMENTS` row and the four CLVM-execution rows are a REGRESSION PIN, not + /// filler. They were on the allowlist in + /// the first draft and the adversarial gate used one of them to construct a sequence in which + /// the inputs are freed for a bundle that later lands. They read as bundle properties and are + /// not, so the only thing preventing their return is an assertion that names them. + /// + /// **Catches:** inverting the default, matching by substring or prefix, or moving a + /// view-dependent name onto the allowlist -- including re-adding the four that were removed. + #[test] + fn only_a_bundle_intrinsic_reason_is_definitive() { + for definitive in [ + "FAILED: BAD_AGGREGATE_SIGNATURE", + "FAILED: MINTING_COIN", + "FAILED: RESERVE_FEE_CONDITION_FAILED", + // The composition is not part of the claim: a bare reason is still a reason. + "BAD_AGGREGATE_SIGNATURE", + // Case is the source's choice, not a classification. + "FAILED: bad_aggregate_signature", + ] { + assert!( + super::refusal_is_bundle_intrinsic(definitive), + "{definitive} is a property of the bundle; holding it for the full TTL strands a \ + user's coins over a spend no node will ever admit" + ); + } + + for held in [ + // The #460 path, verbatim: a second destination reporting its OWN mempool. + "FAILED: ALREADY_INCLUDING_TRANSACTION", + "FAILED: DOUBLE_SPEND", + "FAILED: MEMPOOL_CONFLICT", + // A node that has not caught up, not a bad bundle. + "FAILED: UNKNOWN_UNSPENT", + // Per-node relay policy. + "FAILED: INVALID_FEE_LOW_FEE", + "FAILED: INVALID_FEE_TOO_CLOSE_TO_ZERO", + // Evaluated against the asked node's peak. + "FAILED: ASSERT_HEIGHT_ABSOLUTE_FAILED", + "FAILED: ASSERT_SECONDS_RELATIVE_FAILED", + // Height-dependent at the CONDITION level: the announcement countdown is decremented + // only when `COST_CONDITIONS` is clear, and that flag comes from the answering node's + // height. Reads as a pure bundle property and is not. + "FAILED: TOO_MANY_ANNOUNCEMENTS", + // The CLVM-EXECUTION names. These LOOK intrinsic and are not: bundle validation runs + // under flags derived from the answering node's height and under a caller-supplied cost + // budget, so a node above a hard fork and a node below it can disagree on identical + // bytes. They were on the allowlist in the first draft of dig-node#460 and the + // adversarial gate built the free-then-lands sequence from `BLOCK_COST_EXCEEDS_MAX`. + // Their presence HERE is what stops them being re-added. + "FAILED: BLOCK_COST_EXCEEDS_MAX", + "FAILED: GENERATOR_RUNTIME_ERROR", + "FAILED: INVALID_BLOCK_COST", + "FAILED: INVALID_SPEND_BUNDLE", + // Nothing this crate enumerated -- must land on the safe side. + "FAILED: THE_NODE_WAS_HAVING_A_BAD_DAY", + "PENDING: ", + "", + // An allowlisted name EMBEDDED in wider text is not a match: exact only. + "FAILED: MEMPOOL_CONFLICT (see also BAD_AGGREGATE_SIGNATURE)", + "FAILED: BAD_AGGREGATE_SIGNATURE_MAYBE", + ] { + assert!( + !super::refusal_is_bundle_intrinsic(held), + "{held:?} was treated as the network's definitive verdict; another push \ + destination may be holding this very bundle" + ); + } + } + + /// **Proves (dig-node#460):** a peer's error string reaches the classifier VERBATIM, all the way + /// from the wire ack. + /// + /// Everything else in this file starts from a hand-built `TxStatus`, so the whole chain rests on + /// an unpinned assumption: that `chia_query` hands the full node's own words through unaltered. + /// If it ever normalised, prefixed or title-cased the error, every exact match here would stop + /// matching — silently, and in the HOLD direction, so no test would go red and no operator would + /// see anything except coins held for ten minutes more often than before. + /// + /// So this composes the REAL `chia_query::peer::translate::ack_to_tx_status` with + /// `stated_rejection` and `refusal_is_bundle_intrinsic`, driven by the chia ack STATUS BYTE + /// rather than by a label this crate chose. The same route `spend.rs` takes for its own fixture + /// (dig-node#444), for the same reason. + /// + /// **Catches:** an upstream change to the error passthrough or to the status labelling, either of + /// which would quietly disable the free path this guard is built around. + #[test] + fn a_peers_own_words_reach_the_classifier_through_the_real_translation() { + // Chia ack status 3 = FAILED. The reason is the full node's `Err` variant name. + let peer_local = chia_query::peer::translate::ack_to_tx_status( + 3, + Some("ALREADY_INCLUDING_TRANSACTION".to_string()), + ); + let stated = super::ChainTransport::stated_rejection(&peer_local) + .expect("the node stated a reason, so one must survive translation"); + assert_eq!( + stated, "FAILED: ALREADY_INCLUDING_TRANSACTION", + "the peer's own words did not survive the wire-to-outcome path intact" + ); + assert!( + !super::refusal_is_bundle_intrinsic(&stated), + "the #460 refusal arrived intact and was still read as the network's verdict" + ); + + let intrinsic = chia_query::peer::translate::ack_to_tx_status( + 3, + Some("BAD_AGGREGATE_SIGNATURE".to_string()), + ); + let stated = super::ChainTransport::stated_rejection(&intrinsic).expect("a stated reason"); + assert!( + super::refusal_is_bundle_intrinsic(&stated), + "a bundle no node will admit is being held for the full TTL; the translation changed the reason's spelling and every exact match silently stopped matching" + ); + + // Status 2 = PENDING, the node declining to admit without saying why. + assert_eq!( + super::ChainTransport::stated_rejection( + &chia_query::peer::translate::ack_to_tx_status(2, None) + ), + None, + "a bare ack must not acquire a reason in translation" + ); + } + /// **The hex form round-trips.** Pinned because the wire carries hex, not a struct: a bundle /// that re-encodes to different bytes would be pushed as a DIFFERENT transaction than the one /// the wallet signed, and its signature would no longer cover it. diff --git a/crates/dig-wallet/src/sage/rpc.rs b/crates/dig-wallet/src/sage/rpc.rs index c2679f9b..f4b7aa8e 100644 --- a/crates/dig-wallet/src/sage/rpc.rs +++ b/crates/dig-wallet/src/sage/rpc.rs @@ -2183,8 +2183,11 @@ impl WalletBackend { /// /// # What counts as definitive, and what this does NOT claim /// - /// A refusal is definitive only when the mempool STATED its reason (`accepted == false` with a - /// `rejection`). A bare `accepted: false` with no reason is an unexplained denial and is held. + /// A refusal is definitive only when the mempool STATED its reason AND that reason is a + /// property of the BUNDLE rather than of the answering node's own view — + /// [`super::chain::refusal_is_bundle_intrinsic`]. A bare `accepted: false` with no reason is an + /// unexplained denial and is held; so, since dig-node#460, is a reason that only the node that + /// sent it can vouch for. /// /// This does not make the flag trustworthy — a hostile source can fabricate a rejection string, /// and nothing here can verify one without an independent chain read. What it does is make the @@ -2196,8 +2199,34 @@ impl WalletBackend { /// `available=4000000 selectable=0`, renewable indefinitely. Requiring a STATED reason is what /// keeps a genuine mempool rejection (a bad signature, say) from locking the user's coins for /// the full TTL. + /// + /// # Why the presence of a reason was not enough (dig-node#460) + /// + /// One push is not one transmission. `chia_query`'s `push_tx` runs + /// `peer_then_coinset(peer, peer_retry, coinset)` and only the LAST answer arrives here, so a + /// refusal from destination B can free the inputs of a bundle destination A already admitted. + /// The first attempt fails as `Err` on a `request timed out` raised AFTER the bundle bytes went + /// out, and the peer that answers next has by then SEEN the gossip — so the reason it states is + /// `DOUBLE_SPEND`, `MEMPOOL_CONFLICT` or `ALREADY_INCLUDING_TRANSACTION`. Those are one node's + /// report of its OWN mempool, and on this path they mean the bundle IS in flight. Keying on the + /// mere PRESENCE of a reason therefore freed the coins precisely when they were least free. + /// + /// One peer's refusal is one peer's opinion; the network has no verdict this code can read. + /// What CAN be read is whether the stated reason is one every honest node would reach from the + /// same bytes. That question is answered without trusting the source, because the classifier is + /// an ALLOWLIST with a HOLD default: an unrecognised or view-dependent reason holds. + /// + /// **What that does and does not buy.** It removes the ACCIDENTAL free — the honest race above, + /// and a source denying a relay it performed. It does NOT defeat a DELIBERATE attacker in the + /// answering position, who can read the allowlist and emit a name from it. The free set shrank; + /// the ATTACKER's free set did not. Claiming otherwise would invite the next reader to treat the + /// reason string as trusted, which it is not and cannot be made. fn is_definitive_rejection(outcome: &PushOutcome) -> bool { - !outcome.accepted && outcome.rejection.is_some() + !outcome.accepted + && outcome + .rejection + .as_deref() + .is_some_and(super::chain::refusal_is_bundle_intrinsic) } /// Record an accepted bundle as in-flight and hold its inputs out of further selection. @@ -11041,7 +11070,7 @@ mod tests { assert_eq!(selectable[0].amount, 500, "the wrong coin was held"); } - /// **The control:** a mempool refusal that STATES ITS REASON reserves nothing. + /// **The control:** a mempool refusal that states a BUNDLE-INTRINSIC reason reserves nothing. /// /// Without it, reserving unconditionally satisfies the tests above while stranding a user's /// coins over a spend that will never happen — the lockout that is the worse of the two @@ -11051,6 +11080,11 @@ mod tests { /// `a_bundle_denied_without_a_reason_is_held_rather_than_freed`: the two fixtures differ in the /// `rejection` field alone and demand opposite outcomes, so together they pin the bound from /// both sides. Neither is meaningful without the other. + /// + /// The reason text was `"mempool said no"` until dig-node#460, which is a shape no mempool + /// emits and which made this test reason-AGNOSTIC — it asserted only that SOME reason frees, + /// so it could not see that a peer-local reason must not. It now carries a real Chia error + /// name, and the #460 pair below carries the two it must be told apart from. #[tokio::test] async fn a_refused_bundle_reserves_nothing() { let mut refused = spendable_row(0xa1, 100); @@ -11058,7 +11092,7 @@ mod tests { let refusing = FakePusher::answering(Ok(PushOutcome { accepted: false, transaction_id: None, - rejection: Some("mempool said no".into()), + rejection: Some("FAILED: BAD_AGGREGATE_SIGNATURE".into()), verdict: "FAILED".into(), })); let be = backend_with(vec![refused.clone()], true) @@ -11083,6 +11117,172 @@ mod tests { ); } + /// **Proves (dig-node#460):** a refusal that is a property of ONE peer's VIEW does not return + /// the inputs to selection, because another destination may be holding the very bundle. + /// + /// **The defect this catches.** `chia_query`'s `push_tx` is not one transmission. It runs + /// `peer_then_coinset(peer, peer_retry, coinset)`: peer A, then a DIFFERENT peer B, then + /// coinset, and only the LAST answer reaches this crate. Attempt 1 fails as `Err` on a + /// `request timed out` raised AFTER the bundle bytes went out, so peer A may have admitted it + /// and gossiped it. Peer B, having now SEEN it, refuses with a stated conflict. Keying the + /// free on the mere PRESENCE of a reason then frees the inputs of a bundle sitting in a public + /// mempool -- and the strings that arrive on this path (`DOUBLE_SPEND`, + /// `ALREADY_INCLUDING_TRANSACTION`, `MEMPOOL_CONFLICT`) are precisely the ones that mean the + /// bundle IS in flight. The race does not merely reach the unsafe branch occasionally; when it + /// happens, the unsafe branch is the EXPECTED one. + /// + /// FIXTURE DESIGN. The refusal is AMBIGUOUS, which is the only kind that can see this bug: an + /// unambiguous refusal (`BAD_AGGREGATE_SIGNATURE`, the sibling below) is refused identically + /// by every node and freeing it is correct. Two coins, one spent by the bundle, so a + /// mis-scoped reservation that simply empties selection cannot pass for a correct one. This + /// and `a_bundle_intrinsic_refusal_still_frees_its_inputs` differ ONLY in the reason TEXT and + /// demand OPPOSITE outcomes: that pairing is what proves the guard reads the reason's CLASS + /// rather than its presence, and neither test means anything without the other. + #[tokio::test] + async fn a_peer_local_refusal_holds_inputs_another_destination_may_be_carrying() { + let mut spent_by_the_bundle = spendable_row(0xa1, 100); + let (bundle_hex, transaction_id) = a_bundle_spending(&mut spent_by_the_bundle); + let untouched = spendable_row(0xb2, 500); + // The second destination's answer, verbatim in the shape `stated_rejection` composes: + // a node that already holds this bundle telling us it conflicts. + let already_seen_it = FakePusher::answering(Ok(PushOutcome { + accepted: false, + transaction_id: None, + rejection: Some("FAILED: ALREADY_INCLUDING_TRANSACTION".into()), + verdict: "FAILED".into(), + })); + let be = backend_with(vec![spent_by_the_bundle.clone(), untouched.clone()], true) + .await + .with_pusher(already_seen_it); + + assert_eq!( + be.spendable_coins(None).await.unwrap().len(), + 2, + "the fixture must start with BOTH coins selectable, or the assertion below is vacuous" + ); + + let outcome = be.push_signed_bundle(&bundle_hex).await.unwrap(); + assert!( + !outcome.accepted, + "the refusal is reported honestly -- what changes is what is RESERVED, not what is said" + ); + assert_eq!( + outcome.rejection.as_deref(), + Some("FAILED: ALREADY_INCLUDING_TRANSACTION"), + "the operator must still be shown the peer's own words verbatim" + ); + + let pending = be.get_pending_transactions().await.unwrap().transactions; + assert_eq!( + pending.len(), + 1, + "one peer's view-local refusal freed the inputs of a bundle another destination may \ + already hold; a second send inside the confirmation window can reselect them" + ); + assert_eq!(pending[0].transaction_id, transaction_id); + + let selectable = be.spendable_coins(None).await.unwrap(); + assert_eq!( + selectable.len(), + 1, + "the possibly-in-flight bundle's input is still offered to a second spend" + ); + assert_eq!( + selectable[0].amount, 500, + "the wrong coin was held: the untouched control left selection" + ); + } + + /// **Proves (dig-node#460):** a reason NOBODY ENUMERATED is held, not freed. + /// + /// This is the property that makes classifying an untrusted string acceptable at all. The + /// definitive set is an ALLOWLIST with a hold DEFAULT, so the enumeration being incomplete -- + /// a Chia error name that did not exist when this was written, a source with its own + /// vocabulary, a peer inventing text -- costs at most a bounded `RESERVATION_TTL_MS` hold. A + /// DENYLIST of the same reasons would read almost identically and would fail the opposite way: + /// every unforeseen string would free the coins. + /// + /// **Catches:** re-inverting the default, and any refactor that treats "not recognised as + /// peer-local" as "therefore definitive". + #[tokio::test] + async fn an_unrecognised_refusal_reason_is_held_rather_than_freed() { + let mut spent_by_the_bundle = spendable_row(0xa1, 100); + let (bundle_hex, _) = a_bundle_spending(&mut spent_by_the_bundle); + let untouched = spendable_row(0xb2, 500); + let speaking_its_own_language = FakePusher::answering(Ok(PushOutcome { + accepted: false, + transaction_id: None, + // Not a Chia error name, and deliberately not one this crate could ever enumerate. + rejection: Some("FAILED: THE_NODE_WAS_HAVING_A_BAD_DAY".into()), + verdict: "FAILED".into(), + })); + let be = backend_with(vec![spent_by_the_bundle.clone(), untouched.clone()], true) + .await + .with_pusher(speaking_its_own_language); + + assert_eq!(be.spendable_coins(None).await.unwrap().len(), 2); + be.push_signed_bundle(&bundle_hex).await.unwrap(); + + assert_eq!( + be.get_pending_transactions() + .await + .unwrap() + .transactions + .len(), + 1, + "an unrecognised reason freed the inputs; the allowlist's default must be to HOLD, or \ + every reason this crate has not enumerated is a free path" + ); + let selectable = be.spendable_coins(None).await.unwrap(); + assert_eq!(selectable.len(), 1); + assert_eq!(selectable[0].amount, 500, "the wrong coin was held"); + } + + /// **The paired control (dig-node#460):** a BUNDLE-INTRINSIC refusal still frees its inputs + /// immediately. + /// + /// Without this the #460 fix degenerates into "hold on every refusal", which is the lockout + /// that is the worse of the two failures -- measured on dig-account as `available=4000000 + /// selectable=0`. `BAD_AGGREGATE_SIGNATURE` is a property of the BUNDLE: every honest node + /// refuses it identically, so no destination can be holding it and there is nothing to protect. + /// + /// It differs from `a_peer_local_refusal_holds_inputs_another_destination_may_be_carrying` in + /// the reason text ALONE. A guard that went back to keying on presence, or forward to keying + /// on nothing, fails exactly one of the pair. + #[tokio::test] + async fn a_bundle_intrinsic_refusal_still_frees_its_inputs() { + let mut spent_by_the_bundle = spendable_row(0xa1, 100); + let (bundle_hex, _) = a_bundle_spending(&mut spent_by_the_bundle); + let untouched = spendable_row(0xb2, 500); + let bad_signature = FakePusher::answering(Ok(PushOutcome { + accepted: false, + transaction_id: None, + rejection: Some("FAILED: BAD_AGGREGATE_SIGNATURE".into()), + verdict: "FAILED".into(), + })); + let be = backend_with(vec![spent_by_the_bundle.clone(), untouched.clone()], true) + .await + .with_pusher(bad_signature); + + assert_eq!(be.spendable_coins(None).await.unwrap().len(), 2); + be.push_signed_bundle(&bundle_hex).await.unwrap(); + + assert!( + be.get_pending_transactions() + .await + .unwrap() + .transactions + .is_empty(), + "a bundle no node will ever admit was recorded as in flight" + ); + assert_eq!( + be.spendable_coins(None).await.unwrap().len(), + 2, + "a bundle refused for its OWN contents stranded the coins it never committed for the \ + full TTL; the #460 fix must narrow the free path, not close it" + ); + } + /// **The defect (#2764).** `get_pending_transactions` returned a hardcoded empty list. A /// caller that pushed a bundle and polled was told, as a measured fact, that nothing was in /// flight.