Skip to content

fix(combine_psbt): Propagate Psbt::combine errors - #324

Open
tvpeter wants to merge 1 commit into
bitcoindevkit:masterfrom
tvpeter:fix/combine-psbt-propagate-error
Open

tvpeter wants to merge 1 commit into
bitcoindevkit:masterfrom
tvpeter:fix/combine-psbt-propagate-error

Conversation

@tvpeter

@tvpeter tvpeter commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Description

wallet ... combine_psbt discarded the result of every Psbt::combine call, so two BIP174-incompatible PSBTs (different unsigned transactions) produced exit code 0 and a PSBT that was silently missing the other PSBT's inputs and signatures.

This PR propagates the error with ?. BDKCliError already has a #[from] impl for bitcoin::psbt::Error, so no new error variant was needed.

Fixes #323

Changelog notice

  • Fixed combine_psbt to fail when PSBTs cannot be combined instead of silently dropping them

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

The fold merging the input PSBTs discarded each combine result with
`let _ = acc.combine(x)`. Per BIP174, Psbt::combine returns Err whenever
the two PSBTs do not describe the same unsigned transaction, so
incompatible inputs left the accumulator untouched.

This fix propagates the error with `?`. BDKCliError already converts from
bitcoin::psbt::Error, so the command now exits non-zero.

Fixes bitcoindevkit#323
@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.78%. Comparing base (5afbc8a) to head (682e414).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #324   +/-   ##
=======================================
  Coverage   57.78%   57.78%           
=======================================
  Files          22       22           
  Lines        3733     3733           
=======================================
  Hits         2157     2157           
  Misses       1576     1576           
Flag Coverage Δ
rust 57.78% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tvpeter tvpeter self-assigned this Sep 16, 2026

@vadim-anfv vadim-anfv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

tACK 682e414

Reproduced offline with two PSBTs spending the same dummy input but with different outputs, so their unsigned txs differ:

A=$(bitcoin-cli createpsbt '[{"txid":"1111111111111111111111111111111111111111111111111111111111111111","vout":0}]' '[{"data":"aa"}]')
B=$(bitcoin-cli createpsbt '[{"txid":"1111111111111111111111111111111111111111111111111111111111111111","vout":0}]' '[{"data":"bb"}]')
echo $A; echo $B
cHNidP8BAD8CAAAAARERERERERERERERERERERERERERERERERERERERERERAAAAAAD9////AQAAAAAAAAAAA2oBqgAAAAAAAAA=
cHNidP8BAD8CAAAAARERERERERERERERERERERERERERERERERERERERERERAAAAAAD9////AQAAAAAAAAAAA2oBuwAAAAAAAAA=

D=$(mktemp -d)
DESC=$(cargo run -q -- -n regtest -d $D descriptor --type tr | jq -r .private_descriptors.external)
cargo run -q -- -n regtest -d $D wallet -w demo config -e "$DESC" --database-type sqlite

master: A B exits 0 and returns B, A is silently dropped.

$ cargo run -q -- -n regtest -d $D wallet -w demo combine_psbt $A $B; echo "exit=$?"
{
  "psbt": "cHNidP8BAD8CAAAAARERERERERERERERERERERERERERERERERERERERERERAAAAAAD9////AQAAAAAAAAAAA2oBuwAAAAAAAAA=",
  "is_finalized": false
}
exit=0
$ cargo run -q -- -n regtest -d $D wallet -w demo combine_psbt $A $A; echo "exit=$?"
{
  "psbt": "cHNidP8BAD8CAAAAARERERERERERERERERERERERERERERERERERERERERERAAAAAAD9////AQAAAAAAAAAAA2oBqgAAAAAAAAA=",
  "is_finalized": false
}
exit=0

this branch: A B fails, A A unchanged.

$ cargo run -q -- -n regtest -d $D wallet -w demo combine_psbt $A $B; echo "exit=$?"
Error: PsbtError: different unsigned transaction: expected 26defa6e8fa255cd46eac60b7a3f9e415515297498da7cecdffdd8bc818aa37a, actual 1a67d0770fe1ffa6d1d745fe12aee522cdb44985c6aee69ebe15ed80caa225ae
exit=1
$ cargo run -q -- -n regtest -d $D wallet -w demo combine_psbt $A $A; echo "exit=$?"
{
  "psbt": "cHNidP8BAD8CAAAAARERERERERERERERERERERERERERERERERERERERERERAAAAAAD9////AQAAAAAAAAAAA2oBqgAAAAAAAAA=",
  "is_finalized": false
}
exit=0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

combine_psbt silently drops PSBTs that fail to combine (exit 0, incomplete output)

2 participants