refactor: remove SortedMultiVec and use Terminal::SortedMulti by trevarj · Pull Request #915 · rust-bitcoin/rust-miniscript

trevarj · 2026-04-04T13:12:54Z

Utilize the Miniscript parsing to handle sortedmulti as a Terminal.
Deleted sortedmulti.rs (SortedMultiVec)
Refactor Wsh to only wrap a Miniscript now that SortedMultiVec isn't used.
Refactor ShInner to remove SortedMulti variant and only use the Ms variant for
sortedmulti scripts

Now that sortedmulti_a is supported as a Terminal, I think it makes sense to
move sortedmulti over in the same way by following what multi does and applying
the sorting functions that were introduced in eba1aff.

Like sortedmulti_a, sortedmulti is sorted upon encoding and cannot be decoded
into from Script.

apoelstra · 2026-04-04T13:52:42Z

(CI is failing because the integration tests don't compile)

Hmmmm, okay, this seems reasonable enough to me. Does Core allow sortedmulti in arbitrary positions or only as a top-level descriptor?

trevarj · 2026-04-04T14:02:26Z

(CI is failing because the integration tests don't compile)

Hmmmm, okay, this seems reasonable enough to me. Does Core allow sortedmulti in arbitrary positions or only as a top-level descriptor?

Working on fixing the integration tests now! Will ping you when it's ready, my bad.

trevarj · 2026-04-04T15:00:04Z

                Descriptor::Sh(sh) => match sh.as_inner() {
-                    miniscript::descriptor::ShInner::Wsh(wsh) => match wsh.as_inner() {
-                        miniscript::descriptor::WshInner::SortedMulti(ref smv) => {
-                            let ms = Miniscript::from_ast(smv.sorted_node()).unwrap();


The key point here was to call sorted_node() and now we don't have something so convenient. See the HACK in find_sks_ms

Interesting. So, the issue is that we want to get all the keys in the descriptor, in the order they'd be needed, and we're using pk_iter for this. Since pk_iter can't sort the keys, we have to sort them after the fact.

This is a bit of a weird thing to desire -- does this test actually fail without this? It seems like we never use the ordering of sks_reqd. In fact, we lose the ordering because we iterate over it, produce signatures, then stick them into the BTreeMap psbt.inputs[0].partial_sigs.

Also, since #910 I think the logic in this example is actually wrong, or at least inconsistent -- it would sort sortedmulti but not sortedmulti_a. And your hack doesn't change this.

So can we just drop the sortedmulti logic here and treat sortedmulti like multi?

trevarj · 2026-04-04T15:02:01Z

+                ShInner::Wsh(ref wsh) => {
+                    let ms = wsh.as_inner();
+                    if let Terminal::SortedMulti(ref thresh) = ms.node {
+                        PkIter::from_sortedmulti(thresh.data())


Cannot call thresh.into_sorted_bip67() due to lacking ToPublicKey on Pk here. ~~It would clean up the integration test changes (HACK)~~. I'm sure it would be useful for users as well.

Right -- I get the desire that pk_iter return the keys in the correct order. But as you note, without a ToPublicKey bound, we don't actually know the correct order (and the user might not either, e.g. if there are wildcards in the keys).

We also can't make this do the right thing for ToPublicKey keys because Rust lacks specialization. (There are tons of places in this crate where we suffer from this, and you'll find that if you keep trying to clean stuff up, you will run into missing language features basically every single time.)

I think we need to just live with this.

There are tons of places in this crate where we suffer from this, and you'll find that if you keep trying to clean stuff up, you will run into missing language features basically every single time.

@apoelstra don't worry, in 2035 when specialization stabilizes we can get to it!

trevarj · 2026-04-04T15:03:54Z

    }
+
+    #[test]
+    fn too_many_pubkeys_for_p2sh() {


FYI: migrated from deleted sortedmulti.rs file.

trevarj · 2026-04-04T15:05:34Z

-        let mut witness = match self.inner {
-            WshInner::SortedMulti(ref smv) => smv.satisfy(satisfier)?,
-            WshInner::Ms(ref ms) => ms.satisfy_malleable(satisfier)?,
+        let mut witness = if let Terminal::SortedMulti(..) = self.ms.node {


All the if let Terminal::SortedMulti(..)'s are kind of bothering me.

Does it make sense to add a function on Miniscript called is_sortedmulti() or something?

Probably yes, but I'm curious what the heck is going on here. Why are we calling the non-malleable satisfier for sortedmultis? (What I suspect is: the old SortedMultiVec type had a satisfy method but no satisfy_malleable method, presumably because for multisigs, malleability is irrelevant. We needed to branch because SortedMultiVec is a distinct type, and then the branch wound up looking sus with one side saying satisfy and the other side saying satisfy_malleable, just because of this API gap.)

SO I think that in this PR, we just always want to call self.ms.satisfy_malleable, and not branch at all.

I based it off of this, which is from the deleted SortedMultiVec.

- /// Attempt to produce a satisfying witness for the - /// witness script represented by the parse tree - pub fn satisfy<S>(&self, satisfier: S) -> Result<Vec<Vec<u8>>, Error> - where - Pk: ToPublicKey, - S: Satisfier<Pk>, - { - let ms = Miniscript::from_ast(self.sorted_node()).expect("Multi node typecheck"); - ms.satisfy(satisfier)

We needed to branch because SortedMultiVec is a distinct type, and then the branch wound up looking sus with one side saying satisfy and the other side saying satisfy_malleable, just because of this API gap.

I don't think I could have guessed that lol...trusting you and changing to only call satisfy_malleable

trevarj · 2026-04-05T04:29:44Z

@apoelstra did a self-review with some questions. Let me know what you think, thanks!

apoelstra · 2026-04-09T14:41:04Z

816620a needs rebase

apoelstra · 2026-04-09T17:10:48Z

f9d0bdd needs rebase

trevarj · 2026-04-09T17:37:59Z

+                ShInner::Wsh(ref wsh) => {
+                    let ms = wsh.as_inner();
+                    if let Terminal::SortedMulti(ref thresh) = ms.node {
+                        PkIter::from_sortedmulti(thresh.data())


There are tons of places in this crate where we suffer from this, and you'll find that if you keep trying to clean stuff up, you will run into missing language features basically every single time.

@apoelstra don't worry, in 2035 when specialization stabilizes we can get to it!

trevarj · 2026-04-09T17:50:32Z

-        let mut witness = match self.inner {
-            WshInner::SortedMulti(ref smv) => smv.satisfy(satisfier)?,
-            WshInner::Ms(ref ms) => ms.satisfy_malleable(satisfier)?,
+        let mut witness = if let Terminal::SortedMulti(..) = self.ms.node {


I based it off of this, which is from the deleted SortedMultiVec.

- /// Attempt to produce a satisfying witness for the - /// witness script represented by the parse tree - pub fn satisfy<S>(&self, satisfier: S) -> Result<Vec<Vec<u8>>, Error> - where - Pk: ToPublicKey, - S: Satisfier<Pk>, - { - let ms = Miniscript::from_ast(self.sorted_node()).expect("Multi node typecheck"); - ms.satisfy(satisfier)

We needed to branch because SortedMultiVec is a distinct type, and then the branch wound up looking sus with one side saying satisfy and the other side saying satisfy_malleable, just because of this API gap.

I don't think I could have guessed that lol...trusting you and changing to only call satisfy_malleable

trevarj · 2026-04-09T17:52:58Z

+            | Terminal::Multi(_)
+            | Terminal::SortedMulti(_)
+            | Terminal::MultiA(_)
+            | Terminal::SortedMultiA(_) => {


Saw SortedMultiA was also missing from this.

trevarj · 2026-04-09T17:55:32Z

        match (&self.node, n) {
            (Terminal::PkK(key), 0) | (Terminal::PkH(key), 0) => Some(key.clone()),
            (Terminal::Multi(thresh), _) => thresh.data().get(n).cloned(),
+            (Terminal::SortedMulti(thresh), _) => thresh.data().get(n).cloned(),


I missed this on the first iteration since type-driven development failed me with the _ => fallback.

@apoelstra It resolves the HACK that was mentioned in this comment.

trevarj · 2026-04-09T17:59:03Z

+                Terminal::Multi(ref thresh) | Terminal::SortedMulti(ref thresh)
+                    if !thresh.iter().all(&mut pred) =>
+                {


After rebasing one of the tests from 1062144 caught that I missed this! Thanks @febyeji

febyeji

I left a couple of comments. One on missing SortedMulti arms in context.rs and one stale comment nit.

trevarj · 2026-04-10T12:27:11Z

I left a couple of comments. One on missing SortedMulti arms in context.rs and one stale comment nit.

Thanks again, @febyeji ! Can't believe I missed so many match arms 😞

apoelstra · 2026-04-10T16:23:11Z

+                ShInner::Wsh(ref wsh) => {
+                    let ms = wsh.as_inner();
+                    if let Terminal::SortedMulti(ref thresh) = ms.node {
+                        PkIter::from_sortedmulti(thresh.data())


In a65deea:

Here, and below, you can just delete the from_sortedmulti method and get rid of this branch. It is now covered by the from_miniscript_* methods.

apoelstra · 2026-04-10T16:23:59Z

-                },
+                ShInner::Wsh(ref wsh) => {
+                    if let Terminal::SortedMulti(..) = wsh.as_inner().node {
+                        DescriptorType::ShWshSortedMulti


In a65deea:

As a followup let's maybe delete this from DescriptorType since it's no longer a distinct kind of descriptor. But let's not do it here since it'd be an API-breaking change and I'm not certain that we want to do it.

apoelstra · 2026-04-10T16:42:24Z

Done reviewing a65deea. All good except the iterpk thing. Frustrating how many matches there are in this crate that have _ fields. In general it seems super hard to find them all.

- Utilize the Miniscript parsing to handle sortedmulti as a Terminal. - Deleted sortedmulti.rs (SortedMultiVec) - Refactor Wsh to only wrap a Miniscript now that SortedMultiVec isn't used. - Refactor ShInner to remove SortedMulti variant and only use the Ms variant for sortedmulti scripts

apoelstra · 2026-04-11T14:46:11Z

Nice :) great to see such a red diff.

apoelstra

ACK 01d25c1; successfully ran local tests

apoelstra · 2026-04-13T14:16:38Z

                }
            }
-            Terminal::Multi(ref thresh) => {
+            Terminal::Multi(ref thresh) | Terminal::SortedMulti(ref thresh) => {


Oops, I missed this -- in this case we need to change the logic. In SortedMultiA we have special logic to sort keys, but in SortedMulti we don't.

We should also add a test case for this.

I will fix this in #895 -- though if you have an earlier fix I'd appreciate since that PR is likely to be in limbo for at least several more weeks.

Yes, big oversight on my part...I will fix that and a test. I guess we test it in the bitcoind tests but not using our own satisfier.

3f20a69 fix: add sorting to satisfy_helper for sortedmulti (Trevor Arjeski) Pull request description: This logic was done in sortedmulti_a but accidentally overlooked when refactoring sortedmulti. Wrote a test that proves two differently ordered sortedmulti descriptors produce the same witness. Found by Andrew in #915 (comment) ACKs for top commit: apoelstra: ACK 3f20a69; successfully ran local tests Tree-SHA512: f81642afeb8415aec564260d352176cf479f316fae1e07f2b9d455449d4749224c0bb05b51f7836718d43cdfe59387062596d4282c9b208bb1b2ae651e19da79

trevarj force-pushed the refactor-sortedmulti branch from 2c77295 to 9b0b1c0 Compare April 4, 2026 13:14

trevarj force-pushed the refactor-sortedmulti branch from 9b0b1c0 to 816620a Compare April 4, 2026 14:54

trevarj commented Apr 4, 2026

View reviewed changes

trevarj force-pushed the refactor-sortedmulti branch from 816620a to f9d0bdd Compare April 9, 2026 17:02

trevarj force-pushed the refactor-sortedmulti branch 3 times, most recently from b4d73ec to 366974f Compare April 9, 2026 17:32

trevarj commented Apr 9, 2026

View reviewed changes

trevarj force-pushed the refactor-sortedmulti branch from 366974f to 941205b Compare April 9, 2026 18:01

febyeji suggested changes Apr 10, 2026

View reviewed changes

Comment thread src/miniscript/context.rs Outdated

Comment thread src/miniscript/context.rs Outdated

Comment thread src/miniscript/context.rs Outdated

Comment thread src/miniscript/context.rs Outdated

Comment thread src/descriptor/sh.rs Outdated

trevarj force-pushed the refactor-sortedmulti branch from 941205b to a65deea Compare April 10, 2026 12:26

apoelstra reviewed Apr 10, 2026

View reviewed changes

trevarj mentioned this pull request Apr 10, 2026

Delete DescriptorType::ShWshSortedMulti #919

Closed

trevarj force-pushed the refactor-sortedmulti branch from a65deea to 01d25c1 Compare April 10, 2026 17:54

trevarj requested review from apoelstra and febyeji April 10, 2026 17:55

apoelstra approved these changes Apr 11, 2026

View reviewed changes

apoelstra merged commit ddc42c7 into rust-bitcoin:master Apr 12, 2026
26 checks passed

apoelstra reviewed Apr 13, 2026

View reviewed changes

trevarj mentioned this pull request Apr 13, 2026

fix: add sorting to satisfy_helper for sortedmulti #921

Merged

apoelstra mentioned this pull request Apr 13, 2026

miniscript: rewrite satisfier to be non-recursive #895

Open

Conversation

trevarj commented Apr 4, 2026

Uh oh!

apoelstra commented Apr 4, 2026

Uh oh!

trevarj commented Apr 4, 2026

Uh oh!

trevarj Apr 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!

Choose a reason for hiding this comment

Uh oh!

trevarj Apr 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!

apoelstra Apr 9, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevarj commented Apr 5, 2026

Uh oh!

apoelstra commented Apr 9, 2026

Uh oh!

apoelstra commented Apr 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

febyeji left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trevarj commented Apr 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Apr 10, 2026

Uh oh!

apoelstra commented Apr 11, 2026

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

trevarj Apr 4, 2026 •

edited

Loading

trevarj Apr 4, 2026 •

edited

Loading

apoelstra Apr 9, 2026 •

edited

Loading