feat(key-wallet): reserve receive addresses on hand-out

[xdustinface]

Adds a reservation lifecycle to addresses so a receive address handed out to a caller is not re-issued before it is funded or explicitly released. This closes the hand-out race where two sequential requests returned the same address.

• Model address lifecycle as an AddressState enum (Available, Reserved { at }, Used { at }) on AddressInfo, replacing the separate used/used_at fields. The states are mutually exclusive by construction, so the invariant "a used address is never reserved" holds structurally instead of being maintained by hand.
• Add next_unused_and_reserve, release_reservation, and sweep_expired_reservations (TTL backstop) on AddressPool; reserved addresses are excluded from hand-out, count against the gap limit, and are never pruned or aged out when clockless.
• Wire next_receive_address_and_reserve, release_receive_reservation, and sweep_expired_receive_reservations through ManagedCoreFundsAccount, bumping the monitor revision on change.
• Add reserved_count to PoolStats.
• Cover reserve/release/sweep, serde and bincode round-trips, gap-limit and prune interaction, and end-to-end promotion on funding.

Summary by CodeRabbit

Release Notes

• New Features
  • Added reservation-based receive-address flow for Standard accounts, including reserve/release and TTL-based sweeping with monitor revision updates.
  • Introduced structured address lifecycle states (available/reserved/used) and updated address pool selection to avoid returning reserved addresses.
• Configuration
  • Dash SPV: added reservation_sweep_ttl_secs (default enabled at 3600s), with helpers to disable the sweep; validation now rejects Some(0).
• Improvements
  • Enhanced SPV block-header sync with fork-buffer handling and dynamic locator-based requests for better fork robustness.
• Error Handling
  • Added “Invalid state” error and ensured it propagates through to FFI error codes.
• Tests
  • Expanded reservation, promotion, release, and sweeping test coverage.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds reservation-based receive-address handling across wallet layers and replaces chain-header sync primitives with fork buffering, locator-based requests, and DGW v3 difficulty logic.

Changes

Address Reservation Lifecycle

Layer / File(s)	Summary
Address state and pool rules key-wallet/src/managed_account/address_pool.rs, key-wallet/src/error.rs	Defines AddressState, stores it in AddressInfo, updates lifecycle accessors and transitions, changes selection/scanning to use availability, extends stats and pruning with reserved counts, and adds invalid-state handling for gap-limit maintenance.
Reservation and sweep APIs key-wallet/src/managed_account/address_pool.rs, key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs, key-wallet/src/managed_account/managed_core_funds_account.rs, key-wallet-manager/src/process_block.rs, key-wallet-manager/src/wallet_interface.rs, dash-spv/src/client/config.rs, dash-spv/src/client/event_handler.rs, dash-spv/src/client/sync_coordinator.rs	Adds reservation and sweep methods on AddressPool, wallet interfaces, managed funding accounts, wallet manager fan-out, and client sweep task wiring with TTL configuration.
FFI mapping and fixtures key-wallet-ffi/src/address_pool.rs, key-wallet-ffi/src/error.rs, key-wallet-manager/src/events.rs	Updates FFI address conversion and error mapping for the new address state and adjusts downstream test fixtures to construct AddressInfo with AddressState::Available.
Reservation tests key-wallet/src/managed_account/address_pool.rs, key-wallet/src/tests/address_pool_tests.rs, key-wallet/src/tests/address_reservation_tests.rs, key-wallet/src/tests/mod.rs, key-wallet-manager/src/process_block.rs, dash-spv/src/client/config_test.rs	Expands pool and wallet tests to cover reservation, release, TTL sweeping, promotion to used, FFI fixture updates, config validation, and module registration.

Chain primitives and fork sync

Layer / File(s)	Summary
Chain primitives and difficulty dash-spv/src/chain/mod.rs, dash-spv/src/chain/chain_tip.rs, dash-spv/src/chain/chain_work.rs, dash-spv/src/chain/difficulty.rs, dash/src/pow.rs	Changes the chain module surface, makes U256 public byte-conversion helpers available, and adds work accumulation and DGW v3 difficulty helpers with tests.
Fork buffering and header routing dash-spv/src/error.rs, dash-spv/src/network/mod.rs, dash-spv/src/sync/block_headers/fork_buffer.rs, dash-spv/src/sync/block_headers/mod.rs, dash-spv/src/sync/block_headers/manager.rs, dash-spv/src/sync/block_headers/pipeline.rs, dash-spv/src/sync/block_headers/segment_state.rs	Adds the fork buffer, header locator APIs, fork-chain error handling, and header pipeline updates that route non-active-chain batches through buffered fork handling.
Sync manager wiring dash-spv/src/sync/block_headers/sync_manager.rs, dash-spv/src/client/lifecycle.rs, dash-spv/src/test_utils/mod.rs, dash-spv/src/test_utils/chain_tip.rs	Threads fork-buffer maintenance and locator-based requests through sync startup, tick handling, peer events, and network message handling.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~90+ minutes

