fix(platform-wallet): stop the startup sequence reporting integrity it did not establish - #4426
fix(platform-wallet): stop the startup sequence reporting integrity it did not establish#4426bfoss765 wants to merge 2 commits into
Conversation
…t did not establish Three defects on the bring-up path introduced or left open by #4359, all of which end the same way: `start_wallet_subsystems` returns a status that promises a contact's DIP-15 addresses exist before Core SPV starts, when they do not. An address the wallet is not watching when the compact-filter scan passes its funding height produces no transaction at all, so each of these is a silent data gap rather than a cosmetic mislabel. 1. A contact pass that reached nobody was recorded as a completed sync. `sync_contact_requests` is log-and-continue per identity — correct for the recurring sweep, and it collapsed "Platform answered, nothing new" into the same `Ok(vec![])` as "Platform answered nobody". With DAPI unreachable the sweep returned an empty success, startup called `record_sync_ran`, and `status()` reported `Ready`. The pass now reports what it reached. `sync_contact_requests_reporting` returns a `ContactSyncReport` carrying the per-identity failure set; `sync_contact_requests` keeps its shape and raises `ContactSyncUnreachable` when nothing at all was read. Startup records the sync only on a complete pass, so a degraded one stays `PartialAccountsPending`. Failures retry themselves: a failed fetch leaves that direction's high-water cursor unadvanced, so the next sweep re-requests exactly the range it missed. 2. A partial identity scan was never retried once any identity was on file. `ScanTally::is_trustworthy` is `identities_seen > 0 || failed_probes == 0`, so a scan that saw index 0 and got no answer at index 1 returns `Ok`; the warm-launch shortcut then skipped discovery on every later launch, and nothing recorded that the scan had been partial. The second identity, and all of its contacts, stayed invisible for the life of the installation. A scan now publishes a verdict — complete, or the indices it could not answer — which the shortcut consults. Two things follow: a partial scan is retried inside its own launch, with the scan key already resolved and within the budget the caller granted; and the verdict rides the changeset so a host that persists it re-opens the question on the next launch. Absence of a verdict deliberately reads as "unknown", not "incomplete", so hosts that have not adopted the field keep the shortcut instead of paying for a scan plus a Keychain round trip before every Core SPV start. The budget-expiry path records the same verdict, since a scan dropped mid-await never reaches its own bookkeeping. Refs #4365. Not closed: no persister vtable carries the field yet, so cross-launch retention still needs the host slot. 3. The seed-binding gate existed only in the Swift wrapper. Everything the drain does with key material is unauthenticated, and `register_contact_account` keys its existence check on `(index, us, them)` rather than on the xpub — so a provider resolving the wrong seed writes contact receiving addresses once, and every later correct-seed pass no-ops. The corruption is permanent and its only symptom is payments that never arrive. iOS gated this in Swift; a JNI binding added later would have inherited the ungated path. The shared sequence now verifies the binding itself, immediately before the drain and only when something is actually queued, so a warm launch with an empty queue still resolves no key material. It fails closed on any error — a provider that cannot answer has not been shown to own the wallet — and skipping costs nothing unrecoverable, because the queue is left intact for the next signer-present drain. Reported as the new `SeedBindingUnverified` status rather than raised, since Core sync must start regardless. Also fixes a latent misreport the rescan path exposed: the discovery-failure statuses claim the identity question is open, and a rescan can now reach them with an identity already on file. They are gated on `identity_id.is_none()`. 20 new tests, including three that drive the real `start_wallet_subsystems` over a mock SDK: a wrong-seed provider registers no contact account and leaves the queue intact, the owning seed drains, and an empty queue never consults the gate at all. The mock's failing fetches reproduce the DAPI-unreachable case for (1) end to end. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 12 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🔍 Review in progress — actively reviewing now (commit c968b30) |
…ardening Follow-up to bad2093 on this branch. Each of these is the same failure the original commit set out to fix, surviving on a path it did not cover. 1. The seed-binding gate still had an ungated entry point. The gate landed in `start_wallet_subsystems`, but `platform_wallet_drain_pending_contact_crypto` — the FFI the JNI binding calls — went straight to `drain_pending_contact_crypto` and `drain_auto_accepts` with no check at all. A JNI client draining with a wrong-seed resolver therefore wrote contact receiving accounts from the wrong seed, permanently: `register_contact_account` keys its existence check on `(index, us, them)` and not on the xpub, so no later correct-seed pass revisits them. That is the exact defect the commit message said was closed, still reachable from the entry point it named as the reason to close it. The gate moves into `PlatformWallet::drain_pending_contact_crypto_verified` — one primitive that verifies, then runs both drains — and the startup sequence and the FFI now both drain through it. Behaviour is unchanged on each: the check is still skipped when nothing is queued (a warm launch resolves no key material), still fails closed on every verification error and not only on a mismatch, and still leaves the queue intact. The FFI reports a refusal as `ErrorInvalidParameter` for `SeedMismatch` — the same code the standalone verify already returns, so a host recognises the wrong-seed condition identically however it arrives — and `ErrorWalletOperation` for a provider that simply could not answer. 2. A known-incomplete identity scan could still report `Ready`. `StartupTally` had no way to say "an identity is known and the set it belongs to is not". The discovery signals are gated on `identity_id.is_none()` (correctly — a rescan reaches them with an identity on file), so a launch whose rescan was forced by an incomplete verdict and then failed fell through every check to `Ready`: the status that promises the identity set is settled, on the one launch that knows it is not. `identity_scan_incomplete` is recorded from the verdict on record once discovery is done, and `status()` returns the new `IdentityScanIncomplete` rather than `Ready`. Reading the verdict rather than this call's discovery counters is what makes it correct on the launches that have no counters to read — a warm shortcut, or a rescan abandoned before it started — and it catches the same defect reached from the other side, a first scan that came back partial. The check is ranked last, so the only run whose status changes is the one that used to lie; every other run keeps a status clients already handle and reads the flag on the outcome. `discovery_worth_retrying` covers the new status: the unanswered indices are exactly what another scan could answer. The test that pinned this, `a_failed_rescan_does_not_reopen_a_settled_ identity`, asserted `Ready` for precisely the incomplete-rescan case. Its real subject — a failed rescan must not re-open an identity already on file — is preserved and now sits alongside an assertion that the scan gap IS reported, under a name that says so. 3. A local fault mid-scan published no verdict at all. `publish_scan_verdict` has one call site, below four `?` early returns in `discover_inner` (breadcrumb derivation, the wallet-info lookup, `add_identity`, `add_keys`). A persistence write that failed, or a wallet that left the manager, therefore abandoned the walk part-way through the index space and recorded nothing — and "unknown" is what keeps the warm-launch shortcut armed. Worse, when a previous scan had recorded a COMPLETE verdict, that stale verdict survived the abandoned scan. #4365's shape, on the local-fault path. The scan body now runs in a block whose result is carried out, so no `?` inside it can skip the publish — including any added later. The index the walk died on is recorded as unanswered first, because a verdict built from the probe bookkeeping alone would see an empty failed-index list and publish an abandoned scan as complete, which is strictly worse than publishing nothing. The abort index is tracked apart from `failed_indices` so `failed_probes` and `IdentityDiscoveryIncomplete` keep meaning "probes Platform never answered"; the verdict merges the two, since to a later launch they are the same fact. Note the scan loop is re-indented by one level and not otherwise touched — `git diff -w` shows the real change. The FFI outcome struct gains `identity_scan_incomplete` and the status enum appends `IdentityScanIncomplete = 6`; the Swift mirror follows both, as the `SeedBindingUnverified` append did. 11 new tests. The gate: a wrong-seed provider is refused with the typed error and registers zero contact accounts while the queue survives, the owning seed drains, an empty queue never consults the provider. The scan signal: both directions at the tally level, plus the wire-up driven through the real `start_wallet_subsystems`. The verdict: a real local fault injected mid-scan (a seedless wallet against the resident-key derive) replaces a stale complete verdict with an incomplete one, so the next launch re-scans. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed c968b30 addressing the three findings. All are the same failure this PR set out to fix, surviving on a path the first pass didn't cover. 1 — the seed-binding gate had an ungated entry point. The gate landed in Rather than add a second copy of the check, I moved it into 2 — a known-incomplete scan could still report
I have to flag that my own test was pinning the defect. 3 — a local fault mid-scan published no verdict. The scan body now runs in a block whose result is carried out, so no One review note on that file: the loop body is re-indented one level and not otherwise touched — The FFI outcome struct gains 11 new tests. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4426 +/- ##
============================================
- Coverage 87.74% 87.74% -0.01%
============================================
Files 2681 2681
Lines 342632 342632
============================================
- Hits 300658 300657 -1
- Misses 41974 41975 +1
🚀 New features to boost your workflow:
|
2 participants
Three defects on the wallet bring-up path, all of which end the same way:
start_wallet_subsystemsreturns a status that promises a contact's DIP-15 addresses exist before Core SPV starts, when they do not. An address the wallet is not watching when the compact-filter scan passes its funding height produces no transaction at all — so each of these is a silent data gap, not a cosmetic mislabel.Two come from the automated review of #4359 (findings F1 and F2); the third is the follow-through on a deferral #4368 took deliberately.
1. A contact pass that reached nobody was recorded as a completed sync (F1)
sync_contact_requestsis log-and-continue per identity — right for the recurring sweep, and it collapsed two opposite endings into one return value. "Platform answered, and there is nothing new" and "Platform answered nobody" both arrived asOk(vec![]). With DAPI unreachable every identity's fetch hit thecontinue, the sweep returned an empty success,startup.rscalledrecord_sync_ran, andstatus()reportedReady.Readyis precisely the claim that a contact pass completed, so this fed the persistence-corruption class the audit was tracking: SPV starts against an address set that is silently short.Fix. The pass now reports what it reached rather than only what it found:
sync_contact_requests_reportingreturns aContactSyncReportcarryingidentities_attempted, the identities nothing was ingested for, and the identities whose sent side alone failed.sync_contact_requestskeeps its shape and raisesContactSyncUnreachablewhen there were identities to read and not one was read.dashpay_sync_ran = false, sostatus()staysPartialAccountsPending.Partial passes keep sensible semantics: what was fetched is real and stays persisted, and the failures retry themselves — a failed fetch leaves that direction's high-water cursor unadvanced, so the next sweep re-requests exactly the range it missed. A partial pass is still not complete, because the identities it missed have contact requests nobody looked at and account builds nobody enqueued.
The recurring sweep at
dashpay_sync.rsalready logs-and-continues on anErrfrom this call, so the new error is a strict improvement there too: a total outage used to be recorded as a successful sweep.2. A partial identity scan was never retried once any identity was on file (F2 = #4365)
ScanTally::is_trustworthyisidentities_seen > 0 || failed_probes == 0, so a scan that saw index 0 and got no answer at index 1 returnsOk— correctly, since discarding what it found would be worse. Butstart_wallet_subsystemsskipped discovery whenever any identity was on file, and nothing recorded that the scan had been partial. There was no next scan. The second identity, and every contact hanging off it, stayed invisible for the life of the installation.Fix. A scan now publishes a verdict — complete, or the specific indices it could not answer — and the shortcut consults it. Two things follow:
PlatformWalletChangeSet::identity_scan_stateand restores throughIdentityManagerStartState::scan_states, so a host that persists it re-opens the question on the next launch.The budget-expiry path gets the same treatment, since a scan dropped mid-await never reaches its own bookkeeping — without it that path reproduces the bug in its own right, by consulting local state, finding the sighting persisted before cancellation, and recording a warm launch.
Absence of a verdict deliberately reads as "unknown", not "incomplete". Treating unknown as incomplete would make every launch on a non-adopting host pay for a full gap-limit scan plus a Keychain round trip before every Core SPV start — the cost the shortcut exists to avoid, and which review specifically asked to remove.
Refs #4365 rather than closing it: no persister vtable carries the field yet, so cross-launch retention still needs the host slot. See Residual limitations.
3. The seed-binding gate existed only in the Swift wrapper (#4368 follow-through)
Everything the drain does with key material is unauthenticated, and
register_contact_accountkeys its existence check on(index, us, them)— not on the xpub. A provider resolving the wrong seed therefore writes contact receiving addresses once, and every later correct-seed pass no-ops forever. The corruption is permanent and its only symptom is payments that never arrive.iOS enforces this in
PlatformWalletManagerStartup.swiftbefore it calls across. #4368 named the exposure and deferred it:Fix. The shared sequence verifies the binding itself, via the existing
PlatformWallet::verify_seed_binds, immediately before the signer-present drain. Three properties worth calling out:drainable_contact_crypto_count() > 0. With nothing queued the drain would derive nothing, so there is no wrong-seed write to prevent — and a warm launch still resolves no key material at all.WalletStartupStatus::SeedBindingUnverified(FFI discriminant 5, SwiftseedBindingUnverified), plusseed_binding_unverifiedon the outcome. Core sync must start regardless.The Swift check stays. The two are not redundant: Swift throws and refuses the call outright, which is the right behaviour on a host that can, while the Rust one fails closed and reports — it has to let Core SPV start.
Also fixed
A latent misreport the rescan path exposed.
DiscoveryFailedandPartialNoIdentityboth claim the identity question is still open. That used to be structurally guaranteed — discovery ran only when nothing was on file, and every branch that found something returned early — but a rescan forced by an incomplete prior scan reaches those branches with an identity already recorded. Both are now gated onidentity_id.is_none(), so a failed rescan no longer hides a sync and drain that both ran.Tests
20 new tests;
cargo test -p platform-wallet --features shieldedgoes 837 → 857, 0 failures. Clippy clean.Three drive the real
start_wallet_subsystemsover a mock SDK rather than restating the tally rules:a_wrong_seed_provider_never_reaches_the_drainSeedBindingUnverified, no contact account registered, queue intact for the next drainthe_owning_seed_passes_the_gate_and_the_drain_runsan_empty_queue_skips_the_gate_entirelya_contact_pass_that_reached_nobody_is_not_a_completed_syncdashpay_sync_ran == falsePlus unit coverage for the rules themselves:
ContactSyncReportacross clean-empty / no-identities / partial / sent-side-only / total;ScanTally::verdictfor the exact #4365 shape (found at 0, failed at 1 → trustworthy and incomplete); and the scan-verdict round trip, including that unknown ≠ incomplete and that a clean rescan clears an earlier partial verdict.Residual limitations
pending_contact_crypto_addedalready carries. Within a process it redirects a second bring-up, and the in-launch retry closes the window whenever Platform recovers inside the budget. What remains open is a probe that fails for the entire budget and is never revisited after a restart. Adopting it needs an FFI vtable slot plus SwiftData/Room columns; the SQLite reference persister cannot demonstrate the round trip either, since itsload()still does not rehydrateClientStartState::wallets(WALLET_RESTOREis not attested).SeedBindingUnverifiedis a new enum variant in Rust, the FFI (discriminant 5) and Swift. Additive and appended, but a host matching exhaustively on the status will need the arm.🤖 Generated with Claude Code