The PR branch HEAD was ec72f35 at the time of this review club meeting.
This is a 2-part Review Club. See the notes of the first part for an introduction. If you weren’t able to attend, please go through the meeting logs as we won’t discuss those Miniscript basic concepts in this second session again.
In this second part, we’ll look at the Miniscript Output Descriptor implementation. We’ll focus on the last 9 commits from “miniscript: tiny doc fixups” to “qa: functional test Miniscript watchonly support”.
Output script descriptors are strings that contain all the information necessary to allow a wallet or other program to track payments made to or spent from a particular script or set of related scripts (i.e. an address or a set of related addresses such as in an HD wallet).
Descriptors combine well with Miniscript in allowing a wallet to handle tracking and signing for a larger variety of scripts. Since Bitcoin Core 23.0 descriptor wallets have become the default wallet type.
Which function is responsible for parsing the output descriptor strings? How does it determine whether the string represents a MiniscriptDescriptor, instead of any other type (including a WSHDescriptor)
Does MiniscriptDescriptor accept Miniscript policy or Miniscript or both?
Node<Key>::ContainsDuplicateKey returns a bool. What is the return type of TreeEvalMaybe<std::set<Key>>(upfn), and how does it get cast to a bool? What does Key represent, and why is it templated?
Why does ScriptMaker use a vector of CPubKey and StringMaker a vector of PubkeyProvider pointers? What’s the difference between the two?
When choosing between two available satisfactions, why should the one that involves less or no signatures be preferred? For example, consider the policy or(and(older(21), pk(B)), thresh(2, pk(A), pk(B))) which can always be spent when both A and B sign, and can be spent after 21 blocks when just B signs. After 21 blocks, both satisfactions are available, but why would the satisfaction that involves just B’s signature be preferable?
In Node<Key>::FindInsaneSub(), for a given Node& node in the tree, what do we expect Span<const Node*> subs to be? In practice, in if (sub) return sub;, what value would sub have in order to return sub? (Or: what value would it not have?)
<stickies-v> welcome everyone! We'll be continuing our review of #24148 (https://bitcoincore.reviews/24148-2). Last week we covered some of the Miniscript fundamentals, this week we'll dive deeper into the new MiniscriptDescriptor output descriptor implementation.
<stickies-v> today's session will be more code heavy than last week, but feel free to ask about general concepts too (please check if we didn't cover it last week already to avoid repetition: https://bitcoincore.reviews/24148-2 )
<stickies-v> first up: which function is responsible for parsing the output descriptor strings? How does it determine whether the string represents a `MiniscriptDescriptor`, instead of any other type (including a `WSHDescriptor`)?
<stickies-v> __gotcha: only the function wrapper is removed from expr, so essentially expr becomes the arguments. Like theStack gave as an example: Func("foo", "foo(bar,xyz)") would change expr to become "bar,xyz"
<stickies-v> `Node<Key>::ContainsDuplicateKey` returns a bool. What is the return type of `TreeEvalMaybe<std::set<Key>>(upfn)`, and how does it get cast to a `bool`? What does `Key` represent, and why is it templated?
<stickies-v> __gotcha: personally I find that quite important. It's such a waste of time to have to go read the docs when a better func/arg name would have explained it right away. Obviously it's personal preference and not everyone may agree with your suggestion, but if you have a better alternative you should definitely suggest that in a review
<stickies-v> this is actually a question for later on, but maybe now is a better time to cover it. We use `TreeEvalMaybe()` (and `TreeEval()`) quite a lot throughout this part of the codebase. Could anyone explain how that works? Or are there specific parts that are unclear and we can cover here?
<larryruane> in a way it's consistent with what std::optional is often replacing, a pointer that can be NULL (nullptr) ... lots of old code would say !p (where p is a pointer) to test if p is not valid
<OliverOff> For me, the interesting thing about `TreeEvalMaybe` is that the way it was implemented allows for a recursive walk of the three without actual recursive function calls. When you think of it, it's just parsing left-to-right and accumulating values. Not much different than when I'm writing LISP and need to count the parentheses ;)
<sipa> @larryruane Note that std::optional only has an *explicit* operator bool, so it won't get automatically converted to bool or int, only in places where only a bool makes sense (such as in an if condition).
<stickies-v> for example, consider the policy `or(and(older(21), pk(B)), thresh(2, pk(A), pk(B)))` which can always be spent when both A and B sign, and can be spent after 21 blocks when just B signs. After 21 blocks, both satisfactions are available, but why would the satisfaction that involves just B’s signature be preferable?
<stickies-v> __gotcha: witness data is not covered by signatures, so third parties (e.g. when relaying a tx) can change this at will. In most cases, this will make the tx invalid because the witness doesn't satisfy the scriptPubKey anymore, but there are cases where the witness remains valid even after it is modified
<sipa> @theStack Imagine a situation where you have the choice between two satisfaction paths. One involves no signatures, but is bigger. One involves signature but is smaller. You should *still* prefer the bigger, no signatures one. Why?
<OliverOff> sipa: that scenario, a 3rd party could drop A and force the script to go through the first path. However, the other way around is not possible. If you only announce A, the 3rd party couldn't possibly drop B.
<stickies-v> svav: we mean third-party malleability here, which is, as explained in my previous example to __gotcha, the ability for someone not participating in the tx (not holding the required keys to sign) to modify the witness of a transaction
<sipa> @theStack A third party can't add signatures but can drop them. So if the honest signers use a construction with a "redundant" signature, even if it is smaller, third parties can mutate it into the bigger no-sig variant.