Possibly related issues

• key-wallet: native ephemeral Reserved address state + atomic next_unused_and_reserve in AddressPool #791: The PR implements the same AddressState reservation lifecycle and reservation/sweep APIs described in that issue.

Suggested reviewers

• llbartekll

Poem

🐇 I hop through states: Available, Reserved, Used,
Then sweep the stale away when time has fused.
Locators long, fork buffers bright,
The rabbit syncs by moonlit light.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: reserving receive addresses when they are handed out.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/address-reservation

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

❌ Patch coverage is 94.70405% with 68 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.57%. Comparing base (03096dd) to head (7a244cc).

Files with missing lines	Patch %	Lines
dash-spv/src/sync/block_headers/manager.rs	92.99%	22 Missing ⚠️
key-wallet/src/managed_account/address_pool.rs	92.56%	22 Missing ⚠️
dash-spv/src/client/config.rs	33.33%	8 Missing ⚠️
dash-spv/src/chain/difficulty.rs	97.31%	4 Missing ⚠️
key-wallet-manager/src/wallet_interface.rs	0.00%	3 Missing ⚠️
dash-spv/src/sync/block_headers/fork_buffer.rs	99.42%	2 Missing ⚠️
dash-spv/src/sync/block_headers/pipeline.rs	94.44%	2 Missing ⚠️
.../src/managed_account/managed_core_funds_account.rs	95.23%	2 Missing ⚠️
dash-spv/src/client/event_handler.rs	95.83%	1 Missing ⚠️
dash-spv/src/client/sync_coordinator.rs	85.71%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #818      +/-   ##
==========================================
+ Coverage   73.16%   73.57%   +0.40%     
==========================================
  Files         323      324       +1     
  Lines       72438    73499    +1061     
==========================================
+ Hits        52999    54076    +1077     
+ Misses      19439    19423      -16     

Flag	Coverage Δ
core	76.94% <100.00%> (ø)
ffi	47.69% <100.00%> (-0.01%)	⬇️
rpc	20.00% <ø> (ø)
spv	90.60% <95.61%> (+0.26%)	⬆️
wallet	72.44% <92.34%> (+0.66%)	⬆️

Files with missing lines	Coverage Δ
dash-spv/src/chain/chain_work.rs	84.92% <100.00%> (+6.45%)	⬆️
dash-spv/src/client/config_test.rs	100.00% <100.00%> (ø)
dash-spv/src/client/lifecycle.rs	78.23% <100.00%> (+0.11%)	⬆️
dash-spv/src/error.rs	33.33% <ø> (ø)
dash-spv/src/network/mod.rs	98.03% <100.00%> (-0.06%)	⬇️
dash-spv/src/sync/block_headers/segment_state.rs	97.43% <100.00%> (+0.06%)	⬆️
dash-spv/src/sync/block_headers/sync_manager.rs	84.61% <ø> (ø)
dash/src/pow.rs	93.67% <100.00%> (ø)
key-wallet-ffi/src/address_pool.rs	40.06% <100.00%> (-0.10%)	⬇️
key-wallet-ffi/src/error.rs	67.02% <100.00%> (ø)
... and 14 more

... and 7 files with indirect coverage changes

[coderabbitai]

Actionable comments posted: 3

