Implement `TreeLike` for `policy::Concrete` by tcharding · Pull Request #594 · rust-bitcoin/rust-miniscript

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

    }
 }
+
+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.

…icy::Concrete` 1efc7a3d3a50ea7ea775a15ba486cfb7b8702623 Refactor check_timelocks helper (Tobin C. Harding) 4749931b389a7393b1bdf66770d52db7e5eae816 Use TreeLike to implement check_timelocks_helper (Tobin C. Harding) 1cbc018bd067739962fff4f72af2892c48348c8b Add unit test for check_timelocks API (Tobin C. Harding) 64488d6f9b2e3a0ff4a4afad78eb722c17f82bca Use TreeLike to implement translate_pk (Tobin C. Harding) 3282193c74b0b4297fee9390dccafd59a2851e67 Add basic unit test for translate_pk (Tobin C. Harding) c81389f1b447cf589c84f1aa3dd1e24a549ea262 Use TreeLike to implement for_each_key (Tobin C. Harding) b3c0955eed3f905d6739c355bfdce84bdcc1c5b6 Add unit test coverage for_each_key (Tobin C. Harding) 9280db237ea500b6a196190a05409989985c5580 Use TreeLike to implement num_tap_leaves (Tobin C. Harding) 3fc604af009265c3b03d273c4426539ce2017c02 Add basic unit test for num_tap_leaves (Tobin C. Harding) b7f893911963848a2e164cf170d62e97ab5fc427 Use TreeLike to implement keys (Tobin C. Harding) 984576c3249b4b2fbfbe67d217a102ad71e68d42 Add basic unit test for keys (Tobin C. Harding) 2b11f64cd638b24117f4c918f781d9853d43e635 Use TreeLike to implement translate_unsatisfiable_pk (Tobin C. Harding) 4d22908d8b989641377997b5809372ae4c4653f2 Add basic unit test for translate_unsatisfiable_pk (Tobin C. Harding) 578dc6992798833dfdf503c11e9694fffaa81286 Remove code comment from soon-to-be duplicated pattern (Tobin C. Harding) 277b58777b2659cddcbcf464d19ebd0e2590cc20 Implement TreeLike for policy::Concrete (Tobin C. Harding) 82c0eee458dcee1e98482d2376240b4470f12e25 Remove PolicyArc (Tobin C. Harding) Pull request description: Make a start on removing recursion in the `policy::concrete` module. - Patch 1: Remove`PolicyArc` - 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. ACKs for top commit: apoelstra: ACK 1efc7a3d3a50ea7ea775a15ba486cfb7b8702623 sanket1729: ACK 1efc7a3d3a50ea7ea775a15ba486cfb7b8702623 Tree-SHA512: 529fcf60adda1ff43a95ee896dccac746870b527fec5a638e2377206220aa87369132d037a44a048ca25fc67df1b6309d53d8fdfb7134299421396538ec043d4

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

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

Conversation

tcharding commented Sep 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Sep 4, 2023

Uh oh!

tcharding commented Sep 4, 2023

Uh oh!

apoelstra commented Sep 11, 2023

Uh oh!

tcharding commented Sep 11, 2023

Uh oh!

tcharding commented Sep 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tcharding commented Sep 2, 2023 •

edited

Loading

tcharding commented Sep 23, 2023 •

edited

Loading