Skip to content

feat: Improve error handling#4

Merged
evanlinjin merged 4 commits intoevanlinjin:evans_simplificationsfrom
ValuedMammal:feat/errors
May 1, 2025
Merged

feat: Improve error handling#4
evanlinjin merged 4 commits intoevanlinjin:evans_simplificationsfrom
ValuedMammal:feat/errors

Conversation

@ValuedMammal
Copy link
Copy Markdown

@ValuedMammal ValuedMammal commented Apr 21, 2025

Changed some functions that return Option to return a Result instead, which makes for better error handling.

This is based on the work in #2

Added

  • FromPsbtInputError
  • CoinbaseMismatch
  • CannotMeetTarget
  • SelectorError
  • GetForeignUnspentError
  • ExtractReplacementsError

I also fixed a few leftover review comments from bitcoindevkit/bdk-tx#1.

@ValuedMammal
Copy link
Copy Markdown
Author

Rebased onto fee4e67

  • Fixed a typo in rust.yml
  • Renamed CanonicalUnspentsError to GetForeignUnspentError
  • Added CoinbaseMismatch error
  • Changed return type of Selector::try_finalize back to Option, as it seemed to make sense
  • Added Selector::select_with_algorithm

@ValuedMammal ValuedMammal requested a review from evanlinjin April 25, 2025 16:01
Copy link
Copy Markdown
Owner

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just a find request.

Can address the TODOs later on.

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.
Copy link
Copy Markdown
Owner

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5bc533d

@evanlinjin evanlinjin merged commit 6b92cdd into evanlinjin:evans_simplifications May 1, 2025
evanlinjin added a commit to bitcoindevkit/bdk-tx that referenced this pull request May 14, 2025
c806d5e chore(ci): Make `pin-msrv.sh` exec (志宇)
22ebcfe refactor: Rename `InputStatus` to `TxStatus` (志宇)
0e4a4a6 fix(finalizer): return true if already finalized in `finalize_input` (valued mammal)
ad78206 feat: Add `Selector::select_with_algorithm` (valued mammal)
866f881 feat!: Improve error handling (valued mammal)
1124439 ci: fix typo in rust.yml (valued mammal)
60fa9aa chore: Add examples directory (valued mammal)
386bdae ci: update for rust 1.63.0 (valued mammal)
9bdd0b1 feat!: Remove `bdk_chain` dependency (志宇)
4250de1 feat!: More progress (志宇)
3a5a67e feat!: Replace `PsbtUpdater` and `Builder` with `create_psbt` method (志宇)

Pull request description:

  ## 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):

  - [x] Fix up error types. Some methods return `Option`s when they should return custom error enums with variants. It is helpful to know why something failed.
  - [x] Documentation. Things should be better documented. evanlinjin#4
  - [x] 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. evanlinjin#5
  - [x] 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.

ACKs for top commit:
  evanlinjin:
    self-ACK c806d5e

Tree-SHA512: a37fdd2683ed1967912ef2f2828d40a0939d956d8b4372591fcfa3e8e5d458bf478706345ae988d5966efcbf557f1396e4bb8f6f98c0dedc891b5cb790c97e77
@ValuedMammal ValuedMammal deleted the feat/errors branch May 15, 2025 14:56
evanlinjin pushed a commit that referenced this pull request Jan 25, 2026
179860e feat!: implement anti-fee-sniping protection (Abiodun)
c95e8ed refactor: remove unnecessary clippy allow attributes (Abiodun)

Pull request description:

  This PR implements Anti-Fee-Sniping with randomization as discussed in issue #4

  ## Notes to the reviewers
  The implementation adds randomization to anti-fee-sniping behavior:

  1. Uses a 50/50 chance to choose between nLockTime and nSequence (when possible)
  2. Adds a 10% chance to set either value further back in time (by a random value between 0-99)
  3. Detects taproot inputs and their confirmation status

  ## Changelog notice

  ### Changed
  - `CreatePsbtError::MissingFullTxForLegacyInput` and `CreatePsbtError::MissingFullTxForSegwitV0Input` now wrap `Input` in `Box` (breaking change)

  ### Added
  - `PsbtParams::enable_anti_fee_sniping` field for BIP326 anti-fee-sniping protection
  - `CreatePsbtError::InvalidLockTime` and `CreatePsbtError::UnsupportedVersion` error variants
  - `Selection::create_psbt_with_rng` method for custom RNG

  * [x] I've signed all my commits
  * [x] I ran `cargo fmt` and `cargo clippy` before committing
  * [x] I've added docs for the new feature

  Closes #4

ACKs for top commit:
  ValuedMammal:
    ACK 179860e
  nymius:
    ACK 179860e

Tree-SHA512: 52a9190560e2714f99afae519f7facd304013afc0cf4167204be92b084feed166496250f041378e0c676ac383006a7d90ebd37d4cd486bb7c0664f834ca188e4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants