[Taproot API Project] replace TaprootSpendInfo with new miniscript-specific structure
#815
Conversation
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
We have the function `update_item_with_descriptor_helper` which does a few things: it derives a descriptor (replacing all the xpubs with actual public keys) and updates the appropriate input or output map to map the derived keys to their keysources. It treats Tr outputs differently from other kinds of outputs, because the relevant maps are different. However, in doing so, it duplicates a bunch of work in ways that are hard to follow. Essentially, the algorithm does three things: (a) derives all the keys (and the descriptor), (b) optionally checks that the resulting scriptpubkey is what we expect, and (c) updates the maps. The existing code handles (a) separately for Tr and non-Tr descriptors. In the Tr case, we derive all the keys using Descriptor::<DescriptorPublicKey>::derived_descriptor which derives all the keys and throws away the conversion. Then separately it keeps around the un-derived descriptor, iterates through the keys, and populates the `tap_key_origins` map by re-computing the derivation. In the non-Tr case, we derive all the keys using the `KeySourceLookUp` object, which does exactly the same thing as `derived_descriptor` except that it stores its work in a BTreeMap, which is directly added to the PSBT's `item.bip32_derivation` field. This commit pulls out (a) into common code; it then copies all the data out of the key map into `item.tap_key_origins` along with an empty vector of tapleaves. It then goes through all the leaves, and for each key that appears in each leaf, appends that leaf's hash to the vector of tapleaves. This is still a little ineffecient but will be much cleaner after a later commit when we improve the Taproot SpendInfo structure. The original code dates to Lloyd's 2022 PR rust-bitcoin#339 which introduces logic to populate these maps. That algorithm underwent significant refactoring in response to review comments and I suspect that the duplicated logic went unnoticed after all the refactorings.
This commit introduces a new data structure but **does not** use it. The next commit will do this. I have separated them so that this one, which introduces a bunch of algorithmic code, can be reviewed separately from the API-breaking one. When computing a `Tr` output, we need to encode all its tapleaves into Script, put these into a Merkle tree and tweak the internal key with the root of this tree. When spending from one of the branches of this output, we need the Merkle path to that output. We currently do this by using the `TaprootSpendInfo` structure from rust-bitcoin. This is not a very good fit for rust-miniscript, because it constructs a map from Tapscripts to their control blocks. This is slow and memory-wasteful to construct, and while it makes random access fairly fast, it makes sequential access pretty slow. In Miniscript we almost always want sequential access, because all of our algorithms are some form of either "try every possibility and choose the optimum" or "aggregate every possibility". It also means that if there are multiple leaves with the same script, only one copy will ever be accessible. (If they are at different depths, the low-depth one will be yielded, but if they are at the same depth it's effectively random which one will get priority.) Having multiple copies of the same script is a pointless malleability vector, but this behavior is still surprising and annoying to have to think about. To replace `bitcoin::TaprootSpendInfo` we create a new structure `TrSpendInfo`. This structure doesn't maintain any maps: it stores a full Merkleized taptree in such a way that it can efficiently yield all of the leaves' control blocks in-order. It is likely that at some point we will want to upport this, or some variant of it, into rust-bitcoin, since for typical usecases it's much faster to use and construct.
See previous commit for details about this data structure. This commit stops using the rust-bitcoin `TaprootSpendInfo` in favor of our new `TrSpendInfo` structure. This one allows us to iterate over all the leaves of the Taptree, easily accessing their leaf hashes and control blocks in order, which simplifies satisfaction logic.
Moves a bit of ugly logic out of the PSBT module into the spendinfo module so that it's available for other users. We can convert from a TrSpendInfo to a bitcoin::TapTree but we can't do the opposite conversion since TrSpendInfo expects to have a Miniscript for each leaf.
This is a bit of a pain because the old lookup-by-script behavior doesn't map cleanly onto the new iterate-through-everything behavior. But this test is definitely useful (the unit tests in the previous commit came from it.)
apoelstra
force-pushed
the
2025-03--taproot-api
branch
from
700feab to
3d12b42
Compare
|
ping @sanket1729 |
|
Reviewing, 2 commits in. Will continue with the rest tomorrow. |
sanket1729
approved these changes
Jun 15, 2025
There was a problem hiding this comment.
ACK 3d12b42
Thanks for adding the fixed test vectors, I cross checked that the same vectors pass before the changes in this PR.
Approving this, my feedback can be addressed in subsequent PRs.
|
|
||
| for (pk_pkh_derived, pk_pkh_xpk) in leaf_derived |
There was a problem hiding this comment.
I wonder why this was pk_pkh. Anycase, the new code is correct and much cleaner
| /// you need access to multiple control blocks at once, will likely need to clone and | ||
| /// store them separately. | ||
| #[inline] | ||
| pub fn control_block(&self) -> &ControlBlock { &self.control_block } |
There was a problem hiding this comment.
Why return &ControlBlock here if Item contains the contains ControlBlock. This is fine, but we might also want to give users into_control_block that consumes self?
TaprootMerkleBranch::try_from(merkle_stack)
.expect("merkle stack guaranteed to be within allowable length")I don't think there are any lifetime issues here because we allocate and copy the entire merkle branch into this structure?
There was a problem hiding this comment.
Yeah, this seems reasonable. Will do in a followup.
| let spend_info = Arc::new(data); | ||
| *self.spend_info.lock().expect("Lock poisoned") = Some(Arc::clone(&spend_info)); | ||
| spend_info | ||
| TrSpendInfo::from_tr(self) |
There was a problem hiding this comment.
I think you forgot to cache this inside the descriptor?
There was a problem hiding this comment.
Oops, yep. Will fix this in a followup. I think maybe I had an earlier iteration of this PR that didn't cache it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In Miniscript, to compute control blocks, estimate satisfaction costs, or otherwise iterate through all the leaves of a Taptree, we use the
bitcoin::TaprootSpendInfostructure to maintain a map of all leaves. This map is inappropriate for Miniscript (it may not be appropriate for anyone actually..) for a few reasons:ToPublicKeykeysWe fix all these issues by introducing a new
TrSpendInfostruct which stores the entire Merkle tree in a flat representation and can produce an iterator over all the leaves. The iterator item can be used to access the Script, the Miniscript, the leaf version, and the control block for each leaf, while theTrSpendInfostructure itself can be used to access the internal and external keys. In other words, this one structure efficiently implements APIs for everything that rust-miniscript needs.This completes the Taproot API overhaul project. After this I'll go back to fixing error types, eliminating recursive structures, or overhauling the validation parameters, whichever one seems most tractable from the current state of
master.