feat(wallet): bound a live fallback-tier answer by the peak its held peers announce - #510
feat(wallet): bound a live fallback-tier answer by the peak its held peers announce#510MichaelTaylor3d wants to merge 3 commits into
Conversation
…t-scoped reads Salvage anchor. No production change yet. Refs #290
…peers announce The five Source::Fallback arms that CONSULT the chain tier wrote synced: false and peak_height: None as literals, so control.rs's stale_by was null on every non-wallet-scoped read and dig-app's absence warrant could never be obtained. Both fields now derive from one chain_tier_answer_height() measurement, so a claim never travels without its bound. The two CACHE-served arms are unchanged: nothing recorded when those rows were taken. Closes #290 Co-Authored-By: Claude <noreply@anthropic.com>
…inert literals Three tests asserted synced: false / peak_height: None on a LIVE fallback arm. Each chose a fallback fixture precisely because those fields were hardcoded, so the peer tier in them was inert; it is now decisive. Updated with reasoning in place, never deleted, and the peers-ahead fixture is what proves the bound comes from the peers rather than leaking from the replica. Co-Authored-By: Claude <noreply@anthropic.com>
loop-security — IN PROGRESS, not the verdictAudited head: Posting this finding now rather than holding it to the verdict. FINDING 1 (GATING, HIGH) — the peak that licenses
|
Adversarial gate: REFUTED — do not merge. The bound is a latch, and the doc asserts it is not.Third leg of the triple gate, fresh context, prompted to refute. It did. The central premise of The defect
pub fn peak_height(&self) -> u32 { self.peak_height.load(Ordering::Relaxed) }backed by This PR's own doc (
It is a latch, and structurally a weaker one than The crate had already measured this exact state
Not using its peers is exactly the Reachable with no adversary: peers announce H → Adversarial variant: The sharpest framing: a polarity flip on a measurement already in use
This PR reuses the identical measurement inverted, to certify an answer current, where a That also answers the separability question I asked, and not in my favour: PR #500 rewrites Also blocking, and also mine
What it attacked and could NOT refute — recorded so it is not re-litigated
DispositionNot merging. Not undrafting. The minimum honest fix is to make the bound's liveness structural: Holding the fix round until the correctness and security gates return, so all findings land in one |
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
IN PROGRESS — not the verdict. Gate finding 1 of N, head 6a4c753.
| /// [`super::fallback::ChainPeerTier::peak_height`]. That is not a tautology dressed as | ||
| /// evidence — it is one measurement bounding both sides, and the honest reading is that the two | ||
| /// agree BECAUSE the same measurement bounds both. | ||
| async fn chain_tier_answer_height(&self) -> Option<u32> { |
There was a problem hiding this comment.
IN PROGRESS — not the verdict. Independent correctness gate, head 6a4c753cab39b1b0ea2fc47715aa9f89fd2ad51f.
GATING — the fail-toward-false ordering is NOT structural: ChainPeerTier::peak_height survives total peer loss.
chain_tier_answer_height (crates/dig-wallet/src/sage/rpc.rs:1086) reads chain_peer_tier().await.peak_height, and its rustdoc + SPEC.md §18.7b both justify synced: true on the claim that "this node's OWN held Chia peers independently name that tip at the same moment" and that "a node holding no chain peers has no announced height, so it keeps claiming nothing". Traced to source, neither holds:
crates/dig-wallet/src/sage/chain.rs:566-575folds the raw reading throughPeerLiveness::observe.chain.rs:135-141nulls onlypeer_countwhen the 180 s liveness window lapses. Its own comment says so: "The peak is reported exactly as the peers gave it… it froze when they died". Sopeak_heightis deliberately NOT gated on liveness.- Underneath,
chia-query 0.20.0peer/pool.rs:232,421holds the peak as a monotonicfetch_maxAtomicU32. It is never decremented and never cleared on peer ejection; only building a whole new client resets it.
Net effect on the exact fleet configuration this repo already measured (chain.rs:71-72, #3159 — all tcp/8444 blocked, HTTPS untouched): the node holds zero live peers, peer_count correctly degrades to None after 180 s, and peak_height stays frozen at the last announced height indefinitely. Every live fallback arm then emits synced: true, peak_height: Some(frozen) forever — from a node that cannot establish it is caught up. That is the ticket's stated hard ordering ("Fail toward false") violated, and it is reachable without an attacker.
It also makes the peak_height/network_peak_height pair mutually reassuring while both are the same stale number, and grants dig-app's absence_warrant (dig-app-core/src/chain/source.rs) on evidence that is arbitrarily old.
What the fix must do: require a LIVE tier, not just a remembered height — e.g. in chain_tier_answer_height, let t = self.chain_peer_tier().await; t.peer_count.and(t.peak_height), so the liveness gate that already exists on peer_count transfers to the bound. Add a test with ChainPeerTier { peer_count: None, peak_height: Some(h) } (the frozen-peak shape production actually produces) asserting (false, None); note that shape is not currently constructible by any fixture in the new tests — peers_level_at and peers_unobservable vary both fields together, so the suite cannot see this state at all.
What the fix must NOT do: it must not relax PeerLiveness's peak semantics or make the peak liveness-gated inside chain.rs, because control.wallet.peak and syncStatus consume the same field for a different purpose and that would need its own audit.
What would have to be true for this finding to be wrong: that some caller re-creates the ChiaQuery client (resetting the atomic) whenever the pool empties, on the read path. I found no such path — warm() discards an empty-on-build client only, and the pool refills lazily from inside a request.
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
IN PROGRESS — findings 2-4, head 6a4c753. Verdict follows.
| /// by design, because a spent coin's record is immutable (dig_ecosystem#3044, | ||
| /// dig_ecosystem#3050). Stamping the CURRENT peer peak on one would claim it is level with the | ||
| /// network when nothing measured that, which is this same defect mirrored. Those arms keep | ||
| /// `synced: false, peak_height: None`. |
There was a problem hiding this comment.
GATING (contract split) — six public field doc-comments in this same file still say the OLD rule, and were not swept.
The SPEC was updated; the crate's own rustdoc — which is the contract a consumer of dig-wallet actually reads, and closer to the code than SPEC.md — still states the behaviour this PR removed. All in crates/dig-wallet/src/sage/rpc.rs, none touched by the diff:
:333WalletCoinByIdResult::synced— "alwaysfalseon a [Source::Fallback] one":337WalletCoinByIdResult::peak_height— "Noneon a [Source::Fallback] one, where a caller bounding confirmations readscontrol.wallet.peakinstead":373WalletCoinSpendResult::synced— "Alwaysfalse":375WalletCoinSpendResult::peak_height— "AlwaysNone":397/:399WalletCoinsByParentResult— the same two sentences:441-443WalletBalanceResult::synced— "a fallback answer reportsfalsehowever caught-up the DB happens to be":452-453WalletBalanceResult::peak_height— "Nonefor a [Source::Fallback] answer"
Each is now false for the live arms. Two of them (:337, :375) additionally send a caller to control.wallet.peak on a rationale this PR just invalidated.
This is the superseded-wording sweep, not a doc nit: a reimplementer or a consumer reading WalletCoinSpendResult::synced learns that true is unreachable and can legitimately treat it as a constant.
Fix: rewrite each to the §18.7b rule the SPEC now states (live consult → the peers' announced height with synced: true when one is known; no peer height, or a cache-served answer → false/None). Sweep by searching the crate for the old phrasing rather than spot-checking these seven lines.
| because nothing bounds the answer at all. The two fields MUST be derived from that SINGLE | ||
| measurement and MUST travel together: an implementation MUST NOT emit `synced: true` beside | ||
| `peak_height: null` on this tier, nor a `peak_height` beside `synced: false`. | ||
|
|
There was a problem hiding this comment.
MEDIUM/GATING — SPEC.md's control.wallet.peak row now contradicts §18.7b, in two ways. (The implementer flagged this as deliberately untouched; on inspection one half is not merely "weaker", it is false.)
The row (unchanged by this PR) states:
- "a balance reports
peak_height: nullon every"fallback"-tier answer by design (§18.7b), so a caller bounding a claimed confirmation could not obtain one from the node that most needs to answer." — now false. After this change a live fallback balance DOES report a height, which removes the stated reason this method exists as its own verb. Leaving a normative row whose rationale asserts the opposite of the shipped behaviour is the stale-contract shape, not a stylistic residue. - "
syncedcarries EXACTLY itscontrol.wallet.balancemeaning (§18.7b) and MUST be MEASURED by the same predicate", followed by "A chain-tier answer reportssynced: false." — after this PR,control.wallet.balanceon a chain-tier answer with a known peer peak reportssynced: truewhilecontrol.wallet.peakon the same tier at the same moment reportssynced: false. Two endpoints, one tier, one measurement, opposite flags — and the SPEC says they carry EXACTLY the same meaning.
Fix: either state on the .peak row that its synced means the REPLICA is caught up and is therefore NOT the same predicate as the tier-fields' (dropping the "EXACTLY … same predicate" clause), or align the two. Correct the "reports peak_height: null on every fallback answer" rationale either way. Do NOT resolve this by making .peak report synced: true on the chain tier without its own audit — that endpoint is what a caller uses to bound a claimed confirmation.
| @@ -7886,8 +7984,11 @@ mod tests { | |||
| .await | |||
| .unwrap(); | |||
| assert_eq!(result.source, Source::Fallback); | |||
There was a problem hiding this comment.
MEDIUM (non-blocking on its own) — the "fixture chose fallback because those fields were inert" sweep found three tests and missed three more, all on LIVE arms.
Still asserting the superseded rule, and now passing only because their fixture leaves the peer tier at the default UNOBSERVABLE:
crates/dig-wallet/src/sage/rpc.rs:6052-6060a_fallback_served_read_never_reports_the_dbs_sync_state— "a coinset answer is never a synced local read, however synced the DB is" / "the DB's peak does not bound a coinset answer's freshness". Its real subject (no DB leak) does survive, because a leak would surfaceSome(9_000_000); the two assertion MESSAGES now state a rule the code no longer follows.crates/dig-wallet/src/sage/rpc.rs:7408-7409an_arbitrary_address_reads_its_coins_from_the_chain_tier— "nor does it bound its freshness". This one is now inert in both directions: the DB has no peak, so(false, None)is what a leak would produce too.crates/dig-wallet/src/sage/rpc.rs:7000-7001an_unspent_coin_answers_with_no_spend_rather_than_an_error— bareassert!(!result.synced)on the livecoin_spendarm.
Coverage gap in the same area: coins_for_address's live arm (rpc.rs:1751) has no positive test of the new bound. no_live_fallback_arm_pairs_a_claim_with_a_missing_bound covers coin_by_id, coin_spend, coins_by_parent; the rewritten a_fallback_answer_still_claims_neither_freshness_nor_a_height covers balance_for_address. That is 4 of the 5 arms, and the test's own docstring says its purpose is to catch a future arm computing the fields independently — so the arm it does not cover is exactly the hole.
Fix: restate the three messages as "with no observable peer tier", and add coins_for_address to the pairing loop.
loop-security — IN PROGRESS, not the verdict (2 of 2)Head FINDING 2 (GATING, HIGH) — the warrant rests on a monotone MAX over UNVERIFIED single-peer claims, which a sibling module in this same crate explicitly rejects as an anchorTraced and the only writer, So the value is:
This crate already documents that anchoring on this value is unsafe.
Attack A — one peer manufactures the warrant (licensing direction)dig-node dials strangers (NC-12). A node with zero warranted reads becomes fully warranted the moment one dialled peer emits one Attack B — one peer pins the peak permanently high (denial direction)
The two findings compose. Finding 1 says the peak never decays; Finding 2 says one peer sets it. Together: one hostile peer connects once, announces once, disconnects — and this node keeps issuing warranted absences on that peer's say-so indefinitely, while The asymmetry that makes this sharpest (question 7)For What would make it not exploitable: anchor on the same corroborated evidence the sibling module already built — a quorum/median height, or at minimum require the peak to be corroborated by more than one session, plus FINDING 3 (GATING, MEDIUM-HIGH) —
|
| arm | fallback method | routing |
|---|---|---|
balance_for_address |
coin_records_by_puzzle_hashes |
chain.rs:717-722 — CoinsetFallback only |
| coins | coin_records_by_hints |
chain.rs:724-728 — CoinsetFallback only |
coins_by_parent |
coin_records_by_parent |
chain.rs:765-769 — CoinsetFallback only |
coin_by_id |
coin_record_by_id |
chain.rs:729-735 — peers if configured, else oracle |
coin_spend |
coin_spend |
chain.rs:737-744 — peers if configured, else oracle |
On the balance, coins and lineage arms the answer comes from the third-party oracle every time, and the bound comes from the peer tier, which had no involvement in producing it. The PR states this residual risk honestly and I credit that — but it states it as a possibility ("a third-party oracle could serve a materially stale answer") when for those three arms the oracle is the only possible server, and it does not mention that the change simultaneously flattens stale_by to a constant 0, removing the downstream signal that could have surfaced it.
coins_by_parent is the worst case: rpc.rs:2121-2122 notes a lineage walk reads an empty page as "this is the tip". A stale oracle page therefore becomes a believed end-of-lineage, now carrying synced: true and stale_by: 0.
What would make it not exploitable: report a bound the answering sub-tier actually earned — attribute the sub-tier on the result so a consumer can weigh it, or leave the oracle-only arms unbounded and warrant only the peer-corroborated arms. Failing that, stale_by must not be computed from two copies of one number.
Still to report: the cached arms, rate-limiting/amplification, and §908. Verdict follows.
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
IN PROGRESS — finding 5, head 6a4c753. Cross-repo contract. Verdict next.
| synced: false, | ||
| peak_height: None, | ||
| synced: bound.is_some(), | ||
| peak_height: bound, |
There was a problem hiding this comment.
GATING — this violates the PUBLISHED cross-repo contract in dig-node-control-interface 0.30.0. The PR's wire-contract check verified the field TYPE and not the field's stated CONTRACT.
The PR body says: "Confirmed rather than assumed: dig-node-control-interface 0.30.0 src/results.rs declares pub peak_height: Option<u32> on all five result types (lines 554, 704, 835, 913, 985). The field goes from always-null to sometimes-a-number — additive per §5.1." The type is indeed Option<u32>. The contract those types publish is not silent about the value:
dig-node-control-interface-0.30.0/src/results.rs:874-879, on WalletCoinByIdResult, in normative voice:
"A
fallbackanswer MUST reportsyncedfalseandpeak_heightnullhowever caught-up the node's own replica is, because the oracle produced the figures and the replica neither produced them nor bounds their freshness."
And the same rule restated on the sibling fields, all five result types:
:832/:834— "alwaysfalsefor a fallback answer" / "every fallback answer":910/:912— "falsefor every fallback answer" / "every fallback answer":982/:984— same:1074/:1076— same
So this is not an additive value change inside a permissive contract; it is a node that stops conforming to the shared crate every consumer builds against, while that crate keeps telling them the old rule. A dig-app or third-party consumer reading WalletCoinByIdResult::synced is entitled by the published doc to treat true as impossible on source: "fallback" — which is precisely the branch this change exists to unlock.
§4.1 / §1.3b make this release-first, not a follow-up: the contract crate must state the new §18.7b rule and be RELEASED, then dig-node consumes it by version, in the same unit of work. Shipping the node first leaves the ecosystem half-migrated with the authority pointing the wrong way.
What would have to be true for this to be wrong: that dig-node depends on a newer control-interface than 0.30.0 whose docs already carry the new rule. The PR body itself names 0.30.0, and 0.30.0 is what is on disk.
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
IN PROGRESS — finding 5, head 6a4c753. Cross-repo contract. Verdict next.
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
CHANGES-REQUIRED — independent correctness gate
Head reviewed: 6a4c753cab39b1b0ea2fc47715aa9f89fd2ad51f (resolved from the remote; isDraft: true).
The change is well-argued and its classification work is genuinely correct. Two of its central claims do not survive tracing to source: the fail-toward-false ordering is not structural, and the published cross-repo contract forbids the value this now emits.
Ranked findings (each posted as its own thread)
| # | severity | where | what |
|---|---|---|---|
| 1 | GATING | crates/dig-wallet/src/sage/rpc.rs:1086 |
ChainPeerTier::peak_height is a monotonic fetch_max atomic that is NOT liveness-gated (chain.rs:135-141, chia-query-0.20.0/src/peer/pool.rs:232,421). A node that held peers and lost them keeps a frozen peak forever, so every live arm emits synced: true indefinitely on a node that cannot establish it is caught up. Fail-toward-false is structural only for a node that NEVER held a peer. |
| 2 | GATING | rpc.rs:333,337,373,375,397,399,441,452 |
Eight public field doc-comments in this same file still state the removed rule ("always false" / "Always None" on a fallback answer). The SPEC was swept; the crate rustdoc was not. |
| 5 | GATING | dig-node-control-interface-0.30.0/src/results.rs:875 (+ 832,834,910,912,982,984,1074,1076) |
The published contract says a fallback answer MUST report synced: false / peak_height: null. The PR verified the field TYPE and not its stated contract. Release-first: the contract crate updates and ships first (§4.1/§1.3b). |
| 3 | MEDIUM | SPEC.md control.wallet.peak row |
Its rationale ("a balance reports peak_height: null on every fallback answer by design") is now FALSE, and its "synced … MUST be MEASURED by the same predicate" clause now contradicts §18.7b — same tier, same moment, opposite flags on two endpoints. |
| 4 | MEDIUM | rpc.rs:6052, 7000, 7408 + rpc.rs:1751 |
Three more tests assert the superseded rule on LIVE arms, passing only because their fixtures leave the tier UNOBSERVABLE; and coins_for_address live arm has no positive test of the new bound (the pairing test covers 3 arms, the rewritten test covers balance_for_address). |
What I actually verified vs reasoned about
Verified by reading the code:
- (1) fail-toward-false — traced
chain_tier_answer_heighttochain_peer_tier, tofallback.peer_tier(), tochain.rs:548-575, toPeerLiveness::observe, to thechia-query0.20.0 pool atomic. Finding 1 is a source trace, not an inference. Note the new fixtures (peers_level_at,peers_unobservable) vary both fields together and CANNOT construct the{peer_count: None, peak_height: Some(h)}state production produces, so the suite is blind to it. - (2) live-vs-cached classification: CORRECT and COMPLETE. Independently enumerated every
Source::Fallbackresult construction inrpc.rs:1602(balance, live),1751(coins, live),1860(coin_by_id cached),1893(coin_by_id live),1999(coin_spend both-halves cached),2042(coin_spend live),2129(coins_by_parent live). Seven, matching the table. No eighth —2183isChainPeak, a different type and a different endpoint. The partially-cachedcoin_spendcase (1984-2002) correctly falls to the live path, which does perform a real read. - (3) pairing invariant: HOLDS by construction. Each of the five sites is a struct literal returned immediately, both fields from one
let bound; no later mutation of either field on any path. - (4) tests at the decision: YES for the new ones.
(true, Some(PEERS_PEAK))cannot pass against an implementation that never emits a bound, andPEERS_PEAK = REPLICA_PEAK + 12defeats an implementation reaching for the replica measurement. The withhold cases are asserted, not assumed. Caveat: the reported RED (2 failed) is the two MODIFIED tests; the four new ones were not part of that demonstration, and the cached-hit one is documented as passing both before and after. - (5) the three changed tests preserve their subjects. The two falls-through tests keep
ORACLE_AMOUNT/call_countand the AMOUNT assertion. The rewrittena_fallback_answer_still_claims_neither_freshness_nor_a_heightis STRONGER than what it replaced: peers atREPLICA_PEAK + PEERS_AHEAD_BYmean a replica-peak leak now fails, where the old(false, None)could not distinguish a leak from a hardcoding. I do not read it as weakened to pass. The sharper question — which OTHER tests picked a fallback fixture because the fields were inert — found three more (finding 4). - (7)
control.rsgenuinely unaffected: CONFIRMED. Its fixtures (control.rs:5320,5413,5473,5522with the wire assertions at5339,5423,5496,5532) construct the result structs directly and assert serialization. A struct still carryingfalse/Nonestill serializes that way, and that value is still producible (the two cached arms, and the no-peer case), so none asserts something production cannot emit. - Consumer reach: CONFIRMED.
dig-app-core/src/chain/source.rs:214-231gatesabsence_warrantonfreshness.syncedalone. The change does reach the predicate — which is also why finding 1 matters: the warrant is then granted on evidence of unbounded age. - Wire type:
peak_height: Option<u32>atresults.rs:554,704,835,913,985, as stated. The type is fine; see finding 5 for the contract. - Versions: root
0.247.0to0.254.0,dig-wallet0.47.0to0.49.0; both increment and disagree with nothing. Minor is the right band. The root +7 jump is unexplained in the body but is not a gate failure.
Reasoned about, not executed: I did not run the suite (shared worktree, no build). Every test judgement above is from reading fixtures and assertions, so "the suite is green" is the PR claim, not mine. I did not exercise the chia-query pool against a live node; its write path (fetch_max, pool.rs:865) is read from source.
Boundary against PR #500 — independently checked, not echoed
Semantically separable: yes. #500 works replica_answer_is_current and the Db/phase arms of syncStatus; this works the Fallback arms. Neither subsumes nor contradicts the other.
Mechanically NOT independent, and the PR body does not say so. #500 rpc.rs hunks are at -1035,7, -1046,12 and -1065,35 — the exact region into which this PR inserts chain_tier_answer_height (1030-1088). Both also bump the root and dig-wallet versions. Whichever merges second MUST rebase and re-read its version from the file on disk, never from its commit log: git rebase --onto silently DROPS a bump commit whose patch collides with the one just merged (RC 0, no conflict, one line reading dropping <sha>), leaving the branch at main version with the gate green against a stale base.
One substantive interaction worth naming: after both land, syncStatus narrows Synced while the coin reads widen synced on the same underlying peer-peak measurement. That is defensible only once finding 1 is fixed — otherwise #500 tightens the wallet-scoped claim while this loosens the non-wallet-scoped one using a value that outlives the peers it names.
To clear this gate
Findings 1, 2 and 5 are blocking. 3 and 4 are cheap and belong in the same pass. I am not fixing any of them — a reviewer that fixes is no longer independent.
loop-security VERDICT: CHANGES-REQUIREDAudited head: Read-only audit. No edits, no merge, no undraft. Diffed The relaxation is deliberate, the harm it replaces is real, and the PR is unusually honest about its own residual risk. I am still gating it, for one reason: the thing it names as the bound does not have the property it is said to have. The doc claims the peak is what "this node's OWN held Chia peers independently name at the same moment" — but the value read is a monotone The fix is small and the evidence for it already exists in this crate. This is not a redesign. Ranked findingsFull detail with the traces is in my two interim comments above; this is the ranked summary. 1. GATING / HIGH — the peak that licenses
|
| arm | fallback method | routing |
|---|---|---|
balance_for_address |
coin_records_by_puzzle_hashes |
chain.rs:717-722 — CoinsetFallback only |
| coins | coin_records_by_hints |
chain.rs:724-728 — CoinsetFallback only |
coins_by_parent |
coin_records_by_parent |
chain.rs:765-769 — CoinsetFallback only |
coin_by_id |
coin_record_by_id |
chain.rs:729-735 — peers if configured, else oracle |
coin_spend |
coin_spend |
chain.rs:737-744 — peers if configured, else oracle |
Scenario: the oracle serves a materially stale page while held peers sit at the tip. coins_by_parent reads an empty page as "this is the tip" (rpc.rs:2121-2122), and it now carries synced: true and stale_by: 0. A lineage walk stops early against a superseded singleton — the failure SPEC.md itself says produces "a spend built against a superseded singleton" and a mint funded twice.
The PR names this risk in prose and I credit that. What it does not say is that it also removes the field that could have surfaced it, and it states the risk as a possibility ("could serve a materially stale answer") where on those three arms the oracle is the only possible server.
Fix: attribute the answering sub-tier on the result so a consumer can weigh it, or bound only the peer-corroborated arms. stale_by must not be computed from two copies of one number.
4. GATING (same fix) / MEDIUM — the new SPEC text asserts a provenance the code cannot deliver on three arms
SPEC.md §18.7b, added by this PR:
"The bound is legitimate because the answer came from a full node reading the chain at its tip moments ago, while this node's own peers independently name that tip at the same moment."
Written in normative voice, and false for balance, coins and coins_by_parent, where the answer comes from the coinset.org HTTP oracle, never from a full node this node holds. "At the same moment" is not enforced anywhere (Finding 1). Per §4.2 a SPEC must describe behaviour the implementation has; this clause is born false in the commit that writes it. Reword to match whichever bound the fix lands on.
Note also that the retained SPEC sentence — "a zero is a positive claim that the figure is level with the network" — is now emitted unconditionally on the fallback tier without any comparison having occurred. The literal MUST (stale_by non-null only when both heights are known) is still satisfied; it is the stated MEANING of 0 that no longer holds.
5. NOT GATING / LOW — defense-in-depth, recommend a follow-up ticket
crates/dig-wallet/src/sage/peer_reads.rs:235-237: the live coin_record_by_id re-checks the cache before drawing peers. The outer arm (rpc.rs:1849-1862) already probed the same function with the same predicate, so a hit here requires a concurrent writer landing a row in the window between the two probes. The row would then be seconds old and stamped with the current peak — approximately truthful, but it does breach the PR's own stated rule that "only the arms that consulted the chain on THIS call may claim a bound." Worth a ticket, not a gate.
Areas I checked and found CLEAR — with the bound named
Amplification / egress (question 5) — CLEAR, and this is the part I want to praise explicitly. chain_tier_answer_height opens no network activity. chain_peer_tier (rpc.rs:1023-1028) calls ChainTransport::peer_tier (chain.rs:548-557), which uses sources.existing_client() (sources.rs:213-215 — a lock-and-clone that never builds, so asking never dials), then reads client.peer_count() (an in-memory pool registry) and client.peer_peak_height() (an AtomicU32 load). Zero outbound rounds per read. The open unauthenticated loopback endpoint does not become an egress amplifier.
Limiter ordering — CLEAR and untouched. In every arm the order is unchanged: replica fast path, then cache (deliberately ahead of both liveness and the limiter, since a hit sends nothing), then is_live(), then fallback_rate.try_acquire(), then the read, and only then the new chain_tier_answer_height(). The new call sits strictly INSIDE the rate-limited region and cannot be reached before the check. Verified on coin_by_id (rpc.rs:1865-1890) and coins_by_parent (rpc.rs:2089-2124).
The cached arms (question 4) — CLEAR. coin_by_id's cache hit (rpc.rs:1853-1861) and coin_spend's both-cached hit (rpc.rs:1993-1999) return false/None and return EARLY, so no cached value reaches a live-classified arm through the outer path. The reasoning is stated correctly at both sites. The only residual is Finding 5's narrow race.
§908 custody (question 6) — CLEAR. The diff touches crates/dig-wallet/src/sage/rpc.rs, SPEC.md, two Cargo.tomls and Cargo.lock. Grepping every added line for secret|seed|mnemonic|private_key|sign|sk_|derive|keystore|passphrase|master_key returns only the English word "derive(d)" in prose. No key is held, derived, or signed with; these remain pure public-data chain reads. crates/dig-node-service and crates/dig-node-core are untouched.
Fail-toward-withheld with no peers — CLEAR and genuinely structural. With no client, peer_tier returns UNOBSERVABLE and PeerLiveness::observe RESETS rather than ageing (chain.rs:114-118), so a node that has established no chain view keeps saying None, and both fields fall to false/null together. no_live_fallback_arm_pairs_a_claim_with_a_missing_bound pins the pairing across the arms, and it is a real test — it varies the peer tier and asserts both fields, so it would fail if either were set independently.
Deriving both fields from one Option — CLEAR, and the right shape. synced: bound.is_some(), peak_height: bound makes {synced: true, peak_height: null} unconstructible on these arms. That part of the design is sound and should survive whatever fix lands; my objection is to WHAT is measured, never to how the two fields are paired.
Test honesty — CLEAR. The two rewritten assertions (rpc.rs:6751-6760, :6805-6810) are relabelled correctly: they previously pinned a hardcoded literal, so they were asserting the defect. a_fallback_answer_still_claims_neither_freshness_nor_a_height was rewritten to keep its real subject by putting the peers AHEAD of the replica, so an implementation leaking the replica's peak still fails. PEERS_PEAK = REPLICA_PEAK + 12 is deliberately distinct so a wrong-measurement implementation cannot pass by coincidence. No vacuous or self-satisfying assertions found.
What would turn this into a PASS
- Gate the bound on tier liveness —
tier.peer_count.and(tier.peak_height)(Finding 1). - Anchor on a corroborated height rather than the raw
fetch_max, using the quorum machinery this crate already has (Finding 2). - Stop
stale_bydegenerating tox - x: attribute the sub-tier, or bound only the peer-corroborated arms (Finding 3). - Reword the new SPEC provenance claim to match what the code actually measures (Finding 4).
(1) alone materially reduces the exposure and is a one-line change. (1)+(2) close the forgeable-warrant path. (3) is what keeps a stale oracle answer visible to a consumer. I will re-gate on the security leg only — the diff is confined to this crate, so a scoped fix does not need the full trio (§1.10).
Nothing here disputes that the previous state was harmful. Making the honest answer reachable is the right goal; it needs a bound an adversary or a partition cannot forge.
loop-decider — SHAPE DECISION for #510 / #290Fresh context, dispatched because three gates returned CHANGES-REQUIRED at Recommendation, firstOption (c), a narrower shape: reject the peers' high-water mark as the bound, and bound a fallback One line of justification: the bound must come from the party that produced the answer, and The bound already exists and needs no new crate. Failure direction of a wrong version of each option
Why (b) is rejected on its merits, not just on cost — and the hole in the proposed one-linerBoth the security gate and the adversarial gate proposed
A correct (b) needs §2.0 already-shipped check — measured, so nobody re-derives it
The contract, verified at 0.30.0 rather than at the stale checkout
Note the contract's stated REASON, because (c) is written to satisfy it rather than to override it: Release-first DAG for (c) — two links, not three
The doc sweep, exactly
Discriminating tests — the ones that fail an implementation reaching for the wrong measurement
The six fixturesThree the PR already moved ( The chia-query finding is filed, not chased hereThe unquorumed If the cascade is judged too heavy for phase 1Decided fallback, not a question: leave the withhold in place and fix the dig-app copy so it reads What closes #290A fallback-tier read carries the peak of the tier that answered it; |
|
Closing as not planned (orchestrator #290 stays OPEN. The fix proceeds on the narrower shape the decider chose: bound a fallback answer by the peak of the tier that actually answered it ( |
dig-node-control-interface 0.31.0 defines synced on a fallback answer exactly: it is true if and only if the reported peak_height is the height the tier that ANSWERED reported in the same read that produced the figures. Otherwise the answer reports peak_height: null and synced: false. Every fallback arm previously hardcoded false/null, so a node could never report a current chain-tier answer at all -- a caller bounding a claimed confirmation got nothing from the node that most needs to answer. The bound is produced by the answering tier, while it answers, in WalletBackend::fallback_answer_bound. That excludes all three heights within easy reach at these call sites, each a measurement of something else: the replica's peak (a local database this answer did not come from), the peer tier's high-water mark (the bound #510 proposed and #290 REFUTED -- a monotone fetch_max that outlives the peers that produced it, so a departed peer's height would be stamped onto a figure the oracle served), and any value carried over from an earlier read. An unobtainable bound is not a failed read: the figures are still SERVED, labelled unbounded. The two CACHED arms keep null/false unconditionally, because no live read produced those rows. control.wallet.peak is deliberately untouched. ChainPeak carries no source field, so the answering-tier rule cannot be expressed on it -- see DIG-Network/dig-node-control-interface#46. Closes #290 Co-Authored-By: Claude <noreply@anthropic.com>
dig-node-control-interface 0.31.0 defines synced on a fallback answer exactly: it is true if and only if the reported peak_height is the height the tier that ANSWERED reported in the same read that produced the figures. Otherwise the answer reports peak_height: null and synced: false. Every fallback arm previously hardcoded false/null, so a node could never report a current chain-tier answer at all -- a caller bounding a claimed confirmation got nothing from the node that most needs to answer. The bound is produced by the answering tier, while it answers, in WalletBackend::fallback_answer_bound. That excludes all three heights within easy reach at these call sites, each a measurement of something else: the replica's peak (a local database this answer did not come from), the peer tier's high-water mark (the bound #510 proposed and #290 REFUTED -- a monotone fetch_max that outlives the peers that produced it, so a departed peer's height would be stamped onto a figure the oracle served), and any value carried over from an earlier read. An unobtainable bound is not a failed read: the figures are still SERVED, labelled unbounded. The two CACHED arms keep null/false unconditionally, because no live read produced those rows. control.wallet.peak is deliberately untouched. ChainPeak carries no source field, so the answering-tier rule cannot be expressed on it -- see DIG-Network/dig-node-control-interface#46. Closes #290 Co-Authored-By: Claude <noreply@anthropic.com>
…ad (#514) * chore: open #290 lane on the 0.31.0 contract Co-Authored-By: Claude <noreply@anthropic.com> * feat(wallet): salvage the same-read peak bound for fallback-tier answers Recovered from a lane killed mid-work. Adds ChainFallback::answer_peak_height -- the peak the ANSWERING tier reports as part of the read it is serving -- and the T1-T6 red tests in sage/rpc.rs. Not yet wired into the answer path. Refs #290 Co-Authored-By: Claude <noreply@anthropic.com> * test(wallet): make the T4/T5 fixtures reach the assertions they were written for Both errored before asserting -- coin_spend composes the spend with the coin record and fails closed on a source reporting a spend of a coin its own record calls unspent, so neither test ever exercised the peak bound. They would have gone green when the fix landed, for reasons unrelated to the fix. The oracle fixture now holds two coins with distinct states, each needed by some arm to answer NON-EMPTY: c0 spent (so coin_spend composes) and u1 unspent (so the balance and coins arms serve a real figure rather than a zero and an empty page -- a bound travelling with an empty answer does not show it travels with a figure). T4's cached coin gains a spent height for the same reason. Also brings SPEC.md onto the 0.31.0 rule in four places: SS18.7b, the normative home, plus the balance, coinById and coinSpend method rows, all of which stated the superseded "a fallback answer always reports synced: false and peak_height: null". Refs #290 Co-Authored-By: Claude <noreply@anthropic.com> * fix(wallet): bound a fallback answer by the peak from its OWN read dig-node-control-interface 0.31.0 defines synced on a fallback answer exactly: it is true if and only if the reported peak_height is the height the tier that ANSWERED reported in the same read that produced the figures. Otherwise the answer reports peak_height: null and synced: false. Every fallback arm previously hardcoded false/null, so a node could never report a current chain-tier answer at all -- a caller bounding a claimed confirmation got nothing from the node that most needs to answer. The bound is produced by the answering tier, while it answers, in WalletBackend::fallback_answer_bound. That excludes all three heights within easy reach at these call sites, each a measurement of something else: the replica's peak (a local database this answer did not come from), the peer tier's high-water mark (the bound #510 proposed and #290 REFUTED -- a monotone fetch_max that outlives the peers that produced it, so a departed peer's height would be stamped onto a figure the oracle served), and any value carried over from an earlier read. An unobtainable bound is not a failed read: the figures are still SERVED, labelled unbounded. The two CACHED arms keep null/false unconditionally, because no live read produced those rows. control.wallet.peak is deliberately untouched. ChainPeak carries no source field, so the answering-tier rule cannot be expressed on it -- see DIG-Network/dig-node-control-interface#46. Closes #290 Co-Authored-By: Claude <noreply@anthropic.com> * docs(control): control.wallet.peak is no longer the only route to a height Its doc comment stated that a balance reports peak_height: null on EVERY fallback-tier answer, which was true of the pre-0.31.0 contract and is now false: a fallback answer carries the peak its own answering tier produced in that same read. Also records WHY this endpoint is deliberately untouched by #290 -- ChainPeak carries no source field, so the answering-tier rule cannot be expressed on it. Refs #290 Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
DO NOT MERGE — gate round in progress.
Closes #290
The defect
Seven
Source::Fallbackresult constructions incrates/dig-wallet/src/sage/rpc.rswrotesynced: false, peak_height: Noneas literals. Becausepeak_heightwasNone,control.rs'sstale_by(answer_height, network_peak)returnedNoneon every non-wallet-scoped read; and becausesyncedwasfalse, dig-app'sControlChainSource::absence_warrant(dig-app-core/src/chain/source.rs) withheld the warrant unconditionally. Both consumer-side freshness guards therefore degenerated into permanent refusals — which is what the SPEC's owncoinByIdrow already warns turns a mint watch into "the chain could not be reached" on a healthy node.The fix
One private helper,
WalletBackend::chain_tier_answer_height(), returnsself.chain_peer_tier().await.peak_height— cached peer-announcement state, no outbound round. The five arms that ACTUALLY CONSULTED the chain tier on the call derive BOTH fields from that singleOption<u32>:Some(h)givessynced: true, peak_height: Some(h)Nonegivessynced: false, peak_height: NoneThe pairing invariant, and where its single constructor lives.
{synced: true, peak_height: null}has no constructor anywhere on these arms: each of the five readslet bound = self.chain_tier_answer_height().await;and then writessynced: bound.is_some(), peak_height: bound. The two fields cannot disagree because they are the same value read twice.no_live_fallback_arm_pairs_a_claim_with_a_missing_boundasserts it over the ARMS rather than over one fixture, so a future arm computing them independently fails.Per-arm classification, re-verified from the code
balance_for_addresscoins_for_addresscoin_by_idcached_coin_record_by_idhitNonecoin_by_idcoin_record_by_idcoin_spendNonecoin_spendcoins_by_parentThe two CACHED arms keep
false/Noneand now say why in-line: those rows were taken at an earlier, unrecorded moment and are permanent by design (a spent coin's record is immutable), so stamping the current peer peak would claim a currency nothing measured — this same defect mirrored.Why this does not overstate
The bound is a real, concurrent measurement of the chain: a full node answered at its tip moments ago, and this node's own held peers independently name that tip at the same moment. It is not the latched-
initial_sync_completefailure — that flag can sit thousands of blocks behind (#416 measured 8,380); a full node's current view is a read, not a latch.Fail-toward-withheld is structural, not asserted:
ChainPeerTier::peak_heightisNoneuntil a held peer announces one, so a node with no chain view of its own keeps claiming nothing. There is no arm that can claim a bound it did not take.Stated honestly rather than talked down: the residual risk is a third-party oracle serving a materially stale answer while this node's peers sit at the tip, and nothing here detects that. It is weighed in the helper's rustdoc against the measured harm it replaces — a warrant no read could ever carry.
stale_bytypically becomes0on these reads.0is a POSITIVE claim ("nothing known puts this answer behind the network"), which is exactly what is meant and which that field's own null-vs-zero language already distinguishes.peak_heightandnetwork_peak_heightwill usually be the SAME number, becausecontrol.rstakes the network peak fromwallet_sync_status().chia_peer_peak_height— the sameChainPeerTier::peak_height. The helper's doc says so out loud: that is one measurement bounding both sides, not a tautology dressed as evidence.TDD
RED, against unmodified production code:
Both failures read
left: (false, None)/right: (true, Some(9140652)).GREEN, whole crate:
Four tests added: the warrant reachable through the field the consumer reads (a
coin_by_idfor a coin that does not exist — the ticket's own acceptance); the unobservable-tier withhold; the cached-hit withhold with a peer peak present (this one passes before AND after, and its doc says so — it is the over-application guard, not part of the RED); and the pairing invariant across three live arms under both tiers. The peer peak in the new fixtures isREPLICA_PEAK + 12, deliberately distinct from the replica's, so an implementation reaching for the wrong measurement fails rather than passing by coincidence.Blast radius
Done by
Grepplus direct read, NOT by gitnexus — its index for this repo is roughly 301 commits stale and returns a false-safeimpactedCount: 0. Nothing here cites a gitnexus zero.chain_tier_answer_heightis private and called from exactly the five live arms.crates/dig-node-service/src/control.rs:stale_byand the five*_wireserializers need no logic change. Its fixture tests construct the result structs directly and assert serialization, so a struct still carryingfalse/Nonestill serializes that way — none required updating. Verified by grep over"peak_height": nulland"synced": falsein that file.dig-wallettests DID move, all inrpc.rs, each updated with its reasoning in place and none deleted:a_coin_the_replica_does_not_hold_still_falls_through_to_the_chain— chose a fallback fixture precisely because those fields were inert; its subject (that the read MOVED tiers) is still carried byORACLE_AMOUNTandcall_count.a_coin_held_by_a_non_authoritative_replica_is_not_served_from_it— same; its subject is the AMOUNT, untouched.a_fallback_answer_still_claims_neither_freshness_nor_a_height— rewritten. The old assertion conflated "does not inherit the REPLICA's state" (still true, and the subject) with "is bounded by nothing" (no longer true, and never what it was for). Its peers-ahead-of-the-replica fixture is now what proves the bound comes from the peers rather than leaking from the replica.Wire contract — no release needed
Confirmed rather than assumed:
dig-node-control-interface0.30.0src/results.rsdeclarespub peak_height: Option<u32>on all five result types (lines 554, 704, 835, 913, 985). The field goes from always-null to sometimes-a-number — additive per §5.1, no contract change, no consumer break.SPEC.md
§18.7b's fallback paragraph rewritten normatively, and the
control.wallet.balance,.coinByIdand.coinSpendrows updated (.coinsand.coinsByParentdefer to those by reference and needed no separate clause). The existingstale_bynull-versus-zero language is intact — this change makes it operative. The.coinSpendrow's "always fallback / false / null" is corrected: the tier is still always"fallback", the other two are not.Boundary against PR #500 (issue #495)
Two changes, not one, and the relationship is worth stating precisely. #500 makes the wallet-scoped
syncStatusPHASE stop OVERSTATING, by refusing aSyncedthat carries no height. This makes the non-wallet-scoped coin reads stop UNDERSTATING, by computing asyncedthat was hardcoded. Both enforce the same invariant — a currency claim never travels without the height that bounds it — from opposite directions, on different endpoints, in different regions ofrpc.rs(#500 worksreplica_answer_is_currentand the Db arms; this works the Fallback arms). Nothing from #500 was branched from or adopted.sync_supervisor.rsis untouched.#490 / PR #492 (merged
adf03d8) putnetwork_peak_heightandstale_byon these reads; this is what makes them produce a number on the fallback tier. No second freshness field was added beside them.Scope correction taken mid-lane
The original brief said not to touch
synced. That was reversed after the consumer was measured: dig-app'sFreshnessstruct carries{source, synced, peak_height}and reads neitherstale_bynornetwork_peak_height, so populatingpeak_heightalone would have landed on nobody.syncedis the field this ecosystem actually uses as the absence warrant.Versions and gates
Root
0.254.0,dig-wallet0.49.0(main is 0.47.0; #500 reserved 0.48.0).Cargo.lockupdated by building.cargo clippy -p dig-wallet --all-targets -- -D warningsexit 0;cargo fmt -p dig-wallet --checkexit 0.cargo check -p dig-node-serviceexit 0. It first hit ENOSPC (os error 112, 6.2 GB free on this host) rather than a compile error; the lane freed its OWNtarget/debug/incrementalonly, never another repo's cache, and re-ran withCARGO_INCREMENTAL=0.