fix(chain): sanitize and bound-check derivation indices near BIP32_MAX_INDEX

[busayo-OD]

Fixes bdk_wallet/issues/60

Description

KeychainTxOutIndex::apply_changeset accepted ChangeSet::last_revealed values with no upper bound, allowing an index greater than BIP32_MAX_INDEX (2^31 - 1) to be stored, violating an invariant relied on by next_index().

The changes in this PR address boundary issues around derivation indices:

1. apply_changeset: clamps last_revealed to BIP32_MAX_INDEX before storing it, instead of storing the raw value.
2. replenish_inner_index: fixes an off-by-one in the stop_index calculation that caused a panic ("we just inserted it") when a keychain's derivation index was revealed exactly up to BIP32_MAX_INDEX. The exclusive upper bound was clamped to BIP32_MAX_INDEX instead of BIP32_MAX_INDEX + 1, silently excluding the boundary value itself.
3. lookahead_to_target: fixes an integer overflow panic when called with target_index == u32::MAX, and changes out of range targets to clamp and replenish up to BIP32_MAX_INDEX instead of silently doing nothing.

Notes to the reviewers

This revisits the earlier implementation attempt (#1792) and implements the clamp-in-apply_changeset approach from that review discussion.

The second and third fixes were found while adding the boundary tests requested in review of an earlier version of this PR.

Also added a test documenting current saturation behavior: reveal_next_spk/next_unused_spk return the last revealed script with an empty ChangeSet, not None, once the index is saturated. This matches reveal_next_spk's existing doc comment.

Went through the other index-taking methods (spk_at_index, is_used, mark_used, unmark_used) as well. These are plain lookups with no boundary-sensitive logic, so no dedicated tests were added there.

Changelog notice

• Clamped last_revealed to BIP32_MAX_INDEX in KeychainTxOutIndex::apply_changeset to prevent it accepting an out-of-range derivation index.
• Fixed an off-by-one in replenish_inner_index that could panic when revealing up to BIP32_MAX_INDEX.
• Fixed an integer overflow in lookahead_to_target when called with target_index == u32::MAX, and changed out of range targets to clamp to BIP32_MAX_INDEX instead of silently doing nothing.

Checklists

All Submissions

• I've signed all my commits
• I followed the contribution guidelines
• I ran just p before committing

Bugfixes

• This pull request breaks the existing API
• I've added tests to cover the issue which are now passing
• I'm linking the issue being fixed by this PR

[codecov]

Codecov Report

❌ Patch coverage is 98.82353% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.05%. Comparing base (acc06e5) to head (1ca8d29).
⚠️ Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
crates/chain/src/indexer/keychain_txout.rs	98.82%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2265      +/-   ##
==========================================
+ Coverage   78.71%   79.05%   +0.34%     
==========================================
  Files          31       31              
  Lines        5966     6054      +88     
  Branches      282      284       +2     
==========================================
+ Hits         4696     4786      +90     
+ Misses       1194     1193       -1     
+ Partials       76       75       -1     

Flag	Coverage Δ
rust	79.05% <98.82%> (+0.34%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[evanlinjin]

Thanks for the PR.

Given you have already started the work, we should probably also test:

• Given the derivation index is saturated, next_unused_spk should return None and reveal_next_spk should return None.
• What happens if we use an index greater than BIP32_MAX_INDEX for reveal_to_target.
• What happens if we use BIP32_MAX_INDEX for reveal_to_target.
• Any method that takes an index value should be tested with BIP32_MAX_INDEX and > BIP32_MAX_INDEX.

[busayo-OD]

Given you have already started the work, we should probably also test:

* Given the derivation index is saturated, `next_unused_spk` should return `None` and `reveal_next_spk` should return `None`.

* What happens if we use an index greater than `BIP32_MAX_INDEX` for `reveal_to_target`.

* What happens if we use `BIP32_MAX_INDEX` for `reveal_to_target`.

* Any method that takes an index value should be tested with `BIP32_MAX_INDEX` and `> BIP32_MAX_INDEX`.

Thanks for the review. I’ve added the requested boundary tests. Two additional issues surfaced while adding them and have been fixed too.

For saturation, the current behavior is to return the last revealed SPK with an empty ChangeSet, rather than None, which matches the existing reveal_next_spk documentation. Is None expected here?

[evanlinjin]

ACK 1ca8d29

[notmandatory]

ACK 1ca8d29

Thanks @busayo-OD this looks like a good focused bug fix.