fix: `maintain_gap_limit` target calculation off by one by xdustinface · Pull Request #286 · dashpay/rust-dashcore

xdustinface · 2025-12-18T04:05:25Z

The target index was calculated incorrectly, causing one extra address to be generated.

See the adjusted test in 0f33cb2 which fails without the fix.

Summary by CodeRabbit

Bug Fixes
- Refined address generation behavior in account pool maintenance to more conservatively manage gap limits, reducing unnecessary address generation in certain scenarios.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

The target index was calculated incorrectly, causing one extra address to be generated. See the test added which fails without the fix.

coderabbitai · 2025-12-18T04:05:40Z

Walkthrough

The gap-limit maintenance logic in maintain_gap_limit has been reworked to generate addresses more conservatively. When no addresses have been used, the target moves from gap_limit to gap_limit - 1. When addresses are used, the target shifts from highest_used + gap_limit + 1 to highest_used + gap_limit, reducing the number of new addresses generated in each case.

Changes

Cohort / File(s)	Summary
Gap-limit logic adjustment `key-wallet/src/managed_account/address_pool.rs`	Modified `maintain_gap_limit` calculation: when no highest_used exists, target is now `gap_limit - 1` (was `gap_limit`); when highest_used exists, target is now `highest_used + gap_limit` (was `highest_used + gap_limit + 1`). Updated tests verify the new behavior: pool initialized with `gap_limit` addresses, `maintain_gap_limit` called with no used addresses produces no new addresses, and subsequent calls generate addresses conservatively as indices are marked used.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Focus areas:
- Verify the new gap-limit calculation logic is correct and intentional
- Confirm test cases accurately reflect the revised address generation behavior
- Check that the conservative approach aligns with address pool security requirements

Poem

🐰 The pool learns restraint, a calculus refined,
Gaps measured anew, fewer addresses aligned,
Conservative wisdom in numbers that grow,
Each address earned when the used ones show! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: fixing an off-by-one error in the maintain_gap_limit target calculation. This directly aligns with the primary purpose of the PR and the detailed changes in address_pool.rs.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix/gap-limit-target

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

key-wallet/src/managed_account/address_pool.rs (1)

903-917: Verify generate_lookahead target calculation for consistency.

Line 906 uses the pattern highest + self.lookahead_size + 1, which is similar to the pattern that was just fixed in maintain_gap_limit. If the intended semantics are the same (generate lookahead_size addresses beyond the highest used), this may also be off by one.

However, if "lookahead" is intentionally more aggressive than gap limit maintenance, the + 1 might be correct. Please verify the intended behavior.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 84a9552 and caf9231.

📒 Files selected for processing (1)

key-wallet/src/managed_account/address_pool.rs (2 hunks)

🧰 Additional context used

📓 Path-based instructions (6)

**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code

**/*.rs: Each crate keeps sources in src/; unit tests live alongside code with #[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code with rustfmt (see rustfmt.toml); run cargo fmt --all before commits
Lint Rust code with clippy; avoid unwrap()/expect() in library code; use error types (e.g., thiserror)
Use snake_case for function and variable names in Rust
Use UpperCamelCase for types and traits in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async via tokio where applicable
Unit tests should be placed near code with descriptive names (e.g., test_parse_address_mainnet)
Use #[ignore] for network-dependent or long-running tests; run with -- --ignored flag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code

Files:

key-wallet/src/managed_account/address_pool.rs

key-wallet/**/{account,managed_account}/**/*.rs

📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)

key-wallet/**/{account,managed_account}/**/*.rs: Separate immutable and mutable concerns: Use Account for immutable identity information (keys, derivation paths) and ManagedAccount for mutable state (address pools, metadata, balances)
Use strongly typed enum-based system for AccountType with specific variants (Standard, Identity, Masternode, etc.) to provide compile-time safety for different account purposes

Files:

key-wallet/src/managed_account/address_pool.rs

key-wallet/**/{wallet,account,managed_account,address_pool}/**/*.rs

📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)

Always validate network consistency: use the account's network type when creating addresses and assert that operations use the expected network to prevent mainnet/testnet mixing

Files:

key-wallet/src/managed_account/address_pool.rs

key-wallet/**/managed_account/**/*.rs

📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)

Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency

Files:

key-wallet/src/managed_account/address_pool.rs

key-wallet/**/*.rs

📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)

key-wallet/**/*.rs: Never panic in library code; use Result<T> for all fallible operations with custom Error type variants and provide context in error messages
Use ? operator for error propagation in Rust code to maintain clean error handling patterns

Files:

key-wallet/src/managed_account/address_pool.rs

key-wallet/**/{wallet,account,managed_account}/**/*.rs

📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)

key-wallet/**/{wallet,account,managed_account}/**/*.rs: Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance
Clearly separate watch-only wallets from full wallets using the wallet type system; never attempt signing operations on watch-only accounts and validate external signatures match expected pubkeys

Files:

key-wallet/src/managed_account/address_pool.rs

🧠 Learnings (11)

📓 Common learnings

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Implement staged gap limit management using `GapLimitStage` structure with tracking of last_used_index and used_indices HashSet to enable efficient address discovery without loading entire address chains into memory

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Pre-generate addresses in batches (typically 20-100) and store in address pools; only derive on-demand when pool is exhausted to optimize performance and memory usage