Remove more recursion from concrete::policy
#606
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
Currently the `is_valid` function recursively calls itself, we can use the `pre_order_iter` to loop over the policy nodes instead and remove the recursion. Note that this whole `is_valid` function is pretty inefficient because we have already iterated the nodes twice already (in `check_timelocks` and `check_duplicate_keys`).
As we have done for other `Policy` functions remove the recursive calls in `is_safe_nonmalleable` and use the `post_order_iter` to process each node accumulating the required results in a vector during iteration.
Currently there are a bunch of places where when matching on the `Policy` enum variants are matched in different order to the enum. During recent work this was maintained to make diffs easier to review however since we just modified a lot of this file lets clean it up while we are here. Move the match arms around so that they are ordered in the same order as the variants are defined in the `Policy` enum. Refactor only, no logic changes.
|
Looks great! Interesting observation that we're iterating thrice in I think this is fine, because improving this would result in much uglier code... but we should keep this in the back of our minds, and if it's a common pattern maybe we should try to refactor it out somehow. |
apoelstra
approved these changes
Oct 2, 2023
|
Yeah this is where my question the other day about why we call |
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.
Remove the last of the recursion in functions that process policies. Note there is still recursion in the
from_tree_probfunction that creates the policies.