feat(key-wallet-manager): carry addresses_derived on TransactionDetected / BlockProcessed#725
Open
QuantumExplorer wants to merge 5 commits intov0.42-devfrom
Open
feat(key-wallet-manager): carry addresses_derived on TransactionDetected / BlockProcessed#725QuantumExplorer wants to merge 5 commits intov0.42-devfrom
QuantumExplorer wants to merge 5 commits intov0.42-devfrom
Conversation
…ted / BlockProcessed Address-pool extensions triggered by gap-limit maintenance during tx / block processing now ride along on the existing event the persister already consumes, rather than living silently in Rust memory. UTXOs landing on freshly derived addresses keep their parent CoreAddress row in downstream stores. `AddressPool::maintain_gap_limit` returns `Vec<AddressInfo>` (was `Vec<Address>`); the wallet checker tags each new entry with the owning account type and pool type via a new `DerivedAddressInfo`, and the manager projects that into `DerivedAddress` (account_type, pool_type, derivation_index, derivation_path, address, public_key) on the event. Block emission de-duplicates by (account_type, pool_type, index) so two records pushing the same boundary collapse to one entry. Non-ECDSA pools (BLS / EdDSA) are silently skipped during projection. IS-lock and sync-height events are unchanged — those paths don't extend the pool. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 24 minutes and 17 seconds.Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #725 +/- ##
=============================================
+ Coverage 70.86% 70.95% +0.08%
=============================================
Files 319 319
Lines 68183 68403 +220
=============================================
+ Hits 48320 48535 +215
- Misses 19863 19868 +5
|
`AccountType` is now imported in account_checker.rs, which makes the explicit `(crate::account::AccountType)` link target redundant under `-D warnings`. And `[AddressInfo]` in key-wallet-manager/src/lib.rs needs the fully-qualified path since the type isn't imported into the crate root. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ivedAddress `account_type` (which carries Dashpay identity ids, IdentityTopUp's registration_index, and the like), `pool_type`, and `derivation_index` fully determine the path. Consumers that need a rendered path can recompute it deterministically rather than carrying a redundant String on every event. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
reviewed
May 4, 2026
Contributor
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean expansion of TransactionDetected/BlockProcessed with addresses_derived, plus solid plumbing through key-wallet's DerivedAddressInfo. No blocking issues: the FFI deferral and Dashpay xpub gap are pre-existing limitations explicitly out of scope. Concerns are about test fidelity (dedup branch isn't actually exercised; payload fields aren't pinned), silent drops in DerivedAddress::from_info that hide future regressions, and small fragility around maintain_gap_limit's new shape.
Reviewed commit: ee0adcd
🟡 5 suggestion(s) | 💬 3 nitpick(s)
1 additional finding
💬 nitpick: Dashpay Core accounts still skip gap-limit maintenance entirely
key-wallet/src/wallet/helper.rs (lines 863-866)
extended_public_key_for_account_type returns None for DashpayReceivingFunds / DashpayExternalAccount, and WalletTransactionChecker only runs maintain_gap_limit when an xpub is available (wallet_checker.rs:170-183). So although the standard tx router still routes Dashpay account types, no addresses_derived entries can ever be emitted for them. This is pre-existing behavior — not a regression from this PR — but it does mean the new persistence contract has a known blind spot for Dashpay single-pool ECDSA accounts. Worth either wiring the helper or documenting the gap so consumers don't assume the contract holds for every routed account type.
🤖 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 `key-wallet-manager/src/event_tests.rs`:
- [SUGGESTION] lines 910-956: Dedup test does not actually exercise the dedup branch
`test_block_with_two_records_pushing_external_boundary_dedupes` sends two txs paying to the same `highest_addr` and asserts that the resulting `addresses_derived` is exactly `gap_limit` entries. But it cannot trigger the dedup branch in `project_derived_addresses`. Walking the wallet_checker flow (`wallet_checker.rs:166-199`):
- After tx1: `mark_address_used` sets `highest_used = highest_before`; `maintain_gap_limit` derives indices `highest_before+1 ..= highest_before+gap_limit` (so `target == highest_generated == highest_before + gap_limit`).
- After tx2: `mark_address_used` is a no-op for the same index; `maintain_gap_limit` recomputes `target = highest_used + gap_limit = highest_before + gap_limit`, the `while highest_generated < target` guard is false, and the loop runs zero times.
So `per_wallet_derived` ends up with exactly `gap_limit` entries from tx1, no overlapping `(account, pool, index)` keys are fed into `project_derived_addresses`, and the `seen.insert` early-continue arm is never hit. The `keys.dedup()` invariant in the assert passes vacuously. A regression that drops the dedup key (e.g. switching to `(account, pool)` only, or removing `seen.insert` entirely) would not fail this test.
A real dedup test would need to either unit-test `project_derived_addresses` directly with two overlapping `DerivedAddressInfo` entries, or construct a flow where two checker invocations within the same block actually produce the same `(account_type, pool_type, index)` triple.
- [SUGGESTION] lines 743-957: New tests don't pin the persistence-critical address/public_key payload
The new event tests assert pool counts, pool types, and contiguous derivation indices, but never verify that each emitted `DerivedAddress` actually carries the expected `address` and matching 33-byte compressed `public_key`. Since the entire purpose of this field is so persisters can write `CoreAddress` rows transactionally, a regression in `DerivedAddress::from_info` (e.g. wrong key bytes copied, address built from the wrong derivation path) could corrupt the persisted data while the suite stays green. At least one test should pin `derived.address` and `derived.public_key` against the wallet's `AddressInfo` for the same `(account, pool, index)`. Coverage of an absent/single-pool special account would also surface failure modes that the External/Internal-only tests miss.
In `key-wallet-manager/src/events.rs`:
- [SUGGESTION] lines 58-67: DerivedAddress::from_info silently drops non-ECDSA / non-33-byte entries with no observability
`from_info` returns `None` in three distinct cases — `public_key.is_none()`, `PublicKeyType::ECDSA(bytes)` with `bytes.len() != 33`, and BLS / EdDSA — and `project_derived_addresses` then silently discards them. Today the producer is `address_pool::generate_address_at_index`, which always stores 33-byte compressed keys via `dashcore::PublicKey::new(...).to_bytes()`, so the length-mismatch arm is unreachable from this code path. The BLS/EdDSA arm is reachable in principle if a future change wires gap-limit extension into a BLS/EdDSA pool from the Core-tx path — and at that point the freshly derived address will silently disappear from `TransactionDetected` / `BlockProcessed`, defeating the persistence contract this PR is built around (downstream persisters will orphan UTXOs landing on those addresses with no log to trace it from).
A one-line `tracing::warn!` (or `debug!` for the BLS/EdDSA case + `warn!` for the ECDSA length mismatch, which would indicate a bug) at the drop sites would make the failure observable. Including `account_type`, `pool_type`, and `index` in the log makes it actionable.
- [SUGGESTION] line 57: DerivedAddress::from_info should be pub(crate)
`DerivedAddress::from_info` is `pub`, but its only argument type is `key_wallet::transaction_checking::DerivedAddressInfo` and the only in-tree caller is the private `project_derived_addresses` helper. Making it `pub` exposes a `key_wallet::transaction_checking` type as part of `key-wallet-manager`'s public API surface and turns future refactors of `DerivedAddressInfo` (or its projection rules) into avoidable source-compatibility hazards for external users. Restricting to `pub(crate)` keeps the compatibility boundary at `WalletEvent` / `DerivedAddress`, where it belongs.
In `dash-spv-ffi/src/callbacks.rs`:
- [SUGGESTION] lines 787-855: FFI dispatch drops addresses_derived; persistence guarantee not yet realized for FFI consumers
`FFIWalletEventCallbacks::dispatch` matches both affected variants with `addresses_derived: _` (lines 787 and 855), and the existing `OnTransactionDetectedCallback` / `OnWalletBlockProcessedCallback` C function pointers have no slot for the new payload. The Rust side correctly avoids an ABI break (`WalletEvent` is not `repr(C)`, so growing the variant is invisible to C), but Swift / mobile consumers behind `dash-spv-ffi` cannot yet observe the derived-address metadata, so the orphaned-`CoreAddress`-row failure mode this PR is designed to prevent persists for FFI persisters until a paired callback (or new signature) lands.
Per the PR description this is an intentional deferral, which is fine — but a short tracking comment / TODO in the dispatch arms (or a follow-up issue link) would keep the deferral from being lost. No correctness or panic-safety issue at the boundary as written.
…al dedup test Address review feedback on PR #725: - `DerivedAddress::from_info` is now `pub(crate)` since the only caller is the in-crate `project_derived_addresses` helper. Keeps the compatibility boundary at `WalletEvent` / `DerivedAddress` and avoids re-exporting `key_wallet::transaction_checking::DerivedAddressInfo` through this crate's public API. - Drops in `from_info` now log via `tracing` — `warn!` for the unreachable-by-construction "ECDSA but not 33 bytes" arm (which signals a bug if ever hit), `warn!` for "no public key", and `debug!` for the BLS / EdDSA fall-through (expected if a future change wires gap-limit extension into one of those pools, currently unreachable). - Mempool path now attributes derivations per-record via partition on `record.account_type == DerivedAddressInfo.account_type`. A tx that pays into multiple accounts of the same wallet now ships each record with its own account's derivations — consistent with the field's documented "transactional with `record`" semantics. Doc tightened accordingly. - `maintain_gap_limit` previously silently produced a shorter `Vec` if the just-inserted entry wasn't found; now it panics with an explicit invariant message, turning a regression that breaks the post-insert invariant into a loud failure rather than a silent infinite loop on the outer `while`. - New unit tests for `project_derived_addresses` directly exercise the dedup branch (overlapping `(account, pool, index)` → first-wins), distinct indices, distinct pools, and the missing-pubkey drop. The existing block-level test couldn't reach the dedup branch since `maintain_gap_limit` is idempotent on a second call. - The "highest external" event test now pins each emitted `(address, public_key)` against the wallet's own `AddressInfo` for the same `(account, pool, index)`. Catches regressions in `from_info` that would silently corrupt persister rows while keeping the count-and-index assertions green. - `dash-spv-ffi` dispatch arms tagged with TODOs documenting that `addresses_derived` is dropped at the FFI seam pending a paired callback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Closes the FFI-side TODOs left by PR #725: the Rust event already carries `Vec<DerivedAddress>`, but the C callback signature had no slot for it, so the field was dropped at the dispatch seam — defeating the persistence contract for FFI consumers (Swift / mobile persisters mirroring the address pool to disk). - New `FFIDerivedAddress { account_type, pool_type, derivation_index, address, public_key[33] }` and `FFIDerivedAddressPoolType` (4-variant, mirrors `key_wallet::AddressPoolType` 1:1; kept distinct from the existing `FFIAddressPoolType` which collapses Absent / AbsentHardened). - `OnTransactionDetectedCallback` and `OnWalletBlockProcessedCallback` signatures grow `addresses_derived: *const FFIDerivedAddress, addresses_derived_count: u32`. The C ABI is broken — every consumer has to update its callback signature; this is intentional, since the alternative (a paired auxiliary callback) would let consumers persist records without the matching address rows and reintroduce the orphan-`CoreAddress`-row failure mode the field is designed to prevent. - Memory: `FFIDerivedAddress` owns the `address` C string (freed on drop via `CString::from_raw`) and the `FFIAccountType` (which has its own Drop for the optional Dashpay identity ids). The `Vec` lives on the Rust stack across the callback and is dropped after. - `dash-spv-ffi/src/bin/ffi_cli.rs` and the dashd integration test callbacks updated for the new signatures (counts logged in the CLI; arrays are not retained in tests). - cbindgen header regenerates correctly (verified via `cargo build`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
TransactionDetected/BlockProcessedevents (no new event variant), so consumers can persist new pool rows transactionally with the tx/block records that triggered them.CoreAddressrow in downstream persisters that mirror the pool to disk via the existing event seam.(account_type, pool_type, derivation_index)so two records pushing the same boundary collapse to one entry.Shape
New
DerivedAddress(inkey-wallet-manager) carriesaccount_type,pool_type(External/Internal/Absent/AbsentHardened),derivation_index,derivation_path,address,public_key: [u8;33]. Non-ECDSA pools (BLS / EdDSA) are silently skipped during projection — those paths don't currently trigger gap-limit extension on Core txs.TransactionInstantLockedandSyncHeightAdvancedare unchanged — IS-lock application doesn't mark addresses used, and sync-height isn't bound to a tx.Plumbing
AddressPool::maintain_gap_limitnow returnsVec<AddressInfo>(wasVec<Address>).wallet_checker.rstags each freshly derivedAddressInfowith the owning account type and pool type via a newDerivedAddressInfoinkey-wallet.key-wallet-managerprojectsDerivedAddressInfointoDerivedAddressand dedups before emitting.Test plan
cargo test -p key-wallet -p key-wallet-managercargo clippy -p key-wallet -p key-wallet-manager --all-targetscargo fmt --check -p key-wallet -p key-wallet-managercargo build --workspace --testsevent_tests.rscases:addresses_derivedof lengthgap_limit, all on the right (account, pool), contiguous indicesaddresses_derivedemptygap_limit(not 2×gap_limit)TransactionInstantLockedexhaustively pattern-matched withoutaddresses_derived(compile-time guard against accidental drift)Notes for reviewers
v0.42-dev(the worktree's tracked base), notv0.42-platform-nightlyas the spec mentioned — happy to rebase onto the platform branch if that's what the reviewer wants.dash-spv-fficallback dispatch acknowledges the new field withaddresses_derived: _; no FFI surface change yet (downstream Swift adapter will add the fan-out).🤖 Generated with Claude Code