Repository navigation
feat(platform-wallet-storage): persist DashPay backfill coverage and outbound-account markers - #5479
HashEngineering wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 70.97% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 62 functions across 18 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Issues #5026and#4302are closed. They provide historical context only. No active directly linked issue supplies coding requirements. The reported SQLite implementation matches the current PR intent …Out of Scope Changes check The changes stay within the stated SQLite persistence scope. Migration, schema code, restore logic, tests, schema documentation, and the test-only encryption helper support DashPay backfill or outboun… Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the primary changes: persisting DashPay backfill coverage and outbound-account markers in platform wallet storage. Full details: Linked Issues check
-
Explanation
Issues
#5026and#4302are closed. They provide historical context only. No active directly linked issue supplies coding requirements. The reported SQLite implementation matches the current PR intent for backfill persistence, cursor rewinds, and outbound-account markers.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within the stated SQLite persistence scope. Migration, schema code, restore logic, tests, schema documentation, and the test-only encryption helper support DashPay backfill or outbound-account marker persistence. The public type re-exports support the tested registration behavior. No unrelated change is demonstrated.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 70.97% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 62 functions across 18 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
|
⛔ Final review complete — 1 blocking finding(s) (commit 30bcd54) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head b3a06b1. Two blocking persistence defects remain: restoration can advance an explicitly rewound cursor, and the backfill writer accepts records that its reader rejects; the new integration tests also expose a smaller public-API usability issue. This was a static review: current CI checks pass, including Kotlin build/tests, but do not independently confirm the reported Rust storage-suite results.
🔴 2 blocking | 🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 6: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The change intricately couples durable backfill coverage, cursor rewinds, buffered persistence and restart recovery while changing storage schema in migrations/V019__dashpay_backfill.rs and cursor semantics in core_state.rs::upsert_sync_state. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:293-299: Preserve an explicit rewind below the wallet-birth seed
The claimed clamp does not exist in the pinned dependency: WalletInfoInterface::update_synced_height directly assigns its argument, and reconcile_dashpay_rescan passes the contact checkpoint without clamping it to wallet birth. For example, a wallet with birth_height = 501 can persist a backfill cursor of 100, but reopening seeds synced_height at 500 and this max advances the restored cursor to 500. Because this PR now restores the coverage record too, reconciliation treats the contact as covered and does not rewind again. When the shared scanner has history below this wallet's birth, such as alongside an older wallet, the unscanned range between the saved cursor and 500 is skipped for this wallet. Restore the explicit stored synced_height instead of treating the initialization seed as observed progress, and add a relaunch regression with a checkpoint below a nonzero birth height.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/dashpay_backfill.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/dashpay_backfill.rs:37-42: Reject coverage blobs that the load path cannot read
The writer inserts record.covered_bytes() without enforcing the 16 MiB limit checked by load_for_wallet at line 75. DashPayBackfillRecord::from_entries does not cap the entry count, V019 only requires a nonempty multiple of 72 bytes, and the connection's SQLite limit is 32 MiB. A record containing 233,017 distinct account keys therefore produces a 16,777,224-byte blob that store can successfully commit, but the next default load fails with BlobTooLarge and aborts restoration; Recovery mode omits the affected wallet. Although this requires a large coverage set, it lets a successful persistence operation make the wallet unrestorable through the normal load path. Check the encoded length before allocating and writing the blob, and add a boundary regression proving rejection preserves both the previous record and its cursor.
In `packages/rs-platform-wallet-storage/tests/integration/sqlite_dashpay_backfill_relaunch.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/tests/integration/sqlite_dashpay_backfill_relaunch.rs:469: Expose registration outcomes instead of asserting Debug strings
The new test compares the Debug strings "Built" and "AlreadyExisted" because ExternalAccountRegistration is public inside the private network::contacts module but is not re-exported. The public DashPayView method therefore returns an outcome whose variants external callers cannot name, and the integration test works around that API gap by depending on presentation text rather than typed outcomes. Re-export ExternalAccountRegistration and RegisterExternalError from the wallet's public identity API, then use typed assertions here and at the relaunch assertion. This is a non-blocking API-usability improvement, not a layer-ownership violation.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Track SQLite outbound-account rotation conflicts separately — Both the base and reviewed head reject an outbound-account rebuild when a genuinely rotated xpub supplies different typed pool keys at existing indices: core_pool::apply_pools returns TypedPoolKeyConflict. The rotation teardown removes only in-memory accounts, leaving the conflicting SQLite pool rows in place for subsequent retries. This predates the PR, and the registration transaction rolls back rather than persisting a falsely trusted marker.
- Follow-up: Track a separate SQLite rotation operation that atomically replaces the old outbound pool with its account registration, preserves immutable-key protections for other account types, and covers rotation plus reopen through the real manager.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 2 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
b3a06b1 to
88283da
Compare
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Reviewed the complete diff at 88283da and independently confirmed that all three prior findings are fixed, with no remaining in-scope defects identified. SQLite preserves atomic record/cursor writes, restores deliberate rewinds faithfully, and persists outbound-account markers while leaving wallet policy in rs-platform-wallet. Validation was static locally; current exact-head CI passes scoped Rust wallet tests and clippy, Swift schema/build/tests, and Kotlin build/tests, while the full Rust workspace suite was skipped.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 9: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 10: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The change combines a storage migration in migrations/V019__dashpay_backfill.rs with intricate persistence and recovery semantics spanning atomic backfill records, deliberate cursor rewinds, and restart rehydration. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Coordinate explicit below-birth backfills with the SPV scan floor — Both the pinned and current default rust-dashcore revisions derive earliest_required_height from the minimum loaded-wallet birth height, and dash-spv applies that floor during scan startup and restart. With only a wallet born at 501 loaded, correctly restoring its cursor to 100 therefore does not make the scanner traverse heights 101–500, although reconcile_dashpay_rescan can record coverage from that earlier checkpoint. This is a concrete pre-existing wallet/scanner contract limitation, not a defect introduced by SQLite persistence; restoring the stored cursor remains the correct adapter behavior.
- Follow-up: Track a separate coordinated platform-wallet/rust-dashcore fix that communicates explicit backfill requirements to the scanner and ensures recorded coverage reflects the reachable scan range. Include a real-scanner regression with a contact checkpoint below the only loaded wallet's birth height.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
|
/self-reviewed |
|
Ready for review — |
github-actions
Bot
added
ready-for-human
|
Bots are done — your move: address lklimek requested changes, then post |
github-actions
Bot
added
waiting-self-review
lklimek
left a comment
lklimek
left a comment
There was a problem hiding this comment.
Reviewed rs-platform-wallets-storage, some suggestions to the db schema.
…outbound-account markers The SQLite backend loaded every wallet with an empty DashPay backfill record and kept a monotonic-max cursor, so on this backend the coreHeight backfill re-ran on every launch (#4302). Outbound-account markers were handed to it in Immediate mode (ACCOUNT_ADDRESS_POOLS is an alias of the ASSET_LOCK_FUNDING_INDICES it already declares) but silently dropped, since no column held them, so each cold start rebuilt every outbound account. - V019 adds `dashpay_backfill` (one row per wallet: floor, rewound_from, and the covered receival accounts as one `covered_bytes` blob — the layout the FFI hosts store) and a nullable `contacts.external_account_reference`. - The record is written in the store's transaction beside the cursor it vouches for; every write replaces it whole and an empty record clears it. Load hands it back through `ClientWalletStartState::dashpay_backfill`; a torn cover set reads as no record, the codec's safe direction. - `stores_dashpay_backfill()` is true; durable coverage still needs `FlushMode::Immediate` via `store_commits_inline`. - A cursor marked `synced_height_is_rewind` overwrites the stored `synced_height`; advances keep the monotonic max. Tests pin the order through the Manual-mode buffer, a failed-flush restore, and identity re-add segments. - New `DashPayBackfill` version domain; migration fingerprints bumped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…irth and refuse unreadable cover sets - Restore the stored `synced_height` as written instead of maxing it against the `birth_height - 1` seed. A DashPay backfill rewind can lower the cursor below the wallet's birth (`reconcile_dashpay_rescan` passes the contact checkpoint to `update_synced_height` unclamped), and the restored record vouches for that cursor, so the seed skipped the range between them. Matches the FFI restore path; replaces a comment that claimed a clamp the pinned key-wallet does not have. - Refuse, before writing, a backfill record whose encoded cover set exceeds the blob limit the loader reads under, so a successful store can never leave the wallet unloadable. The round fails whole; the record and cursor already on file stand. - Re-export `ExternalAccountRegistration` and `RegisterExternalError` from the identity API so callers (and the relaunch test) can match the outcome of `register_external_contact_account` by type. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…as plain columns Review asked for values rather than a blob, on the tables that already hold the facts. The record's extent moves to core_sync_state.dashpay_backfill_floor / dashpay_backfill_rewound_from, beside the cursor it lowered, and each covered entry becomes account_registrations.dashpay_covered_from on the receiving account it names. The dashpay_backfill table and its 72-byte cover-set blob are gone, and with them the blob size limit the writer had to enforce. A covered entry whose receiving account has no row is dropped (re-armed, the safe direction); when none remain the extent is cleared too. The account upsert only rewrites the xpub, so coverage survives a rotation. The migration is renumbered V022: V019 is taken on v5.1-dev and V020 / V021 are claimed by open PRs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
88283da to
30bcd54
Compare
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified both reviewer sets against head 30bcd54: one blocking consistency issue remains because wallet restoration can combine a pre-rewind cursor with newly committed backfill coverage. All three prior findings are fixed; the identifier-fallback and release-note findings do not establish actionable defects. This was a static review only; current CI still has Rust workspace tests and PR Hygiene pending, and the supplied base-drift evidence reports no conflicting files.
🔴 1 blocking
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The change combines a storage migration in migrations/V022__dashpay_backfill.rs with intricate transactional persistence, rehydration, and deliberate cursor-rewind semantics in schema/dashpay_backfill.rs and related code. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:1864-1865: Restore the backfill record and cursor from one SQLite snapshot
The newly restored coverage suppresses historical rescanning, but `load()` does not open a read transaction. `core_state::load_state` finishes its cursor query before this later coverage read, so another connection can commit between them. The connection mutex and open-path registry exclude only in-process access; normal opens do not enable lifetime EXCLUSIVE locking, and the default journal mode is WAL. For example, with the receiving account and contact already persisted, a reader can capture cursor 1000, then another process can atomically commit rewind 100 plus coverage and exit, after which the reader captures that coverage. A fresh manager retains it, and `reconcile_dashpay_rescan` skips the rewind because the contact is covered, leaving the historical range untested against its addresses and potentially hiding payments. Hold one SQLite read snapshot across reconstruction, including accounts, cursor, contacts, and coverage. A transaction only inside `dashpay_backfill::load_for_wallet` starts too late. Add a two-connection regression that commits the rewind between the cursor and coverage reads.
| let dashpay_backfill = schema::dashpay_backfill::load_for_wallet(conn, &wallet_id) | ||
| .map_err(PersistenceError::from)?; |
There was a problem hiding this comment.
🔴 Blocking: Restore the backfill record and cursor from one SQLite snapshot
The newly restored coverage suppresses historical rescanning, but load() does not open a read transaction. core_state::load_state finishes its cursor query before this later coverage read, so another connection can commit between them. The connection mutex and open-path registry exclude only in-process access; normal opens do not enable lifetime EXCLUSIVE locking, and the default journal mode is WAL. For example, with the receiving account and contact already persisted, a reader can capture cursor 1000, then another process can atomically commit rewind 100 plus coverage and exit, after which the reader captures that coverage. A fresh manager retains it, and reconcile_dashpay_rescan skips the rewind because the contact is covered, leaving the historical range untested against its addresses and potentially hiding payments. Hold one SQLite read snapshot across reconstruction, including accounts, cursor, contacts, and coverage. A transaction only inside dashpay_backfill::load_for_wallet starts too late. Add a two-connection regression that commits the rewind between the cursor and coverage reads.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality)
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
Basic explanation
What this does: Teaches the SQLite wallet store to save two facts that #5026 added to the wallet's persistence contract: how far the DashPay "backfill" rescan has already got (so a restart does not start it again), and which account version each DashPay contact's outbound payment account was built for (so a restart does not rebuild it).
Value: Wallets on the SQLite backend stop re-walking the same block range on every launch once a DashPay backfill has run — the same fix #5026 gives the Kotlin and Swift hosts. On the reference mainnet wallet that re-walk was ~377,000 filters per launch. Contacts' outbound accounts also stop being torn down and rebuilt on every cold start.
Risks: Low. No consensus impact — this is client-side wallet storage only. The migration only adds nullable columns, so an upgraded database behaves exactly as before until the first backfill is recorded (one more re-run on first launch). The one behaviour change to existing data is that a stored sync cursor can now go down when Rust marks it as a deliberate rewind; ordinary advances still keep the highest value.
Issue being fixed or feature implemented
Follows #5026 (fixes #4302 for the FFI hosts). That PR made the DIP-15 §12.6 coreHeight backfill durable through a new
DashPayBackfillRecordthat hosts store and hand back on load, plus asynced_height_is_rewindflag the host must honour. Kotlin implements it; the SQLite backend did not:synced_height, so on this backend the backfill re-ran on every launch — the Persist DashPay coreHeight backfill completion (rescan_triggered is in-memory only) #4302 behaviour.FlushMode::Immediateit already qualified for outbound-account markers (ACCOUNT_ADDRESS_POOLSis an alias of theASSET_LOCK_FUNDING_INDICESit declares), so markers were handed to it — and silently dropped, since no column held them.What was done?
Migration
V022__dashpay_backfill.rs(purely additive, nullable columns only — no new table, no blob)core_sync_state.dashpay_backfill_floor/dashpay_backfill_rewound_from: the record's extent, beside the cursor it lowered. CHECK: both set or both NULL, each au32.account_registrations.dashpay_covered_from: the height a covered DashPay receiving account is covered from. A covered entry names exactly one receiving account (owner, contact, index), which is that row's key. CHECK:u32, and only on'dashpay_receiving'rows.contacts.external_account_reference: the outbound-account marker, au32.v5.1-dev(fix(platform-wallet)!: drain Core balance with restart-safe shielded recovery #5396), and open PRs claim V019 (feat(platform-wallet)!: support DashPay shielded tips with dedicated accounts #5288), V020 (feat(platform-wallet)!: persist engine spent claims and restore them across backends #5306) and V021 (fix(platform-wallet)!: restore Core spending state and repair persisted accounting #5150). Whichever of these merges later must renumber above the others, because refinery refuses to migrate a database that has applied a higher version than a migration it has not run (MissingVersion, its defaultabort_missing).Backfill record —
src/sqlite/schema/dashpay_backfill.rsapplyreplaces the wallet's record inside the store's transaction, after the round's account registrations: it clears everydashpay_covered_from, sets it on each covered account's row (addressed through the same typed-column helpers the account writer binds), then sets or clears the extent. An empty record only clears (the upstream invalidation).account_xpub_bytes, so coverage survives an xpub rotation.load_for_walletjoins the columns back throughDashPayBackfillRecord::from_entries(canonical order). Coverage without an extent reads as "no record". Wired intoload_one_wallet→ClientWalletStartState::dashpay_backfill.SqlitePersister::stores_dashpay_backfill()returnstrue. Durable coverage still requiresFlushMode::Immediatethrough the existingstore_commits_inlinegate inpersists_backfill_coverage; Manual mode writes and clears records but keeps coverage session-only.Domain::DashPayBackfillinschema/versions.rs(replaces the explicitdashpay_backfill: _ignore). Its version label is"dashpay_backfill", like"core"a domain rather than a table name.Rewind —
schema/core_state.rs::upsert_sync_staterehydrate.rs::apply_persisted_core_state) now assigns the storedsynced_heightas written instead of maxing it against thebirth_height - 1seed — a backfill rewind can sit below the wallet's birth, and the FFI restore path already assigns it directly.synced_heightmarkedsynced_height_is_rewindoverwrites the stored cursor; advances,last_processed_heightandchainlock_heightkeep the monotonic max. A rewind can only lower the tombstone finality boundary (min(chainlock, synced)), so it never collects more.Outbound-account marker —
schema/contacts.rsWalletPersister::storealready strips it on backends that cannot keep it, and the manager's load clears restored markers it cannot trust. A trusted marker relies onapply_registrationsoverwriting the account xpub on rotation, plus the load-side check that distrusts a marker when an external pool holds addresses its current xpub does not derive.Identity API —
ExternalAccountRegistration/RegisterExternalErrorare re-exported fromplatform_wallet::wallet::identity, so callers can matchregister_external_contact_account's outcome by type.Docs —
SCHEMA.md(the new columns oncore_sync_state,account_registrationsandcontacts, the V022 log entry, the rewind exception oncore_sync_state); a stale comment inrs-platform-wallet/src/changeset/changeset.rsthat said SQLite ignores the rewind flag.How Has This Been Tested?
cargo test -p platform-wallet-storage --all-features(572 integration + 431 unit, all pass) andcargo clippy -p platform-wallet-storage -p platform-wallet --all-features --all-targets -- -D warnings(clean).tests/integration/sqlite_dashpay_backfill_relaunch.rs(new, end-to-end): the realPlatformWalletManagerover a realSqlitePersister, with the file closed and reopened between sessions:DashpayExternalAccountpresent and the same xpub, so registering it again reportsAlreadyExisted.test_support::encrypt_contact_xpub_for_test(test-utilsonly) so the storage tests can build a contact's encrypted xpub offline.tests/integration/sqlite_dashpay_backfill.rs(new):sqlite_contacts_keys_rehydration.rs: the outbound-account marker round-trips.sqlite_version_bump.rs,sqlite_schema_pinning.rs, the new domain and the migration fingerprints.Breaking Changes
None. The schema change is an additive migration; an older binary cannot open a V022 database, which is the existing forward-version gate for every migration.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
30bcd54/self-reviewedwallet-storage(packages/rs-platform-wallet-storage/SCHEMA.md,packages/rs-platform-wallet-storage/migrations/V022__dashpay_backfill.rs,packages/rs-platform-wallet-storage/src/sqlite/persister.rsand 12 more) — lklimekrs-platform-wallet(packages/rs-platform-wallet/src/changeset/changeset.rs,packages/rs-platform-wallet/src/test_support.rs,packages/rs-platform-wallet/src/wallet/identity/mod.rsand 1 more) — ZocoLini or llbartekll or romchornyiWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.