🤖 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/address_pool.rs`:
- Around line 1053-1058: The needs_more_addresses() method now evaluates
available addresses using the is_available() predicate to determine if more
addresses are needed, but the maintain_gap_limit() function only checks against
highest_used when deciding whether to generate new addresses. This creates a
mismatch where needs_more_addresses() can return true for reserved-but-unused
pools while maintain_gap_limit() generates no new addresses. Update the
maintain_gap_limit() function to use the same availability predicate that counts
is_available() addresses when deciding whether to replenish the pool, ensuring
both methods use consistent logic for determining when the gap limit has been
breached.
- Around line 676-679: Replace the expect() call on the get_mut(&next_index)
operation with proper error handling using ok_or() to convert the Option into a
Result, then propagate this error through the return type of the containing
function instead of panicking. This ensures that missing pool entries result in
a returned error rather than a process crash, which is appropriate for library
code.
- Around line 250-251: The state field on AddressState lacks backward
compatibility support for deserialization of existing wallet payloads. Since the
legacy used and used_at fields have been removed, older serialized wallets will
fail to deserialize. Add the #[serde(default)] attribute to the state field
declaration to allow missing values during deserialization, or implement a
custom Deserialize handler or deserialize_with function that maps the old used
and used_at fields to the appropriate AddressState enum variant. Verify the fix
works by adding a test that attempts to deserialize a wallet payload with the
legacy field structure.

🪄 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: 53d08b96-0bb9-4ded-afe8-c70aa70862c6

📥 Commits

Reviewing files that changed from the base of the PR and between 95a3c8f and b4a2439.

📒 Files selected for processing (7)

• key-wallet-ffi/src/address_pool.rs
• key-wallet-manager/src/events.rs
• key-wallet/src/managed_account/address_pool.rs
• key-wallet/src/managed_account/managed_core_funds_account.rs
• key-wallet/src/tests/address_pool_tests.rs
• key-wallet/src/tests/address_reservation_tests.rs
• key-wallet/src/tests/mod.rs

[github-actions]

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

[ZocoLini]

I don't remember any usage for the used_at field, why do we need it??

[xdustinface]

It was just there before. Maybe better to fix it up (its currently pre-dating to this PR set to 0).

[lklimek]

start it by default so that the developer doesn't need to care about that, with opt out option.

[lklimek]

this is "good enough", but the idea was to return a "guard" with release-on-drop.

[lklimek]

this is "good enough", but the idea was to return a "guard" with release-on-drop.

[lklimek]

I would start a worker or create event handler to automatically run this method, with optional opt-out.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (2)

key-wallet-manager/src/process_block.rs (1)

536-557: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

This never exercises the multi-wallet fan-out it names.

Only one wallet is reserved here, so an implementation that swept just the first wallet would still pass. Add a second wallet with its own reservation and assert the reclaimed total across both wallets.

🤖 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-manager/src/process_block.rs` around lines 536 - 557, The test
test_sweep_expired_reservations_fans_out_over_wallets only reserves one wallet,
so it does not verify the fan-out behavior it claims. Update the setup to
include a second wallet with its own expired reservation, then assert that
sweep_expired_reservations reclaims the combined total across both wallets and
that subsequent sweeps return zero. Use the existing setup_manager_with_wallet
helper and the manager/get_wallet_info_mut/next_receive_address_and_reserve flow
to locate and extend the test.

key-wallet/src/tests/address_reservation_tests.rs (1)

1-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move this coverage into an approved key-wallet/src/tests module.

address_reservation_tests.rs does not match the repository’s expected test-module layout, so this new file will drift from the structure the rest of key-wallet/src/tests uses. Please fold these cases into one of the approved modules instead.
As per path instructions, key-wallet/src/tests/**/*.rs: Organize unit tests by functionality into separate test modules: account_tests.rs, address_pool_tests.rs, transaction_tests.rs, wallet_tests.rs, integration_tests.rs, balance_tests.rs, spent_outpoints_tests.rs, special_transaction_tests.rs, advanced_transaction_tests.rs, managed_account_collection_tests.rs, backup_restore_tests.rs, edge_case_tests.rs, performance_tests.rs.

