Add support for sortedmulti_a by trevarj · Pull Request #910 · rust-bitcoin/rust-miniscript

trevarj · 2026-03-22T12:35:23Z

See individual commit messages for finer details.

I think the previous attempts at adding sortedmulti_a were prior to the parser
rewrite. With the current parser it seems to me that sortedmulti_a is basically
acopy and paste of multi_a with the key sorting added. See Andrew's comment below.

The sorting is done upon encode()ing of the Miniscript.

I'm a little suspicious on if the unit and integration tests are effective for
sortedmulti_a, so I'd like to have some extra eyes/feedback on that. It might be
better to add an integration test that can match against what Core produces on a
round trip from Descriptor to Address at least.

Resolves #320

trevarj · 2026-03-22T20:29:45Z

src/miniscript/decode.rs

                        let thresh = Threshold::new(k as usize, keys).map_err(Error::Threshold)?;
-                        term.push(Miniscript::multi_a(thresh));
+                        if thresh.is_sorted() {
+                            term.push(Miniscript::sortedmulti_a(thresh));


Second guessing this...do I need to check if keys.len() == k && thresh.is_sorted() and then that will decode as a sortedmulti_a?

No, keys.len() == k implies that this is a n-of-n threshold, i.e. an AND. It's not a necessary condition to be a sortedmulti.

I think this code is fine. TBH I would kinda prefer that we never decode as a sortedmulti_a but I guess it's harmless and it will be easy to make optional later down the line once my verification-knob PRs all land.

On reflection I think we should just drop this, and never decode to a sortedmulti. Later, when we have more flexible validation parameters, we can provide an opt-in option to do this.

Thanks. My original comment was such a brainfart.

I couldn't find any Script decoding specs anywhere (is this the only library that does it?) so I wasn't sure about this one. I will remove it.

In some sense the "script decoding spec" is the Miniscript spec -- but it doesn't have sortedmulti_a in it. I guess it ought to be updated.

Script decoding was a big priority for us when we first started Miniscript in 2017/18 but it quickly dropped as a priority as we realized that Script itself lacked the information to reconstruct Miniscripts (for example, given a scriptpubkey hash Script construction, you cannot reverse the hash to get the public key, which Miniscript demands for its pk_h combinator), and that Script decoding wasn't really very useful anyway. So yeah, this might be the only library out there that does it..

src/miniscript/astelem.rs

src/miniscript/mod.rs

apoelstra · 2026-03-31T12:09:43Z

src/miniscript/mod.rs

                    .verify_threshold(|sub| sub.verify_terminal("public_key").map_err(Error::Parse))
                    .map(Terminal::MultiA)
                    .and_then(Miniscript::from_ast),
+                "sortedmulti_a" => node


In a307788:

In the PR description you say that a sortedmulti_a must be "at the top level of Taproot descriptors", which I had thought meant "at the root of Taproot trees", but here it appears that you're allowing it to appear anywhere.

Furthermore, you're making no attempt to sort the keys here...

but here it appears that you're allowing it to appear anywhere.

You are correct, @apoelstra. I believe the spec states that it cannot be at the top level (we have a check for it in multi_a_tests). I suppose it can exist anywhere in a tap tree? I will fix the commit message and PR description.

Furthermore, you're making no attempt to sort the keys here...

I toyed with putting sorting here and where a user can construct a sortedmulti_a, but then just decided to follow how sortedmulti works already and only sort upon encoding into Script.

From the BIP:

the x-only public keys are sorted lexicographically prior to the creation of the output script. This sorting is on the keys that are to be put into the output script, i.e. after all extended keys are derived.

...but this isn't how it's done in SortedMultiVec (sortedmulti).

Thoughts?

Side note: I have another branch where I'm refactoring sortedmulti in the same way as I implemented sortedmulti_a, which will allow us to get rid of SortedMultiVec.

...but this isn't how it's done in SortedMultiVec (sortedmulti).

Yes, it is. Look at SortedMultiVec::encode, which sorts the keys and then serializes itself as a normal multi.

Side note: I have another branch where I'm refactoring sortedmulti in the same way as I implemented sortedmulti_a, which will allow us to get rid of SortedMultiVec.

Refactorings are always welcome, but currently SortedMultiVec is correct and this PR is not :).

