Skip to content

feat(wallet): bound a live fallback-tier answer by the peak its held peers announce - #510

Closed
MichaelTaylor3d wants to merge 3 commits into
mainfrom
loop/290-fallback-sync-honesty
Closed

feat(wallet): bound a live fallback-tier answer by the peak its held peers announce#510
MichaelTaylor3d wants to merge 3 commits into
mainfrom
loop/290-fallback-sync-honesty

Conversation

@MichaelTaylor3d

@MichaelTaylor3d MichaelTaylor3d commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

DO NOT MERGE — gate round in progress.

Closes #290

The defect

Seven Source::Fallback result constructions in crates/dig-wallet/src/sage/rpc.rs wrote synced: false, peak_height: None as literals. Because peak_height was None, control.rs's stale_by(answer_height, network_peak) returned None on every non-wallet-scoped read; and because synced was false, dig-app's ControlChainSource::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 own coinById row 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(), returns self.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 single Option<u32>:

  • Some(h) gives synced: true, peak_height: Some(h)
  • None gives synced: false, peak_height: None

The pairing invariant, and where its single constructor lives. {synced: true, peak_height: null} has no constructor anywhere on these arms: each of the five reads let bound = self.chain_tier_answer_height().await; and then writes synced: 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_bound asserts 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

method arm class peak_height
balance_for_address Fallback LIVE consult measured bound
coins_for_address Fallback LIVE consult measured bound
coin_by_id cached_coin_record_by_id hit CACHED stays None
coin_by_id live miss to coin_record_by_id LIVE consult measured bound
coin_spend both halves cached CACHED stays None
coin_spend live consult LIVE consult measured bound
coins_by_parent only arm LIVE consult measured bound

The two CACHED arms keep false/None and 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_complete failure — 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_height is None until 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_by typically becomes 0 on these reads. 0 is 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_height and network_peak_height will usually be the SAME number, because control.rs takes the network peak from wallet_sync_status().chia_peer_peak_height — the same ChainPeerTier::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:

test result: FAILED. 139 passed; 2 failed; 0 ignored; 0 measured; 633 filtered out; finished in 2.50s

Both failures read left: (false, None) / right: (true, Some(9140652)).

GREEN, whole crate:

test result: ok. 773 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in 15.82s

