fix(wallet): don't panic in create_tx when a descriptor has no policy - #583
Open
artofbitcoin wants to merge 2 commits into
Open
artofbitcoin wants to merge 2 commits into
artofbitcoin wants to merge 2 commits into
Conversation
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.
Description
Fixes #579
When
TxParams::conditionis not set,create_txderives the spending condition from thepolicies of the wallet descriptors and called
.unwrap()on theOption<Policy>returned byextract_policy(once for the external descriptor, once for the internal one). Policyextraction returns
Nonefor descriptors without a policy representation, e.g.c:expr_raw_pkh(<hash160>)andsh(c:expr_raw_pkh(<hash160>)). Such descriptors are acceptedwhen creating a wallet, so every
build_tx().finish()on them panicked.This PR treats a missing policy as contributing no requirements (
Condition::default()) forthat keychain instead of unwrapping. Both the external and the internal (change) descriptor
paths are covered.
Notes to the reviewers
The issue suggests returning an error (e.g. a
CreateTxError). I went with "no policy meansno requirements" instead, for these reasons:
CreateTxErroris a public enum and not#[non_exhaustive], so a new variant would be abreaking change. None of the existing variants describes this case well
(
SpendingPolicyRequiredmeans a policy path is missing,Policy(PolicyError)has nofitting
PolicyErrorvariant, andPolicyErroris not#[non_exhaustive]either).Condition(csv / timelock). A top-levelNoneis only produced when no fragment of the descriptor has a policy representation, so there is
no policy path to select and no timelock to derive.
Condition::default()is also what acaller would pass through
TxBuilder::set_conditionfor such a descriptor, in which casethe policy is not extracted at all.
create_txdoes not depend on the policy. With the fix, a funded raw-pkh walletproduces a PSBT (version 2, default sequence) rather than failing somewhere later.
If you prefer an explicit error for descriptors without a policy, I am happy to change this to
a new
CreateTxErrorvariant; that would need to be marked as breaking.Not addressed here:
extract_policysilently drops raw-pkh fragments inside largerminiscripts (
make_and/make_or/threshskipNonechildren). That is existing behaviourand unrelated to the panic.
The branch has two commits (fix, then tests) because I committed through the GitHub web editor.
Commands run locally (Rust stable 1.97.0, since the pinned 1.96.0 toolchain and nightly were
not available to me), following the
justfilepre-pushrecipe:masterbefore the fix: 4 failed, all withcalled Option::unwrap() on a None valueatsrc/wallet/mod.rs:1300(external) andsrc/wallet/mod.rs:1309(internal).cargo fmt --all -- --check(stable rustfmt): no diff.cargo check --all-targets --no-default-features --features miniscript/no-std,bdk_chain/hashbrown,with and without
--cfg bdk_wallet_unstable: ok.cargo check --all-targets --features std,compiler,all-keys,rusqlite,file_store,test-utils: ok.RUSTFLAGS="--cfg bdk_wallet_unstable" cargo check --all-targets --all-features: ok.cargo clippy --all-targetswith-D warnings(both feature sets): fails on 1.97.0 withclippy::needless_return_with_question_markin code not touched by this PR(
src/descriptor/policy.rs:707,730,src/wallet/signer.rs:340,465,468, andsrc/wallet/mod.rs:3646in the unstable build). With that one lint allowed, clippy passesfor both feature sets.
RUSTFLAGS="" cargo test --workspace --features std,compiler,all-keys,rusqlite,file_store,test-utils:422 passed, 0 failed, 3 ignored (
tests/wallet.rs: 131 passed, including the 4 new tests).RUSTFLAGS="--cfg bdk_wallet_unstable" RUSTDOCFLAGS="--cfg bdk_wallet_unstable" cargo test --workspace --all-features:450 passed, 0 failed, 3 ignored.
RUSTDOCFLAGS="-D warnings --cfg bdk_wallet_unstable" cargo doc --workspace --all-features --no-deps: ok.Not run:
cargo +nightly fmt(stable rustfmt ignores the nightly-only options inrustfmt.toml), anything on the pinned 1.96.0 toolchain or on the MSRV (1.85.0).Changelog notice
Fixed
TxBuilder::finishno longer panics for descriptors without an extractable policy, such asc:expr_raw_pkh(..)Before submitting
I prepared this change with the help of an AI assistant and reviewed it; the test results above come from actual runs.