bitcoindevkit / bitcoindevkit/bdk
CanonicalView admits an unconfirmed coinbase transaction
- 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
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