Four tests added: the warrant reachable through the field the consumer reads (a coin_by_id for 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 is REPLICA_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 Grep plus direct read, NOT by gitnexus — its index for this repo is roughly 301 commits stale and returns a false-safe impactedCount: 0. Nothing here cites a gitnexus zero.

  • chain_tier_answer_height is private and called from exactly the five live arms.
  • crates/dig-node-service/src/control.rs: stale_by and the five *_wire serializers need no logic change. Its fixture tests construct the result structs directly and assert serialization, so a struct still carrying false/None still serializes that way — none required updating. Verified by grep over "peak_height": null and "synced": false in that file.
  • Three dig-wallet tests DID move, all in rpc.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 by ORACLE_AMOUNT and call_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-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, no contract change, no consumer break.

SPEC.md

§18.7b's fallback paragraph rewritten normatively, and the control.wallet.balance, .coinById and .coinSpend rows updated (.coins and .coinsByParent defer to those by reference and needed no separate clause). The existing stale_by null-versus-zero language is intact — this change makes it operative. The .coinSpend row'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 syncStatus PHASE stop OVERSTATING, by refusing a Synced that carries no height. This makes the non-wallet-scoped coin reads stop UNDERSTATING, by computing a synced that 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 of rpc.rs (#500 works replica_answer_is_current and the Db arms; this works the Fallback arms). Nothing from #500 was branched from or adopted. sync_supervisor.rs is untouched.

#490 / PR #492 (merged adf03d8) put network_peak_height and stale_by on 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's Freshness struct carries {source, synced, peak_height} and reads neither stale_by nor network_peak_height, so populating peak_height alone would have landed on nobody. synced is the field this ecosystem actually uses as the absence warrant.

Versions and gates

Root 0.254.0, dig-wallet 0.49.0 (main is 0.47.0; #500 reserved 0.48.0). Cargo.lock updated by building. cargo clippy -p dig-wallet --all-targets -- -D warnings exit 0; cargo fmt -p dig-wallet --check exit 0.

cargo check -p dig-node-service exit 0. It first hit ENOSPC (os error 112, 6.2 GB free on this host) rather than a compile error; the lane freed its OWN target/debug/incremental only, never another repo's cache, and re-ran with CARGO_INCREMENTAL=0.

MichaelTaylor3d and others added 3 commits September 2, 2026 06:00
…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>
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security — IN PROGRESS, not the verdict

Audited head: 6a4c753cab39b1b0ea2fc47715aa9f89fd2ad51f (resolved from remote, matches worktree).

Posting this finding now rather than holding it to the verdict.

FINDING 1 (GATING, HIGH) — the peak that licenses synced: true has NO liveness gate, and the struct it comes from decays the OTHER field on purpose

crates/dig-wallet/src/sage/rpc.rs:1086-1088

async fn chain_tier_answer_height(&self) -> Option<u32> {
    self.chain_peer_tier().await.peak_height
}

This reads ChainPeerTier.peak_height and ignores ChainPeerTier.peer_count. Those two fields are
NOT equally trustworthy, and the code that produces them says so explicitly.

crates/dig-wallet/src/sage/chain.rs:114-142, PeerLiveness::observe:

ChainPeerTier {
    peer_count: fresh.then_some(count),
    // The peak is reported exactly as the peers gave it. It was already honest on the
    // fleet -- it froze when they died -- and it is documented as "what the peers announced"
    // rather than as a claim about now.
    peak_height: raw.peak_height,
}

peer_count is gated on fresh (chain.rs:131-133, a 180s PEER_LIVENESS_WINDOW,
chain.rs:60). peak_height is passed through with no expiry at all. Its own comment names
the exact state this PR now treats as a warrant: "it froze when they died".

The doc on the new method claims a property the code does not measure. rpc.rs:1042-1044:

"this node's OWN held Chia peers independently name that tip at the same moment. Two parties
that did not consult each other agree on where the chain is"

There is no "at the same moment" in the implementation. peak_height is the last value any peer
ever announced, at any point in the past, with no upper bound on its age. Some(h) is a pure
existence check on a value that never expires.

The concrete scenario — and it is a MEASURED one, not hypothetical

chain.rs:66-72 documents the fleet state this was found on (#3159): every Chia peer and all
outbound tcp/8444 blocked, HTTPS untouched. In that state:

  1. The pool ejects an entry only when a request routed to that peer FAILS, and
    Router::get_blockchain_state consults the coinset HTTP tier FIRST — so no request is ever
    routed to a peer, no peer is ever ejected, and the client keeps existing.
  2. peer_count correctly decays to None after 180s. The node reports "I cannot tell you how many
    peers I hold."
  3. peak_height stays Some(frozen_h) indefinitely.
  4. chain_tier_answer_height() therefore returns Some(frozen_h)synced: true on all five
    live fallback arms.

So the node reports, simultaneously, "I don't know if I have any peers" and "this absence is
warranted."
The one field that would have caught the partition is the field the new method drops.

Why this reaches money

coin_by_id returning coin: None with synced: true is what dig-app's ControlChainSource
requires to mint an AbsenceWarrant; chain_mint.rs turns that into a mint verdict, and
dig-account's mint_status concludes "this mint can never confirm" from an absent DID coin beside
a spent funding coin. A partitioned node whose reads have fallen through to the oracle — or are
failing — can therefore tell a user their mint permanently failed while it is merely pending.

stale_by does not catch it either, and the PR's own doc explains why without drawing the
conclusion (rpc.rs:1077-1085): control.rs takes network_peak from
wallet_sync_status().chia_peer_peak_height, which is this same un-decayed field. So
stale_by compares the frozen number against itself and yields 0 — a positive all-clear
("nothing known puts this answer behind the network") emitted by a node that has been network-
partitioned for an unbounded period.

Not equivalent to the existing use of the same field

replica_answer_is_current (rpc.rs:1149-1157) also reads .peak_height and ignores
peer_count — but it uses it as one side of a COMPARISON against the replica's independently
measured peak (is_following(replica_peak, peer_peak)). Two numbers must agree. The new arm has no
second measurement: Some(anything) alone flips the bool. That is strictly weaker than the
precedent it resembles.

What would make it not exploitable

The liveness signal already exists, in the same struct, on the same call — no new measurement, no
new egress:

async fn chain_tier_answer_height(&self) -> Option<u32> {
    let tier = self.chain_peer_tier().await;
    tier.peer_count.and(tier.peak_height)   // a peak only counts while the tier is CORROBORATED
}

peer_count is Some exactly when PeerLiveness confirmed the tier within the 180s window, which
is the "at the same moment" property the new doc already claims. This keeps every case the PR's own
tests exercise (peers_level_at sets both fields) and closes the frozen-peak path.

If that pairing is judged too strict, the alternative is to carry an observation instant on the
peak and reject a stale one — but the cheap fix above is already available and needs nothing new.

Continuing the audit: sub-tier attribution, the cached arms, rate limiting/amplification, and §908.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

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
this PR — mine, from the design comment on #290 — is false at the data source.

The defect

chain_tier_answer_height() (rpc.rs:1086) does not read a concurrent measurement. It reads a
monotone, untimestamped, never-reset high-water mark:

rpc.rs:1086chain.rs:548chia-query 0.20.0 peer/mod.rs:818peer/pool.rs:421

pub fn peak_height(&self) -> u32 { self.peak_height.load(Ordering::Relaxed) }

backed by peak_height: Arc<AtomicU32> (pool.rs:232), documented at pool.rs:170 as
"a fetch_max across every session", and updated from inbound NewPeakWallet messages with no
network call. Never decreasing. No age. Not cleared when peers are ejected or the pool empties
only when the whole ChiaQuery client is dropped.

This PR's own doc (rpc.rs:1055-1058) says:

"A full node's CURRENT view cannot [lag]: it is not a latch, it is a read."

It is a latch, and structurally a weaker one than initial_sync_complete — that at least clears
on a backwards chain move; this atomic clears on nothing. The PR replaces the #416 latched-replica
failure with a latched-peer-peak failure while asserting the opposite in prose.

The crate had already measured this exact state

chain.rs:69-75, the #351 PeerLiveness doc:

"On the #3159 fleet — every Chia peer and all outbound tcp/8444 blocked, HTTPS untouched — the
count sat at 5 for 180 s while the node could reach zero. The belief has unbounded age
precisely when the node is not using its peers."

Not using its peers is exactly the Source::Fallback path this PR modifies. And #351's remedy
deliberately gates only peer_count, passing peak_height through frozen (chain.rs:135-141)
on the stated grounds that it "is documented as 'what the peers announced' rather than as a claim
about now."
This PR takes that field and makes it a claim about now.

Reachable with no adversary: peers announce H → tcp/8444 blocked, HTTPS fine → reads route to
the fallback arms → after 180s peer_count honestly goes None, but peak_height stays Some(H)
indefinitely → every fallback read answers synced: true, peak_height: H, and since control.rs
takes network_peak from the same frozen value, stale_by = 0. A node holding zero peers reports
synced: true and "level with the network", from two frozen copies of one number.

Adversarial variant: fetch_max means the maximum any single peer ever announced wins permanently.
One unauthenticated NewPeakWallet pins the bound forever, with no quorum — the opposite of NC-12's
agreement across ~5 peers.

The sharpest framing: a polarity flip on a measurement already in use

rpc.rs:1153, inside replica_answer_is_current, already reads this same field — to accuse the
replica of being behind. A stale-high peak there fails toward synced: false. Safe.

This PR reuses the identical measurement inverted, to certify an answer current, where a
stale-high peak fails toward synced: true. Unsafe. Neither the code nor the SPEC notes the
flip.

That also answers the separability question I asked, and not in my favour: PR #500 rewrites
replica_answer_is_current, the one existing consumer of this field. The two PRs are the two
polarities of one measurement
, and only the safe one had been checked.

Also blocking, and also mine

  • SPEC.md now carries a born-false normative claim — it says a fallback answer reports "the
    height this node's own held Chia peers announce", which is not what a high-water mark is. Landing
    this makes the contract wrong, not merely the code.
  • The eighth site. chain_peak() at rpc.rs:2183 is a LIVE chain-tier consult with real
    rate-limited egress that still hardcodes synced: false. My brief's enumeration counted
    Source::Fallback struct constructions, not live consults, so it missed one — and the result is
    that on one node at one moment coin_by_id would say synced: true while control.wallet.peak
    says false. That endpoint is the one the PR's own deleted assertion pointed callers at.

What it attacked and could NOT refute — recorded so it is not re-litigated

  • Independence. Not a defect: the bound comes from the peers-only path while the oracle read is
    a different source (chain.rs:539-543). No self-vouching. The problem is age and monotonicity.
    This line of argument should be dropped, not pursued.
  • Test evidence. Every new test injects ChainPeerTier via with_chain_peer_tier_for_tests, so
    the fixture supplies a peak by construction and no test can observe that the production value is
    a stale fetch_max.
    The fail-toward-withheld control only proves the mapping None → false; it
    cannot see that production reaches None before the first announcement and never again. A fixture
    starting in a state production cannot durably occupy.
  • Whether dig-app has dormant logic silently activated — unread, left open.

Disposition

Not merging. Not undrafting. The minimum honest fix is to make the bound's liveness structural:
the peak may warrant an answer only while peer_count is Some — inside PEER_LIVENESS_WINDOW
which is the gate #351 already built and deliberately did not apply to this field. The unquorumed
fetch_max is a separate finding that lives upstream in chia-query and touches
replica_answer_is_current too; it will be filed rather than chased here.

Holding the fix round until the correctness and security gates return, so all findings land in one
pass — dispatching a fixer into a worktree two gates are still reading would break single-writer and
void their reads.

@MichaelTaylor3d MichaelTaylor3d left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. crates/dig-wallet/src/sage/chain.rs:566-575 folds the raw reading through PeerLiveness::observe.
  2. chain.rs:135-141 nulls only peer_count when 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". So peak_height is deliberately NOT gated on liveness.
  3. Underneath, chia-query 0.20.0 peer/pool.rs:232,421 holds the peak as a monotonic fetch_max AtomicU32. 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 testspeers_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 MichaelTaylor3d left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • :333 WalletCoinByIdResult::synced"always false on a [Source::Fallback] one"
  • :337 WalletCoinByIdResult::peak_height"None on a [Source::Fallback] one, where a caller bounding confirmations reads control.wallet.peak instead"
  • :373 WalletCoinSpendResult::synced"Always false"
  • :375 WalletCoinSpendResult::peak_height"Always None"
  • :397 / :399 WalletCoinsByParentResult — the same two sentences
  • :441-443 WalletBalanceResult::synced"a fallback answer reports false however caught-up the DB happens to be"
  • :452-453 WalletBalanceResult::peak_height"None for 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.

Comment thread SPEC.md
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`.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. "a balance reports peak_height: null on 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.
  2. "synced carries EXACTLY its control.wallet.balance meaning (§18.7b) and MUST be MEASURED by the same predicate", followed by "A chain-tier answer reports synced: false." — after this PR, control.wallet.balance on a chain-tier answer with a known peer peak reports synced: true while control.wallet.peak on the same tier at the same moment reports synced: 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);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-6060 a_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 surface Some(9_000_000); the two assertion MESSAGES now state a rule the code no longer follows.
  • crates/dig-wallet/src/sage/rpc.rs:7408-7409 an_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-7001 an_unspent_coin_answers_with_no_spend_rather_than_an_error — bare assert!(!result.synced) on the live coin_spend arm.

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.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security — IN PROGRESS, not the verdict (2 of 2)

Head 6a4c753cab39b1b0ea2fc47715aa9f89fd2ad51f. Continuing from the previous comment.

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 anchor

Traced chain_tier_answer_height to its producer, end to end:

rpc.rs:1087   chain_peer_tier().await.peak_height
chain.rs:552  peak_height: client.peer_peak_height().await
chia-query-0.20.0/src/lib.rs:359      observed_peak(self.router.peer.peak_height())
chia-query-0.20.0/src/peer/mod.rs:818 self.pool.peak_height()
chia-query-0.20.0/src/peer/pool.rs:421 self.peak_height.load(Relaxed)

and the only writer, chia-query-0.20.0/src/peer/pool.rs:884, inside the receiver task:

ProtocolMessageTypes::NewPeakWallet => {
    let Ok(new_peak) = NewPeakWallet::from_bytes(&msg.data) else { ... };
    last_peak.fetch_max(new_peak.height, Ordering::Relaxed);
    let prev = peak.fetch_max(new_peak.height, Ordering::Relaxed);

So the value is:

  • announced, never attested — a bare NewPeakWallet frame, decoded and taken. No proof of weight, no signature check, no cross-peer agreement;
  • set by ONE peerpool.rs:170 says so in its own words: "The pool's shared peak_height is a fetch_max across every session";
  • monotone and irreversiblefetch_max never lowers, for the life of the pool;
  • Some on any non-zero valueobserved_peak (lib.rs:683) filters only a literal 0.

This crate already documents that anchoring on this value is unsafe. crates/dig-wallet/src/sage/sync.rs:299-301, justifying PeakCeiling:

"Anchoring on the supervisor's chia_peer_peak_height would import the vulnerability being fixed: it is a monotone MAX over unverified claims, and quorum::eligible anchors on the MEDIAN precisely because a max is one-frame-pinnable."

chia_peer_peak_height is this field (sync_supervisor.rs:496,604chia_peer_peak_height: tier.peak_height). The sync family rejected it as an anchor and built a quorum-median PeakCeiling instead; this PR adopts it as the sole anchor for the absence warrant on five money reads. Both positions cannot be right.

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 NewPeakWallet frame with any non-zero height. Nothing about that peer is verified, nothing corroborates the frame, and the effect is immediate on all five arms. Before this PR the fields were literals, so no peer could influence them at all; after it, a single unauthenticated peer announcement is the whole gate on AbsenceWarrant -> chain_mint -> dig-account mint_status.

Attack B — one peer pins the peak permanently high (denial direction)

fetch_max means a single frame claiming a very large height raises the pool peak forever; the pool has no path to lower it. sync.rs:277-284 describes exactly this consequence for the writer path — "an inflated peak is effectively permanent" and it "permanently disables the two liveness guards". Post-merge that pinned value also becomes network_peak_height and the peak_height on every fallback money read.

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 peer_count has long since decayed to None.

The asymmetry that makes this sharpest (question 7)

For coin_by_id / coin_spend with peer_reads configured, the coin ANSWER is quorum-corroborated across peers (chain.rs:323-325 — a refused quorum is deliberately not overruled by the oracle). The WARRANT that licenses believing that answer is a fetch_max over one unverified frame. The bound is strictly weaker than the thing it bounds. An attacker who cannot forge the answer can still single-handedly manufacture the warrant that makes an absence believable.

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 peer_count.is_some() (Finding 1). A fetch_max over one frame is not a second opinion.

FINDING 3 (GATING, MEDIUM-HIGH) — stale_by becomes structurally always 0 on these arms, destroying the one field that could have caught the residual risk the PR names

crates/dig-node-service/src/control.rs:1645,3352,3383,3428,3450 all compute:

"stale_by": stale_by(r.peak_height, network_peak),

with network_peak from held_peers_peak(ctx) (control.rs:1678-1685) = wallet_sync_status().chia_peer_peak_height = tier.peak_height.

After this PR, r.peak_height on a fallback answer is that same tier.peak_height. So stale_by evaluates x.saturating_sub(x) and returns Some(0) on every fallback read, unconditionally. It is not "usually the same number" — it is the same expression, so the field carries zero information on these arms.

The PR anticipates the objection and rejects it (rpc.rs:1077-1085): "That is not a tautology dressed as evidence — it is one measurement bounding both sides". I do not think that survives stale_by's own stated contract (control.rs:1660-1668): the field exists to say how far the ANSWER sits behind the NETWORK, and "a missing network peak means... the node has nothing to measure itself against." When the answer's height IS the network peak, no comparison occurred. A consumer reading stale_by: 0 cannot distinguish "the answer is at the tip" from "nothing measured this answer at all." null said the second thing honestly; 0 asserts the first.

This is not academic, because three of the five arms can NEVER be peer-served:

arm fallback method routing
balance_for_address coin_records_by_puzzle_hashes chain.rs:717-722CoinsetFallback only
coins coin_records_by_hints chain.rs:724-728CoinsetFallback only
coins_by_parent coin_records_by_parent chain.rs:765-769CoinsetFallback 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 MichaelTaylor3d left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IN PROGRESS — finding 5, head 6a4c753. Cross-repo contract. Verdict next.

synced: false,
peak_height: None,
synced: bound.is_some(),
peak_height: bound,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 fallback answer MUST report synced false and peak_height null however 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"always false for a fallback answer" / "every fallback answer"
  • :910 / :912"false for 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 MichaelTaylor3d left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IN PROGRESS — finding 5, head 6a4c753. Cross-repo contract. Verdict next.

Comment thread crates/dig-wallet/src/sage/rpc.rs

@MichaelTaylor3d MichaelTaylor3d left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_height to chain_peer_tier, to fallback.peer_tier(), to chain.rs:548-575, to PeerLiveness::observe, to the chia-query 0.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::Fallback result construction in rpc.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 — 2183 is ChainPeak, a different type and a different endpoint. The partially-cached coin_spend case (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, and PEERS_PEAK = REPLICA_PEAK + 12 defeats 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_count and the AMOUNT assertion. The rewritten a_fallback_answer_still_claims_neither_freshness_nor_a_height is STRONGER than what it replaced: peers at REPLICA_PEAK + PEERS_AHEAD_BY mean 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.rs genuinely unaffected: CONFIRMED. Its fixtures (control.rs:5320,5413,5473,5522 with the wire assertions at 5339,5423,5496,5532) construct the result structs directly and assert serialization. A struct still carrying false/None still 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-231 gates absence_warrant on freshness.synced alone. 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> at results.rs:554,704,835,913,985, as stated. The type is fine; see finding 5 for the contract.
  • Versions: root 0.247.0 to 0.254.0, dig-wallet 0.47.0 to 0.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.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security VERDICT: CHANGES-REQUIRED

Audited head: 6a4c753cab39b1b0ea2fc47715aa9f89fd2ad51f (resolved from remote via gh pr view --json headRefOid; the read-only worktree at C:\tmp\worktrees\dn-290 is at the same SHA, tree clean, nothing mutated).

Read-only audit. No edits, no merge, no undraft. Diffed 476055fb..6a4c753c.

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 fetch_max over unverified single-peer announcements with no expiry. Neither "independently" nor "at the same moment" is enforced.

The fix is small and the evidence for it already exists in this crate. This is not a redesign.

Ranked findings

Full detail with the traces is in my two interim comments above; this is the ranked summary.

1. GATING / HIGH — the peak that licenses synced: true has no liveness gate, while the struct it comes from decays the other field on purpose

crates/dig-wallet/src/sage/rpc.rs:1086-1088 reads .peak_height and drops .peer_count. PeerLiveness::observe (crates/dig-wallet/src/sage/chain.rs:114-142) gates peer_count on a 180s window and passes peak_height through untouched, with the comment "it froze when they died".

Scenario (degraded network, and it is a MEASURED one — chain.rs:66-72, the #3159 fleet): all outbound tcp/8444 blocked, HTTPS untouched. Peers are never routed to, so none is ejected and the client survives. peer_count correctly decays to None; peak_height stays Some(frozen) indefinitely. The node then reports "I don't know if I hold any peers" and "this absence is warranted" in the same response, and keeps doing so for as long as it runs.

Impact: coin: None + synced: true is what dig-app's ControlChainSource needs to mint an AbsenceWarrant; chain_mint.rs turns that into a verdict and dig-account's mint_status concludes "this mint can never confirm." A partitioned node tells a user their mint permanently failed while it is merely pending.

Fix: tier.peer_count.and(tier.peak_height) — the liveness signal is already on the same struct, from the same call, at no cost.

2. GATING / HIGH — the anchor is a monotone MAX over UNVERIFIED single-peer claims, which a sibling module in this crate explicitly rejects as an anchor

chia-query-0.20.0/src/peer/pool.rs:884: peak.fetch_max(new_peak.height, Relaxed) on any decoded NewPeakWallet. Announced, not attested; one peer sets it; fetch_max never lowers; observed_peak filters only literal 0.

crates/dig-wallet/src/sage/sync.rs:299-301 already rules on this exact field: "Anchoring on the supervisor's chia_peer_peak_height would import the vulnerability being fixed: it is a monotone MAX over unverified claims, and quorum::eligible anchors on the MEDIAN precisely because a max is one-frame-pinnable." That is the same value (sync_supervisor.rs:496,604).

Attack A (licensing): dig-node dials strangers (NC-12). One hostile peer, one NewPeakWallet frame, and every live fallback arm becomes warranted. Composed with Finding 1, the peer can then disconnect and the warrant persists indefinitely.

Attack B (denial): one frame claiming a huge height pins the pool peak permanently — sync.rs:277-284 describes this as "effectively permanent" and as "permanently disabl[ing] the two liveness guards".

The escalation that matters most: a colluding-quorum forged absence was already possible (peer_reads.rs:263 notes required_agreement(2) == 2), but it previously carried synced: false and was inert. This PR makes the same forged absence actionable. That is the change in exploitability, and it is why this is gating rather than pre-existing.

Fix: anchor on the corroborated height this crate already computes — quorum / common_height, with PEAK_LAG_TOLERANCE — not the raw pool max. At minimum require corroboration by more than one session.

3. GATING / MEDIUM-HIGH — stale_by collapses to a structural 0 on these arms, removing the only downstream signal that could catch the residual risk the PR names

crates/dig-node-service/src/control.rs:1645,3352,3383,3428,3450 compute stale_by(r.peak_height, network_peak), where network_peak is held_peers_peak() = chia_peer_peak_height = the same tier.peak_height this PR now writes into r.peak_height. So it evaluates x.saturating_sub(x) and is Some(0) on every fallback answer.

No control-plane code changed in this diff — the field silently changes meaning because of the value it now receives. That is what makes it easy to miss.

Three of the five arms can never be peer-served, so their answer always comes from the third-party oracle while the bound comes from the peer tier:

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

  1. Gate the bound on tier liveness — tier.peer_count.and(tier.peak_height) (Finding 1).
  2. Anchor on a corroborated height rather than the raw fetch_max, using the quorum machinery this crate already has (Finding 2).
  3. Stop stale_by degenerating to x - x: attribute the sub-tier, or bound only the peer-corroborated arms (Finding 3).
  4. 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.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-decider — SHAPE DECISION for #510 / #290

Fresh context, dispatched because three gates returned CHANGES-REQUIRED at 6a4c753 and no decider had
posted. Read-only; no worktree taken in dn-290.

Recommendation, first

Option (c), a narrower shape: reject the peers' high-water mark as the bound, and bound a fallback
answer by the peak of the tier that ACTUALLY ANSWERED IT — ChainFallback::peak_height(), already on
the trait these five arms hold.
Close #510 as not planned (its classification work is correct and is
carried forward; its measurement is not). Reword #290 to that acceptance.

One line of justification: the bound must come from the party that produced the answer, and
ChainPeerTier::peak_height's own doc says it is not that party —
crates/dig-wallet/src/sage/fallback.rs:122, verbatim: "It is deliberately NOT the chain's peak as a
public oracle would give it."
No amount of liveness-gating repairs a measurement of the wrong thing,
and the PR body concedes the residual itself ("a third-party oracle serving a materially stale answer
while this node's peers sit at the tip, and nothing here detects that").

The bound already exists and needs no new crate. ChainFallback::peak_height()
(fallback.rs:248), implemented by CoinsetFallback at fallback.rs:340 via peak_height_opt(),
returns the answering oracle's own reported height. That is the same shape the contract already accepts
for a db answer — the answering view reports its own peak — and it makes stale_by non-tautological
for the first time, because network_peak_height can then come from the peers as a genuine independent
second opinion instead of being a second copy of the same frozen number.

Failure direction of a wrong version of each option

option fails toward verdict
(a) keep fail-toward-false UNDERSTATING#290's measured harm: absence_warrant withheld unconditionally, stale_by permanently None, a mint watch reading "the chain could not be reached" on a healthy node. Never claims unsettled money is settled. safe direction, real product harm. Viable as a phase-0, not as the answer.
(b) liveness-gated peer peak + contract change OVERSTATINGsynced: true on a node with no live chain view; stale_by = 0 from two frozen copies of one number; chain_mint.rs concluding a pending mint "can never confirm" after real XCH left the wallet. The money/custody-lie class §1.2 and §2.6 do not ship. rejected. Also the most expensive: it needs a chia-query change.
(c) answering-tier peak NoneChainFallback::peak_height returns Ok(None) for a tier that tracks no peak (EmptyFallback), which is exactly today's safe behaviour. adopted.

Why (b) is rejected on its merits, not just on cost — and the hole in the proposed one-liner

Both the security gate and the adversarial gate proposed tier.peer_count.and(tier.peak_height). That
does not close the finding
, and this is the load-bearing new fact in this decision:

ChainTransport::peer_tier builds the raw reading at chain.rs:551 as
peer_count: u32::try_from(client.peer_count().await).ok(). A live client holding zero peers
therefore reports Some(0), not None. The count CHANGING from 5 to 0 satisfies the alive test at
chain.rs:127-130, which re-arms confirmed_at, so fresh is true and peer_count stays Some(0)
for a further PEER_LIVENESS_WINDOW. The proposed gate passes, with zero peers held and a frozen
peak
— the same defect, 180 seconds narrower.

A correct (b) needs peer_count >= CORROBORATION_FLOOR (NC-12's quorum) and a median rather than a
fetch_max. The median exists — reference_peak — but it is a private free function at chia-query
src/peer/pool.rs:949, so (b) requires a chia-query release before anything else can move.

§2.0 already-shipped check — measured, so nobody re-derives it

  • chia-query latest published is 0.20.0 (index.crates.io), and origin/main is
    2f9cfde chore(release): v0.20.0 — main is the published version. peak_height there is still a
    bare peak_height: Arc<AtomicU32> (pool.rs:232) read at pool.rs:421, no timestamp, no reset.
    No liveness-gated or timestamped peak is shipped or staged anywhere. The adversarial gate's trace
    is current.
  • What chia-query 0.20.0 DOES already have, which a future ticket should use rather than build:
    per-peer last_peak that dies with the entry (pool.rs:177), lag eviction against the median
    (evict_lagging_peers, pool.rs:670), and CorroborationReadiness with Armed / Insufficient
    (pool.rs:214).
  • Both local checkouts are stale and must not be read as current: chia-query sits at 0.18.0,
    dig-node-control-interface at 0.24.0. Every claim above is from origin/main.

The contract, verified at 0.30.0 rather than at the stale checkout

dig-node-control-interface origin/main is 8cb778b chore(release): v0.30.0. src/results.rs:875
states the rule normatively — a fallback answer MUST report synced false and peak_height null
— and eight field docs repeat it (832,834,910,912,982,984,1074,1076). The reviewer's finding 5 is
correct: the PR verified the field TYPE and not its stated contract.

Note the contract's stated REASON, because (c) is written to satisfy it rather than to override it:
"because the oracle produced the figures and the replica neither produced them nor bounds their
freshness."
The objection is to a bound from a party that did not produce the answer. (c) takes the
bound from the party that did, so the amendment refines that rule rather than reversing it.

Release-first DAG for (c) — two links, not three

  1. dig-node-control-interface to 0.31.0. Amend the normative rule on the five result types plus
    the eight field docs: a fallback answer reports the peak of the tier that answered it, and synced
    MEASURED against that peak; false / null remain the honest answer for a tier that tracks no peak
    and for a CACHED row. Minor per §5.1 — additive semantics on an existing Option<u32>, no shape
    change, no consumer break. Publish and verify on the index before link 2 opens.
  2. dig-node. Adopt 0.31.0, and per §2.4b bring this crate's other dig-* / chia-* declarations
    to latest in the SAME PR. Implement the bound on the five live arms from
    ChainFallback::peak_height(); keep false / None on the two cached arms; sweep the docs, the
    SPEC and the tests below.
  3. No chia-query link. No chia-query release. (c) needs nothing from it.

The doc sweep, exactly

  • The eight field doc-comments in crates/dig-wallet/src/sage/rpc.rs at 333, 337, 373, 375, 397, 399, 441, 452 (reviewer finding 2 — the SPEC was swept, the crate rustdoc was not).
  • SPEC.md control.wallet.peak row (:5480) — its rationale and its "synced MUST be MEASURED
    by the same predicate" clause (reviewer finding 3, which is what makes it contradict §18.7b).
  • SPEC.md §18.7b's fallback paragraph, rewritten to the answering-tier rule. The adversarial gate
    is right that the current PR would land a born-false normative claim; under (c) the clause is true
    of the measurement actually taken.
  • The eighth site the adversarial gate found: chain_peak() at rpc.rs:2183. A live consult with
    real egress still hardcoding synced: false. Under (c) it takes the same treatment — its answering
    tier is the oracle, whose peak it already reads — which is what resolves the cross-endpoint
    contradiction (coin_by_id saying true while control.wallet.peak says false at the same moment
    on one node).
  • The helper rustdoc at rpc.rs:1042-1058: delete the "at the same moment" and "not a latch, it is a
    read" claims. They were true of nothing.

Discriminating tests — the ones that fail an implementation reaching for the wrong measurement

  • T1 (the ticket's own bar). A node that HELD peers and then LOST them must NOT report synced.
    Fixture ChainPeerTier with peer_count: None and peak_height: Some(h) — the state production
    reaches and which the current fixtures cannot construct, because peers_level_at /
    peers_unobservable vary both fields together (reviewer finding 1). Add a constructor that sets them
    independently; that gap is why the existing suite is blind to the defect. Also assert the
    zero-peers-but-fresh case above.
  • T2 (the discriminator). Oracle peak X, peers' peak X + 12; assert the emitted peak_height is
    Some(X). The mirror of the PR's own REPLICA_PEAK + 12 trick, and what fails an implementation that
    reaches for the peer tier.
  • T3. EmptyFallback, a tier tracking no peak, gives synced: false and peak_height: None.
  • T4. The two CACHED arms keep false / None with a live oracle peak present — the
    over-application guard. The PR already has this one; keep it and keep its note that it passes before
    and after.
  • T5. The pairing invariant across all live arms, retained from the PR.
  • T6. stale_by is NON-ZERO when the oracle lags the peers. This is the test that proves the field
    stopped comparing a number to itself.

The six fixtures

Three the PR already moved (a_coin_the_replica_does_not_hold_still_falls_through_to_the_chain,
a_coin_held_by_a_non_authoritative_replica_is_not_served_from_it,
a_fallback_answer_still_claims_neither_freshness_nor_a_height) plus the three the reviewer's finding 4
found — rpc.rs:6052, 7000, 7408 — plus rpc.rs:7986 from the dispatch brief. Each chose a
fallback fixture because those fields were inert; update each with its reasoning in place, delete none,
and preserve each one's actual subject. Separately, coins_for_address's live arm (rpc.rs:1751) has
no positive test of the bound and needs one.

The chia-query finding is filed, not chased here

The unquorumed fetch_max — one unauthenticated NewPeakWallet pins the pool's peak permanently, the
opposite of NC-12's agreement across ~5 peers — is real and belongs upstream. It is not gating,
because its only current consumer, replica_answer_is_current (rpc.rs:1153), uses it to ACCUSE the
replica of lag, where a stale-high peak fails toward synced: false. The fix is additive: expose
reference_peak publicly (chia-query 0.21.0) and have that call site use the median. File it against
chia-query, cross-linked both ways per §1.3; do not fold it into #290.

If the cascade is judged too heavy for phase 1

Decided fallback, not a question: leave the withhold in place and fix the dig-app copy so it reads
"not yet established" rather than "the chain could not be reached". That is a string fix with no
freshness claim in it. Worth saying plainly: a permanent "this mint can never confirm" minted from an
unbounded oracle answer is the dangerous claim, so #290's current withhold is partly correct behaviour
rather than purely a defect. (c) is still the right destination; this is what to ship first if the
two-link cascade must wait.

What closes #290

A fallback-tier read carries the peak of the tier that answered it; absence_warrant becomes reachable
on that basis; and a node with no live chain view still withholds. Evidence: T1 + T2 green, plus one
real dign read on a host showing a fallback answer carrying a number, per §2.6 — a green suite is not
the bar.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Closing as not planned (orchestrator e93b41, 19:14Z wake), per the loop-decider SHAPE DECISION posted above: the value this PR binds to (ChainPeerTier::peak_height) is a monotone fetch_max with no liveness or age, and its own doc says it is deliberately not the chain's peak as an oracle would give it -- liveness-gating cannot repair a measurement of the wrong thing, and the proposed peer_count.and(peak_height) gate does not close the finding either (Some(0) peers re-arms it for 180s). Three independent gates (reviewer, security, adversarial) reached CHANGES-REQUIRED at 6a4c753.

#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 (ChainFallback::peak_height()), via a two-link release-first DAG -- dig-node-control-interface 0.31.0 (amend the normative rule at results.rs:875 + the eight field docs) then dig-node adoption + the T1-T6 discriminating tests + the six fixtures. The branch loop/290-fallback-sync-honesty is kept as reference; do not reopen this PR.

MichaelTaylor3d added a commit that referenced this pull request Sep 2, 2026
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>
MichaelTaylor3d added a commit that referenced this pull request Sep 3, 2026
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>
MichaelTaylor3d added a commit that referenced this pull request Sep 3, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

routing.rs reports synced: false for every non-wallet-scoped read, so an absence can never be warranted

1 participant