🤖 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/tests/address_reservation_tests.rs` around lines 1 - 168, The
new address-reservation coverage is in an unapproved standalone test file
instead of the expected `key-wallet/src/tests` module layout. Move the cases in
`test_reserved_receive_address_promotes_on_funding`,
`test_reserve_receive_without_key_source_errors`,
`test_reserve_receive_on_non_standard_account_errors`,
`test_sweep_expired_receive_reservations_reclaims_through_account`, and
`test_sweep_expired_reservations_reclaims_across_wallet` into one of the
approved test modules, most likely the account- or wallet-focused tests, and
delete this separate module so the test organization matches the repository
structure.

Source: Path instructions

🤖 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 `@dash-spv/src/client/config.rs`:
- Around line 75-81: The reservation sweep TTL API currently allows `Some(0)`,
which makes `with_reservation_ttl_secs` enable a no-op sweep while still
appearing active. Update `Config::with_reservation_ttl_secs` and the
`reservation_sweep_ttl_secs` handling to either reject zero with validation
(`secs > 0`) or treat zero as disabled by normalizing it to `None`, and keep the
behavior consistent with `without_reservation_sweep` and the field
documentation.

In `@dash-spv/src/client/sync_coordinator.rs`:
- Around line 67-71: The reservation sweep is being spawned too early in
`run()`, before `self.start().await` has completed successfully, so move the
`spawn_reservation_sweep` setup to after the successful startup path or
otherwise defer it until `start()` returns Ok. Update the
`reservation_sweep_task` flow in `run()` so it only begins after `self.start()`
succeeds, while preserving the existing `monitor_shutdown` and
`spawn_reservation_sweep` behavior.

In `@key-wallet/src/tests/address_reservation_tests.rs`:
- Around line 21-22: The test setup in address_reservation_tests currently uses
TestWalletContext::new_random(), which makes the wallet data vary between runs.
Replace that initialization with a fixed mnemonic or seed fixture in the
TestWalletContext setup, and keep using the xpub from that deterministic context
so the test remains reproducible.

---

Nitpick comments:
In `@key-wallet-manager/src/process_block.rs`:
- Around line 536-557: The test
test_sweep_expired_reservations_fans_out_over_wallets only reserves one wallet,
so it does not verify the fan-out behavior it claims. Update the setup to
include a second wallet with its own expired reservation, then assert that
sweep_expired_reservations reclaims the combined total across both wallets and
that subsequent sweeps return zero. Use the existing setup_manager_with_wallet
helper and the manager/get_wallet_info_mut/next_receive_address_and_reserve flow
to locate and extend the test.

In `@key-wallet/src/tests/address_reservation_tests.rs`:
- Around line 1-168: The new address-reservation coverage is in an unapproved
standalone test file instead of the expected `key-wallet/src/tests` module
layout. Move the cases in `test_reserved_receive_address_promotes_on_funding`,
`test_reserve_receive_without_key_source_errors`,
`test_reserve_receive_on_non_standard_account_errors`,
`test_sweep_expired_receive_reservations_reclaims_through_account`, and
`test_sweep_expired_reservations_reclaims_across_wallet` into one of the
approved test modules, most likely the account- or wallet-focused tests, and
delete this separate module so the test organization matches the repository
structure.

🪄 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: 2317f6cb-21b6-42a2-977d-6a4072e096b9

📥 Commits

Reviewing files that changed from the base of the PR and between fc0668f and b812307.

📒 Files selected for processing (15)

• dash-spv/src/client/config.rs
• dash-spv/src/client/event_handler.rs
• dash-spv/src/client/sync_coordinator.rs
• key-wallet-ffi/src/address_pool.rs
• key-wallet-ffi/src/error.rs
• key-wallet-manager/src/events.rs
• key-wallet-manager/src/process_block.rs
• key-wallet-manager/src/wallet_interface.rs
• key-wallet/src/error.rs
• key-wallet/src/managed_account/address_pool.rs
• key-wallet/src/managed_account/managed_core_funds_account.rs
• key-wallet/src/tests/address_pool_tests.rs
• key-wallet/src/tests/address_reservation_tests.rs
• key-wallet/src/tests/mod.rs
• key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs

🚧 Files skipped from review as they are similar to previous changes (8)

• key-wallet/src/tests/mod.rs
• key-wallet-ffi/src/error.rs
• key-wallet/src/error.rs
• key-wallet-manager/src/events.rs
• key-wallet-ffi/src/address_pool.rs
• key-wallet/src/tests/address_pool_tests.rs
• key-wallet/src/managed_account/managed_core_funds_account.rs
• key-wallet/src/managed_account/address_pool.rs

[github-actions]

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

[coderabbitai]

Actionable comments posted: 7

🧹 Nitpick comments (2)

dash-spv/src/chain/mod.rs (1)

1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Split or retitle the chain-sync work.

These chain/fork/DGW module changes are unrelated to the stated feat(key-wallet): reserve receive addresses on hand-out scope. Please split them into a focused PR or retitle/rescope this PR so the semantic title describes all logic changes.

As per path instructions, **: Check whether the PR title prefix allowed in the pr-title.yml workflow accurately describes the changes, and suggest splitting unrelated concerns.

🤖 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 `@dash-spv/src/chain/mod.rs` around lines 1 - 11, The changes under chain
primitives in mod and its re-exports (chain_tip, chain_work, checkpoints,
difficulty, and ForkCandidate) are unrelated to the stated feature scope, so
split them into a separate PR or retitle/rescope the current PR to accurately
describe the chain-sync work. Check the PR title prefix allowed by pr-title.yml
and ensure the title matches all logic changes; if the wallet reservation
feature is still intended here, remove the chain/fork/DGW module changes from
this diff.

Source: Path instructions

dash-spv/src/sync/block_headers/manager.rs (1)

109-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Split or retitle the fork-buffer work.

This header fork-buffering/reorg-detection functionality is separate from feat(key-wallet): reserve receive addresses on hand-out. Please split it into a focused PR or retitle/scope this PR so the semantic title describes the actual changes. As per path instructions, “If a PR mixes unrelated concerns … suggest splitting it into separate focused PRs.”

🤖 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 `@dash-spv/src/sync/block_headers/manager.rs` around lines 109 - 180, This PR
mixes unrelated fork-buffer/reorg-detection changes in manager.rs with the
key-wallet receive-address work, so separate the header sync logic into its own
focused PR or retitle/rescope the current PR to match the actual changes. Update
the PR scope so it clearly reflects the fork handling work in ingest_fork,
take_pending_fork_candidate, and prune_fork_tip_index, and keep the key-wallet
address reservation changes out of this review.

Source: Path instructions

🤖 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 `@dash-spv/src/chain/difficulty.rs`:
- Around line 15-20: The SPV difficulty validation currently hardcodes network
consensus values in difficulty.rs, including DASH_TARGET_SPACING and the
pow-limit byte patterns. Update the logic in the difficulty validation path to
read these values from dashcore::consensus::Params or a shared network-constants
source, and add authoritative constants there if they do not already exist. Make
sure the affected checks in the difficulty-related functions use the centralized
parameters instead of duplicated literals.
- Around line 57-59: The fallback in `next_difficulty` is too permissive: it
returns `pow_limit_bits` whenever `previous_headers` is shorter than
`DGW_PAST_BLOCKS`, even if the tip is already past the DGW window. Update this
logic so missing history at high height is treated as a dependency/defer
condition instead of accepting the easiest target; use the existing
`next_difficulty` flow and `previous_headers`/`DGW_PAST_BLOCKS` checks to gate
the fallback only for legitimate early-height cases.
- Around line 64-72: The DGW averaging logic in the difficulty window loop is
using the wrong weighted-average denominator in the `past_target_avg` update, so
the `difficulty.rs` calculation in the `window.iter().rev()` path is biased.
Adjust the formula in that averaging branch so the accumulated prior average is
weighted by the current sample count and divided by the correct total sample
count for the combined average, preserving the intended DGW smoothing behavior
for `past_target_avg`.

In `@dash-spv/src/sync/block_headers/fork_buffer.rs`:
- Around line 86-95: The fork-validation check currently uses debug_assert_eq!
on history.last() versus ancestor_header, which is removed in release builds. In
the header-processing path that builds rolling_history and validates
peer-supplied headers, replace this with a runtime validation that returns a
SyncError when history is empty or does not end at ancestor_header. Make sure
the error is propagated before any MTP/DGW-related validation uses the history
window, and apply the same runtime guard anywhere the same history check
appears.

In `@dash-spv/src/sync/block_headers/manager.rs`:
- Around line 268-271: The overlap handling in block header sync is routing
active-chain headers into ingest_fork via prev_height/tip_height alone, which
can misclassify a batch that starts on the active chain as a fork. Update
manager::ingest_headers (or the surrounding prev_height check) to inspect the
first header’s active-chain height before calling ingest_fork, and trim or skip
any known active-chain prefix so only true fork data is buffered. Use the
existing tip/active-chain height lookup logic in block_headers::manager to
ensure duplicate overlap is ignored and only the new extension is processed
normally.

In `@dash-spv/src/sync/block_headers/segment_state.rs`:
- Around line 75-82: Replace the `debug_assert!` in
`SegmentState::request_block_headers` with a runtime validation that checks the
locator is non-empty and starts with `self.current_tip_hash`; if not, return
`SyncError::InvalidState` instead of proceeding. Keep the existing request flow
in `request_block_headers`, but only call
`requests.request_block_headers(locator)?` and
`self.coordinator.mark_sent(&[self.current_tip_hash])` after the validation
succeeds so release builds cannot desynchronize in-flight tracking. Ensure the
new error is propagated using the existing `SyncError`/thiserror pattern used in
`segment_state.rs`.

In `@dash-spv/src/sync/block_headers/sync_manager.rs`:
- Around line 134-143: The `SyncManager` warning block is draining the fork
candidate with `take_pending_fork_candidate()` even though it only logs and does
not promote anything yet. Change this path to inspect the pending fork candidate
without removing it, or defer calling `take_pending_fork_candidate()` until the
coordinator is ready to promote, so the candidate remains available for the next
step.

---

Nitpick comments:
In `@dash-spv/src/chain/mod.rs`:
- Around line 1-11: The changes under chain primitives in mod and its re-exports
(chain_tip, chain_work, checkpoints, difficulty, and ForkCandidate) are
unrelated to the stated feature scope, so split them into a separate PR or
retitle/rescope the current PR to accurately describe the chain-sync work. Check
the PR title prefix allowed by pr-title.yml and ensure the title matches all
logic changes; if the wallet reservation feature is still intended here, remove
the chain/fork/DGW module changes from this diff.

In `@dash-spv/src/sync/block_headers/manager.rs`:
- Around line 109-180: This PR mixes unrelated fork-buffer/reorg-detection
changes in manager.rs with the key-wallet receive-address work, so separate the
header sync logic into its own focused PR or retitle/rescope the current PR to
match the actual changes. Update the PR scope so it clearly reflects the fork
handling work in ingest_fork, take_pending_fork_candidate, and
prune_fork_tip_index, and keep the key-wallet address reservation changes out of
this review.

🪄 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: 245e289d-17a2-4432-a60a-fe6a8ef0d293

📥 Commits

Reviewing files that changed from the base of the PR and between 05c4581 and 7a244cc.

📒 Files selected for processing (32)

• dash-spv/src/chain/chain_tip.rs
• dash-spv/src/chain/chain_work.rs
• dash-spv/src/chain/difficulty.rs
• dash-spv/src/chain/mod.rs
• dash-spv/src/client/config.rs
• dash-spv/src/client/config_test.rs
• dash-spv/src/client/event_handler.rs
• dash-spv/src/client/lifecycle.rs
• dash-spv/src/client/sync_coordinator.rs
• dash-spv/src/error.rs
• dash-spv/src/network/mod.rs
• dash-spv/src/sync/block_headers/fork_buffer.rs
• dash-spv/src/sync/block_headers/manager.rs
• dash-spv/src/sync/block_headers/mod.rs
• dash-spv/src/sync/block_headers/pipeline.rs
• dash-spv/src/sync/block_headers/segment_state.rs
• dash-spv/src/sync/block_headers/sync_manager.rs
• dash-spv/src/test_utils/chain_tip.rs
• dash-spv/src/test_utils/mod.rs
• dash/src/pow.rs
• key-wallet-ffi/src/address_pool.rs
• key-wallet-ffi/src/error.rs
• key-wallet-manager/src/events.rs
• key-wallet-manager/src/process_block.rs
• key-wallet-manager/src/wallet_interface.rs
• key-wallet/src/error.rs
• key-wallet/src/managed_account/address_pool.rs
• key-wallet/src/managed_account/managed_core_funds_account.rs
• key-wallet/src/tests/address_pool_tests.rs
• key-wallet/src/tests/address_reservation_tests.rs
• key-wallet/src/tests/mod.rs
• key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs

💤 Files with no reviewable changes (2)

• dash-spv/src/test_utils/mod.rs
• dash-spv/src/test_utils/chain_tip.rs

✅ Files skipped from review due to trivial changes (1)

• dash-spv/src/sync/block_headers/mod.rs

🚧 Files skipped from review as they are similar to previous changes (16)

• key-wallet/src/tests/mod.rs
• dash-spv/src/client/config_test.rs
• key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
• key-wallet-ffi/src/error.rs
• dash-spv/src/client/config.rs
• key-wallet/src/tests/address_pool_tests.rs
• key-wallet-manager/src/wallet_interface.rs
• dash-spv/src/client/sync_coordinator.rs
• dash-spv/src/client/event_handler.rs
• key-wallet-manager/src/events.rs
• key-wallet/src/tests/address_reservation_tests.rs
• key-wallet-manager/src/process_block.rs
• key-wallet-ffi/src/address_pool.rs
• key-wallet/src/managed_account/managed_core_funds_account.rs
• key-wallet/src/error.rs
• key-wallet/src/managed_account/address_pool.rs

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Move consensus parameters out of hardcoded literals.

DASH_TARGET_SPACING and the pow-limit byte patterns are network consensus parameters. Please source them from dashcore::consensus::Params/network constants, or add authoritative constants there, instead of duplicating literals in SPV validation.

As per coding guidelines, **/*.rs: Never hardcode network parameters, addresses, or keys.

Also applies to: 132-154

🤖 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 `@dash-spv/src/chain/difficulty.rs` around lines 15 - 20, The SPV difficulty
validation currently hardcodes network consensus values in difficulty.rs,
including DASH_TARGET_SPACING and the pow-limit byte patterns. Update the logic
in the difficulty validation path to read these values from
dashcore::consensus::Params or a shared network-constants source, and add
authoritative constants there if they do not already exist. Make sure the
affected checks in the difficulty-related functions use the centralized
parameters instead of duplicated literals.

Source: Coding guidelines

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Do not accept pow-limit difficulty when high-height history is missing.

The doc says callers must provide the DGW window, but this fallback returns the easiest target even when tip_height >= DGW_PAST_BLOCKS. For fork validation, missing history should be a dependency error/defer, not a looser consensus rule.

🤖 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 `@dash-spv/src/chain/difficulty.rs` around lines 57 - 59, The fallback in
`next_difficulty` is too permissive: it returns `pow_limit_bits` whenever
`previous_headers` is shorter than `DGW_PAST_BLOCKS`, even if the tip is already
past the DGW window. Update this logic so missing history at high height is
treated as a dependency/defer condition instead of accepting the easiest target;
use the existing `next_difficulty` flow and `previous_headers`/`DGW_PAST_BLOCKS`
checks to gate the fallback only for legitimate early-height cases.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Fix the DGW averaging denominator.

On the second sample, past_target_avg represents one prior target, but the code multiplies it by 2 and divides by 3. That overweights recent targets and computes the wrong DGW average for non-constant windows.

Proposed fix

     let mut past_target_avg = U256::ZERO;
     for (i, header) in window.iter().rev().enumerate() {
-        let count = (i + 1) as u64;
+        let count = (i + 1) as u64;
         let target = U256::from_be_bytes(Target::from_compact(header.bits).to_be_bytes());
         if count == 1 {
             past_target_avg = target;
         } else {
-            // past_target_avg = (past_target_avg * count + target) / (count + 1)
+            // `past_target_avg` already covers `count - 1` samples.
             past_target_avg =
-                (past_target_avg * U256::from(count) + target) / U256::from(count + 1);
+                (past_target_avg * U256::from(count - 1) + target) / U256::from(count);
         }
     }

📝 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

	for (i, header) in window.iter().rev().enumerate() {
	let count = (i + 1) as u64;
	let target = U256::from_be_bytes(Target::from_compact(header.bits).to_be_bytes());
	if count == 1 {
	past_target_avg = target;
	} else {
	// past_target_avg = (past_target_avg * count + target) / (count + 1)
	past_target_avg =
	(past_target_avg * U256::from(count) + target) / U256::from(count + 1);
	for (i, header) in window.iter().rev().enumerate() {
	let count = (i + 1) as u64;
	let target = U256::from_be_bytes(Target::from_compact(header.bits).to_be_bytes());
	if count == 1 {
	past_target_avg = target;
	} else {
	// `past_target_avg` already covers `count - 1` samples.
	past_target_avg =
	(past_target_avg * U256::from(count - 1) + target) / U256::from(count);
	}

🤖 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 `@dash-spv/src/chain/difficulty.rs` around lines 64 - 72, The DGW averaging
logic in the difficulty window loop is using the wrong weighted-average
denominator in the `past_target_avg` update, so the `difficulty.rs` calculation
in the `window.iter().rev()` path is biased. Adjust the formula in that
averaging branch so the accumulated prior average is weighted by the current
sample count and divided by the correct total sample count for the combined
average, preserving the intended DGW smoothing behavior for `past_target_avg`.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Enforce fork-validation history at runtime.

debug_assert_eq! disappears in release. If history is empty or does not end at ancestor_header, MTP/DGW validation is computed from the wrong window and can fall back to weak targets. Return a SyncError before validating peer-supplied headers.

Proposed fix

-        debug_assert_eq!(
-            history.last().map(|h| h.block_hash()),
-            Some(ancestor_header.block_hash()),
-            "history.last() must be the ancestor header"
-        );
+        if history.last().map(|h| h.block_hash()) != Some(ancestor_header.block_hash()) {
+            return Err(SyncError::MissingDependency(
+                "fork validation history must end at the ancestor header".to_string(),
+            ));
+        }

As per coding guidelines, **/*.rs: Use proper error types (thiserror) and propagate errors appropriately.

Also applies to: 117-128

🤖 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 `@dash-spv/src/sync/block_headers/fork_buffer.rs` around lines 86 - 95, The
fork-validation check currently uses debug_assert_eq! on history.last() versus
ancestor_header, which is removed in release builds. In the header-processing
path that builds rolling_history and validates peer-supplied headers, replace
this with a runtime validation that returns a SyncError when history is empty or
does not end at ancestor_header. Make sure the error is propagated before any
MTP/DGW-related validation uses the history window, and apply the same runtime
guard anywhere the same history check appears.

Source: Coding guidelines

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Avoid routing active-chain overlap as a fork.

A peer can send headers whose prev_blockhash is below our tip but whose first header is already on the active chain. This path buffers that duplicate/overlapping active-chain batch as a fork, and an overlap plus one new header can become a “winning” fork candidate instead of storing the tip extension normally. Check the first header’s active-chain height and ignore or trim known active prefixes before calling ingest_fork.

🤖 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 `@dash-spv/src/sync/block_headers/manager.rs` around lines 268 - 271, The
overlap handling in block header sync is routing active-chain headers into
ingest_fork via prev_height/tip_height alone, which can misclassify a batch that
starts on the active chain as a fork. Update manager::ingest_headers (or the
surrounding prev_height check) to inspect the first header’s active-chain height
before calling ingest_fork, and trim or skip any known active-chain prefix so
only true fork data is buffered. Use the existing tip/active-chain height lookup
logic in block_headers::manager to ensure duplicate overlap is ignored and only
the new extension is processed normally.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Validate the locator in release builds.

debug_assert! is stripped in release, but Line 81 sends the supplied locator while Line 82 records current_tip_hash as in-flight. A bad caller would desynchronize request tracking and make valid responses look unsolicited; return SyncError::InvalidState instead.

Proposed fix

-        debug_assert!(
-            !locator.is_empty() && locator[0] == self.current_tip_hash,
-            "segment {} locator must start at current_tip_hash {}",
-            self.segment_id,
-            self.current_tip_hash
-        );
+        if locator.first().copied() != Some(self.current_tip_hash) {
+            return Err(SyncError::InvalidState(format!(
+                "Segment {}: locator must start at current_tip_hash {}",
+                self.segment_id, self.current_tip_hash
+            )));
+        }

As per coding guidelines, **/*.rs: Use proper error types (thiserror) and propagate errors appropriately.

📝 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

	debug_assert!(
	!locator.is_empty() && locator[0] == self.current_tip_hash,
	"segment {} locator must start at current_tip_hash {}",
	self.segment_id,
	self.current_tip_hash
	);
	requests.request_block_headers(locator)?;
	self.coordinator.mark_sent(&[self.current_tip_hash]);
	if locator.first().copied() != Some(self.current_tip_hash) {
	return Err(SyncError::InvalidState(format!(
	"Segment {}: locator must start at current_tip_hash {}",
	self.segment_id, self.current_tip_hash
	)));
	}
	requests.request_block_headers(locator)?;
	self.coordinator.mark_sent(&[self.current_tip_hash]);

🤖 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 `@dash-spv/src/sync/block_headers/segment_state.rs` around lines 75 - 82,
Replace the `debug_assert!` in `SegmentState::request_block_headers` with a
runtime validation that checks the locator is non-empty and starts with
`self.current_tip_hash`; if not, return `SyncError::InvalidState` instead of
proceeding. Keep the existing request flow in `request_block_headers`, but only
call `requests.request_block_headers(locator)?` and
`self.coordinator.mark_sent(&[self.current_tip_hash])` after the validation
succeeds so release builds cannot desynchronize in-flight tracking. Ensure the
new error is propagated using the existing `SyncError`/thiserror pattern used in
`segment_state.rs`.

Source: Coding guidelines

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Don’t drain the fork candidate before promotion is wired.

take_pending_fork_candidate() removes the candidate, but this block only logs it. The next coordinator step will have nothing left to promote, so a winning fork can be silently discarded.

Proposed fix

-        if let Some(candidate) = self.take_pending_fork_candidate() {
+        if let Some(candidate) = self.pending_fork_candidate.as_ref() {
             // Reorg promotion is not yet wired into the coordinator. Log here
             // so detection can be confirmed end-to-end before that lands.
             tracing::warn!(

📝 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

	if let Some(candidate) = self.take_pending_fork_candidate() {
	// Reorg promotion is not yet wired into the coordinator. Log here
	// so detection can be confirmed end-to-end before that lands.
	tracing::warn!(
	"Fork candidate ready (ancestor={} headers={} work_bytes={:?}), \
	reorg promotion not yet wired",
	candidate.ancestor_height,
	candidate.headers.len(),
	candidate.total_work.as_bytes()
	);
	if let Some(candidate) = self.pending_fork_candidate.as_ref() {
	// Reorg promotion is not yet wired into the coordinator. Log here
	// so detection can be confirmed end-to-end before that lands.
	tracing::warn!(
	"Fork candidate ready (ancestor={} headers={} work_bytes={:?}), \
	reorg promotion not yet wired",
	candidate.ancestor_height,
	candidate.headers.len(),
	candidate.total_work.as_bytes()
	);

🤖 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 `@dash-spv/src/sync/block_headers/sync_manager.rs` around lines 134 - 143, The
`SyncManager` warning block is draining the fork candidate with
`take_pending_fork_candidate()` even though it only logs and does not promote
anything yet. Change this path to inspect the pending fork candidate without
removing it, or defer calling `take_pending_fork_candidate()` until the
coordinator is ready to promote, so the candidate remains available for the next
step.