miniscript: change tap_leaf_internal to return an Option in the satisfier#941
Open
apoelstra wants to merge 3 commits intorust-bitcoin:masterfrom
Open
miniscript: change tap_leaf_internal to return an Option in the satisfier#941apoelstra wants to merge 3 commits intorust-bitcoin:masterfrom
tap_leaf_internal to return an Option in the satisfier#941apoelstra wants to merge 3 commits intorust-bitcoin:masterfrom
Conversation
Right now we have a method `tap_leaf_internal` which unconditionally computes the "tapleaf" of a Miniscript. If the Miniscript is a Taproot script, this method is exposed by the public function `tap_leaf`. This diff inverts `tap_leaf` and `tap_leaf_internal` -- now `tap_leaf` does the actual calculation (and is implemented on a wider set of Miniscripts, no longer restricting the key type unnecessarily) and `tap_leaf_internal` calls `tap_leaf` after downcasting. The internal method is called by Miniscript::satisfy, which passes it into the recursive functions of src/miniscript/satisfy.rs, which carefully avoid using it except when Ctx is equal to Tap. Aside from being wasteful, this logic is hard to verify the correctness of, and won't survive the removal of the Ctx parameter. This commit just sets the TapLeafHash to all zeroes in the case that it's unused. The next commit will replace it with an Option<TapLeafHash>, but I didn't want to do that in this commit because it increased the diff a lot by forcing a bunch of API changes. The goal of this commit is just to introduce the downcasting logic, which uses unsafe code and ought to be reviewed carefully.
We now return an Option<TapLeafHash> from tap_leaf_internal, and use the optionality of this to determine whether we are doing a Schnorr signature or an ECDSA signature. This lets us remove a Ctx parameter from the internal Witness::signature method, and several methods that call it. Also eliminates some logic which theoretically could lead to an ECDSA signature produced for multi_a or a Schnorr sig for multi; now we have one new `expect()` which unwraps the Option for multi_a. There are still Ctx parameters in Witness::pkh_signature and pkh_public_key, which are used for key size estimation. These will be eliminated when we introduce ValidationParams, which will do size estimation in a more flexible way.
Member
Author
|
On 1f9e818 successfully ran local tests |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Now that we have #895 and #930 we have room to change the API for helpers to the satisfaction functions.
Change the method
tap_leaf_internal, which currently computes the "tapleaf hash" for an arbitrary Miniscript, even non-Taproot ones, to detect whether this is a Taproot script and only compute the tapleaf hash for Taproot scripts.Once we are returning an
Optionfromtap_leaf_internal, we can change a bunch of satisfaction code that currently takes aCtxparamater and introspects it to determine whether we are in Taproot mode or not, and have it simply use theOptionto decide.Aside from speeding up and simplifying the code, this removes several instances of the
Ctxparameter, which we want to eventually eliminate.