[Taproot API Project] flatten internal representation of TapTree
#808
+219
−161
Conversation
This file contains 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
… in Tr This makes it impossible to construct a `TapTree` whose depth exceeds the limit, which means that we can assume that our maximum depth is 128 in all of our code. Also introduces a new error type for this case distinct from "maximum recursive depth exceeded" which is supposed to be about limitations of this library, or at least of Script, rather than about the Taproot limit.
This constructor will be part of the new API introduced in the following "flatten TapTree" commit. To reduce the diff of that commit, introduce it now.
We fix the return value of Tr::tap_tree to return a Option<&T> instead of an &Option<T>; we already do this in Descriptor::tap_tree, which is likely to be more commonly called ... and both methods are pretty obscure. Also remove the iter_scripts method in favor of leaves. We did this rename in the previous PR. However, that PR broke its API anyway by making it yield an opaque struct rather than a tuple. Better to just do all the breakage at once. Maaybe we should backport the rename to 12.x as a pure rename, so that people will update their code and get nicer error messages.
This replaces the internal representation of `TapTree` with one that matches the BIP371 iterator: it's a vector of depths and leaves and nothing else. It does not track its maximum height (except during construction to return an error if 128 is exceeded); it does not track "internal nodes"; it does not offer an API to iterate through its "internal nodes" except by calling Liftable::lift to get a Semantic copy of the tree. This greatly simplifies the type and many algorithms associated with it. It automatically eliminates recursion from display, lifting, dropping, and so on. As it turns out, we never use TapTrees except to iterate over the leaves, and there is no data about internal nodes that we can even compute, let alone store, so there's no reason to have nodes for them. In later commit/PR we will create a second TapTree-like type, which can be constructed from a TapTree for ToPublicKey keys, and can be used to extract Merkle paths and control blocks. But without ToPublicKey we can't compute these, so TapTree is not the type for that.
|
Ooo, it looks like I broke our satisfaction tests somehow. |
|
The satisfaction tests seem to require bitcoin 22, which is a bit of a PITA for me to obtain since it needs boost 1.75 to build. I will work on updating it to a more modern version of Core in a separate PR before moving forward on this. |
This gets our benchmarks to roughly where we were before we flattened the data structure. The `_tr_oneleaf_` benchmarks appear to be measurably faster than before but everything else is pretty-much the same. Since the old code was definitely slower -- using Arcs, making tons of tiny allocations, storing internal nodes -- we can infer that actual construction of TapTrees is just not a meaningful portion of the time taken to parse expressions, and further optimization effort should go to the expression parser.
apoelstra
force-pushed
the
2025-04--taproot-3
branch
from
fd88f16 to
b25ec35
Compare
|
Actually I'll just do it independently. Since I was able to get the integration tests running it was easy for me to find the bug -- I had introduced an off-by-one error in our maximum Taproot depth checking. It turns out that BIP 341 specifies valid depths from 0 to 128 inclusive so actually there are 129 depths that might be used. This is surprisingly inconvient for something that sipa designed in parallel with a C++ implementation, but it is what it is. Anyway I extended my |
TapTreeIter is now a wrapper around core::slice::Iter. So we can propagate several traits: ExactSizeIterator, DoubleEndedIterator and FusedIterator. Annoyingly we cannot propagate TrustedLenIterator since that's nightly-only, nor can we propagate Default which was introduced some time after 1.63.0. core::slice::Iter also implements AsRef<&[T]> which is tempting, but we don't propagate that since it feels like it reveals too much about the internals of TapTreeIter.
apoelstra
force-pushed
the
2025-04--taproot-3
branch
from
b25ec35 to
adf3164
Compare
Interesting, is this because of some deprecated RPCs? |
|
It seems to be some change in how default wallets are created (or not). |
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.
This PR completely rewrites the
TapTreemodule to store a taptree as a flat list of leaves and heights. For a non-ToPublicKeykey this is the only useful information that we ever extract from this, so storing a big recursive structure is wasteful. We also eliminate recursive code from everythingTapTreerelated, including the implicitDropimpl.In the next PR we'll introduce a new tree structure that contains control block information -- but
TapTreecan't be this structure becauseTapTreedoesn't assume that its keys can be serialized.Also breaks the
iter_scriptsiterator API in a couple small ways.