feat: Improve error handling#4
Merged
evanlinjin merged 4 commits intoevanlinjin:evans_simplificationsfrom May 1, 2025
Merged
Conversation
4 tasks
evanlinjin
reviewed
Apr 23, 2025
ea8305f to
62063e7
Compare
Author
|
Rebased onto fee4e67
|
evanlinjin
reviewed
Apr 30, 2025
evanlinjin
reviewed
Apr 30, 2025
Owner
evanlinjin
left a comment
There was a problem hiding this comment.
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.
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
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changed some functions that return
Optionto return aResultinstead, which makes for better error handling.This is based on the work in #2
Added
FromPsbtInputErrorCoinbaseMismatchCannotMeetTargetSelectorErrorGetForeignUnspentErrorExtractReplacementsErrorI also fixed a few leftover review comments from bitcoindevkit/bdk-tx#1.