The new `TxBuilder` #1

evanlinjin · 2025-04-04T10:47:46Z

Rationale

There were many problems with the design of the old TxBuilder currently residing in bdk_wallet.

No separation of concerns - All options are crammed into a single TxParams struct.
- Some combinations of parameters are hard to reason about, or may even create invalid transactions.
- It's difficult to maintain. Adding new features tends to exacerbate the issue by introducing even more variants.
No planning module - The planning module can help pick the best spending branch just by specifying available assets.
Inaccurate weight calculations - Coin selection does not currently account for varints of inputs/outputs.
"Policies" are applied to all transactions, irrelevant of which inputs were actually chosen.
Cannot accurately determine when relatively-locked outputs are spendable.
The "no breaking changes" constraint This made it difficult to introduce new features to the existing codebase without introducing more footguns.
Coupled to wallet - Limits flexibility.

Rewriting the TxBuilder and decoupling it from bdk_wallet allows us to:

Make it usable independently of bdk_wallet.
Fix all the problems mentioned above cleanly.
Avoid waiting for bdk_wallet v2.0 to ship these fixes/improvements.

Proposed Repository Layout

I propose turning this repo into a Cargo workspace, containing the following crates:

bdk_tx_core - This will contain the core Input, InputGroup types (and maybe more). Both bdk_chain and bdk_wallet would only need to output these types to interoperate with the rest of the tx-building crates. These types are expected to be relatively stable, with most changes being non-breaking.
bdk_tx - Handles the coin selection + psbt creation/signing/finalization. @nymius suggested splitting coin selection into its own crate, but I'm not fully convinced that's necessary (for now).
bdk_coin_select - It would be convenient to move this crate into the workspace for easier development.
bdk_coin_control - Contains CoinControl. This is the only crate in the workspace that depends on bdk_chain and bdk_core. It handles canonicalization and ensures the caller does not provide a bad set of input candidates (cc. @stevenroose).

Where to Start Reviewing?

Start with tests/synopsis.rs - For now, it's more of an example than a test.

It includes:

A basic create-transaction example.
A more involved (but well-commented) cancel-transaction example.

These examples demonstrate how the various types in the library interact across the different stages of transaction building.

How can I help?

The main architecture/flow is complete.

However, there are still a whole bunch of TODOs scattered over the codebase and should be done before merging. These TODOs be summarized as follows (these should be done before merging):

Fix up error types. Some methods return Options when they should return custom error enums with variants. It is helpful to know why something failed.
Documentation. Things should be better documented. feat: Improve error handling evanlinjin/bdk-tx#4
Examples. The tests/synopsis.rs should be moves to a examples folder. The Wallet type should be contained in examples/common/mod.rs so that it can be shared across examples. chore: Add examples directory evanlinjin/bdk-tx#5
Update README.

This is enough to get a merge and get users to start using/testing.

To help:

Review the codebase.
Add PRs addressing various TODO comments. The PRs should be made to my fork (evanlinjin/bdk-tx).

How long would it take for this to be stable?

Without any hold-backs, and multiple contributors, I think 2 months is a reasonable time.

src/create_selection.rs

src/input.rs

nymius

I've been looking at the following sources to try to get the whole idea behind this:

bitcoindevkit/bdk#899
TxBuilder redesign

I think we should try to minimize the inter dependency as much as possible (bdk_chain and bdk_coin_select). Maybe we are thinking too close to the previous TxBuilder design, where the transaction constrains were specified and all the building process was done downstream in the same context.
I see no problem in leaving some parts undefined, stating some assumptions about the inputs, like whether they have already been selected, and working under those assumptions.

Right now I think this repo should try to complete steps 4, 5 and 6 of bitcoindevkit/bdk#899 and maybe move input grouping and filtering (step 1) to a separate project, or leave it here but work on them separately. Without touching coin selection here and just adding examples as tests/synopsis.rs in the right places (bdk_wallet) to show how these different modules integrate together.

src/create_psbt.rs

src/create_selection.rs

tests/synopsis.rs

nymius · 2025-04-05T16:58:46Z

src/create_selection.rs

+        .add(pks)
+        .add(additional_assets);
+
+    tx_graph


Could this be removed in favor of outsourcing it to bdk_chain and just receiving the data as some parameter? Thinking in removing the bdk_chain dependency here.

src/finalizer.rs

src/output.rs

tests/synopsis.rs

src/input.rs

evanlinjin · 2025-04-05T23:45:48Z

@nymius

In terms of the dependency tree, I'm thinking of putting the Input, InputGroup, Output types in a separate crate called bdk_tx_core. This way, bdk_wallet and bdk_chain will only need to depend on bdk_tx_core so that they can output a list/iterator of raw candidates.

In terms of separating out the coin selection and psbt creation into separate crates... I'll need to give it more thought.

src/create_selection.rs

evanlinjin · 2025-04-07T01:20:13Z

src/input_candidates.rs

+        let mut pks = vec![];
+        for desc in descriptors.values() {
+            desc.for_each_key(|k| {
+                pks.extend(k.clone().into_single_keys());
+                true
+            });
+        }


