Remove recursion in is_valid
#601
Closed
+445
−387
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
The `concrete::PolicyArc` type is identical to `Policy` except that it uses `Arc` to wrap the nested `Policy` structs. In preparation for implementing `TreeLike` and removing recursive functions that process the `Policy` type remove the `PolicyArc` and add `Arc` wrappers around the nested `Policy` fields. Note, this patch does a bunch on cloning in the recursive functions. This will dissapear when they are re-written to use tree iteration. There are more optimizaitons available by taking `Arc<Policy<Pk>>` as a parameter to functions in the `compiler` module, left for later. The unit tests are bit klunky, much mapping of iterators to add `Arc`, I think this will come out in the wash as we keep working.
In preparation for removing recursion in the `policy::concrete` module implement `TreeLike` for `policy::Concrete` (reference, and `Arc`).
We are about to start using the `TreeLike` trait everywhere, hence it will become an easily recognizable pattern. The current single case has a code comment that is noisy if duplicated many times.
In preparation for modifying the `translate_unsatisfiable_pk` function add a basic unit test.
We just implemented `TreeLike` for `concrete::Policy`, use it to implement `translate_unsatisfiable_pk`, removing recursive calls from the function.
In preparation for modifying the `keys` function add a basic unit test.
Remove recursive calls and use `TreeLike`'s post order iterator to get all the keys from a `concrete::Policy`.
In preparation for modifying the `num_tap_leaves` function add a basic unit test.
Remove recursive calls and use `TreeLike`'s post order iterator to get the total number of tap leaves for a `concrete::Policy`.
In preparation for modifying the `for_each_key` add an additional unit test that checks we correctly handle a failing predicate. While we are at it rename the existing test.
Remove recursive calls and use `TreeLike`'s post order iterator to implement `for_each_key` for the `concrete::Policy`. No additional unit tests required, this code path is already tested.
In preparation for modifying the `translate_pk` function add a basic unit test.
Remove recursive calls and use `TreeLike`'s post order iterator to implement `translate_pk` for the `concrete::Policy`. Note, no additional unit tests added for this code path, this is a familiar code pattern now.
Add a unit test that attempts to parse a `concrete::Policy` with both absolute height and time locks. Done in preparation for patching `check_timelocks_helper`.
Remove recursive calls and use `TreeLike`'s post order iterator to implement `check_timelocks_helper` for the `concrete::Policy`. Note, no additional unit tests added for this code path, this is a familiar code pattern now.
Rename the helper function and local variable at its call site to better describe the functionality. Refactor only, no logic changes.
In preparation for modifying the `is_valid` function add a bunch of basic unit tests.
Note that currently the `in_valid` function does a bunch of checks on or/and/thresh that are also done in `from_tree_prob`. Create a private function `check_timelocks_within_rang` that checks that older/after locks are within valid ranges (above 0 and below 2^31). Note this patch removes a bunch of logic but as can be seen by the passing unit tests this code is still logically correct.
|
Yes, you can directly construct these types because they are public enums. |
|
Superseded by #606 |
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.
Just the last 2 patches.
Draft because I can't work out why we do validity checks when parsing a policy string and then also have the
Concrete::is_validfunction that enforces,OrandAndbeing exactly length 2. Is there some code path that allows creation of an invalid policy that we will then catch withis_valid?