...but this isn't how it's done in SortedMultiVec (sortedmulti).

Yes, it is. Look at SortedMultiVec::encode, which sorts the keys and then serializes itself as a normal multi.

Right, this was my understanding the whole time. Sorting is done on encoding and not parsing. That's how I'm doing it here as well.

Am I missing something?

We must be talking past each other. In an earlier version of this PR, you were not sorting on encoding (or I misread the code and thought you weren't). Now you are, so I'm happy.

So I think we are good to go.

src/miniscript/satisfy.rs

src/primitives/threshold.rs

src/util.rs

apoelstra · 2026-03-31T12:27:49Z

I think the previous attempts at adding sortedmulti_a were prior to the parser rewrite. With the current parser it seems to me that sortedmulti_a is basically a copy and paste of multi_a with the key sorting added.

Well, no, there are a couple issues that are still present here:

encode no longer round-trips with decode because sometimes decode will think that the user has a sortedmulti_a when really he just happens to have sorted keys. In particular with BIP32 wildcards, this will result in different-looking descriptors depending on the index, which is surprising. This is the reason that the existing sortedmulti is not a terminal, but instead a special descriptor type that users need to deliberately construct
satisfy needs to be different and slower because of the key sorting (in practice I doubt this matters because you can't even fit enormous numbers of keys into a block)

trevarj · 2026-03-31T21:17:41Z

@apoelstra thanks for your time and review, Andrew.

I split out the commits and addressed all your feedback except sorting the keys on parsing. If you think that's better then I'd like to do it in a later PR after getting rid of SortedMultiVec and turning it into Terminal::SortedMulti, if you're on board with that.

Am I understanding correctly that Core also does sorting during encoding? If I look at the C++ for too long I may turn to stone.

apoelstra · 2026-04-02T15:49:18Z

concept ack 576765c. Let me do a more thorough review.

src/primitives/threshold.rs

apoelstra · 2026-04-02T16:10:49Z

Done reviewing 576765c. This look great! Other than the into_sorted thing.

trevarj · 2026-04-02T17:28:43Z

@apoelstra
Thanks for the quick review again!
I did fixups on each of the commits. The only changes are with the into_sorted* functions in dd053cc and then using them in the commits after it.

src/primitives/threshold.rs

Added functions to the `Threshold` type that lexicographically sort the underlying Vec of keys as written in BIP67. Functions for sorting xonly public keys are provided for usage within taproot scripts.

sortedmulti_a behaves identical to multi_a, except that the x-only pubkeys are sorted in the final Script. Like the sortedmulti implementation, the keys are unsorted until encoding into Script. Keys are also sorted the same way before satisfaction, so that signatures are in the correct order. Note that sortedmulti_a cannot currently be decoded from Script into Miniscript due to the lack of context in the validation parameters.

The example is to show that the address is the same regardless of the x-only pubkey order within the miniscript.

trevarj · 2026-04-02T17:53:42Z

Fixup!'d eba1aff to remove AsRef<[u8]> on type bounds since we only need Ord.

apoelstra · 2026-04-02T17:55:42Z

Looks great! Thanks for the quick iteration. I will run this through my local test system and we should have an ACK/merge later today.

apoelstra

ACK ffd5c9e; successfully ran local tests

trevarj force-pushed the support-sortedmulti_a branch 2 times, most recently from ff5c979 to e62cb50 Compare March 22, 2026 12:45

trevarj commented Mar 22, 2026

View reviewed changes

trevarj force-pushed the support-sortedmulti_a branch from e62cb50 to a307788 Compare March 26, 2026 08:30