Refactor extract_key logic in Taproot compiler
#732
Merged
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
This test takes ~28 seconds on my system, but it is doing two independent compilations in serial. If I split it into two, then they can run in parallel and the total time is about 16 seconds.
The Taproot compiler has a couple algorithms used to find an optimal internal key to extract from a policy. One is `to_tapleaf_prob_vec` which is a fundamental part of the compiler, which iterates over the disjunction-only root of the tree and returns every branch along with its probabilities. This is currently implemented using both allocations and recursion. By pulling the logic out into an iterator we can get a clearer, faster algorithm that cannot stack-overflow. This algorithm is then fed into Concrete::extract_key. The goal of this algorithm is to find the highest-probability tapbranch consisting of just a single key. This algorithm inexplicably works by: * Lifting the policy to a semantic policy, maybe returning an error * Iterating over the entire policy, extracting a list of every key. * Calling to_tapleaf_prob_vec to get a vector of tapleaves, which it then iterates over to find the key-only branches, which it collects into a BTreeMap mapping the key to probabilities. * Iterating over the extracted lists of keys, and for each key, reducing the semantic policy by that key and comparing to Trivial (logically, this is asking "is this key in a tree of disjunctions from the root"). * For each such key that it finds, looking it up in the map, potentially returning an error (actually this error path is impossible to hit). * and manually minimizing the looked up probability. With to_tapleaf_prob_vec replaced by an iterator there is a simpler and more direct algorithm: * Iterate through all the tapbranches/probability pairs, filtering for key-only branches, and maximizing by probability. This can only fail if there are no key-only branches, and this is reflected by only having one error branch.
We have a few error returns that are impossible to hit: * A sanity check on a tapleaf that just came out of the compiler (if this is hit it is a compiler bug and we want to know about it). * An error return from with_huffman_tree which can only happen if it's given an empty input (impossible) * An error if the final compilation (all tapleaves assembled into a tree) can't fit into a Descriptor::tr; but again, this is a compiler bug if we hit it. (Actually, I think that by manually constructing a policy that exceeds the maximum recursion depth you can trigger this error path, but the compiler output is not the place to flag this manual violation of invariants). The next commit will clean up the error types. These changes are in their own commit because they are potentially controversial.
This adds a couple variants to policy::compiler::Error and eliminates a couple calls to errstr. It also changes a few internal functions to return compiler errors instead of the top-level error.
sanket1729
approved these changes
Aug 30, 2024
This one was not actually impossible. 😅 See #677 for a test (that now panics) and a fix. |
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.
Right now our Taproot compiler does some complicated analysis to identify a key within a policy that may be extracted to use as an internal key. Specifically, if you look at the "disjunction tree" at the root of a policy, every leaf of this tree can be a tapbranch and any leaf which is just a pubkey-check and nothing else is a candidate to be the internal key.
However, our logic to do this is convoluted, has many unnecessary error paths, and lots of gratuitous allocations (which ironically make the code harder, not easier, to follow).
This PR smooths all that out by introducing a new iterator over the potential tapbranches of a policy (along with their probabilities). It also cleans up much of the error handling in the policy module, eliminating 3 calls to the stringly-typed
errstrfunction.