bitcoindevkit / bitcoindevkit/bdk
Improve safety of FilterIter
- Dominant language
- Rust
- Stars
- 1.1k
- Forks
- 483
- Avg merge
- 20d 3h
- Merged PRs (30d)
- 3
Description
**Describe the bug**
`FilterIter` (`crates/bitcoind_rpc/src/bip158.rs`) trusts several fields from the RPC
endpoint's header responses without validating them against the local checkpoint chain, in
`find_base`/`next`. Three distinct problems, of different character:
1. **Stale tip survives the `find_base` walkback — a real bug, no hostile server needed.**
When the caller's checkpoint is sparse and its tip references a block that isn't on the
node's main chain (a reorged-out saved tip, or a stale `insert_block(height, hash)`),
`find_base` walks back to a confirmed ancestor but never updates `self.cp`. The orphaned
entries above the ancestor get re-attached on top of the freshly inserted block by
`CheckPoint::insert`, so `Event::height()` (`self.cp.height()`) reports the stale orphan
height instead of the height of the block actually being emitted. Docs and
`examples/filter_iter.rs` tell callers to use `Event::height()` as the height for
`apply_block_relevant`, so this mis-indexes transactions and inflates the reported
confirmation count — reachable with a perfectly honest, fully synced node.
2. **Height `0` reaches the genesis assert → panic.** `next` takes the next header's height
from the RPC response and feeds it straight into `cp.insert(next_height, next_hash)`.
Nothing rejects `next_height == 0`. `CheckPoint::insert` treats height 0 as immutable
genesis and panics the moment the inserted hash doesn't
match the wallet's real genesis. An honest `bitcoind` never reports height 0 for a "next"
header, so this only bites if the endpoint itself is compromised or the connection is
MITM'd.
3. **Unbounded rewind loop.** The reorg-rewind loop has no depth bound and no cycle
detection:
```rust
while next_header.confirmations < 0 {
let prev_hash = next_header.previous_block_hash.ok_or(Error::ReorgDepthExceeded)?;
let prev_header = self.client.get_block_header_info(&prev_hash)?;
next_header = prev_header;
}
```
`Error::ReorgDepthExceeded` only fires if the server sends `previous_block_hash: null`; a
server that just keeps answering with `confirmations: -1` and never sends `null` (a
`previous_block_hash` cycle, or an endless linear chain) spins a single
`FilterIter::next()` call forever — one RPC round-trip per iteration, unbounded
CPU/traffic, scan never returns. Same precondition as (2): requires a compromised or
MITM'd endpoint, not something an honest node does.
`FilterIter`'s RPC connection is authenticated, so treating the endpoint as trusted is a
reasonable design default, and (2)/(3) are consistent with that — they're not reachable
through normal use. They're filed here as safety/robustness hardening (a single bad response
shouldn't be able to permanently crash or hang the scan loop) rather than as exploitable bugs
in the current threat model. (1) is the one genuine bug: it needs no hostile server, just a
stale checkpoint the caller legitimately holds after being offline through a reorg.
**To Reproduce**
(1) is a self-contained automated test against regtest — no hostile server involved. (2) and
(3) both need a response an honest `bitcoind` will never produce (height 0 on a "next"
header; a `previous_block_hash` cycle), which isn't something bdk's test environment (real
regtest `bitcoind`) can manufacture without a mock RPC endpoint — there isn't one in-tree, so
both are described rather than shipped as automated tests.
*1. Stale tip after walkback* — add to `crates/bitcoind_rpc/tests/test_filter_iter.rs`:
```rust
#[test]
fn filter_iter_event_height_after_walkback_from_orphan_tip() -> anyhow::Result<()> {
let env = testenv()?;
let _ = env.mine_blocks(10, None)?;
let genesis_hash = env.genesis_hash()?;
// Sparse checkpoint: genesis + a non-existent block at height 5. `find_base` walks
// past the fake entry (not on the main chain) and returns genesis as the base.
let fake_hash: bitcoin::BlockHash = bitcoin::hashes::Hash::hash(b"not-a-real-block");
let cp = CheckPoint::new(0, genesis_hash).insert(5, fake_hash);
let client = ClientExt::get_rpc_client(&env)?;
let mut iter = FilterIter::new(&client, cp, [ScriptBuf::new()]);
let event = iter.next().unwrap()?;
assert_eq!(
event.height(),
1,
"Event::height() must reflect the actual emitted block (1), not the stale orphan cp tip (5)"
);
Ok(())
}
```
On the affected code this fails with `event.height() == 5`.
*2. Height-0 genesis panic* — needs a mock JSON-RPC endpoint that, once agreement is
reached, answers the "next header" lookup with `height: 0` and a hash that doesn't match the
wallet's real genesis. `FilterIter::next` forwards that straight into
`cp.insert(0, next_hash)`, which hits `CheckPoint::insert`'s genesis assert and panics
the scan thread.
*3. Unbounded rewind loop* — needs a mock JSON-RPC endpoint that, once agreement is reached,
serves a `previous_block_hash` cycle (`A.prev = B`, `B.prev = A`) with `confirmations: -1`
for both. A single `iter.next()` call never returns; each iteration costs one
`getblockheader` round-trip with no cap.
**Expected behavior**
- `Event::height()` always matches the height of the block actually being emitted, never a
stale checkpoint entry.
- A header height that can't be valid (`0`, or further ahead than a one-block extension)
should be rejected with a typed error, not panic.
- The rewind loop should complete in bounded work per `next()` call — a depth cap and/or
cycle detection, returning `Error::ReorgDepthExceeded` once exceeded — regardless of what
the endpoint serves.
**Build environment**
- BDK tag/commit: `bdk_bitcoind_rpc` 0.22.0
- OS+version: n/a
- Rust/Cargo version: n/a
- Rust/Cargo target: n/a
**Which backend(s) are relevant (if any)?**
- [ ] Electrum
- [ ] Esplora
- [x] Bitcoin Core RPC
- [ ] None / not backend-related (e.g. `bdk_chain`, `bdk_core`)
- [ ] Other (please specify): `____`
**Is this blocking production use?**
- [x] Yes — (1) silently corrupts wallet state (wrong tx heights, inflated confirmations)
under normal use, with no error raised at all. (2) and (3) aren't part of the current
trusted-endpoint threat model, but are worth closing as defense-in-depth.
- [ ] No
**Project or organization (optional)**
**Additional context**
`FilterIter`'s RPC connection is authenticated, so trusting the endpoint by default is
reasonable — (2) and (3) don't fire against an honest node. Still, none of height,
confirmations, or previous_block_hash are checked for internal consistency against the local
chain before being acted on, so a single unexpected response can currently either corrupt
`Event::height()` (1, no hostile server required) or, if the endpoint were ever compromised,
panic (2) or hang (3) the scan loop. Suggested fixes:
- Reset `self.cp` inside `find_base` to the confirmed base, and validate that the found
header's height matches the checkpoint entry's height before accepting it. In `next`
truncate `cp` to the nearest valid base prior to inserting the `next_height`.
- Reject `next_height == 0` and `next_height > cp.height() + 1` before calling `cp.insert`;
surface a dedicated error variant instead of panicking.
- Cap total rewind steps over the life of a `FilterIter` (e.g. 100) and fail closed with
`Error::ReorgDepthExceeded` once the budget is exhausted, so a single `next()` call is
guaranteed to complete in bounded work no matter what the endpoint serves.
Contributor guide
Research direction
Start in crates/bitcoind_rpc/src/bip158.rs, reading FilterIter::find_base and next alongside CheckPoint::insert. Run crates/bitcoind_rpc/tests/test_filter_iter.rs, including the stale-tip regression scenario described in the issue. Done means emitted events use the confirmed block height and malformed heights or unbounded rewind responses return typed errors without panicking or hanging.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100