bitcoindevkit / bitcoindevkit/bdk
Move `CanonicalTxOut` and `ChainPosition` to `bdk_core`
- Dominant language
- Rust
- Stars
- 1.1k
- Forks
- 483
- Avg merge
- 20d 3h
- Merged PRs (30d)
- 3
Description
## Context
`CanonicalView` will replace `bdk_tx::CanonicalUnspents`. Consumers building transactions will need the unspent set in a form that can be turned into `bdk_tx::Input`.
With #2139 merged, `CanonicalTxOut>` already carries everything required:
| `Input` field | source |
| --- | --- |
| `prev_outpoint` | `CanonicalTxOut::outpoint` |
| `prev_txout` | `CanonicalTxOut::txout` |
| `is_coinbase` | `CanonicalTxOut::is_on_coinbase` |
| `status.height` | `CanonicalTxOut::pos` (`ChainPosition::Confirmed`, non-transitive) |
| `status.prev_mtp` | `CanonicalTxOut::prev_mtp` |
| `prev_tx` (for `non_witness_utxo`) | `CanonicalView::tx(outpoint.txid)` |
The `Plan` is not available from the chain layer and does not need to be — it comes from descriptors, which is caller knowledge. Everything crossing the boundary is bitcoin-only.
The problem is where those types live. `CanonicalTxOut` and `ChainPosition` are in `bdk_chain`, so a constructor on the `bdk_tx` side would make `bdk_tx` depend on `bdk_chain` — and through it on `TxGraph`, the indexers, and optional `rusqlite`/`miniscript`. `bdk_tx` should not depend on `bdk_chain` at all.
## Proposal
Move `CanonicalTxOut` and `ChainPosition` to `bdk_core`.
`Anchor` stays in `bdk_chain`. Neither type bounds its generic parameter, and `ChainPosition` needs only `bitcoin::Txid`, so with the conversion reading `anchor.height` directly the trait is not involved — which also keeps `impl Anchor for ConfirmationBlockTime` out of the diff, given that type is slated for removal. `ChainPosition`'s rustdoc currently links to `Anchor`; that reference needs rewording once it sits below `bdk_chain`.
`CanonicalView` itself stays in `bdk_chain` — it needs `TxGraph`. Only the output types move.
`bdk_tx` can then offer the conversion behind a `bdk_core` feature, keeping its default build free of any BDK dependency. Only `ChainPosition` is supported — a generic `A: Anchor` would mean unknown height precision, since `Anchor::confirmation_height_upper_bound` is deliberately pessimistic by default:
```rust
#[cfg(feature = "bdk_core")]
impl Input {
pub fn from_canonical_txout(
txout: CanonicalTxOut>,
plan: Plan,
) -> Result {
let status = match txout.pos {
ChainPosition::Confirmed { anchor, transitively } => {
// Height is an upper bound when transitively anchored, which errs toward
// fewer confirmations. The MTP is not the coin's own, so drop it.
let prev_mtp = txout.prev_mtp.filter(|_| transitively.is_none());
Some(ConfirmationStatus::new(anchor.height, prev_mtp)?)
}
ChainPosition::Unconfirmed { .. } => None,
};
Ok(Self::from_prev_txout(
plan, txout.outpoint, txout.txout, status, txout.is_on_coinbase,
))
}
}
```
Callers holding richer anchors map via `Anchor::anchor_block()` first, leaving:
```rust
let inputs: Vec = view
.filter_unspent_outpoints(outpoints)
.filter_map(|(_, txout)| {
let plan = planner.plan(&txout)?;
Input::from_canonical_txout(txout, plan).ok()
})
.collect();
```
## Rough edges
`CanonicalTxOut::prev_mtp` is `Option`; `ConfirmationStatus::prev_mtp` is `Option`. `absolute::Time::from_consensus` is fallible (values below 500_000_000 are rejected), so the conversion handles a `Result` that cannot fail for any real chain but can for small test fixtures. Leaving it as `u32` at the boundary is probably right — raising it here so the choice is deliberate rather than incidental.
When `ChainPosition::Confirmed` carries `transitively: Some(_)`, the true confirmation height may be lower than the anchor's. Overestimating the height understates the number of confirmations, so relative height locks and coinbase maturity both evaluate conservatively and the height is safe to keep. The conversion above keeps it and drops `prev_mtp`, since that value is computed at the anchor height rather than the coin's own; `prev_mtp: None` already means "time-based locks undeterminable" in `bdk_tx`.
## Rejected alternative
Moving `Input`, `Output` and `ChangeScript` into `bdk_core` so `CanonicalView` could return `Input` directly. `Input` holds a `miniscript::plan::Plan` and `Output` holds a `DefiniteDescriptor`, so `bdk_core` would gain a hard `miniscript` dependency. `bdk_core` currently depends only on `bitcoin` (plus optional `serde`/`hashbrown`), and every chain source — `bdk_electrum`, `bdk_esplora`, `bdk_bitcoind_rpc` — sits on top of it. None of them touch descriptors.
`ConfirmationStatus` was also a candidate to move, since it is bitcoin-only. Not needed: the conversion constructs it from `CanonicalTxOut`. Note it is not unified with `ConfirmationBlockTime`, which we want to remove anyway — timestamps will be decided by `CheckPoint`, and anchors will just use `BlockId`.
## Scope
- [ ] Move `CanonicalTxOut` and `ChainPosition` to `bdk_core`, re-exporting from `bdk_chain`
- [ ] Unlink the `[`Anchor`]` intra-doc reference in `ChainPosition`'s docs — it won't resolve from `bdk_core`
- [ ] Decide whether `prev_mtp` stays `u32` at the boundary
- [ ] Doc example or integration test covering `CanonicalView` -> `Input`
## Depends on
- #2139 (`prev_mtp` on `CanonicalTx` / `CanonicalTxOut`)
Contributor guide
Assessment
This issue has not been assessed yet.