fix(dash-spv): wallet reload pool-state loss past gap limit#759
Open
ZocoLini wants to merge 1 commit into
Open
fix(dash-spv): wallet reload pool-state loss past gap limit#759ZocoLini wants to merge 1 commit into
ZocoLini wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR encapsulates the public ChangesUTXO Field Encapsulation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@key-wallet/src/managed_account/managed_core_funds_account.rs`:
- Around line 155-175: ensure_address_indexed currently mutates address pools
during probing because it calls generate_address_at_index(..., true) on
address_pools_mut(), which can grow pools even when the address is not
derivable; change to a two‑phase approach: probe using non‑mutating reads (use
address_pools() and call generate_address_at_index with the non‑mutating flag or
an equivalent no‑grow path) across MAX_SCAN and pool_count to find a match, and
only if a match is found perform the actual mutation/commit (calling
generate_address_at_index with grow=true or otherwise extending
address_pools_mut()) to update state atomically; refer to
ensure_address_indexed, generate_address_at_index, address_pools(),
address_pools_mut(), and MAX_SCAN to locate the code to modify.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f05f0042-6e45-48cd-b870-e507e569311f
📒 Files selected for processing (12)
dash-spv/tests/dashd_sync/tests_restart.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/utxo_tests.rskey-wallet/src/managed_account/managed_core_funds_account.rskey-wallet/src/test_utils/wallet.rskey-wallet/src/tests/balance_tests.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
Comment on lines
+155
to
+175
| fn ensure_address_indexed( | ||
| &mut self, | ||
| address: &Address, | ||
| key_source: &KeySource, | ||
| ) -> crate::error::Result<()> { | ||
| const MAX_SCAN: u32 = 10_000; | ||
| let pool_count = self.keys.managed_account_type().address_pools().len(); | ||
| for index in 0..MAX_SCAN { | ||
| for pool_idx in 0..pool_count { | ||
| let pool = &mut self.keys.managed_account_type_mut().address_pools_mut()[pool_idx]; | ||
| let derived = pool.generate_address_at_index(index, key_source, true)?; | ||
| if &derived == address { | ||
| return Ok(()); | ||
| } | ||
| } | ||
| } | ||
| Err(crate::error::Error::InvalidAddress(format!( | ||
| "address {} not derivable within scan window for this account", | ||
| address | ||
| ))) | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid mutating address pools while probing for a match.
ensure_address_indexed currently extends pools during scan (generate_address_at_index(..., true)), so a non-derivable address can leave partial pool growth before returning Err. That breaks atomicity and makes failure paths stateful.
💡 Proposed fix (two-phase probe/commit)
fn ensure_address_indexed(
&mut self,
address: &Address,
key_source: &KeySource,
) -> crate::error::Result<()> {
const MAX_SCAN: u32 = 10_000;
let pool_count = self.keys.managed_account_type().address_pools().len();
for index in 0..MAX_SCAN {
for pool_idx in 0..pool_count {
- let pool = &mut self.keys.managed_account_type_mut().address_pools_mut()[pool_idx];
- let derived = pool.generate_address_at_index(index, key_source, true)?;
- if &derived == address {
+ let is_match = {
+ let pool = &mut self.keys.managed_account_type_mut().address_pools_mut()[pool_idx];
+ let derived = pool.generate_address_at_index(index, key_source, false)?;
+ &derived == address
+ };
+ if is_match {
+ let pool = &mut self.keys.managed_account_type_mut().address_pools_mut()[pool_idx];
+ let _ = pool.generate_address_at_index(index, key_source, true)?;
return Ok(());
}
}
}
Err(crate::error::Error::InvalidAddress(format!(
"address {} not derivable within scan window for this account",
address
)))
}As per coding guidelines: "Implement atomic state updates: when modifying wallet state (transactions, UTXOs, balance, used addresses), update all related state atomically in Rust code".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn ensure_address_indexed( | |
| &mut self, | |
| address: &Address, | |
| key_source: &KeySource, | |
| ) -> crate::error::Result<()> { | |
| const MAX_SCAN: u32 = 10_000; | |
| let pool_count = self.keys.managed_account_type().address_pools().len(); | |
| for index in 0..MAX_SCAN { | |
| for pool_idx in 0..pool_count { | |
| let pool = &mut self.keys.managed_account_type_mut().address_pools_mut()[pool_idx]; | |
| let derived = pool.generate_address_at_index(index, key_source, true)?; | |
| if &derived == address { | |
| return Ok(()); | |
| } | |
| } | |
| } | |
| Err(crate::error::Error::InvalidAddress(format!( | |
| "address {} not derivable within scan window for this account", | |
| address | |
| ))) | |
| } | |
| fn ensure_address_indexed( | |
| &mut self, | |
| address: &Address, | |
| key_source: &KeySource, | |
| ) -> crate::error::Result<()> { | |
| const MAX_SCAN: u32 = 10_000; | |
| let pool_count = self.keys.managed_account_type().address_pools().len(); | |
| for index in 0..MAX_SCAN { | |
| for pool_idx in 0..pool_count { | |
| let is_match = { | |
| let pool = &mut self.keys.managed_account_type_mut().address_pools_mut()[pool_idx]; | |
| let derived = pool.generate_address_at_index(index, key_source, false)?; | |
| &derived == address | |
| }; | |
| if is_match { | |
| let pool = &mut self.keys.managed_account_type_mut().address_pools_mut()[pool_idx]; | |
| let _ = pool.generate_address_at_index(index, key_source, true)?; | |
| return Ok(()); | |
| } | |
| } | |
| } | |
| Err(crate::error::Error::InvalidAddress(format!( | |
| "address {} not derivable within scan window for this account", | |
| address | |
| ))) | |
| } |
🤖 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 `@key-wallet/src/managed_account/managed_core_funds_account.rs` around lines
155 - 175, ensure_address_indexed currently mutates address pools during probing
because it calls generate_address_at_index(..., true) on address_pools_mut(),
which can grow pools even when the address is not derivable; change to a
two‑phase approach: probe using non‑mutating reads (use address_pools() and call
generate_address_at_index with the non‑mutating flag or an equivalent no‑grow
path) across MAX_SCAN and pool_count to find a match, and only if a match is
found perform the actual mutation/commit (calling generate_address_at_index with
grow=true or otherwise extending address_pools_mut()) to update state
atomically; refer to ensure_address_indexed, generate_address_at_index,
address_pools(), address_pools_mut(), and MAX_SCAN to locate the code to modify.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #759 +/- ##
=============================================
+ Coverage 72.26% 72.27% +0.01%
=============================================
Files 320 320
Lines 70275 70316 +41
=============================================
+ Hits 50785 50823 +38
- Misses 19490 19493 +3
|
xdustinface
requested changes
May 13, 2026
Collaborator
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
This basically breaks the idea of the gap limit and seems more to be a workaround for an issue/misuse of the platform wallet.
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.
I has a little issue in iOS where, after restarting the app, the address pool didnt know enough addresswes to cover the UTXO that the transaction builder chose, with thi quick fix idea I ensure that, every time we find a UTXO and it gets inserted into the account, the address pool knows the addresses derivation path, so it is able to resolve it in a future
ManagedCoreFundsAccount::utxosand route all insertions through a newinsert_utxo(utxo, key_source)that reconciles the owning address into the account's address pool — fixing an issue where when trying to sign as trasaction using utxo which address past the initial gap limit lost its derivation path and made signing fail.dashd_syncregression test covering wallet rebuild with a UTXO at index 50 can find the derivation path for the address used.Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor