feat(wallet): declare control.wallet.syncStatus, the wallet chain-sync view - #8
Conversation
Declares control.wallet.syncStatus: three-state phase + replica peak + CHIA peer count, kept distinct from the DIG gossip peer count. Co-Authored-By: Claude <noreply@anthropic.com>
…c view Adds an OPEN wallet chain read answering "is the wallet being kept current, how far has it got, and how many CHIA peers is it using?". The phase is a three-state enum, not a bool: "never started" and "synced to height 0" are different facts and a bool beside a height cannot express both. `synced` here requires a LIVE Chia peer connection as well as a completed catch-up, making it strictly stronger than WalletPeakResult::synced -- both types now state that relationship so the two same-named notions cannot drift. `peak_height` is the replica's own height or null and never falls back to the coinset oracle (control.wallet.peak deliberately does; that is a different question). `chia_peer_count` is the chain-sync peer count, explicitly not the DIG gossip count from control.peerStatus. Additive: a consumer that ignores the method parses unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
CORRECTNESS GATE - CHANGES-REQUIRED (head read: a159beb320232c68db87fd5a69f67c5670ee4e4d).
The builder is correct and unusually well-argued. I proved fourteen mutations load-bearing (each mutation grepped-confirmed before the run, restored from a file copy, git status --porcelain empty afterwards):
| mutation | caught by |
|---|---|
drop assert_coin_announcement on secondaries |
every_secondary_input_is_bound_to_the_lead + an_orphaned_secondary_input_is_refused_by_consensus_even_when_correctly_signed (fails at transfer.rs:1776) |
change - 1 |
2 unit tests |
prefix check to if false |
a_well_formed_address_with_a_non_xch_prefix_is_refused_by_prefix |
| drop the predates-push check | 1 unit test |
MIN_CONFIRMATION_DEPTH to 0 |
1 unit + 1 simulator test |
drop the payment-coin re-read in proof_of_death |
1 unit test |
peak None to 0 |
1 unit test |
| drop the self-payment refusal | 1 unit test |
| drop the unconfirmed/spent selection filter | 1 unit test |
is_some_and(is_spent) to is_some() |
1 unit + 1 simulator test |
| drop the input cap | 1 unit test |
| smallest-covering-coin to largest | 1 unit test |
drop reserve_fee |
7 of 8 simulator tests |
hint present to Memos::None |
4 simulator tests |
So the claimed fix in 3ab6ab2 to the orphaned-secondary test is real: with the binding deleted, the re-signed orphan subset is ACCEPTED by the simulator and the test goes red. That was the specific false green the commit set out to close, and it is closed.
Verdict on the two items flagged to me:
-
The swept-in
a_transfer_whose_change_is_off_by_one_is_refused_before_any_signaturerework - PASSES on its own merits. It pins theApprovedarm specifically (notis_ok()), it SIGNS the control as well as approving it, it pinsErrrather than toleratingRequiresConfirmation, and it pins the refusal stringvalue not conservedto the value-conservation guard rather than to any refusal at all. The comment names the real guard (analyzeviaDerivedSpend::derive,dig-wallet-backend-0.14.0/src/client/verify.rs:171) instead of crediting the signer. This is the strongest test in the diff. -
The
to_puzzle_hashdoc is factually ACCURATE. Every claim checks out: the prefix refusal exists and is load-bearing (proved above);nft,txch,did:chia,catandtotally-bogusreally do decode to the same 32 bytes (the test asserts that as its own control); andfrom_confirmedreally would reportConfirmedfor such a payment, since it checks only coin id, height and depth.
What blocks: the three open MUST-FIX threads (MF1, MF2, MF3), plus three further guards below in exactly the MF1 class - guards no test can distinguish from their own absence. MF1 found the first; it did not find the last. CI is fully green (8/8) at this head. Scope discipline, the section 908 boundary, and readable-code all pass.
NON-BLOCKING, adjudicated, no action needed (recorded here rather than as threads so they cannot block merge):
NON-BLOCKING (adjudicated, no action). wallet_sync_status has no default body, so this additive minor is source-breaking for an external ControlHandler implementor. I checked the precedent before ruling rather than ruling from first principles: 0.7.0 (af66f10) added wallet_coin_by_id with no default body under the same MINOR bump. Following the crate's established convention is correct — the alternative would silently give a node a stub for a method it must actually serve, which is worse than a compile error, and the only implementor in the ecosystem is dig-node. The SemVer bump stands.
NON-BLOCKING (observation, no action). chia_peer_count is disclosed on an OPEN read while the DIG peer count (control.peerStatus) is token-gated — a mild asymmetry. I do not think it changes the classification: a bare count of public Chia full-node connections identifies no user, reveals no address, balance or key, and is inseparable from the phase it disambiguates (without it syncing is dishonest). Recording it so the asymmetry is a decision on the record rather than an oversight.
Co-authored-by: MichaelTaylor3d <5665004+MichaelTaylor3d@users.noreply.github.com>
A DIG node is connected to two unrelated networks at once, and today the DIG count is reachable only as `connected_peers` -- a name that says nothing about which network -- in a payload whose liveliest number (`relay.peer_count`) counts something else entirely. A consumer that must KNOW which field means which network eventually picks wrong, and the failure is silent. control.peerCounts answers for both networks in one call, each count named for its own: `dig_peer_count` (DIG content/gossip, port 9445, the node's `connected_peers`) and `chia_peer_count` (Chia full nodes serving the wallet chain sync). Neither may be spelled as a bare peers/connected_peers/peer_count. `0` is an observed zero; `null` is unobservable -- a network that is not running is unknown, not zero. `chia_peer_count` is deliberately duplicated with control.wallet.syncStatus, where it is load-bearing beside the phase: both types now state that the two are ONE observation, that a conforming node serves them from one source, and that they must agree. control.peerStatus's docs gain a line warning that its `relay.peer_count` counts the RELAY's peers, not this node's. Co-Authored-By: Claude <noreply@anthropic.com>
… with a test The gate found two stale open-surface enumerations: SPEC.md §2.1 still said "the four wallet chain reads", and §4.2's exhaustive OPEN-read list omitted `.syncStatus`. The second is not cosmetic -- §4.2 is the rule that UNAUTHORIZED on an open read means UPGRADE THE NODE, so a client implementing the list as written would hit UNAUTHORIZED for syncStatus from any pre-0.8.0 node (the common case for a new method), fall through to the broadcast branch, and tell the user to fix their control token. Wrong remedy, which is what §4.2 exists to prevent. Both places are now reworded so they cannot drift again: the §4 table's Token column is declared authoritative and the open set is NAMED rather than counted. README's Authorization paragraph said every control.* is token-gated, which has been false since the wallet reads shipped, and its "every control method" reference table was missing the entire wallet group; both fixed, plus rows for control.peerCounts and a warning on peerStatus's relay.peer_count. Prose has no compiler, so `the_spec_and_readme_name_every_catalogued_method` now asserts MEMBERSHIP of every ControlMethod::ALL name in both documents -- a count would drift silently, an absent name fails the moment the catalog grows. Co-Authored-By: Claude <noreply@anthropic.com>
|
Both findings are fixed in
Swept further, since prose has no compiler: The sweep is now mechanized rather than repeated: Note on |
…restart state
Three findings from the pre-merge security audit. Docs only -- no wire, type or
gating change -- because crates.io is immutable and a doc edit now is cheaper
than a rollforward later.
`is_open_read`'s doc still called the whole set "chain reads" after
control.peerCounts joined it, so the two functions that jointly define the token
boundary told different stories, and the one an implementer reads when deciding
whether to gate was the wrong one. It now names both buckets and states the
actual membership test: does this disclose only already-public data, or a bare
count of this node's own state -- not "is it a chain read".
WalletSyncPhase::Synced concluded that the replica "is being KEPT current",
which its predicate does not establish: a live connection to a stalled or
lagging peer satisfies catch-up-complete AND peer-attached while the replica
goes stale. Softened to caught-up-and-connected, with the limit stated plainly.
The contract now rules on two field combinations. `{synced, peak_height:null}`
MUST NOT be emitted -- a node records its peak before marking catch-up complete,
so no conforming node can be in that state. `{not_started, peak_height:<n>}` is
the RESTART state and is explicitly PERMITTED: the height is persisted in the
wallet DB while the phase describes this process, so a node that synced earlier
and restarted reports it truthfully. Forbidding it would force a conforming node
to fabricate a phase or discard a height it genuinely has, which is the
dishonesty this method exists to prevent. Pinned by a golden vector.
Co-Authored-By: Claude <noreply@anthropic.com>
What changed
Declares a new OPEN wallet chain read,
control.wallet.syncStatus— the release-first contract half of DIG-Network/dig_ecosystem#2501. It answers: is the wallet's chain replica being kept current, how far has it got, and how many CHIA peers is it using?ControlMethod::WalletSyncStatus, categoryWallet, routingDelegated, open read (no token) alongside.balance/.coins/.coinById/.peak.control.wallet.broadcastremains the ONLY token-gated wallet method.WalletSyncStatusParams {}(empty, likeWalletPeakParams).WalletSyncStatusResult { phase: WalletSyncPhase, peak_height: Option<u32>, chia_peer_count: Option<u32> }, withWalletSyncPhaseserializing as exactlynot_started|syncing|synced.ControlHandler::wallet_sync_status+ its dispatch arm.SPEC.md: method-table row + a normativeWalletSyncStatusResultfield definition;WalletPeakResult's entry now states that itssyncedis the weaker notion.The four normative points the docs state
syncedmeans BEING KEPT CURRENT, not ONCE CAUGHT UP.phase == Syncedrequires the initial catch-up completed and at least one live Chia peer connection. A wallet that caught up yesterday and has been offline since reportssyncing. This is strictly stronger thanWalletPeakResult::synced(a completed-catch-up flag only) — stated in BOTH types' docs so the two same-named notions cannot drift apart.peak_heightnever falls back to a third-party oracle. It is the replica's own height ornull.control.wallet.peakdeliberately DOES fall back to the coinset oracle; that answers "what height is the chain at", not "how far has this replica got".chia_peer_count: 0is the disambiguator, not a fourth phase. A running sync connected to nothing issyncing+0, which a consumer SHOULD render as "syncing — no peers".null= the node cannot observe the count.control.peerStatus'sconnected_peers/relay_peer_count; unrelated numbers. Also explicitly notcontrol.sync.status, which is §21 DIG store sync.Plus: the height is the last existing block the peer view reported (
NewPeakWallet.height/RespondPuzzleState.height); no confirmation-depth arithmetic happens here — dig_ecosystem#2483 records thatpeak_height's meaning differs between a simulator (next height) and a full node (last existing), so a consumer computing depth must floor its own input.Wire shapes (verbatim, for the dig-app lane)
{"phase":"not_started","peak_height":null,"chia_peer_count":0} {"phase":"syncing","peak_height":4000000,"chia_peer_count":3} {"phase":"synced","peak_height":5000000,"chia_peer_count":5}Unobservable peer count:
{"phase":"syncing","peak_height":null,"chia_peer_count":null}Request:
{"jsonrpc":"2.0","id":1,"method":"control.wallet.syncStatus","params":{}}How verified
TDD: KATs written first, confirmed red (compile failure naming the missing items), then implemented to green. New/extended tests:
kats::golden_request_vectors— pins the wire namecontrol.wallet.syncStatus+params:{}.kats::golden_response_result_vectors_are_byte_stable— all three phases round-trip, plus thenullheight andnullpeer-count cases.kats::the_wallet_sync_phase_tokens_are_the_snake_case_wire_spellings— the three tokens pinned as literals, so renaming a Rust variant cannot silently change what a consumer parses; also pins the set at exactly three.kats::never_started_is_distinguishable_from_synced_at_height_zero— the fixture holds the height at 0 (the one value a bool-plus-height shape collapses) and varies only the phase, so the nearest wrong shape (synced:false+peak_height:0) cannot pass.kats::the_dispatcher_routes_each_wallet_chain_method_to_its_own_handler— the mock returns a phase and a heightwallet_peaknever produces, so an arm mis-wired tocontrol.wallet.peakfails.method::tests::the_token_less_surface_is_exactly_the_bootstrap_plus_the_chain_reads— fixture extended to seven named open methods.method::tests::the_push_is_the_one_wallet_method_behind_the_token— existing fixture EXTENDED (not duplicated); it now also proves the new method did not land behind the token.method::tests::delegated_set_matches_the_engine_surface— fixture extended.Load-bearing proof (revert-only, committed first): flipping
rename_allto"lowercase"and droppingWalletSyncStatusfromis_open_readfailed exactly 4 tests —the_wallet_sync_phase_tokens_are_the_snake_case_wire_spellings,golden_response_result_vectors_are_byte_stable,the_push_is_the_one_wallet_method_behind_the_token,the_token_less_surface_is_exactly_the_bootstrap_plus_the_chain_reads— and nothing else. Restored; 74 lib tests + 1 doc-test green.Green locally:
cargo fmt,cargo clippy --all-targets -- -D warnings,cargo test(74 passed / 0 failed, plus 1 doc-test). Compilation confirmed (not a stale-fingerprint "Finished").Blast radius checked
Per-worktree gitnexus indexing is gated (CLAUDE.md §2.0 override), so the radius was established by exhaustive grep for
WalletPeak— the structurally parallel method — plus a direct read of every exhaustive list. Every site extended:method.rs(enum,name,is_open_read,routing,category,summary,ALL, plus two literal test fixtures),params.rs(control_call!),results.rs,traits.rs(trait method + dispatch match),kats.rs(mock handler + vectors). The compiler's exhaustive matches confirm no site was missed. Risk: LOW — purely additive.Nothing existing changed shape.
WalletPeakResultandcontrol.sync.statusare untouched at the wire level; onlyWalletPeakResult's rustdoc/SPEC prose gained the cross-reference. A consumer that ignores the new method parses unchanged.Bump rationale
minor, 0.7.0 → 0.8.0. New capability, fully additive: a new enum variant on a
#[non_exhaustive]enum, new params/result types, and a newControlHandlermethod. The trait method has no default body, so a downstream implementor ofControlHandlermust add it — but this crate's implementors are the dig-node server (landing in the sibling PR) and mocks, and the contract's own convention is that adding a method is a minor.Not done here
The dig-node server side lands separately and consumes the published 0.8.0.
DRAFT — the gate round has not run.
Delta 2 —
control.peerCounts, one count per networkA DIG node is connected to two unrelated networks at once, and the DIG count was reachable only as
connected_peers— a name that says nothing about which network — in a payload whose liveliest number (relay.peer_count) counts something else. A consumer that must KNOW which field means which network eventually picks wrong, and the failure is silent.control.peerCounts→{ dig_peer_count: Option<u32>, chia_peer_count: Option<u32> }. Empty params, open read, categoryPeers, routingDelegated.dig_peer_count— DIG content/gossip network (port 9445); the node MUST source it from dig-node-core'sconnected_peers, the same figurecontrol.peerStatusreports. Stated in the docs so the node implementer has no choice to make.chia_peer_count— Chia full-node peers the wallet chain sync holds.Some(0)= observed zero,null= unobservable. A network that is not running is UNKNOWN, never zero.peers/connected_peers/peer_count.control.peerStatus's description gains a line: itsrelay.peer_countcounts THE RELAY's peers, not this node's, and is never the answer to "how many peers does this node have". Doc line only — the field's shape is dig-node-core's.Wire shapes (verbatim)
{"dig_peer_count":6,"chia_peer_count":3} {"dig_peer_count":0,"chia_peer_count":0} {"dig_peer_count":null,"chia_peer_count":null} {"dig_peer_count":6,"chia_peer_count":null}Request:
{"jsonrpc":"2.0","id":1,"method":"control.peerCounts","params":{}}The duplicated
chia_peer_countis intentionalIt stays on
WalletSyncStatusResultbecause it is load-bearing there —chia_peer_count: 0besidesyncingis the honest "syncing — no peers" state, and a phase separated from its count reads as a contradiction. A DIG content-network count is not a wallet fact, so it is absent from that type rather than added for symmetry. The contract closes the duplication: both types' docs andSPEC.mdstate the two are the SAME observation, that a conforming node MUST serve them from one source, and that they MUST agree within a single node's view; each method cross-references the other in both directions.Delta 3 — the gate's SPEC findings, and the sweep generalized
Both CHANGES-REQUIRED findings fixed, and reworded so the class cannot recur:
Tokencolumn authoritative and NAMES the open set instead of counting it..syncStatusfrom its OPEN branch. Added there (not the gated branch), withcontrol.peerCounts, same non-counting phrasing.Swept beyond what was asked, since the same drift was elsewhere:
README.md's Authorization paragraph claimed everycontrol.*is token-gated (false since the wallet reads shipped), and its "every control method" reference table was missing the entire wallet group. Both fixed, plus thepeerCountsrow and therelay.peer_countwarning.Lines checked for the sweep —
SPEC.md: §2.1 authorization bullet (:36-41), the pairing-bootstrap bullet (:44), the §4 method table's wallet + peers rows (:99-105), the open-surface paragraph under it (:110-115), §4.2's open-read branch (:341-346), and the-3204xwallet error table (:401-410, correct as-is — it describes wallet READ failures and the two new methods add no codes).README.md: Authorization (:43-50), the Peers table (:107-114), the new Wallet table, and the error-code section (unchanged). Every otherSPEC.mdhit forfour|five|six|seven|eight|chain read|open read|OPENwas inspected and is unrelated (e.g. "three of dig-gossip's four handshake send sites", "a fourth phase").Mechanized so the next method cannot repeat this:
kats::the_spec_and_readme_name_every_catalogued_methodasserts everyControlMethod::ALLwire name appears in BOTH documents. Membership, not a count.New tests (delta 2 + 3)
kats::each_peer_count_key_names_its_network— the two wire keys pinned as LITERALS plus the exact emitted bytes; the fixture gives the counts different values so a transposition fails.kats::both_results_spell_the_chia_count_with_the_same_key— the twochia_peer_countfields share one key, which is what makes the "one observation" obligation expressible and comparable at all.kats::an_unobservable_count_is_null_not_zero_on_either_network— varies ONE count at a time against a truthful control, so a collapse of absent-into-zero on either field alone is caught.kats::the_spec_and_readme_name_every_catalogued_method— the doc-drift gate.kats::golden_request_vectors/golden_response_result_vectors_are_byte_stable/the_dispatcher_routes_each_wallet_chain_method_to_its_own_handler— extended.method::tests::the_token_less_surface_is_exactly_the_bootstrap_plus_the_chain_reads/delegated_set_matches_the_engine_surface— fixtures extended (the open surface is now eight named methods).Load-bearing proof (committed first; mutate + run chained in one invocation; restored from file copies, never
git checkout):dig_peer_count/chia_peer_countviaserde(rename)each_peer_count_key_names_its_network,both_results_spell_the_chia_count_with_the_same_key,an_unobservable_count_is_null_not_zero_on_either_networkPeerCountsfromis_open_readthe_token_less_surface_is_exactly_the_bootstrap_plus_the_chain_readscontrol.peerCountsthroughout SPEC.md + README.mdthe_spec_and_readme_name_every_catalogued_methodDelta 1's proof stands:
rename_all = "lowercase"+ droppingWalletSyncStatusfromis_open_readfailed exactly 4 tests. Nothing unexpected failed in any run; all restored, green.78 lib tests + 1 doc-test pass.
cargo fmt --checkandcargo clippy --all-targets -- -D warningsclean.Version
Stays 0.8.0 — still one unreleased minor; this adds to a version that has not shipped.
Single-writer was broken by a bot
32e4118is a@copilotpush to this lane branch. Its change is a strict subset of delta 3 (it bumped "four" to "five" — the drift-prone phrasing this pass removes). I did not rewrite pushed history; my commits are rebased on top of it. The branch is therefore multi-author, so the gate verdict must be re-run over the combined delta, and any required contexts the bot commit left ataction_requiredneed asserting BY NAME rather than read off the rollup.Still DRAFT.
Delta 4 — the security audit's three findings (docs only)
Security PASS at
acd0641; these are the three fixes it asked for. No wire, type or gating change — every KAT and every golden vector is unchanged except one addition. Docs, because crates.io is immutable and an edit now is cheaper than a rollforward.1.
is_open_read's doc described a set it no longer holds. It still called the whole membership "chain reads" aftercontrol.peerCountsjoined;requires_authhad already been given a distinct third bucket, so the two functions that jointly define the token boundary told different stories — andis_open_readis the one an implementer reads when deciding whether to gate. It now names both buckets and states the actual membership test: does this disclose only already-public data, or a bare count of this node's own state? — explicitly not "is it a chain read", because a future method judged against the narrower phrasing would find it already false of a member and widen the predicate by analogy.2.
WalletSyncPhase::Syncedconcluded more than its predicate establishes. It said catch-up completed AND a peer is live, "so the replica is being KEPT current". A live connection to a stalled or lagging peer satisfies that while the replica goes stale, so the "so" does not follow. Now: caught up and connected — that nothing KNOWN prevents freshness — with an explicit statement that this is not a freshness guarantee and that a consumer needing actual freshness must comparepeak_heightagainst something. Same softening inSPEC.mdand inWalletPeakResult's cross-reference.3. Field combinations — ONE prohibition, ONE explicit permission.
{phase:"synced", peak_height:null}MUST NOT be emitted. A node records its peak before marking catch-up complete, so a completed catch-up always has a height behind it; no conforming node can be in that state and a consumer has no honest reading for it.{phase:"not_started", peak_height:<n>}is explicitly LEGITIMATE and MUST be permitted. This is the RESTART state: the height is persisted in the wallet DB while the phase describes whether a sync is running in this process, so a node that synced yesterday and just restarted reports it truthfully — here is the height I reached, and no sync is running right now. Forbidding it would force a conforming node to fabricate a phase or discard a height it genuinely has, which is the dishonesty this method exists to prevent.SPEC.mdnames the restart scenario so no implementer reads silence as a defect.No
validated()added.Test
One new golden vector —
{"phase":"not_started","peak_height":4900000,"chia_peer_count":0}— pinning that the restart state stays expressible.Load-bearing proof: a value-only mutation of that vector does NOT fail it, and could not —
assert_result_round_tripsasserts decode-then-re-encode identity, so any height survives. The nearest wrong implementation is not a wrong number, it is the validator the audit originally suggested, so that is what I mutated in: a hand-writtenDeserializeforWalletSyncStatusResultrejectingnot_started+ a height.golden_response_result_vectors_are_byte_stableFAILED on it (kats.rs:57, the decode assertion). Restored from a file copy; 78 lib + 1 doc-test green,fmt --checkandclippy -D warningsclean.Head
5171dd3— final. Still 0.8.0, still DRAFT, nothing merged, tagged or published.