Implement `TreeLike` for `policy::Concrete` #594

tcharding · 2023-09-02T08:40:50Z

Make a start on removing recursion in the policy::concrete module.

Patch 1: RemovePolicyArc
Patch2: Implement TreeLike for concrete::Policy (does not use it)
Patch 3: Trivial code comment deletion
Subsequent patches are pairs
- Add a basic unit test for the function we are about to patch
- Remove recursion from a function (the one we just tested)
Final patch is a refactoring

There is still some to go but this is already a pretty heavy review burden.

tcharding · 2023-09-04T14:03:25Z

Unless I'm missing something, if this goes in, we can remove the concrete::PolicyArc type (which is now a exactly the same as concrete::Policy)

tcharding · 2023-09-04T14:04:57Z

@apoelstra hats off to your iter module, that tree is hot.

apoelstra · 2023-09-11T16:24:51Z

Are you able to split this into two commits: one that drops PolicyArc in favor of tweaking Policy and one which implements TreeLike?

74cafab looks good to me.

tcharding · 2023-09-11T17:25:07Z

Can do!

tcharding · 2023-09-22T00:46:55Z

Are you able to split this into two commits: one that drops PolicyArc in favor of tweaking Policy and one which implements TreeLike?

I did that then went a bit wild using TreeLike :)

apoelstra · 2023-09-22T17:10:06Z

src/policy/concrete.rs

-            Policy::Threshold(_k, ref subs) => {
-                subs.iter().flat_map(|sub| sub.keys()).collect::<Vec<_>>()
+        let mut keys = vec![];
+        for data in self.post_order_iter() {


In 062e400:

I think we should use the pre-order iterator here, which is more intuitively correct. (In general, the post-order iterator matches recursive logic, but I think that, since we're only counting leaf nodes, the two iterators will actually have the same effect.)

Oh my bad, I had it in my head that the post order iter was the simpler of the two, I was wrong. Defaulting to pre-order iter when we don't need references to the child nodes.

Well, post-order is definitely the most common because it allows you to simulate depth-first recursive algorithms. But conceptually I think pre-order is more natural.

apoelstra · 2023-09-22T17:11:23Z

src/policy/concrete.rs

+                Threshold(k, subs) if *k == 1 => (0..subs.len()).map(num_for_child_n).sum(),
+                _ => 1,
+            };
+            nums.push(num);


In 7e73ffe:

We don't need to maintain a vector of numbers here. We can just do a running sum and avoid some allocations and unwraps.

I'm not sure we can, don't we need to sum the results of nested Ors? I don't think we can do that without recursion or keeping the nums vec. Am I correct in thinking a tree like this should return 4

or / \ or or / \ /\ /\ /\ k k k k k k k

Yep, you're right, I looked at the algorithm too quickly.

No sweat, it made me double check it which was not a bad thing :)

apoelstra · 2023-09-22T17:12:01Z

src/policy/concrete.rs

+            if let Policy::Key(ref pk) = data.node {
+                if !pred(pk) {
+                    return false;
+                }


In 37f3796:

I also think this one should use pre-order iter; I also think it makes no difference here.

tcharding · 2023-09-23T05:40:21Z

Changes in force push:

Used pre_order_iter where appropriate
Got a bit more creative with combinators, if you do not have to look up exactly what all an filter_map do then you're a boss, I still have to look them up.

apoelstra

ACK f145ac0

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.

tcharding · 2023-09-25T22:15:37Z

Rebase and run formatter on each commit.

tcharding · 2023-09-26T01:51:22Z

src/policy/concrete.rs

    /// A list of sub-policies, one of which must be satisfied, along with
    /// relative probabilities for each one.
-    Or(Vec<(usize, Policy<Pk>)>),
+    Or(Vec<(usize, Arc<Policy<Pk>>)>),


/me gently banging head on table crying, "please god no, don't let this be a vector when it is only ever exactly 2 policies"

@apoelstra or @sanket1729 are Or and And policies only ever comprised of exactly 2 sub-policies?

I think we might have such a restriction right now (you'll find some panics enforcing it) but we intend to lift this restriction eventually.

The main issue is the computational complexity of compiling n-ary ors.

but we intend to lift this restriction eventually.

By we do you mean miniscript in general or rust-miniscript? From the miniscript docs it looks like its limited to 2 also but I couldn't tell from the syntax if that was guaranteed. Is it ambiguous for the same reason?

Miniscript in general.

Ah ok, so leave these as vecs then. Perhaps I'll add some comments. I literally nearly fell apart yesterday, after 3 days of mental modelling of vectors and tree iteration to find this "2 limit". I'm better today :)

Glad to hear it :). Sure, some comments would be appreciated.

// Currently these vectors are limited to two elements. Eventually we would like to extend these // to be n-ary, but first we need to decide on a game plan for how to efficiently compile n-ary // disjunctions

or something.

There are no such length restrictions on the Policy::Threshold so when k == 1 we essentially have unbounded Or and when k == v.len() we have unbounded And - should we be restricting the length of the vector in these two cases also?

FTR I'm playing around with creating a Thresh<T> type to maintain the invariants v.len() > 0, k != 0, k <= v.len()`.

apoelstra

ACK 1efc7a3

sanket1729

ACK 1efc7a3

sanket1729 · 2023-09-27T08:17:50Z

src/iter/mod.rs

@@ -68,3 +68,29 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> TreeLike for Arc<Miniscript<Pk, Ctx>
        }
    }
 }
