bitcoindevkit / bitcoindevkit/bdk
KeychainTxOutIndex::lookahead_to_target panics (overflow) or silently no-ops on target_index = u32::MAX
- Dominant language
- Rust
- Stars
- 1.1k
- Forks
- 483
- Avg merge
- 20d 3h
- Merged PRs (30d)
- 3
Description
**Describe the bug**
`KeychainTxOutIndex::lookahead_to_target` computes `target_index + 1` without overflow protection (crates/chain/src/indexer/keychain_txout.rs, line 543):
let temp_lookahead = (target_index + 1)
.checked_sub(next_index)
.filter(|&index| index > 0);
When `target_index == u32::MAX`:
- In a debug build (default `cargo test`/`cargo build`, overflow-checks on): panics with "attempt to add with overflow".
- In a release build (overflow-checks off by default): `u32::MAX + 1` silently wraps to 0, so `checked_sub`/`filter` evaluate to `None` and the function silently does nothing — no error, no scripts derived, no indication to the caller that the call had no effect.
`target_index` is a raw public API parameter with no upstream validation, so it could plausibly come from untrusted input (e.g. a derivation index parsed from an external PSBT).
For comparison, the sibling method `reveal_to_target` handles the same boundary safely — it only ever compares indices (`i > target_index`) rather than doing arithmetic on the raw value. `lookahead_to_target` has no equivalent protection.
**To Reproduce**
Add this test to crates/chain/tests/test_keychain_txout_index.rs:
#[test]
fn repro_lookahead_to_target_overflow() {
let external_descriptor = parse_descriptor(DESCRIPTORS[0]);
let internal_descriptor = parse_descriptor(DESCRIPTORS[1]);
let mut index = init_txout_index(external_descriptor, internal_descriptor, 10, true);
let _ = index.lookahead_to_target(TestKeychain::External, u32::MAX);
}
Run:
cargo test --features miniscript --test test_keychain_txout_index repro_lookahead_to_target_overflow -- --nocapture
Actual output:
thread 'repro_lookahead_to_target_overflow' panicked at crates/chain/src/indexer/keychain_txout.rs:543:34:
attempt to add with overflow
**Expected behavior**
No panic in debug builds, and no silent no-op in release builds. The lookahead should either be extended as far as representable (e.g. via `target_index.saturating_add(1)` instead of `target_index + 1`), or the call should visibly report that the target was unreachable — not fail silently.
**Proposed fix**
Use `saturating_add` instead of raw `+` — the same pattern is already used a few lines below in the same function (line 606: `*_i = spk_i.saturating_add(1);`), so this just makes the function internally consistent:
let temp_lookahead = target_index
.saturating_add(1)
.checked_sub(next_index)
.filter(|&index| index > 0);
This preserves existing behavior for all valid inputs and makes the u32::MAX boundary case degrade gracefully instead of panicking or silently no-opping. A regression test with `target_index = u32::MAX` (and ideally `BIP32_MAX_INDEX`, the actual max valid BIP-32 derivation index) should be added alongside the fix, following the existing table-driven test style already used for `lookahead_to_target` in test_keychain_txout_index.rs.
Contributor guide
Research direction
Start in crates/chain/src/indexer/keychain_txout.rs at lookahead_to_target and compare its boundary handling with reveal_to_target. Add the u32::MAX regression case in crates/chain/tests/test_keychain_txout_index.rs, following the existing table-driven tests, then run the specified cargo test command. Done means the boundary case neither panics nor silently skips the requested lookahead.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 86/100