bitcoindevkit / bitcoindevkit/bdk-tx

Introduce `TxTemplate` as a state between `Selection` and `Psbt`

Open
#57 4 comments 0 reactions 1 assignee Claimed by @evanlinjin View on GitHub
api discussion enhancement
Dominant language
Rust
Stars
13
Forks
17
Avg merge
17h 22m
Merged PRs (30d)
1

Description

## Background

After a recent revisit of the codebase, I was dissatisfied with the API shape as it is now.

Initially, I blamed it on how AFS was added in the codebase. I left a post-merge review on [#5 here](https://github.com/bitcoindevkit/bdk-tx/pull/5#pullrequestreview-4243343806). The problems I pointed out can be summarized as:

* People who don't care about AFS now need to handle 2 additional error variants and an extra input.
* AFS is not "PSBT-creation-related". It's just an algorithm for deciding on locktime or input sequence values.
* There were some complex interactions between `PsbtInput` fields after the PR.

I suggested that AFS should be a step after `create_psbt`. @aagbotemi created a second PR to address my review [here (#56)](https://github.com/bitcoindevkit/bdk-tx/pull/56).

After looking at this PR, I realized my suggestions were incomplete. I did another thorough investigation and I've come up with a more complete direction/approach (which I will address here). Let's start with talking about AFS.

### Where should AFS go?

- **AFS should be a separate step to PSBT-creation**
This is where my inital review of #5 got right. We don't want to pollute `.create_psbt` with inputs and error variants that are only relevant to AFS.

- **We should have the AFS step BEFORE psbt creation**
This is my new realization. AFS is essentially just logic which decides on locktime/sequence. After the decision is made, this should be easily passed into creating a psbt. Additionally, this is how it's done in Bitcoin Core. [`DiscourageFeeSniping` is a step before PSBT creation](https://github.com/bitcoin/bitcoin/blob/10ca73c02cbff59f2134c0c7da3b8d0a7e727475/src/wallet/rpc/spend.cpp#L108).

However, since AFS logic currently depends on values contained in `PsbtParams`, it's difficult to implement it elsewhere. We need an architectural refactor.

## Proposal

I propose putting an additional stage in our transaction-creation pipeline called `TxTemplate`. This stage will sit between `Selection` and `Psbt`. Unlike the `Selection` type, `TxTemplate` will have complete information to create a `bitcoin::Transaction` (including the version, locktime and sequence fields which currently all reside in `PsbtParams`).

```rust
let mut rand = rand::thread_rng();

let mut (psbt, finalizer) = selection
.into_tx_template(TxTemplateParams::default())
.shuffle_inputs(&mut rand)
.apply_anti_fee_sniping(tip_height, &mut rand)?
.create_psbt(PsbtBuildParams::default())?;

let _ = psbt.sign(&signer, &secp);
let res = finalizer.finalize(&mut psbt);
assert!(res.is_finalized());
```

### Benefits of `TxTemplate`

* Clearer separation of concern. `TxTemplateParams` can purely be for deciding on bitcoin transaction fields. `PsbtParams` can be for PSBT-specific options.
* Callers that do not wish to work with `Psbt`s are no longer forced to. Additionally, when we add [support for PSBT V2](https://github.com/bitcoindevkit/bdk-tx/issues/48) in the future, we have a clean way to do it as a method on `TxTemplate::create_psbt_v2()`.
* AFS can be a single method on `TxTemplate`.

### Shape of `TxTemplate` and `TxTemplateParams`

```rust
/// Parameters for resolving a [`Selection`] into a [`TxTemplate`].
///
/// Carries only the *floors* and *defaults* that must be resolved at
/// template construction. Optional transformations (anti-fee-sniping,
/// shuffling, etc.) are exposed as methods on [`TxTemplate`].
#[derive(Debug, Clone)]
pub struct TxTemplateParams {
/// Minimum tx version. Acts as a floor on `tx.version`.
///
/// Default: [`transaction::Version::TWO`].
pub min_version: transaction::Version,

/// Minimum tx lock_time. Acts as a floor on `tx.lock_time`.
///
/// Default: [`absolute::LockTime::ZERO`].
pub min_locktime: absolute::LockTime,

/// Default sequence used for plan-based inputs that don't specify their
/// own (via plan-required relative timelock or via
/// [`Input::set_sequence`]).
///
/// Default: [`FALLBACK_SEQUENCE`] (Bitcoin Core's wallet default).
pub fallback_sequence: Sequence,
}

/// A fully-resolved tx shape, intermediate between [`Selection`] and the
/// final [`Psbt`] / [`bitcoin::Transaction`].
#[derive(Debug, Clone)]
pub struct TxTemplate {
pub(crate) version: transaction::Version,
pub(crate) lock_time: absolute::LockTime,
pub(crate) inputs: Vec,
pub(crate) outputs: Vec,
}
```

In order for the above shape to work, the `Input` type should be able to specify the sequence and sighash values.

* #60
* #61

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.