+
+impl<'a, Pk: MiniscriptKey> TreeLike for &'a policy::Concrete<Pk> {


This is really not a concern for this PR, but I think policy stuff should belong in policy module. Not here. See #603 for more.

tcharding changed the title ~~Implement TreeLike for policy::Concrete~~ Implement TreeLike for policy::Concrete Sep 2, 2023

tcharding force-pushed the 09-02-policy-for-each-key branch from 01a31db to 0c7575b Compare September 4, 2023 14:01

tcharding marked this pull request as ready for review September 4, 2023 14:02

tcharding mentioned this pull request Sep 4, 2023

Remove concrete::PolicyArc #595

Closed

tcharding marked this pull request as draft September 4, 2023 19:07

tcharding force-pushed the 09-02-policy-for-each-key branch from 0c7575b to 74cafab Compare September 5, 2023 10:48

tcharding force-pushed the 09-02-policy-for-each-key branch from 74cafab to b0151f3 Compare September 21, 2023 23:36

tcharding marked this pull request as ready for review September 21, 2023 23:40

tcharding mentioned this pull request Sep 21, 2023

Add the planning module (updated PR) #592

Merged

apoelstra reviewed Sep 22, 2023

View reviewed changes

tcharding force-pushed the 09-02-policy-for-each-key branch from b0151f3 to f145ac0 Compare September 23, 2023 05:39

apoelstra previously approved these changes Sep 23, 2023

View reviewed changes

tcharding added 10 commits September 26, 2023 08:12

Implement TreeLike for policy::Concrete

277b587

In preparation for removing recursion in the `policy::concrete` module implement `TreeLike` for `policy::Concrete` (reference, and `Arc`).

Remove code comment from soon-to-be duplicated pattern

578dc69

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.

Add basic unit test for translate_unsatisfiable_pk

4d22908

In preparation for modifying the `translate_unsatisfiable_pk` function add a basic unit test.

Use TreeLike to implement translate_unsatisfiable_pk

2b11f64

We just implemented `TreeLike` for `concrete::Policy`, use it to implement `translate_unsatisfiable_pk`, removing recursive calls from the function.

Add basic unit test for keys

984576c

In preparation for modifying the `keys` function add a basic unit test.

Use TreeLike to implement keys

b7f8939

Remove recursive calls and use `TreeLike`'s post order iterator to get all the keys from a `concrete::Policy`.

Add basic unit test for num_tap_leaves

3fc604a

In preparation for modifying the `num_tap_leaves` function add a basic unit test.

Use TreeLike to implement num_tap_leaves

9280db2

Remove recursive calls and use `TreeLike`'s post order iterator to get the total number of tap leaves for a `concrete::Policy`.

Add unit test coverage for_each_key

b3c0955

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.

tcharding added 6 commits September 26, 2023 08:13

Use TreeLike to implement for_each_key

c81389f

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.

Add basic unit test for translate_pk

3282193

In preparation for modifying the `translate_pk` function add a basic unit test.

Use TreeLike to implement translate_pk

64488d6

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 unit test for check_timelocks API

1cbc018

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`.

Use TreeLike to implement check_timelocks_helper

4749931

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.

Refactor check_timelocks helper

1efc7a3

Rename the helper function and local variable at its call site to better describe the functionality. Refactor only, no logic changes.

tcharding dismissed apoelstra’s stale review via 1efc7a3 September 25, 2023 22:15

tcharding force-pushed the 09-02-policy-for-each-key branch from f145ac0 to 1efc7a3 Compare September 25, 2023 22:15

tcharding commented Sep 26, 2023

View reviewed changes

apoelstra approved these changes Sep 26, 2023

View reviewed changes

tcharding mentioned this pull request Sep 26, 2023

Document And/Or vectors contain 2 elements #602

Merged

sanket1729 approved these changes Sep 27, 2023

View reviewed changes

sanket1729 merged commit c42b1cb into rust-bitcoin:master Sep 27, 2023
16 checks passed

tcharding deleted the 09-02-policy-for-each-key branch September 28, 2023 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `TreeLike` for `policy::Concrete` #594

Implement `TreeLike` for `policy::Concrete` #594

tcharding commented Sep 2, 2023 •

edited

Loading

tcharding commented Sep 4, 2023

tcharding commented Sep 4, 2023

apoelstra commented Sep 11, 2023

tcharding commented Sep 11, 2023

tcharding commented Sep 22, 2023

apoelstra Sep 22, 2023

tcharding Sep 23, 2023

apoelstra Sep 23, 2023

apoelstra Sep 22, 2023

tcharding Sep 23, 2023

apoelstra Sep 23, 2023

tcharding Sep 24, 2023

apoelstra Sep 22, 2023

tcharding Sep 23, 2023

tcharding commented Sep 23, 2023 •

edited

Loading

apoelstra left a comment

tcharding commented Sep 25, 2023

tcharding Sep 26, 2023

apoelstra Sep 26, 2023

tcharding Sep 26, 2023

apoelstra Sep 26, 2023

tcharding Sep 26, 2023

apoelstra Sep 26, 2023

tcharding Sep 28, 2023

tcharding Sep 28, 2023

apoelstra left a comment

sanket1729 left a comment

sanket1729 Sep 27, 2023

Implement TreeLike for policy::Concrete #594

Implement TreeLike for policy::Concrete #594

Conversation

tcharding commented Sep 2, 2023 • edited Loading

tcharding commented Sep 4, 2023

tcharding commented Sep 4, 2023

apoelstra commented Sep 11, 2023

tcharding commented Sep 11, 2023

tcharding commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Sep 23, 2023 • edited Loading

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement `TreeLike` for `policy::Concrete` #594

Implement `TreeLike` for `policy::Concrete` #594

tcharding commented Sep 2, 2023 •

edited

Loading

tcharding commented Sep 23, 2023 •

edited

Loading