Append witness script for P2WSH in `Plan::satisfy()` by evanlinjin · Pull Request #897 · rust-bitcoin/rust-miniscript

evanlinjin · 2026-02-03T05:14:15Z

Fix Plan::satisfy() to correctly append the witnessScript as the final witness stack element for P2WSH descriptor types (Wsh, WshSortedMulti, ShWsh, ShWshSortedMulti).

Previously, these types were incorrectly grouped with Wpkh and Tr, which don't require a trailing witness script. This caused transactions built using Plan::satisfy() to fail validation with "Witness program hash mismatch" when broadcast.

The fix separates the descriptor type handling:

Wpkh/Tr: return stack as-is (no witness script needed)
Wsh/WshSortedMulti: append witness script, empty script_sig
ShWpkh: return stack with unsigned_script_sig (no witness script)
ShWsh/ShWshSortedMulti: append witness script and unsigned_script_sig

Closes #896

🤖 Generated with Claude Code

evanlinjin · 2026-02-03T06:05:08Z

The other option is to update Plan::satisfy docs to mention that witnessScript will not be pushed to the witness stack. However, I don't think that makes any sense.

evanlinjin · 2026-02-03T06:16:44Z

cc @tcharding

apoelstra · 2026-02-03T14:30:14Z

In 962bcc2:

It looks like you have two identical branches that only differ in their expect text. You should just combine these.

Thanks for the LLM disclosure! And nice job finding this bug.

tcharding · 2026-02-04T04:02:57Z

bitcoind-tests/tests/setup/test_util.rs

 use miniscript::{
-    bitcoin, hash256, Descriptor, DescriptorPublicKey, Error, Miniscript, ScriptContext,
-    Translator,
+    bitcoin, hash256, Descriptor, DescriptorPublicKey, Error, Miniscript, ScriptContext, Translator,


nit: If you feel like it can you throw a patch at the front of this PR that runs the formatter in bitcoind-tests please mate so that these fromatting changes aren't mixed in with your changes.

tcharding · 2026-02-04T04:03:56Z

bitcoind-tests/tests/test_desc.rs

I rekon we can trim a bit more LLM noise out of this patch. I see no value in these comments.

tcharding · 2026-02-04T04:14:44Z

bitcoind-tests/tests/test_desc.rs

+    assert!(num_conf > 0);
+
+    Ok(unsigned_tx.input[0].witness.clone())
+}


This repo is not my baby so defering to @apoelstra.

Do we want to have code like this in the repo? It may do whats needed but its pretty far from clean code.

(Not a dig at you @evanlinjin more a philosophical / high level comment on do we want to merge LLM generated test code since users will look at it and copy it and think we wrote it.)

The idea of this is to test what we have against Bitcoin core - can we broadcast a tx with wsh spend(s)?

In terms of "cleanliness", I would argue it's quite easy to read as it is? Could you expand on this please, thanks.

tcharding · 2026-02-04T04:15:34Z

bitcoind-tests/tests/test_desc.rs

I'd prefer to see two tests instead of these separator style comments.

tcharding · 2026-02-04T04:16:02Z

bitcoind-tests/tests/test_desc.rs

All these println statements are unneeded IMO.

tcharding · 2026-02-04T04:40:35Z

src/plan.rs

+            | DescriptorType::ShWshSortedMulti => {
+                let mut stack = stack;
+                let witness_script = self
+                    .descriptor
+                    .explicit_script()
+                    .expect("wsh descriptors have explicit script");
+                stack.push(witness_script.into_bytes());
+                let script_sig = self.descriptor.unsigned_script_sig();
+                (stack, script_sig)
            }


Suggested change

| DescriptorType::ShWshSortedMulti => {

let mut stack = stack;

let witness_script = self

.descriptor

.explicit_script()

.expect("wsh descriptors have explicit script");

stack.push(witness_script.into_bytes());

let script_sig = self.descriptor.unsigned_script_sig();

(stack, script_sig)

}

| DescriptorType::ShWshSortedMulti => {

let mut stack = stack;

// We need to append the witness script as per BIP-141

let witness_script = self

.descriptor

.explicit_script()

.expect("wsh descriptors have explicit script");

stack.push(witness_script.into_bytes());

(stack, self.descriptor.unsigned_script_sig())

}

I don't hack on this repo much so am not super familiar with the style but I'd write it like that. Specifically making the return statement mirror the other one that is the same while highlighting the difference.

tcharding