As mentioned in @ValuedMammal's HackMD, sometimes the caller may not have all keys available for spending from. What is the right API so that it is not tedious to fill in available assets?

src/canonical_leaves.rs

**Remove `PsbtUpdater` & `Builder`** These was no need for these to be separated. The builder pattern was nice but not needed at this stage. **Remove `DataProvider`** Instead of creating a separate struct for passing in data for inputs/outputs. We can just have the data within our inputs/outputs. **Introduce `Input` and `Output` types** The `Input` type is what is passed through all stages of tx-building. From canonicalization, grouping/filtering, coin-selection and finally used to create the psbt. The `Output` type is used to describe a psbt output. It either is one that we own (in which, we have the descriptor), or one we do not (in which case, we just have a spk).

* Added `CoinControl` which handles our canonical set, filtering and grouping. * Added `InputCandidates` which contains inputs available for coin selection, with the ability to mark inputs as "must select". * Added `tests/synopsis` to see what it will look like with everything put together. * Changed `Input` so that it can be constructed from a PSBT Input. Also added more helper methods so that data such as "is spendable now" can be obtained from the input. * Added `Selection` which represents the final coin selection result. * Added `Selector` which contains the `bdk_coin_select::CoinSelector`. This allows flexible selections. * Added `RbfSet` to contain RBF logic. * Updated `tests/synopsis.rs` to show mempool-policy-conforming tx-cancellation.

`CanonicalUnspents` is our "single source of truth" and all UTXOs added to the selector should really pass through it to ensure we don't create transactions that double-spend themselves. Also replaced `Input::from_finalized_psbt_input` with `from_psbt_input` for more flexibility. The responsibility of grouping and filtering input candidates have been moved to `InputCandidates` (instead of the now-removed `CoinControl` struct). The downside of this is that it makes it more invloved to interface with `bdk_chain` or `bdk_wallet`. Adding methods on `KeychainTxOutIndex` will help.

evanlinjin · 2025-04-15T09:26:47Z

src/selector.rs

+    /// Do branch-and-bound selection with `LowestFee` metric.
+    pub fn select_for_lowest_fee(
+        &mut self,
+        longterm_feerate: FeeRate,
+        max_bnb_rounds: usize,
+    ) -> Result<Ordf32, NoBnbSolution> {
+        self.inner.run_bnb(
+            LowestFee {
+                target: self.target,
+                long_term_feerate: cs_feerate(longterm_feerate),
+                change_policy: self.change_policy,
+            },
+            max_bnb_rounds,
+        )
+    }


Remove this and have select_with_algorithm where algorithm is a closure.

evanlinjin · 2025-04-15T13:33:24Z

Having a payjoin example here will be nice.

Refer to: bitcoindevkit/bdk#913 (comment)
Refer to: bitcoindevkit/bdk-cli#156

KnowWhoami · 2025-04-20T11:04:01Z

While reviewing the PR, I noticed that Signer struct wrapper over the KeyMap is introduced in order to have GetKey implementation over it..
But IMO, Instead rust-miniscript should implement this GetKey trait for KeyMap struct not by their downstream users..

Currently rust-bitcoin implements this trait only for Xpriv/ $set<Xpriv> / $map<PublicKey, PrivateKey>/ $map<XOnlyPublicKey, PrivateKey>..

What do you think @evanlinjin ?

ValuedMammal · 2025-04-27T15:00:52Z

While reviewing the PR, I noticed that Signer struct wrapper over the KeyMap is introduced in order to have GetKey implementation over it.. But IMO, Instead rust-miniscript should implement this GetKey trait for KeyMap struct not by their downstream users..

@KnowWhoami brings up a good point. In fact our implementation really boils down to an implementation of GetKey for DescriptorSecretKey. I suppose if we had that in rust-miniscript, then the implementation for Signer would be as simple as iterating over the values of the keymap and trying get_key on each one. rust-miniscript#765

KnowWhoami · 2025-04-18T09:47:14Z

tests/synopsis.rs

