Provide high-level taproot abstractions #109
Conversation
We let the caller build arbitrary script trees. We then provide helpers that build the merkle proofs and control blocks based on the leaf they would like to spend. It better encapsulate the details of how to build the control block and the merkle proofs, which callers shouldn't need to worry about.
We provide helpers for spending taproot output via the key path or any script path, without dealing with low-level details such as signature version, control blocks or script execution context. It makes it easier and less error-prone to spend taproot outputs in higher level applications.
| val parity = internalPubKey.outputKey(Crypto.TaprootTweak.ScriptTweak(merkleRoot)).second | ||
| val initialBlock = byteArrayOf((TAPROOT_LEAF_TAPSCRIPT + (if (parity) 1 else 0)).toByte()) + internalPubKey.value.toByteArray() | ||
| return leaves.fold(initialBlock) { block, tree -> block + ScriptTree.hash(tree).toByteArray() } | ||
| public fun build(internalPubKey: XonlyPublicKey, scriptTree: ScriptTree, spendingScript: ScriptTree.Leaf): ByteVector { |
There was a problem hiding this comment.
We should also allow users to build a control block even if they don't have the complete script tree but just its merkle root and a a merkle proof
There was a problem hiding this comment.
Why do you think this is necessary? We only have use-cases where we know the whole script tree (especially when building a control block, which means we're spending the output), so why bother with more complexity before we actually have use-cases for it?
It's also very error-prone to ask for a merkleProof argument, because it is quite unclear for the caller what exactly should be provided, a List<ByteVector32> doesn't tell them whether it should include the leaf hash (it shouldn't), in what order it should be provided, etc. Providing a scriptTree is a much safer (hard to misuse) interface, isn't it?
There was a problem hiding this comment.
Because one of the benefits of MAST is the ability to spend from a specific branch without knowing the entire script tree but just what is necessary: the script root and a merkle proof (it is very useful to provide a high-level builder that takes a script tree and a specific leaf but as an addition, not a replacement).
There was a problem hiding this comment.
I agree that this is one of the theoretical use-case for MAST, but I'm pretty sure we won't use it in the short term, so I would have rather waited for a use-case to create better API for it. I've still added it in 67589c6, but I think it's a bad API that is too vague and too easy to misuse. Ideally users of the library should never be directly exposed to the control block, they should have helper functions that let them directly get ScriptWitness objects for their use-case. IMO the goal of the bitcoin-kmp library is to hide low-level details such as control blocks: you shouldn't need to even care about its existence when you're creating a witness.
This is a low-level helper, that is hard to correctly use, but may be useful when participants only know parts of a tapscript tree.
The merkle proofs for script trees are only used when building control blocks for taproot script path witnesses. We can thus directly return the proof encoded as a byte array, with the encoding expected for those witnesses. It makes it harder to misuse this function.
| @@ -740,7 +741,9 @@ public data class Transaction( | |||
| inputs: List<TxOut>, | |||
| sighashType: Int, | |||
| sigVersion: Int, | |||
| executionData: Script.ExecutionData = Script.ExecutionData.empty | |||
| tapleaf: ByteVector32? = null, | |||
There was a problem hiding this comment.
Why split executionData into separate fields ? As much as possible, we try to match bitcoin core's implementation choices.
There was a problem hiding this comment.
Because it's not an executionData that is needed here, since it has at least one field of executionData that it doesn't use (validationWeightLeft). I really believe that Script.ExecutionData is an internal detail that should absolutely not be exposed to users of that library (and with that change, we don't expose it anymore). It is mostly used as a mutable variable during script validation, while hashing a transaction is used in other contexts as well.
Bitcoin Core isn't a library, so it doesn't care about the API it exposes on such functions, that's why it's okay-ish to reuse an object that doesn't completely fit in the context. But here this is a function we expose, so we should be more strict about what exactly we expose and avoid exposing unnecessary internal details.
There was a problem hiding this comment.
Ok, but then ExecutionData's visibility should be reduced (probably to internal)
There was a problem hiding this comment.
Yes that would be great! This isn't in the scope of this PR at all though, but several classes inside Script (and in other places as well) should be made internal, we expose way too much low-level details that callers don't need to know about.
We provide helpers and better high-level abstractions for spending taproot outputs via either the key path or script path. This ensures that callers can't mess up which
SigVersionthey use, or forget to include the script leaf hash or merkle root depending on what they are spending.The caller only needs to remember:
That is information that they should always be able to keep around or recompute, and seems to be minimal.
NB: the two commits are somewhat independent and should be reviewed separately.