Thanks @evanlinjin for the bug find and the fix. I appreciate that you have other work to do so I'm hesitant to ask too much of you but from my personal view point this PR has way too much test code for this fix. The bare minimum test code to prove that the bug exits would be better IMO.

EDIT: hmm, re-reading my review I see I suggested cleaning up the test code then said "there is too much test code" ... I think you will get where I'm coming from.

evanlinjin · 2026-02-04T09:41:54Z

@tcharding I've cleaned things up as much as I can.

I think the integration test is worth keeping as it checks the logic against Bitcoin Core - however, I'm happy to remove them as well.

tcharding · 2026-02-04T22:33:22Z

Thanks @evanlinjin. I did a more thorough read of the test code. Its not perfect but I retract my statement that there is just too much test code. Thanks for iterating.

My review doesn't hold too much weight in this repo but FWIW this LGTM.

One nit: can we format with the nightly toolchain like we do for the rest of the repo?

Reviewed: 026943a

🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

Fix Plan::satisfy() to correctly append the witnessScript as the final witness stack element for P2WSH descriptor types (Wsh, WshSortedMulti, ShWsh, ShWshSortedMulti). Previously, these types were incorrectly grouped with Wpkh and Tr, which don't require a trailing witness script. This caused transactions built using Plan::satisfy() to fail validation with "Witness program hash mismatch" when broadcast. The fix separates the descriptor type handling: - Wpkh/Tr: return stack as-is (no witness script needed) - Wsh/WshSortedMulti: append witness script, empty script_sig - ShWpkh: return stack with unsigned_script_sig (no witness script) - ShWsh/ShWshSortedMulti: append witness script and unsigned_script_sig Closes rust-bitcoin#896 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

Add test_plan_satisfy_wsh() and test_plan_satisfy_sh_wsh() which verify that Plan::satisfy() correctly constructs witness and script_sig for P2WSH descriptors by calling plan.satisfy() directly and broadcasting the resulting transaction to Bitcoin Core. This is a regression test for rust-bitcoin#896. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

evanlinjin · 2026-02-09T02:00:06Z

One nit: can we format with the nightly toolchain like we do for the rest of the repo?

Done!

tcharding

ACK 871e953

apoelstra

ACK 871e953; successfully ran local tests

evanlinjin force-pushed the fix/plan-satisfy-does-not-append-witness-script-for-p2wsh branch from 66692de to 962bcc2 Compare February 3, 2026 05:43

evanlinjin force-pushed the fix/plan-satisfy-does-not-append-witness-script-for-p2wsh branch from 34a5f89 to 8d9a61a Compare February 4, 2026 01:43

tcharding reviewed Feb 4, 2026

View reviewed changes

bitcoind-tests/tests/test_desc.rs Outdated

Copy link

Member

tcharding Feb 4, 2026

Choose a reason for hiding this comment

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

All these println statements are unneeded IMO.

evanlinjin reacted with thumbs up emoji

tcharding reviewed Feb 4, 2026

View reviewed changes

evanlinjin force-pushed the fix/plan-satisfy-does-not-append-witness-script-for-p2wsh branch 4 times, most recently from 1c841fa to 026943a Compare February 4, 2026 09:34

evanlinjin and others added 3 commits February 9, 2026 01:53

bitcoind-tests: Run cargo fmt

3ddad52

🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

evanlinjin force-pushed the fix/plan-satisfy-does-not-append-witness-script-for-p2wsh branch from 026943a to 871e953 Compare February 9, 2026 01:54

evanlinjin requested a review from tcharding February 9, 2026 02:17

tcharding approved these changes Feb 10, 2026

View reviewed changes

apoelstra approved these changes Feb 18, 2026

View reviewed changes

Conversation

evanlinjin commented Feb 3, 2026

Uh oh!

evanlinjin commented Feb 3, 2026

Uh oh!

evanlinjin commented Feb 3, 2026

Uh oh!

apoelstra commented Feb 3, 2026

Uh oh!

tcharding Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

tcharding Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

tcharding Feb 4, 2026 • 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!

evanlinjin Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

tcharding Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

tcharding Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

tcharding Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

tcharding Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

tcharding 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!

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

Uh oh!

Uh oh!

tcharding commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evanlinjin commented Feb 9, 2026

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

tcharding Feb 4, 2026 •

edited

Loading

tcharding left a comment •

edited

Loading

evanlinjin commented Feb 4, 2026 •

edited

Loading

tcharding commented Feb 4, 2026 •

edited

Loading