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

bitcoind-tests/tests/test_desc.rs

                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

src/descriptor/mod.rs

+                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

src/descriptor/mod.rs

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


FYI: migrated from deleted sortedmulti.rs file.

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

src/descriptor/segwitv0.rs

-        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

src/descriptor/mod.rs

+                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

src/descriptor/segwitv0.rs

-        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

src/miniscript/iter.rs

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


Saw SortedMultiA was also missing from this.

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

src/miniscript/iter.rs

        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

src/miniscript/mod.rs

+                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

- 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

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

trevarj Apr 4, 2026 •

edited

Loading

trevarj Apr 4, 2026 •

edited

Loading

apoelstra Apr 9, 2026 •

edited

Loading