miniscript: change tap_leaf_internal to return an Option in the satisfier#941
Merged
Merged
Conversation
Member
Author
|
On 1f9e818 successfully ran local tests |
tcharding
reviewed
May 20, 2026
Comment on lines
+106
to
+108
| pub(crate) trait DowncastMiniscript<Pk: MiniscriptKey> { | ||
| fn downcast<Ctx: ScriptContext>(&self) -> Option<&Miniscript<Pk, Ctx>>; | ||
| } |
Member
There was a problem hiding this comment.
Any reason not to make this a method on Miniscript (in the impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> block)?
This builds for me. NarrowedCtx came from an AI suggestion and is allegedly nomenclature from Typescript.
#[allow(unsafe_code)]
pub(super) fn downcast<NarrowedCtx: ScriptContext>(&self) -> Option<&Miniscript<Pk, NarrowedCtx>> {
use core::any::TypeId;
if TypeId::of::<Ctx>() == TypeId::of::<NarrowedCtx>() {
// SAFETY: this pointer cast is a no-op, as guaranteed by the if guard
// In Rust 1.76 we can use core::ptr::from_ref in place of this cast.
Some(unsafe { &*(self as *const Self).cast() }) // cast needed til Rust 1.76
} else {
None
}
}
Member
Author
There was a problem hiding this comment.
Yeah, I like this. Let me change it.
tcharding
approved these changes
May 20, 2026
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.
apoelstra
force-pushed
the
2026-05/tapleaf-option
branch
from
1f9e818 to
6fd1cc3
Compare
Member
Author
|
ACK 6fd1cc3; successfully ran local tests |
tcharding
approved these changes
May 20, 2026
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
2 participants
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.