The PR branch HEAD was ec72f35 at the time of this review club meeting.
Notes
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`)?
<sipa> Actually something I wonder about, related to this, which I don't remember despite writing it: if you have a wsh(pk(...)) descriptor, how does it get parsed? Miniscript, or pk?
<stickies-v> yes exactly! if the "func_name" and a parenthesis wrapper is found, they get removed from expr and Func returns true. if it is not found, Func returns false and expr remains unchanged
<stickies-v> __gotcha: I know it's not OG but personally I rely quite a bit on vscode's intellisense, it allows you to jump through the code very quickly once you know the hotkeys
<stickies-v> __gotcha: in other parts of the code there are quite a few functions called "Extract..." or "Parse..." that don't return whatever is parsed, but rather use an out argument
<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> `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`?
<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> I've got a couple of those lined up in my review too, FYI. Note that sometimes "bad" names are chosen for consistency, which is also important. Trade-offs...
<larryruane> I think that an expression of type std::optional can be coerced to a boolean, and it will be true if the expression yields a value, or else false if std::nullopt
<otech> __gotcha maybe bad etiquette if you do not include suggestions for alternative names and leave it up to the author to fix if they like. I use `NIT` for "nit-picking" for that kind of thing
<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 ;)
<stickies-v> yep, that is a nice feature. The abstraction and templating does make it a bit harder to wrap your head around exactly how it works, but it is nice how flexibly it can be used
<theStack> first a "state" is computed from root down the leaves, then a "result" is computed up back from the leaves to the root... for that one needs to pass a down- and an up-function
<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?
<sipa> Imagine in @[stickies-v]'s example that the 21 blocks have passed, so B can sign alone, but instead the path that involves a signature from both A and B is used.
<theStack> sipa: not sure if i understand your question. did you mean "if the _larger_ one involved no signatures?" if the smaller one involved no signatures, that's what i would expect
<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
<stickies-v> if the second branch is satisfied, a third party can malleate the satisfaction by removing the signature for A, since having the signature for B is enough
<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.