+            .after(absolute::LockTime::from_height(tip.height).expect("must be valid height"))
+            .add({
+                let mut pks = vec![];
+                for (_, desc) in index.keychains() {


nit:

Suggested change

for (_, desc) in index.keychains() {

for (_, desc) in self.graph.index.keychains() {

Since the above created index variable is used once -> so it's better to write self.graph.index at the caller site..

tests/synopsis.rs

src/selector.rs

src/finalizer.rs

- Moved `synopsis.rs` to examples dir - Added `examples/common.rs` to contain the example wallet - Update README to link to the examples

Change functions that return Option to return a Result instead which makes for better error handling. Added - `FromPsbtInputError` - `CoinbaseMismatch` - `CannotMeetTarget` - `SelectorError` - `GetForeignUnspentError` - `ExtractReplacementsError`

Include the check for `final_script_sig` when evaluating whether the input is finalized.

ValuedMammal · 2025-05-02T13:32:42Z

I realized that in 386bdae pin-msrv.sh was not given executable permissions. chmod +x ci/pin-msrv.sh. We could also change CI to just cargo check on MSRV (std and no-std).

ValuedMammal

ACK 0e4a4a6

evanlinjin

self-ACK c806d5e

evanlinjin force-pushed the evans_simplifications branch from f7bbc03 to 3e1290b Compare April 4, 2025 11:36

evanlinjin commented Apr 4, 2025

View reviewed changes

src/create_selection.rs Outdated Show resolved Hide resolved

src/input.rs Outdated Show resolved Hide resolved

evanlinjin mentioned this pull request Apr 4, 2025

feat(wallet): add TxBuilder::replace_tx bitcoindevkit/bdk#1799

Closed

5 tasks

nymius reviewed Apr 5, 2025

View reviewed changes

thunderbiscuit mentioned this pull request Apr 5, 2025

Exploration: bdk_tx + bdk_wallet #2

Closed

evanlinjin commented Apr 6, 2025

View reviewed changes

src/create_selection.rs Outdated Show resolved Hide resolved

evanlinjin commented Apr 6, 2025

View reviewed changes

src/create_selection.rs Outdated Show resolved Hide resolved

evanlinjin commented Apr 7, 2025

View reviewed changes

evanlinjin force-pushed the evans_simplifications branch from a83abdd to 0f4b6c9 Compare April 8, 2025 09:18

evanlinjin changed the title ~~WIP~~ The new TxBuilder Apr 8, 2025

evanlinjin commented Apr 12, 2025

View reviewed changes

src/canonical_leaves.rs Outdated Show resolved Hide resolved

evanlinjin force-pushed the evans_simplifications branch 4 times, most recently from e24482e to 991a770 Compare April 13, 2025 09:16

evanlinjin added 3 commits April 13, 2025 19:16

evanlinjin force-pushed the evans_simplifications branch from 991a770 to 9bdd0b1 Compare April 13, 2025 09:48

evanlinjin commented Apr 15, 2025

View reviewed changes

This was referenced Apr 20, 2025

Add docs + initial review evanlinjin/bdk-tx#2

Closed

feat: Improve error handling evanlinjin/bdk-tx#4

Merged

[example] Create a payjoin example #3

Open

KnowWhoami reviewed Apr 27, 2025

View reviewed changes

evanlinjin force-pushed the evans_simplifications branch from 6b92cdd to 4b2ff5d Compare May 1, 2025 01:59

ci: update for rust 1.63.0

386bdae

ValuedMammal added 5 commits May 1, 2025 12:01

chore: Add examples directory

60fa9aa

- Moved `synopsis.rs` to examples dir - Added `examples/common.rs` to contain the example wallet - Update README to link to the examples

ci: fix typo in rust.yml

1124439

feat!: Improve error handling

866f881

Change functions that return Option to return a Result instead which makes for better error handling. Added - `FromPsbtInputError` - `CoinbaseMismatch` - `CannotMeetTarget` - `SelectorError` - `GetForeignUnspentError` - `ExtractReplacementsError`

feat: Add Selector::select_with_algorithm

ad78206

fix(finalizer): return true if already finalized in finalize_input

0e4a4a6

Include the check for `final_script_sig` when evaluating whether the input is finalized.

evanlinjin force-pushed the evans_simplifications branch from 4b2ff5d to 0e4a4a6 Compare May 1, 2025 02:01

sdmg15 mentioned this pull request May 5, 2025

Support DNS payment instructions, BIP353 bitcoindevkit/bdk_wallet#13

Open

evanlinjin mentioned this pull request May 5, 2025

fix: Inconsistent checking of RBF rules. bitcoindevkit/bdk_wallet#225

Open

8 tasks

ValuedMammal approved these changes May 7, 2025

View reviewed changes

evanlinjin added 2 commits May 14, 2025 14:54

refactor: Rename InputStatus to TxStatus

22ebcfe

chore(ci): Make pin-msrv.sh exec

c806d5e

evanlinjin commented May 14, 2025

View reviewed changes

evanlinjin merged commit 6b1816a into bitcoindevkit:master May 14, 2025
3 checks passed

aagbotemi mentioned this pull request May 28, 2025

feat: enable random anti-fee sniping #5

Open

3 tasks

	for (_, desc) in index.keychains() {
	for (_, desc) in self.graph.index.keychains() {

The new TxBuilder #1

The new TxBuilder #1

Uh oh!

Conversation

evanlinjin commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Proposed Repository Layout

Where to Start Reviewing?

How can I help?

How long would it take for this to be stable?

Uh oh!

Uh oh!

Uh oh!

nymius left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nymius Apr 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evanlinjin commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evanlinjin Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

evanlinjin Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

evanlinjin commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KnowWhoami commented Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ValuedMammal commented Apr 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KnowWhoami Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ValuedMammal commented May 2, 2025

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

The new `TxBuilder` #1

The new `TxBuilder` #1

evanlinjin commented Apr 4, 2025 •

edited

Loading

nymius left a comment •

edited

Loading

evanlinjin commented Apr 5, 2025 •

edited

Loading

evanlinjin commented Apr 15, 2025 •

edited

Loading

KnowWhoami commented Apr 20, 2025 •

edited

Loading

ValuedMammal commented Apr 27, 2025 •

edited

Loading