Repository navigation
fix(platform-wallet-storage): restore DashPay contact address pools on SQLite load - #5549
romchornyi wants to merge 4 commits into
Conversation
…n SQLite load SQLite stored a DashPay contact's external address pool but never applied it on load: rehydration routes restored addresses only to funds accounts, which exclude dashpay_external, so every reopen handed out contact addresses this wallet had already paid, linking two payments on-chain. load_contact_pool_claims reads the dashpay_external rows that are used, reserved, or carry a materialised core_utxos coin on their script, with their exact (pool_type, address_index). restore_contact_pool_claims derives each claimed slot into the contact's pool, marks it used and refills the gap window with the funds accounts' bounded refill (refill_gap_window, now shared). A slot whose stored script no longer derives there - a re-sent contact request replaced the xpub, or a corrupt row - is still kept used and counted as degraded. A claim past the rehydration derivation cap, on a pool type the account lacks, or on an underived slot without an xpub cannot be kept used and fails a Strict load. Claims on an unregistered contact account are counted per claim. The used-address reader leaves dashpay_external rows out and owning_account_for_script ranks them last, so a script a contact's external row shares with a dashpay_receiving row keeps that account's reuse guard. Fixes #5548. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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
 |
|
⛔ Final review complete — 1 blocking finding(s) (commit 9f8c08f) · triage: normal |
|
@coderabbitai review No review for |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The PR correctly restores DashPay contact address usage state during SQLite rehydration and keeps contact-owned pools separate from wallet funding accounts. Rust wallet tests pass; the Swift SDK build is still queued, with no failed code check reported. I found three non-blocking architecture/maintainability issues in the new storage path.
🟡 3 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Keep gap-refill target calculation in key-wallet
packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:1033-1038
The new contact restoration path depends on refill_gap_window, whose preflight duplicates the private target arithmetic inside the pinned key-wallet::AddressPool::maintain_gap_limit. The gap-window invariant belongs to key-wallet; keeping this formula in rs-platform-wallet-storage means a future upstream change can silently make the preflight under- or over-estimate derivation, causing a pool to remain short or causing valid persisted state to be rejected. Add a checked read-only refill-target or bounded-maintain API in key-wallet and use it here so the funds and contact restoration paths share the owner’s implementation.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large and intricate wallet-storage rehydration change affecting address derivation and persistence, but it does not modify consensus, funds movement, cryptography, or a storage migration surface as defined for the critical tier. - 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(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); 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`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:1033-1038: Keep gap-refill target calculation in key-wallet
The new contact restoration path depends on `refill_gap_window`, whose preflight duplicates the private target arithmetic inside the pinned `key-wallet::AddressPool::maintain_gap_limit`. The gap-window invariant belongs to key-wallet; keeping this formula in `rs-platform-wallet-storage` means a future upstream change can silently make the preflight under- or over-estimate derivation, causing a pool to remain short or causing valid persisted state to be rejected. Add a checked read-only refill-target or bounded-maintain API in key-wallet and use it here so the funds and contact restoration paths share the owner’s implementation.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:458-463: Narrow the contact restoration helper visibility
`restore_contact_pool_claims` is only called by the crate-internal `load_one_wallet`, while the neighboring rehydration helpers are `pub(crate)`. Making this function `pub` unnecessarily expands the public API and is the reason the internal `ContactPoolClaim` type is re-exported through `sqlite::mod`. Narrow the helper to `pub(crate)` unless an external caller is intentionally supported; retain public visibility only for the schema reader/type if those are part of the supported test or storage API.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:356: Centralize the DashPay external account database label
The new ownership and claim queries hardcode `'dashpay_external'`, while the canonical mapping is `accounts::account_type_db_label`. The literal is repeated in `owning_account_for_script`, `load_used_addresses_with_ctx`, and `load_contact_pool_claims`. If the label changes, this compiles while silently changing ownership routing and potentially disabling contact claim restoration. Define one shared crate-visible constant next to the label helper and use it when constructing these SQL statements, or add an assertion that the SQL label remains synchronized with the helper.
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.
- Legacy contact-script UTXOs can still inflate the funds balance — A pre-existing
core_utxosrow whose script matches only adashpay_externalpool is resolved byowning_account_for_script, thenapply_persisted_core_stateroutes the unspent UTXO to the first funding account because external accounts are excluded fromall_funding_accounts_mut(). This can make an old contact payment remain counted as wallet funds after reload. The behavior predates this PR and is not changed by the patch, so it should be tracked separately rather than blocking this contact-claim rehydration fix.- Follow-up: Open a separate wallet-storage issue to define how legacy contact-owned
core_utxosrows should be excluded or migrated from the funds projection.
- Follow-up: Open a separate wallet-storage issue to define how legacy contact-owned
| FROM core_address_pool \ | ||
| WHERE wallet_id = ?1 AND script = ?2 \ | ||
| ORDER BY account_type, account_index, key_class, user_identity_id, \ | ||
| ORDER BY account_type = 'dashpay_external', account_type, account_index, key_class, user_identity_id, \ |
There was a problem hiding this comment.
✅ Resolved at
2bee2705; see the replies below.
🟡 Suggestion: Centralize the DashPay external account database label
The new ownership and claim queries hardcode 'dashpay_external', while the canonical mapping is accounts::account_type_db_label. The literal is repeated in owning_account_for_script, load_used_addresses_with_ctx, and load_contact_pool_claims. If the label changes, this compiles while silently changing ownership routing and potentially disabling contact claim restoration. Define one shared crate-visible constant next to the label helper and use it when constructing these SQL statements, or add an assertion that the SQL label remains synchronized with the helper.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
There was a problem hiding this comment.
Fixed in 2bee270. The label now has one source, dashpay_external_label!() in schema/accounts.rs. account_type_db_label uses it, and so do all the SQL statements that filter or rank external rows: owning_account_for_script, load_used_addresses_with_ctx and load_contact_pool_claims, with their size gates. The macro is spliced in with concat!, so the SQL stays static. account_type_labels_match_enum already ties the label to the enum and to the frozen migration list.
There was a problem hiding this comment.
Resolved (re-reviewed at 2bee2705): The latest commit centralizes the label in dashpay_external_label!() and uses that macro in the account mapping and every affected SQL filter, ordering expression, and size gate.
| pub fn restore_contact_pool_claims( | ||
| wallet_info: &mut ManagedWalletInfo, | ||
| manifest: &[AccountRegistrationEntry], | ||
| claims: &[core_pool::ContactPoolClaim], | ||
| ctx: &LoadCtx, | ||
| ) -> Result<(), WalletStorageError> { |
There was a problem hiding this comment.
✅ Withdrawn at
2bee2705; see the replies below.
🟡 Suggestion: Narrow the contact restoration helper visibility
restore_contact_pool_claims is only called by the crate-internal load_one_wallet, while the neighboring rehydration helpers are pub(crate). Making this function pub unnecessarily expands the public API and is the reason the internal ContactPoolClaim type is re-exported through sqlite::mod. Narrow the helper to pub(crate) unless an external caller is intentionally supported; retain public visibility only for the schema reader/type if those are part of the supported test or storage API.
| pub fn restore_contact_pool_claims( | |
| wallet_info: &mut ManagedWalletInfo, | |
| manifest: &[AccountRegistrationEntry], | |
| claims: &[core_pool::ContactPoolClaim], | |
| ctx: &LoadCtx, | |
| ) -> Result<(), WalletStorageError> { | |
| pub(crate) fn restore_contact_pool_claims( |
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
There was a problem hiding this comment.
Keeping this pub, on purpose. apply_persisted_core_state is the public rehydration entry point, and it now leaves contact-external addresses to restore_contact_pool_claims. If the second function were pub(crate), a caller of the public entry point could not restore that half, and contact pools would reload with paid addresses marked available. ContactPoolClaim is re-exported for the same reason as OwningAccount: the public function's parameter type has to be nameable from a default build, where schema is pub(crate).
There was a problem hiding this comment.
Withdrawn (re-reviewed at 2bee2705): The public visibility is intentional: rehydrate exposes the public core-state entry point and the separate contact-pool restoration operation, while ContactPoolClaim is re-exported so an external persistence implementation can complete both phases. The current code therefore reflects an explicit two-phase API rather than an accidental export.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
…l label, pin the gap-refill estimate The SQL that filters or ranks a contact's external rows now splices the label from `dashpay_external_label!()`, which `account_type_db_label` uses too, instead of repeating the literal. A new test checks `ImpliedRefill` against what key-wallet's `maintain_gap_limit` actually derives, so an upstream change to that private formula fails CI instead of silently mis-costing a refill. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Re 🟡 "Keep gap-refill target calculation in key-wallet": agreed that key-wallet is the right owner. A read-only refill-target API there is a rust-dashcore change, though, and |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix. 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
The contact-pool restoration path correctly restores used slots, but unspent UTXOs belonging only to DashPay external accounts are still inserted into the first wallet funding account and included in the wallet balance. This is a blocking funds-state corruption issue because key-wallet explicitly excludes contact-owned external accounts from wallet funds. One non-blocking architectural concern remains: the SQLite layer duplicates key-wallet's private gap-refill target calculation. Rust workspace tests, the Drive image build, and the JS build were still in progress in the supplied CI snapshot.
🔴 1 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Exclude contact-owned UTXOs from wallet balance rehydration
packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:328-358
apply_persisted_core_state filters spent UTXOs but does not filter unspent UTXOs whose utxo_accounts owner is a dashpay_external account. For a contact-only script, owning_account_for_script records that external owner, route_to_funds_account cannot find it among all_funding_accounts_mut(), and the fallback inserts the contact's UTXO into funding account 0 at line 357. update_balance() then counts the contact-owned output as this wallet's spendable balance. Key-wallet explicitly documents that external accounts are contact-owned and excluded from wallet funds because counting these outputs makes the wallet balance grow by amounts it paid out and can place unspendable coins into coin selection. The later restore_contact_pool_claims call only restores the address slot and cannot remove the already-inserted UTXO. Filter external-owned entries before the missing-account check and funding-account insertion, while retaining them for the dedicated contact-claim loader; the stored-coin regression test should also assert that balance and funding-account UTXOs remain unchanged.
let unspent: Vec<&key_wallet::Utxo> = core
.new_utxos
.iter()
.filter(|u| {
!spent_outpoints.contains(&u.outpoint)
&& !is_contact_external(utxo_accounts.get(&u.outpoint))
})
.collect();
source: gpt-6.1-sol (phase2-reviewer: general)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a substantial SQLite persistence and wallet rehydration change affecting address derivation, pool state, recovery behavior, and resource-bounded loading, but it does not directly change consensus, funds movement, cryptography, key handling, network deserialization, or storage migration semantics. - 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(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - 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 high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); 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:328-358: Exclude contact-owned UTXOs from wallet balance rehydration
`apply_persisted_core_state` filters spent UTXOs but does not filter unspent UTXOs whose `utxo_accounts` owner is a `dashpay_external` account. For a contact-only script, `owning_account_for_script` records that external owner, `route_to_funds_account` cannot find it among `all_funding_accounts_mut()`, and the fallback inserts the contact's UTXO into funding account 0 at line 357. `update_balance()` then counts the contact-owned output as this wallet's spendable balance. Key-wallet explicitly documents that external accounts are contact-owned and excluded from wallet funds because counting these outputs makes the wallet balance grow by amounts it paid out and can place unspendable coins into coin selection. The later `restore_contact_pool_claims` call only restores the address slot and cannot remove the already-inserted UTXO. Filter external-owned entries before the missing-account check and funding-account insertion, while retaining them for the dedicated contact-claim loader; the stored-coin regression test should also assert that balance and funding-account UTXOs remain unchanged.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:1023-1033: Keep gap-refill target calculation in key-wallet
`ImpliedRefill::of` still duplicates the target arithmetic privately owned by key-wallet's `AddressPool::maintain_gap_limit`: an empty pool targets `gap_limit - 1`, while a used pool targets `highest_used + gap_limit`. The new parity test is valuable because it fails when the pinned implementation changes, but it does not remove the second implementation or cover future state-dependent behavior. This PR now uses the preflight for both funds and contact-pool restoration, increasing the number of paths that depend on the copied invariant. The ownership-preserving fix is an upstream checked read-only refill-target or bounded-maintenance API in `dashpay/rust-dashcore`, with both key-wallet maintenance and this preflight consuming that implementation.
| /// What a `maintain_gap_limit` call on a pool would cost, costed before it | ||
| /// runs. | ||
| /// | ||
| /// Mirrors the target upstream computes internally (key-wallet rev | ||
| /// 393b612) — a private formula, so an upstream change would silently | ||
| /// mis-estimate here until `AddressPool` exposes a read-only | ||
| /// `refill_target()` (upstream work, out of scope). Upstream computes that | ||
| /// Mirrors the private target upstream `maintain_gap_limit` computes; | ||
| /// `implied_refill_matches_what_maintain_gap_limit_derives` checks the two | ||
| /// agree, so an upstream change fails that test rather than mis-costing a | ||
| /// refill here. The one known difference over-estimates: an empty pool with | ||
| /// a gap limit of 1 is costed one address, which upstream does not derive. Upstream computes that | ||
| /// target with raw arithmetic — a panic with debug assertions on, a wrapped | ||
| /// target without — so the same formula is applied checked here; the cost | ||
| /// derived from it still saturates, because over-estimating fails closed. |
There was a problem hiding this comment.
🟡 Suggestion: Keep gap-refill target calculation in key-wallet
ImpliedRefill::of still duplicates the target arithmetic privately owned by key-wallet's AddressPool::maintain_gap_limit: an empty pool targets gap_limit - 1, while a used pool targets highest_used + gap_limit. The new parity test is valuable because it fails when the pinned implementation changes, but it does not remove the second implementation or cover future state-dependent behavior. This PR now uses the preflight for both funds and contact-pool restoration, increasing the number of paths that depend on the copied invariant. The ownership-preserving fix is an upstream checked read-only refill-target or bounded-maintenance API in dashpay/rust-dashcore, with both key-wallet maintenance and this preflight consuming that implementation.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, rust-quality)
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix. |
github-actions
Bot
added
waiting-self-review
…ed wallet balance An unspent UTXO whose script only a DashPay contact's external account owns is the contact's coin. Rehydration routed it to the first funds account, so the wallet balance counted it and coin selection could pick an input the wallet cannot sign. It is now filtered out before routing; the contact-claim loader still keeps the slot used. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Re 🔴 "Exclude contact-owned UTXOs from wallet balance rehydration": fixed in 3a3771b, as suggested. Re 🟡 gap-refill target: unchanged since my earlier reply. |
|
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
the
waiting-bots
github-actions
Bot
removed
waiting-self-review
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The contact-pool restoration and contact-owned UTXO exclusion are otherwise implemented consistently, and the prior label-centralization and balance findings are fixed. However, the new SQL-side exclusion of dashpay_external rows bypasses account-label validation, allowing a corrupted funds row to lose its address-reuse guard; this is a blocking security issue. The gap-refill formula remains a non-blocking ownership duplication that should ultimately be exposed by key-wallet.
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large and intricate Rust persistence and wallet-state change, but it does not directly modify consensus, funds movement, cryptography, key handling, network deserialization, or a storage migration. - 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(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); 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/schema/core_pool.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:439-461: Validate pool ownership rows before filtering contact accounts
`core_address_pool.account_type` is an unconstrained TEXT column, and `validate_account_type` is the existing fail-closed check for persisted pool rows. The new aggregate gate and the main query both exclude `dashpay_external` rows before the loop reaches that validation. A corrupted or tampered funds row relabeled with the recognized `dashpay_external` value can therefore disappear from `load_used_addresses_with_ctx`, losing the funds account's address-reuse guard; `load_contact_pool_claims` only restores it when a matching DashPay external account exists, and otherwise records a non-fatal orphan degradation. Strict loading can then succeed and later hand the previously used funds address out again. Query all used rows, retain the size gate for all of them, validate each row in the loop, and only then skip valid external rows. Keep external rows ordered after wallet-owned rows and insert into `seen` only for rows that are retained, so a shared script still resolves to the receiving/funds account.
In `packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:1032-1039: Keep gap-refill target calculation in key-wallet
(existing thread: https://github.com/dashpay/platform/pull/5549#discussion_r4369136921)
`ImpliedRefill::of` still duplicates the private target arithmetic in key-wallet's `AddressPool::maintain_gap_limit` (`None => gap_limit - 1`, otherwise `highest_used + gap_limit`, with the normal-child-index gate). The new parity test makes upstream drift fail loudly, but it does not remove the duplicate ownership: every future key-wallet formula change still requires a coordinated edit here, and the shared `refill_gap_window` path now depends on this copy for both funds and contact accounts. Add a read-only refill-target or bounded refill-count API in `rust-dashcore`/key-wallet and use it here for the pre-flight cap.
| "SELECT MAX(length(script)) FROM core_address_pool \ | ||
| WHERE wallet_id = ?1 AND used = 1 AND account_type <> '", | ||
| dashpay_external_label!(), | ||
| "'", | ||
| ), | ||
| wallet_id, | ||
| )?; | ||
| // Order by script then the pool PK so the first row per script is the | ||
| // tie-break winner; a `HashSet` on script drops the trailing duplicates. | ||
| let mut stmt = conn.prepare( | ||
| // A contact's external rows are left out: they are no reuse guard of | ||
| // this wallet's own (`load_contact_pool_claims` restores them on their | ||
| // account), and one winning over a `dashpay_receiving` row with the same | ||
| // script would cost that account its guard. `owning_account_for_script` | ||
| // ranks them last too, so neither source names the external account for | ||
| // a script it shares with one of this wallet's own rows. | ||
| let mut stmt = conn.prepare(concat!( | ||
| "SELECT script, account_type, account_index, \ | ||
| length(user_identity_id), user_identity_id, \ | ||
| length(friend_identity_id), friend_identity_id \ | ||
| FROM core_address_pool \ | ||
| WHERE wallet_id = ?1 AND used = 1 \ | ||
| WHERE wallet_id = ?1 AND used = 1 AND account_type <> '", | ||
| dashpay_external_label!(), | ||
| "' \ |
There was a problem hiding this comment.
🔴 Blocking: Validate pool ownership rows before filtering contact accounts
core_address_pool.account_type is an unconstrained TEXT column, and validate_account_type is the existing fail-closed check for persisted pool rows. The new aggregate gate and the main query both exclude dashpay_external rows before the loop reaches that validation. A corrupted or tampered funds row relabeled with the recognized dashpay_external value can therefore disappear from load_used_addresses_with_ctx, losing the funds account's address-reuse guard; load_contact_pool_claims only restores it when a matching DashPay external account exists, and otherwise records a non-fatal orphan degradation. Strict loading can then succeed and later hand the previously used funds address out again. Query all used rows, retain the size gate for all of them, validate each row in the loop, and only then skip valid external rows. Keep external rows ordered after wallet-owned rows and insert into seen only for rows that are retained, so a shared script still resolves to the receiving/funds account.
source: gpt-6.1-sol (phase2-reviewer: security-auditor)
There was a problem hiding this comment.
I'm leaving this unchanged, because the proposed reorder detects nothing new. validate_account_type accepts 'dashpay_external': it is in ACCOUNT_TYPE_LABELS (schema/accounts.rs:652). So a funds row relabelled to that value passes validation inside the loop exactly as it passes the SQL filter. Querying every used row, validating it, and then skipping the valid external rows gives the same result for that row.
What validation does catch is still caught. The filter is account_type <> 'dashpay_external', so every other label, unknown ones included, reaches validate_account_type and fails a Strict load. unknown_pool_account_labels_fail_strict_and_isolate_the_wallet_in_recovery covers that.
The remaining case is a row rewritten to a different, valid label. No row-level check can tell that apart from a real row, and anyone who can rewrite account_type can just as well set used = 0. The storage layer does not defend against that threat model, before this PR or after it.
There was a problem hiding this comment.
I agree that validate_account_type alone cannot distinguish a genuine external row from a funds row whose label was changed to the valid dashpay_external value. That is precisely why the current filter is still unsafe: a used funds row with a valid script and its original all-zero identity fields can be relabeled, excluded from load_used_addresses_with_ctx, then treated as an orphan contact claim; note_degraded does not fail Strict, and no funds account receives the reuse guard. The fix therefore needs ownership cross-validation against the registered account set, or an equivalent fail-closed fallback for orphan external rows; merely moving the existing label validation is insufficient.
There was a problem hiding this comment.
Agreed, the orphan path was the gap. Fixed in 9f8c08f: a claim on a DashPay external account this wallet does not register now fails a Strict load (WalletStorageError::UnregisteredContactPoolClaim, Fatal), and Recovery counts it under its own LoadSite::UnregisteredContactPoolClaim instead of OrphanedUtxoOwner. That is safe for real wallets: a contact's registration is written in the same changeset as its pool rows, core_address_pool (V009) postdates that round, and neither table loses rows except through the wallet's cascade. contact_claims_on_an_unregistered_account_fail_a_strict_load covers both policies and fails without the change.
A row moved onto a registered contact's key still can't be told apart from that contact's own row; it keeps its slot used there and is counted as UnresolvedUtxoAddress. The doc comment on restore_contact_pool_claims now says so.
There was a problem hiding this comment.
Still applies (re-reviewed at 9f8c08f4): The new fatal path closes only claims whose external account is absent from the wallet. A row rewritten onto a registered contact still bypasses the funds reuse guard, because the filter removes it and the contact restore accepts an xpub mismatch as a non-fatal unresolved claim; registration atomicity does not authenticate the row's ownership.
… an unregistered account A contact's registration is written in the same round as its pool rows and removed only with the wallet, so a stored claim on a DashPay external account the wallet does not register is a corrupt row. It could be a funds row relabelled as a contact's, which the used-address read leaves out, so counting it as degraded let a Strict load hand that funds address out again. It now fails a Strict load; Recovery counts it under its own site. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The SQLite rehydration changes correctly restore contact address usage, exclude contact-owned coins from funding balances, and fail Strict loads for claims on unregistered contact accounts. One blocking integrity gap remains: a corrupted pool row rewritten onto a registered contact account can still bypass the original funds account's reuse guard; two non-blocking quality issues also remain. No checks have failed, but the Kotlin build, changed-package detection, and PR Hygiene checks are still pending.
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large and intricate Rust persistence and wallet-state rehydration change, but it does not itself modify consensus, cryptography, coin selection, funds movement, or a storage migration. - 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(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); 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/schema/core_pool.rs`:
- [NITPICK] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:345-351: Contact-ownership comment still says orphaned
The comment says a contact-only row causes a legacy contact UTXO to be reported as orphaned. The current rehydration path filters contact-owned unspent UTXOs and used addresses out of the funds routing loops, while registered contact claims are restored separately and unregistered claims use `UnregisteredContactPoolClaim`. Update the comment so it does not suggest that this path contributes to `OrphanedUtxoOwner` degradation.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs:439-461: Validate pool ownership rows before filtering contact accounts
(existing thread: https://github.com/dashpay/platform/pull/5549#discussion_r4372159015)
The new filter removes every `dashpay_external` row before the loader can validate or preserve its original ownership. The latest commit correctly fails Strict when the external account is not registered, but a corrupted funds row can still be rewritten with the identity pair of a registered contact. That row is then omitted from `load_used_addresses_with_ctx`, accepted by `restore_contact_pool_claims`, and treated as a contact claim; if its script does not derive from the contact xpub, the code merely records `UnresolvedUtxoAddress` and keeps the contact slot used. The funds account never receives the reuse guard, so Strict loading can later issue the previously used funds address again. Registration atomicity does not authenticate individual pool rows, and the existing accepted xpub-mismatch path does not distinguish this case. Validate external rows against the registered account and derivation data before discarding them, or fail closed for an external claim whose ownership cannot be established, while preserving the funds-side guard.
In `packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/rehydrate.rs:1033-1083: Keep gap-refill target calculation in key-wallet
(existing thread: https://github.com/dashpay/platform/pull/5549#discussion_r4369136921)
`ImpliedRefill::of` duplicates the private target arithmetic inside rust-dashcore's `AddressPool::maintain_gap_limit`, and this PR now relies on that copy for both funds-account and contact-pool restoration. The compatibility test detects formula drift for the pinned dependency, but it does not remove the two sources of truth or protect a consumer using a different compatible key-wallet revision. Add a checked read-only refill-target or refill-plan API in key-wallet and use it here, leaving this crate responsible only for its SQLite-specific cap and load-policy handling.
| // A DashPay contact's external account never owns funds of this wallet: | ||
| // its addresses are the contact's, which this wallet pays. Its row ranks | ||
| // last, so a script it shares with a `dashpay_receiving` row — two | ||
| // identities of one wallet that are each other's contacts — resolves to | ||
| // the receiving account, agreeing with `load_used_addresses_with_ctx`, | ||
| // which leaves it out; a row of its own still names it, so a legacy | ||
| // contact UTXO is reported as orphaned. |
There was a problem hiding this comment.
💬 Nitpick: Contact-ownership comment still says orphaned
The comment says a contact-only row causes a legacy contact UTXO to be reported as orphaned. The current rehydration path filters contact-owned unspent UTXOs and used addresses out of the funds routing loops, while registered contact claims are restored separately and unregistered claims use UnregisteredContactPoolClaim. Update the comment so it does not suggest that this path contributes to OrphanedUtxoOwner degradation.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
|
The contact sweep rebuilt every outbound account on load, because SQLite does not restore the marker. That undid this PR's pool restore in memory. The fix is in #5587 (draft): the account is kept across relaunches, a rotation replaces its stored pool on every backend, and the account row records which request it was built from. The two PRs share a few storage files and merge without textual conflicts. Whichever lands second will need the new |
github-actions
Bot
removed
the
waiting-bots
Basic explanation
What this does: A wallet that keeps its data in SQLite now remembers, after a restart, which payment addresses it has already used for each DashPay contact.
Value: Every DashPay payment to a contact should go to a fresh address. Today a SQLite-backed wallet forgets the used ones on every restart, so its next payment to that contact reuses an address and links the two payments publicly on-chain. iOS is not affected: it stores these addresses through its own host persistence.
Risks: Low. Only the SQLite load path changes, no consensus code. A Strict load now fails on a contact address it cannot restore: one past the 10,000-index derivation cap (more than 10,000 payments to one contact), or one on a pool type the account lacks (a corrupt row). Recovery loads those wallets and counts the claim. The over-cap regression derives 10,001 addresses and takes about 8 s in debug.
Issue being fixed or feature implemented
Fixes #5548.
apply_persisted_core_stateroutes restored addresses only throughall_funding_accounts_mut(), which excludesdashpay_external_accounts. A contact's storedused = 1andreserved_atrows therefore never reach its pool, and a paid contact address reloads available. Split out of #5465, which reserves the contact address across the broadcast; this limitation exists onv5.1-devindependently of that PR.What was done?
core_pool::load_contact_pool_claims(new) returns everydashpay_externalrow that is used, reserved, or has a materialisedcore_utxoscoin on its script (sweep placeholders excluded), with its exact(pool_type, address_index). The script length is gated before the read, as in the sibling loaders.rehydrate::restore_contact_pool_claims(new, called bySqlitePersister::loadafterapply_persisted_core_state):UnresolvedUtxoAddress;MAX_REHYDRATION_DERIVATION_INDEX, on a pool type the account has no pool for, or on an underived slot with no xpub cannot be kept used:RehydrationEnsureDerived, fatal under Strict;OrphanedUtxoOwner.refill_gap_windowis extracted from the funds-account fixpoint loop, so both paths use the same ImpliedRefill pre-flight andMAX_REHYDRATION_GAP_REFILLcap.load_used_addresses_with_ctxleavesdashpay_externalrows out, andowning_account_for_scriptranks them last. When two identities in one wallet are each other's contacts, a script shared by adashpay_externalrow and adashpay_receivingrow now resolves to the receiving account and keeps its reuse guard.apply_persisted_core_stateno longer routes contact-owned used addresses to funds account 0.How Has This Been Tested?
tests/integration/sqlite_used_core_addresses.rs: full store → reopen →load()for a contact pool:tests/integration/sqlite_core_pool_writer.rs: the claim loader returns an unsettled contact reservation and ignores a sweep placeholder, and a used address shared with a contact's external row guards the receiving account.tests/integration/sqlite_blob_size_gate_on_load.rs: an oversize contact script is rejected before the read.cargo test -p platform-wallet-storage --all-features: 431 lib + 564 integration + 4 + 5 doc pass.cargo clippy -p platform-wallet-storage --all-features --testsandcargo fmt --checkare clean.Breaking Changes
None in the API:
ContactPoolClaim,load_contact_pool_claimsandrestore_contact_pool_claimsare additions. On load, a SQLite wallet holding a contact claim past the derivation cap or on a missing pool type now fails a Strict load instead of silently dropping the claim; Recovery still loads it.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