bitcoindevkit / bitcoindevkit/bdk

CanonicalView admits an unconfirmed coinbase transaction

Open
#2,269 0 comments 2 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
1.1k
Forks
483
Avg merge
20d 3h
Merged PRs (30d)
3

Description

### Describe the bug

`CanonicalView` can hold a coinbase transaction at `ChainPosition::Unconfirmed`, which is not a state that can exist on any real chain. A coinbase is only ever created by a block, so it is confirmed or it does not exist.

The `CanonicalStage::AssumedTxs` stage ([`canonical_task.rs:88`](https://github.com/bitcoindevkit/bdk/blob/master/crates/chain/src/canonical_task.rs#L88)) has no coinbase check. When a caller passes a coinbase txid via `CanonicalParams::assume_canonical` and that tx has no direct anchor and no anchored descendant, `canonical_view_task.rs:183` resolves it to `ChainPosition::Unconfirmed`.

Note the asymmetry: `CanonicalStage::SeenTxs` already guards against this, but only with a `debug_assert!`:

```rust
debug_assert!(
!tx.is_coinbase(),
"Coinbase txs must not have `last_seen` (in mempool) value"
);
```

In release builds that assert is compiled out and the coinbase flows through to `mark_canonical` with `ObservedIn::Mempool`, reaching the same unconfirmed position. So both stages can produce the invalid state; `AssumedTxs` does it in debug builds too.

Downstream code reasonably assumes the invariant holds. `CanonicalTxOut::is_mature` has:

```rust
None => {
debug_assert!(false, "coinbase tx can never be unconfirmed");
return false;
}
```

so any consumer that calls `is_mature` on such an output panics in debug builds and silently gets `false` in release.

### To Reproduce

```rust
#[test]
fn assumed_canonical_coinbase_is_unconfirmed() {
let coinbase = Transaction {
version: transaction::Version::ONE,
lock_time: absolute::LockTime::ZERO,
input: vec![TxIn {
previous_output: OutPoint::null(),
script_sig: ScriptBuf::new(),
sequence: Sequence::MAX,
witness: Witness::new(),
}],
output: vec![TxOut {
value: Amount::from_sat(50_000),
script_pubkey: ScriptBuf::new(),
}],
};
assert!(coinbase.is_coinbase());
let txid = coinbase.compute_txid();

let mut graph = TxGraph::::default();
let _ = graph.insert_tx(coinbase); // no anchor, no last_seen
let chain =
LocalChain::from_blocks([(0, BlockHash::all_zeros())].into_iter().collect()).unwrap();
let tip = chain.tip().block_id();

let view = chain.canonical_view(
&graph,
tip,
CanonicalParams { assume_canonical: vec![txid] },
);

let ctx = view.tx(txid).expect("coinbase is canonical");
assert!(matches!(ctx.pos, ChainPosition::Unconfirmed { .. })); // passes
}
```

On `master` this assertion passes, printing `position = Unconfirmed { first_seen: None, last_seen: None }`.

### Expected behavior

A coinbase transaction should never end up in a `CanonicalView` at an unconfirmed position. A coinbase with no anchor and no anchored descendant has no claim to being canonical, so it should be excluded during canonicalization rather than admitted at an impossible position.

The guard belongs in `CanonicalTask` where all stages route through, so `AssumedTxs`, `SeenTxs` and any future stage are covered at once, and it should hold in release builds — not just as a `debug_assert!`.

### Build environment

- BDK tag/commit: `master` @ acc06e53
- Crate: `bdk_chain`

### Additional context

Found while reviewing #2246 (`classify_outpoints`). That branch makes the latent bug louder rather than causing it: the new `classify_outpoints` calls `is_mature()` before any position check, so the `debug_assert!(false, "coinbase tx can never be unconfirmed")` becomes reachable through `balance()`. The underlying invalid state predates that work and reproduces on `master` as shown above.

Contributor guide

Open the contributing guide

Research direction

Start with the reproduction in issue #2269, then read crates/chain/src/canonical_task.rs around CanonicalStage::AssumedTxs and canonical_view_task.rs:183. Trace how coinbase transactions reach mark_canonical from AssumedTxs and SeenTxs, and verify that the completed behavior excludes an unanchored coinbase in release and debug builds without weakening existing invariants.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
blockchain
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.