Remove recursion in `semantic` module #612

tcharding · 2023-10-11T03:29:43Z

Remove most of the recursion in the semantic module. Does not do normalized and associated functions (ones that either call it or take in a normalized policy).

Includes 4 trivial preparatory clean up patches at the front.

tcharding · 2023-10-11T03:58:56Z

Needs #613

tcharding · 2023-10-11T23:51:11Z

Ooooo, the first two cleanup patches should not technically be part of this PR because I never ended up being able to do normalized - I'm attacking it in a separate PR, building on this one - please excuse me for leaving them in this PR.

apoelstra · 2023-10-12T12:41:28Z

src/policy/semantic.rs

@@ -415,9 +415,11 @@ impl<Pk: MiniscriptKey> Policy<Pk> {

                let n = subs.len() - unsatisfied_count - trivial_count; // remove all true/false
                let m = k.checked_sub(trivial_count).map_or(0, |x| x); // satisfy all trivial
-                                                                       // m == n denotes `and` and m == 1 denotes `or`
+
+                // m == n denotes `and` and m == 1 denotes `or`
                let is_and = m == n;
                let is_or = m == 1;


In 939695d:

We can just delete this comment I think. I can't imagine what value it provides beyond the code itself.

apoelstra · 2023-10-12T12:42:35Z

src/policy/semantic.rs

@@ -380,7 +380,7 @@ impl_from_tree!(
                // thresh(1) and thresh(n) are disallowed in semantic policies
                if thresh <= 1 || thresh >= (nsubs as u32 - 1) {
                    return Err(errstr(
-                        "Semantic Policy thresh cannot have k = 1 or k =n, use `and`/`or` instead",
+                        "Semantic Policy thresh cannot have k=1 or k==n, use `and`/`or` instead",


In 939695d:

I think single equality was fine, but I don't feel strongly about it. But you need to the same = operator in both cases :).

lol, its hard to get good help. Just added the space into the original line. I've been chided recently about doing trivial patches and slowing the dev process down, this is definitely a case of that :(

cc @Kixunil - woops.

apoelstra · 2023-10-12T12:48:05Z

src/policy/mod.rs

+                    subs.iter().map(|s| s.node.lift()).collect();
+                let semantic_subs = semantic_subs?
+                    .into_iter()
+                    .map(|sub| Arc::new(sub))


In 9de17fc:

This map should just be map(Arc::new). (Though I assume you're going to remove this in a later commit, since otherwise clippy would've complained about it.) Same in many other cases.

We don't run clippy in CI :(

I fixed them all, I think.

apoelstra · 2023-10-13T17:47:04Z

Looks like we need a rebase after #616. But e85b046 looks good.

tcharding · 2023-10-13T21:13:49Z

Rebased and removed the _translate_pk helper as we did in concrete.

apoelstra · 2023-10-14T12:40:27Z

cbbc582 onward does not compile after the rebase.

tcharding · 2023-10-16T20:41:43Z

cargo test --workspace --all-features --all-targets runs cleanly on all those commits locally for me. What invocation is failing for you?

apoelstra · 2023-10-17T14:00:22Z

@tcharding just cargo test.

camus 2023-10-17 13:58:39  ~/code/rust-bitcoin/miniscript/pr-review   ➦ cbbc5825  ±  git reset --hard cbbc582
HEAD is now at cbbc5825 Add Arc to the semantic::Policy::Thresh vector
 camus 2023-10-17 13:58:44  ~/code/rust-bitcoin/miniscript/pr-review   ➦ cbbc5825  ±  cargo test
   Compiling miniscript v10.0.0 (/store/home/apoelstra/code/rust-bitcoin/miniscript/pr-review)
error[E0599]: no method named `_translate_pk` found for reference `&Arc<semantic::Policy<Pk>>` in the current scope
   --> src/policy/semantic.rs:147:47
    |
147 |                     subs.iter().map(|sub| sub._translate_pk(t)).collect();
    |                                               ^^^^^^^^^^^^^ help: there is a method with a similar name: `translate_pk`

For more information about this error, try `rustc --explain E0599`.
error: could not compile `miniscript` (lib) due to previous error
warning: build failed, waiting for other jobs to finish...
error[E0599]: no method named `_translate_pk` found for reference `&std::sync::Arc<semantic::Policy<Pk>>` in the current scope
   --> src/policy/semantic.rs:147:47
    |
147 |                     subs.iter().map(|sub| sub._translate_pk(t)).collect();
    |                                               ^^^^^^^^^^^^^ help: there is a method with a similar name: `translate_pk`

error: could not compile `miniscript` (lib test) due to previous error

Same error with --all-features.

tcharding · 2023-10-17T20:42:47Z

Bizarre, I rebased and ran cargo on each commit. Will re-try today.

tcharding · 2023-10-17T22:20:52Z

Had to rebase to fix merge conflicts. Corrected the removal of _translate_pk ... hopefully.

apoelstra

ACK 0421aef

sanket1729 · 2023-12-07T18:24:00Z

@tcharding Sorry for the long absence, I can review this after a rebase.

Remove unnecessary code comment, the local variables names are clear enough.

This function is fiendishly hard to understand, add a line of whitespace as a tiny baby step towards making it clearer.

Currently we are using `map_or` in a convoluted manner, we can just use `unwrap_or` to get the inner value or default to 0. Refactor only, no logic changes.

We have a little typo in an error string, fix it up.

Use standard from for import statements. (Note `Arc` is already in scope in `super`.)

When we implemented the `TreeLike` trait for the concrete `Policy` we put it in the `iter` module, it doesn't really live there, better to put the impl in the same place as the definition of the implementor. Move the impl of `TreeLike` for `concrete::Policy` to the `concrete` module.

In preparation for removing recursive algorithms in the `semantic` module add an `Arc` wrapper around the policies in the `Thresh` vector. Note we use the more explicit `Arc::new` instead of `into` because a lot of these should be removed when we remove recursion, later it may be necessary to check that we are not creating new references when a clone will suffice, the explicit `Arc::new` is easier to check. In tests however just use `into`.

In preparation for removing recursive algorithms in the `semantic` module implement the `TreeLike` trait to enable iteration over policy nodes. This is a direct copy of the `concrete::Policy` impl with the `And` and `Or` variants removed.

Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::for_each_key`.

Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::translate_pk`.

Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::n_terminals`.

Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in the `semantic::Policy::real_*_timelocks` functions. Done together because they are basically identical.

Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::at_age` and `semantic::Policy::at_age`. Done together because they are basically identical.

Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::n_keys`.

Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::minimum_n_keys`.

Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::sorted`.

tcharding · 2023-12-07T19:56:21Z

@sanket1729! Good to have you back. Rebased, no changes required.

apoelstra

ACK f9307c8

sanket1729

utACK f9307c8

tcharding force-pushed the 10-10-rm-recursion-semantic branch 3 times, most recently from 803424f to 79f9583 Compare October 11, 2023 03:58

tcharding force-pushed the 10-10-rm-recursion-semantic branch 2 times, most recently from 6ab8308 to 9507538 Compare October 11, 2023 23:24

tcharding mentioned this pull request Oct 12, 2023

Remove recursion in semantic::Policy::normalized #614

Draft

apoelstra reviewed Oct 12, 2023

View reviewed changes

tcharding force-pushed the 10-10-rm-recursion-semantic branch 3 times, most recently from ae6975e to e85b046 Compare October 12, 2023 23:53

tcharding force-pushed the 10-10-rm-recursion-semantic branch 2 times, most recently from 81055a4 to 333bfb4 Compare October 13, 2023 21:13

tcharding force-pushed the 10-10-rm-recursion-semantic branch 2 times, most recently from 80b8e2c to 0421aef Compare October 17, 2023 22:19

apoelstra previously approved these changes Oct 18, 2023

View reviewed changes

tcharding added 3 commits December 8, 2023 06:47

Remove code comment

61b45a9

Remove unnecessary code comment, the local variables names are clear enough.

Add line of whitespace

10e5982

This function is fiendishly hard to understand, add a line of whitespace as a tiny baby step towards making it clearer.

Use unwrap_or

721e16b

Currently we are using `map_or` in a convoluted manner, we can just use `unwrap_or` to get the inner value or default to 0. Refactor only, no logic changes.

tcharding added 2 commits December 8, 2023 06:47

Improve spacing in error string

2fd42d6

We have a little typo in an error string, fix it up.

Fix imports in test module

161753c

Use standard from for import statements. (Note `Arc` is already in scope in `super`.)

tcharding dismissed apoelstra’s stale review via 14ec788 December 7, 2023 19:50

tcharding force-pushed the 10-10-rm-recursion-semantic branch from 0421aef to 14ec788 Compare December 7, 2023 19:50

tcharding added 11 commits December 8, 2023 06:55

Implement TreeLike for semantic::Policy

df3a85a

In preparation for removing recursive algorithms in the `semantic` module implement the `TreeLike` trait to enable iteration over policy nodes. This is a direct copy of the `concrete::Policy` impl with the `And` and `Or` variants removed.

semantic: Remove recursion in for_each_key

97ba4e9

Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::for_each_key`.

semantic: Remove recursion in translate_pk

211abad

Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::translate_pk`.

semantic: Remove recursion in n_terminals

b1ae1ff

Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::n_terminals`.

semantic: Remove recursion in n_keys

eb75602

Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::n_keys`.

semantic: Remove recursion in minimum_n_keys

09a47cb

Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::minimum_n_keys`.

semantic: Remove recursion in sorted

f9307c8

Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::sorted`.

tcharding force-pushed the 10-10-rm-recursion-semantic branch from 14ec788 to f9307c8 Compare December 7, 2023 19:56

apoelstra approved these changes Dec 8, 2023

View reviewed changes

sanket1729 approved these changes Jan 10, 2024

View reviewed changes

sanket1729 merged commit 469c113 into rust-bitcoin:master Jan 10, 2024
16 checks passed

tcharding deleted the 10-10-rm-recursion-semantic branch February 7, 2024 05:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove recursion in `semantic` module #612

Remove recursion in `semantic` module #612

tcharding commented Oct 11, 2023 •

edited

Loading

tcharding commented Oct 11, 2023

tcharding commented Oct 11, 2023

apoelstra Oct 12, 2023

apoelstra Oct 12, 2023

tcharding Oct 12, 2023 •

edited

Loading

apoelstra Oct 12, 2023

tcharding Oct 12, 2023

tcharding Oct 12, 2023

apoelstra commented Oct 13, 2023

tcharding commented Oct 13, 2023

apoelstra commented Oct 14, 2023

tcharding commented Oct 16, 2023

apoelstra commented Oct 17, 2023

tcharding commented Oct 17, 2023 •

edited

Loading

tcharding commented Oct 17, 2023

apoelstra left a comment

sanket1729 commented Dec 7, 2023 •

edited

Loading

tcharding commented Dec 7, 2023

apoelstra left a comment

sanket1729 left a comment

Remove recursion in semantic module #612

Remove recursion in semantic module #612

Conversation

tcharding commented Oct 11, 2023 • edited Loading

tcharding commented Oct 11, 2023

tcharding commented Oct 11, 2023

apoelstra Oct 12, 2023

Choose a reason for hiding this comment

apoelstra Oct 12, 2023

Choose a reason for hiding this comment

tcharding Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

apoelstra Oct 12, 2023

Choose a reason for hiding this comment

tcharding Oct 12, 2023

Choose a reason for hiding this comment

tcharding Oct 12, 2023

Choose a reason for hiding this comment

apoelstra commented Oct 13, 2023

tcharding commented Oct 13, 2023

apoelstra commented Oct 14, 2023

tcharding commented Oct 16, 2023

apoelstra commented Oct 17, 2023

tcharding commented Oct 17, 2023 • edited Loading

tcharding commented Oct 17, 2023

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 commented Dec 7, 2023 • edited Loading

tcharding commented Dec 7, 2023

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

Remove recursion in `semantic` module #612

Remove recursion in `semantic` module #612

tcharding commented Oct 11, 2023 •

edited

Loading

tcharding Oct 12, 2023 •

edited

Loading

tcharding commented Oct 17, 2023 •

edited

Loading

sanket1729 commented Dec 7, 2023 •

edited

Loading