fix(bitcoin-wallet-snap): repair drifted wallets with a one-time full rescan - #226
fix(bitcoin-wallet-snap): repair drifted wallets with a one-time full rescan#226jeremytsng wants to merge 3 commits into
Conversation
276f370 to
cdd693d
Compare
d63b758 to
215d451
Compare
cdd693d to
3895731
Compare
27561ee to
5b9d467
Compare
3895731 to
f9c6c84
Compare
5b9d467 to
5da96b6
Compare
9157cca to
a9a3dde
Compare
… rescan A full scan runs only once in an account's life, at creation, so a wallet whose funds landed outside the revealed set stays wrong forever. The first regular sync after this update schedules one full scan per existing account, gated by a rescanV1 state marker set after scheduling so a crash retries with duplicate scans instead of silently skipping the repair. Repair scans emit a Scan Discovered Missed Transactions tracking event for each transaction routine sync did not know about, which measures whether the coverage fixes hold in the field.
5da96b6 to
3f694c2
Compare
|
Sonar is mentioning |
|
This new event |
|
Also if a new event is created, it needs to be referenced in Segment through Segment schema repo: https://github.com/Consensys/segment-schema |
There was a problem hiding this comment.
🟡 Changes recommended
Repair events can be permanently skipped, and SnapClientAdapter currently discards the new telemetry event.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a one-time full rescan to repair drifted Bitcoin accounts and track missed transactions.
Changes:
- Schedules repair scans and records completion state.
- Adds missed-transaction telemetry.
- Refactors sync result handling and expands tests/changelog.
File summaries
| File | Description |
|---|---|
CronHandler.ts |
Implements repair scheduling, telemetry, and sync aggregation. |
CronHandler.test.ts |
Tests repair scheduling and transaction tracking. |
entities/snap.ts |
Defines the new tracking event. |
CHANGELOG.md |
Documents repair and telemetry behavior. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Also referenced by Copilot: #226 (comment) |
…s telemetry The one-time repair marked itself complete as soon as its background scans were scheduled, so a scan consumed while the client was locked, or one that failed, left the wallet unrepaired forever. The marker now holds the list of accounts still pending, fixed at first run so later-created accounts never join it. Each sync run repairs one pending account and removes it only after the scan succeeds; deleted accounts are pruned and a malformed marker reinitialises the list. Once the list is empty the repair path costs one state read per run. SnapClientAdapter now maps Scan Discovered Missed Transactions to a tracking message; the event previously fell through to the assertion default and was swallowed before reaching snap_trackEvent. Collapse the duplicated ../entities imports in CronHandler.
|
…sactions Discovered Match the segment-schema definition: Object + Past-tense Verb name, and send the on-chain hash as transaction_hash for this event.
|
Battambang
left a comment
Battambang
left a comment
There was a problem hiding this comment.
Good job also with the new metric event handled in the Segment schema repo.
|
@jeremytsng Could you please also make a preview build for testing? |
Explanation
Last of three PRs splitting #201 per review feedback. Stacked on #225.
The wide address scan runs exactly once in an account's life — at creation. A wallet whose funds landed outside the revealed set before the reveal-at-sign fix (#225) stays wrong forever; nothing in the product re-searches. Two support cases (0.09 BTC and 6.9 BTC) sat in exactly that state until the users recovered manually.
This PR adds a one-time repair, deliberately not a recurring job:
FullScanAccountbackground events), then sets arescanV1state marker and never repeats. The scan uses the BIP44 gap from fix(bitcoin-wallet-snap): use BIP44 gap limit for full account scans #224.Missed Transactions Discoveredtracking event per transaction routine sync did not know about. This is the acceptance metric for the stack: its rate should go to zero. Creation-time scans do not emit it, so expected discovery never pollutes the signal.CronHandlerbecame one typed#finishSynchelper, removing aRecord<string, any>.Recurring coverage (periodic rescans, all-account sync) is out of scope pending an infra discussion on Esplora load: a recurring full scan costs roughly 40 requests per account per run across the fleet, and the drifted-wallet population is a one-time problem.
References
Checklist