feat(platform-wallet): pool BIP44 + BIP32 + DashPay receiving funds on the asset-lock path - #4350
Open
bfoss765 wants to merge 1 commit into
Open
feat(platform-wallet): pool BIP44 + BIP32 + DashPay receiving funds on the asset-lock path#4350bfoss765 wants to merge 1 commit into
bfoss765 wants to merge 1 commit into
Conversation
…n the asset-lock path dashpay#4329 pooled the send path and left asset locks single-account. An invitation, identity registration or top-up could only be funded from ONE BIP44 account, so a wallet holding its balance across the standard families and its DashPay contact-receiving accounts had to sweep them together first and lock out of the sweep — an extra on-chain hop, an extra fee, and a transparent address reused for the privilege. `ASSET_LOCK_FUNDING_SOURCES` (the same set as `SEND_FUNDING_SOURCES`) ends that shape: coin selection draws from the union, change returns to BIP44, and sources the wallet has nothing for are skipped. `build_asset_lock_transaction_with_funding` and `broadcast_funded_asset_lock_with_funding` now take the source list plus a `source_index` instead of a single `AssetLockFundingAccount`, and the historical entry points — `build_asset_lock_transaction`, `broadcast_funded_asset_lock`, `create_funded_asset_lock_proof` — keep their signatures and simply pass the pooled set, so every call site above them (invitation `create_invitation`, identity registration, top-up, platform-address and shielded funding, and the `asset_lock_manager_build_transaction` FFI export) becomes pooled without new plumbing. Nothing gained an elective funding selector it does not need. Reservation reconciliation is the part that had to change shape. `release_reservation_after_rejected_broadcast` now takes the contributing account LIST rather than one `ReservedFundingAccount`: a pooled build reserves in each contributing account's own set under one owner token, so a release reaching only the first account would strand the rest of the inputs until the 24-block TTL backstop and make an immediate retry fail with spurious insufficient funds. The build returns those accounts (upstream's `AssetLockResult::funding_accounts`) and both rejection paths — the undersized-drain abandon and the rejected broadcast — release across all of them, still owner-guarded (dashpay#4185). `ReservedFundingAccount` is gone; `AccountType` already names every family, including the DashPay accounts the old enum could not. `funding_tx_record` had to widen with it. It resolved the funding transaction in BIP44 and CoinJoin at the tracked index only, but key-wallet files a transaction under every account its inputs touch, and a pooled lock may take nothing from BIP44 — funded entirely out of BIP32 or a contact account. The lookup missed it, which burns the proof wait and is outright fatal under `NoPlatformPersistence`, whose persister fallback always returns `None`. It now covers the standard pair and CoinJoin at the index, then the DashPay receiving accounts by txid (they span their own indices) — the same class of gap dashpay#4336's CoinJoin fix closed. CoinJoin funding is untouched: still drain-only, still a single account, now routed through `create_funded_asset_lock_proof_with_funding`, which converts it to a one-element source list. Upstream additionally rejects a CoinJoin source pooled with any other, since spending mixed outputs alongside transparent ones links them and undoes the mixing. Depends on dashpay/rust-dashcore#935; Cargo.toml is pinned to that branch's rev so CI is honest, and MUST be re-pointed to dashpay/rust-dashcore `dev` on merge.
bfoss765
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
August 9, 2026 19:48
Contributor
📝 WalkthroughWalkthroughChangesThe wallet now pools asset-lock funding across BIP44, BIP32, and DashPay receiving accounts. Build, broadcast, tracking, transaction lookup, and reservation cleanup support multiple contributing accounts. Workspace Dash dependencies use a new repository revision. Asset-lock funding contract
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Collaborator
|
⛔ Blockers found — Opus deferred (commit 72e89a3) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Cargo.toml (1)
55-62: 🔒 Security & Privacy | 🔵 TrivialTrack the temporary fork migration before release.
The release workflow runs
yarn buildwithCARGO_BUILD_PROFILE=release, so it can publish artifacts built frombfoss765/rust-dashcore. Whendashpay/rust-dashcore#935merges, replace all eight sources withdashpay/rust-dashcoreand refreshCargo.lock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` around lines 55 - 62, Before release, update all eight rust-dashcore dependency entries—dashcore, dash-network-seeds, dash-spv, key-wallet, key-wallet-ffi, key-wallet-manager, dash-network, and dashcore-rpc—to use the dashpay/rust-dashcore repository after PR `#935` merges, then regenerate Cargo.lock to reflect the new sources and revisions.packages/rs-platform-wallet/src/wallet/asset_lock/build.rs (1)
200-222: 🗄️ Data Integrity & Integration | 🔵 TrivialKeep the current dependency pin until pull request 935 merges. Revision
1a1263a27fa96f7dbf9b283f6fc14101149ab5fdcontains the required API and types. Thedevbranch still has the old API, and pull request 935 remains open. Move the workspace pin todevafter the merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/asset_lock/build.rs` around lines 200 - 222, Keep the workspace dependency pinned to revision 1a1263a27fa96f7dbf9b283f6fc14101149ab5fd for now, since build_asset_lock_with_signer depends on its API and types. Do not switch the pin to dev until pull request 935 has merged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Cargo.toml`:
- Around line 55-62: Before release, update all eight rust-dashcore dependency
entries—dashcore, dash-network-seeds, dash-spv, key-wallet, key-wallet-ffi,
key-wallet-manager, dash-network, and dashcore-rpc—to use the
dashpay/rust-dashcore repository after PR `#935` merges, then regenerate
Cargo.lock to reflect the new sources and revisions.
In `@packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- Around line 200-222: Keep the workspace dependency pinned to revision
1a1263a27fa96f7dbf9b283f6fc14101149ab5fd for now, since
build_asset_lock_with_signer depends on its API and types. Do not switch the pin
to dev until pull request 935 has merged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c5b6ea35-6408-4ba0-a332-31eee9cd0e35
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlpackages/rs-platform-wallet-ffi/src/asset_lock/build.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/wallet/asset_lock/build.rspackages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rspackages/rs-platform-wallet/src/wallet/asset_lock/tracked.rspackages/rs-platform-wallet/src/wallet/core/mod.rspackages/rs-platform-wallet/src/wallet/core/transaction.rspackages/rs-platform-wallet/src/wallet/identity/network/invitation.rspackages/rs-platform-wallet/src/wallet/identity/network/top_up.rspackages/rs-platform-wallet/src/wallet/reservations.rs
thepastaclaw
suggested changes
Aug 9, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The pooled asset-lock implementation has three merge blockers: the workspace still depends on a contributor-owned rust-dashcore fork, invitation persistence failures leak pooled input reservations, and the FFI restart bridge can discard valid BIP32- or DashPay-only funding records. The family-aware lookup is otherwise coherent, but its new branches need direct regression tests and all public foreign-language funding documentation must describe the new pooled semantics.
Source: reviewers gpt-5.6-sol (Codex general, security-auditor, rust-quality, and ffi-engineer); final verifier gpt-5.6-sol (Codex). Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 2 suggestion(s)
3 additional finding(s) omitted (not in diff).
🤖 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 `Cargo.toml`:
- [BLOCKING] Cargo.toml:55-62: Do not merge with workspace dependencies pinned to a personal fork
All eight rust-dashcore workspace dependencies resolve from `bfoss765/rust-dashcore`, including foundational crates such as `dashcore`, `dash-spv`, and `key-wallet`. The exact required revision is currently exposed as `dashpay/rust-dashcore` pull request 935's head, while the project-owned `dev` branch remains at `b056d07c61f8618f05082552bbb88072290d57c1`; the PR description also explicitly says not to merge with this fork pin. Merging this revision would make Platform builds and release artifacts depend on a contributor-owned repository. After rust-dashcore#935 merges, repoint every entry to the project-owned repository at the merged revision and regenerate `Cargo.lock`.
In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:891-900: Release pooled reservations when invitation persistence aborts
After `build_asset_lock_transaction_with_funding` succeeds, each account in `funding_accounts` can hold selected inputs under `reservation_token`. If persisting or flushing the invitation funding index fails, this branch returns before tracking or broadcasting the transaction but never reconciles those reservations. The old single-account branch already retained its reservation here, and this PR worsens that behavior by allowing the abandoned build to reserve inputs across BIP44, BIP32, and DashPay accounts simultaneously. An immediate retry therefore sees those inputs as unavailable until the 24-block TTL sweep. Drop the serialization guard, release the transaction's reservations across every contributing account, and then return the persistence error; extend the existing flush-failure test with an immediate rebuild so it verifies the inputs were released.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:5745-5755: Restore bridge drops pooled locks when the indexed BIP44 account is absent
The pooled source list is lenient: a valid asset lock can be funded entirely by the BIP32 account or DashPay receiving accounts even when `standard_bip44_accounts[account_index]` is absent. The persisted `TrackedAssetLock.account_index` remains only the standard source index, but this restore helper still inserts exclusively into that BIP44 slot and drops the record when it does not exist. After restart, the funding transaction is then absent from every in-memory account map, so the widened `funding_tx_record` lookup cannot recover it and the chain-lock cascade can leave an already-broadcast lock stuck at `Broadcast`. Restore the transaction into an eligible present account family—or extend the restore protocol with sufficient routing information—and add a restart test covering a BIP32- or DashPay-only lock without a BIP44 account at the source index.
In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs:71-80: Add direct coverage for the newly searched BIP32 and DashPay families
The shared lookup now adds behaviorally important BIP32 and DashPay receiving-account branches, but its unit tests still insert records only into BIP44 and CoinJoin accounts. The pooled build tests stop after broadcast and therefore do not exercise proof lookup or the `NoPlatformPersistence` path this widening is intended to support. Add one record stored exclusively in `standard_bip32_accounts` and another stored exclusively in a DashPay receiving account. The DashPay test should use an account whose own index differs from `account_index`, confirming that DashPay accounts are intentionally searched by txid rather than the tracked source index.
In `packages/rs-platform-wallet-ffi/src/identity_top_up.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/identity_top_up.rs:172-175: Foreign API documentation still promises BIP44-only funding
This public C API documentation says `account_index` selects the sole BIP44 funding account, but the call now pools the BIP44 and BIP32 accounts at that index with every DashPay receiving account. The same stale contract remains on identity registration, platform-address funding, shielded asset-lock and seed-pool exports, and their Swift wrappers. Foreign callers following those docs may present an account-specific funding choice while Rust actually spends and links UTXOs from other families. Update every affected C and Swift surface to state that the index addresses the standard families and does not constrain the DashPay contributors included in the pool.
Comment on lines
+55
to
+62
| dashcore = { git = "https://github.com/bfoss765/rust-dashcore", rev = "1a1263a27fa96f7dbf9b283f6fc14101149ab5fd" } | ||
| dash-network-seeds = { git = "https://github.com/bfoss765/rust-dashcore", rev = "1a1263a27fa96f7dbf9b283f6fc14101149ab5fd" } | ||
| dash-spv = { git = "https://github.com/bfoss765/rust-dashcore", rev = "1a1263a27fa96f7dbf9b283f6fc14101149ab5fd" } | ||
| key-wallet = { git = "https://github.com/bfoss765/rust-dashcore", rev = "1a1263a27fa96f7dbf9b283f6fc14101149ab5fd" } | ||
| key-wallet-ffi = { git = "https://github.com/bfoss765/rust-dashcore", rev = "1a1263a27fa96f7dbf9b283f6fc14101149ab5fd" } | ||
| key-wallet-manager = { git = "https://github.com/bfoss765/rust-dashcore", rev = "1a1263a27fa96f7dbf9b283f6fc14101149ab5fd" } | ||
| dash-network = { git = "https://github.com/bfoss765/rust-dashcore", rev = "1a1263a27fa96f7dbf9b283f6fc14101149ab5fd" } | ||
| dashcore-rpc = { git = "https://github.com/bfoss765/rust-dashcore", rev = "1a1263a27fa96f7dbf9b283f6fc14101149ab5fd" } |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Do not merge with workspace dependencies pinned to a personal fork
All eight rust-dashcore workspace dependencies resolve from bfoss765/rust-dashcore, including foundational crates such as dashcore, dash-spv, and key-wallet. The exact required revision is currently exposed as dashpay/rust-dashcore pull request 935's head, while the project-owned dev branch remains at b056d07c61f8618f05082552bbb88072290d57c1; the PR description also explicitly says not to merge with this fork pin. Merging this revision would make Platform builds and release artifacts depend on a contributor-owned repository. After rust-dashcore#935 merges, repoint every entry to the project-owned repository at the merged revision and regenerate Cargo.lock.
source: ['codex']
Comment on lines
+71
to
+80
| let at_index = [ | ||
| accounts.standard_bip44_accounts.get(&account_index), | ||
| accounts.standard_bip32_accounts.get(&account_index), | ||
| accounts.coinjoin_accounts.get(&account_index), | ||
| ]; | ||
| at_index | ||
| .into_iter() | ||
| .flatten() | ||
| .chain(accounts.dashpay_receival_accounts.values()) | ||
| .find_map(|account| account.transactions().get(txid).cloned()) |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Add direct coverage for the newly searched BIP32 and DashPay families
The shared lookup now adds behaviorally important BIP32 and DashPay receiving-account branches, but its unit tests still insert records only into BIP44 and CoinJoin accounts. The pooled build tests stop after broadcast and therefore do not exercise proof lookup or the NoPlatformPersistence path this widening is intended to support. Add one record stored exclusively in standard_bip32_accounts and another stored exclusively in a DashPay receiving account. The DashPay test should use an account whose own index differs from account_index, confirming that DashPay accounts are intentionally searched by txid rather than the tracked source index.
source: ['codex']
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
#4329 pooled the send path and left asset locks single-account. An invitation, identity registration or top-up could only be funded from ONE BIP44 account, so a wallet holding its balance across the standard families and its DashPay contact-receiving accounts had to sweep them together first and lock out of the sweep — an extra on-chain hop, an extra fee, and a transparent address reused for the privilege. On Android that sweep-then-lock shape is what users actually hit.
This is the approved follow-up: the same pooling, for asset locks.
Depends on dashpay/rust-dashcore#935.
Cargo.tomlis pinned to that branch's rev so CI here is honest about what it is testing; the pin must be re-pointed todashpay/rust-dashcoredevwhen #935 merges — please don't merge this with the fork pin in place.What was done?
ASSET_LOCK_FUNDING_SOURCES(deliberately the same set asSEND_FUNDING_SOURCES: BIP44, BIP32,AllDashpayReceivingFunds) is now the default for asset-lock funding. Coin selection draws from the union, the first source supplies the change address so change returns to BIP44, and sources this wallet has nothing for — no BIP32 account, no contacts — are skipped rather than fatal.build_asset_lock_transaction_with_fundingandbroadcast_funded_asset_lock_with_fundingtake the source list plus asource_indexin place of a singleAssetLockFundingAccount. The historical entry points —build_asset_lock_transaction,broadcast_funded_asset_lock,create_funded_asset_lock_proof— keep their exact signatures and just pass the pooled set, so every call site above them becomes pooled with no new plumbing:create_invitation, identity registration, top-up, platform-address and shielded funding, and theasset_lock_manager_build_transactionFFI export. Nothing gained an elective funding selector it does not need, and #4337'sbuild_asset_lock_transaction(…, account_index, …)recovery helper call is unaffected.Reservation reconciliation — the part to review hardest
release_reservation_after_rejected_broadcastnow takes the contributing account list instead of oneReservedFundingAccount. A pooled build reserves in each contributing account's ownReservationSetunder the one owner token, so a release reaching only the first account would strand the rest of the inputs until the 24-block TTL backstop, and an immediate retry would fail with spurious insufficient funds. The build returns those accounts (upstream'sAssetLockResult::funding_accounts, which is the contributor list — selection routinely takes nothing from most offered accounts, and a list naming every contact would make this scale with the address book), and both rejection paths — the undersized-drain abandon and the rejected broadcast — release across all of them, still owner-guarded (#4185).ReservedFundingAccountis deleted:AccountTypealready names every family, including the DashPay accounts the old enum could not express at all.A lookup that had to widen with it
funding_tx_recordresolved the funding transaction in BIP44 and CoinJoin at the tracked index only. key-wallet files a transaction under every account its inputs touch, and a pooled lock may take nothing from BIP44 — funded entirely out of BIP32 or a contact account. The lookup would miss it, which burns the proof wait and is outright fatal underNoPlatformPersistence, whose persister fallback always returnsNone. It now covers the standard pair and CoinJoin at the index, then the DashPay receiving accounts by txid (they span their own indices). This is the same class of gap #4336's CoinJoin fix closed, arriving here because pooling opens it.TrackedAssetLock.account_indexkeeps its meaning as the source index and its persisted shape — no schema change — but its doc now says plainly that it is not a record of which accounts funded the lock.CoinJoin is untouched
Still drain-only, still a single account, now routed through
create_funded_asset_lock_proof_with_funding, which converts it to a one-element source list. Upstream additionally rejects a CoinJoin source pooled with any other, since spending mixed outputs alongside transparent ones links them and undoes the mixing. #4327's flow and itsundersized_drain_abandoned_before_broadcasttest are unchanged.Docs that asserted the old invariant
AssetLockFunding::FromWalletBalance("This exact-amount form is BIP44-only … BIP32 funding remains unsupported"),top_up_identity'saccount_index("Only BIP44 standard accounts are supported today"), and the invitation build comment all described the single-account world. They now describe the pooled one rather than silently contradicting the code.How Has This Been Tested?
cargo test -p platform-wallet— 613 lib tests + integration suites, 0 failures.cargo check --workspace --all-targetsclean;cargo clippy -p platform-wallet -p platform-wallet-ffi --all-targets -- -D warningsclean;cargo fmtapplied.New tests, on the fixtures #4329 added for the pooled send:
pooled_asset_lock_spans_the_standard_families— a 1,000,000-duff lock against two 700,000-duff accounts. Before pooling this wasCoreInsufficientFunds.pooled_asset_lock_spends_dashpay_contact_funds— the same against a realDashpayReceivingFundscontact account, so the contact path cannot silently degrade to BIP44 + BIP32.rejected_pooled_broadcast_releases_every_contributing_account— the reservation hazard. A rejected pooled broadcast, then an identical rebuild that can only succeed if both families' inputs came back.I checked that last one is not vacuous: with the release loop truncated to the first account it fails with
Insufficient funds: available 700000, required 1000000, which is exactly the stranding it is meant to catch. The upstream siblings in #935 were mutation-checked the same way.Breaking Changes
Internal to the crate — no FFI signature changes, no persistence format change:
build_asset_lock_transaction_with_funding/broadcast_funded_asset_lock_with_fundingtakefunding_sources: &[AccountTypePreference], source_index: u32; the former also returns the contributing accounts.create_funded_asset_lock_proof_with_fundingkeepsAssetLockFundingAccountand is now explicitly the whole-balance drain form.pub(crate) enum ReservedFundingAccountremoved in favour ofAccountType.platform_wallet::ASSET_LOCK_FUNDING_SOURCES.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Documentation