Add the planning module (updated PR) by danielabrozzoni · Pull Request #592 · rust-bitcoin/rust-miniscript

danielabrozzoni · 2023-08-18T11:29:08Z

This PR builds on top of #481, fixing all the review comments.

I didn't squash my last commits on purpose to make review easier, I can squash them before merging if preferred.

tcharding

Looks good!

Code reveiw only, I'm not super knowledgeable on this code base.

ACK 4400f85

src/plan.rs

tcharding · 2023-09-07T20:19:32Z

Looks like this is waiting for review please @sanket1729 and/or @apoelstra

danielabrozzoni · 2023-09-12T14:29:45Z

d097867 - rebased and fixed review comments. CI is failing due to (I think) unrelated reasons, it's broken on master as well.

tcharding · 2023-09-12T14:46:38Z

#597 should fix CI

danielabrozzoni · 2023-09-12T18:53:15Z

Nice, rebased!

apoelstra · 2023-09-14T13:56:50Z

Can you run through each commit and make sure that cargo test passes?

danielabrozzoni · 2023-09-15T15:03:27Z

Can you run through each commit and make sure that cargo test passes?

Should be cleaner now, sorry!

CI is failing, but #598 should fix it.

The Satisfaction struct now contains `relative_timelock` and `absolute_timelock`, which represent the needed timelocks for that particular spending path. This is useful for the plan module. Co-authored-by: Daniela Brozzoni <danielabrozzoni@protonmail.com>

Add a `plan` module that contains utilities to calculate the cheapest spending path given an AssetProvider (that could keys, preimages, or timelocks). Adds a `get_plan` method on the various descriptor types. Co-authored-by: Daniela Brozzoni <danielabrozzoni@protonmail.com>

Co-authored-by: Alekos Filini <alekos.filini@gmail.com>

danielabrozzoni · 2023-09-18T08:22:38Z

a358076 - rebased on master

apoelstra · 2023-09-21T21:06:53Z

src/descriptor/mod.rs

+            Descriptor::Wsh(ref wsh) => wsh.plan_satisfaction(provider),
+            Descriptor::Sh(ref sh) => sh.plan_satisfaction(provider),
+            Descriptor::Tr(ref tr) => tr.plan_satisfaction(provider),
+        };


In d29c298:

In a followup PR, these are easy candidates to un-recursify (using iter.rs from #567 which was created long after this PR but merged long before).

For more context, what he means is what I'm doing in #594. The iter module implements a pretty hard core data structure, if you don't have the time or inclination to work it out right now just holla at me and I'll hack it up for you.

apoelstra · 2023-09-21T21:12:43Z

src/miniscript/satisfy.rs

+    ///
+    /// NOTE: If a descriptor mixes time-based and height-based timelocks, the implementation of
+    /// this method MUST only allow timelocks of either unit, but not both. Allowing both could cause
+    /// miniscript to construct an invalid witness.


In d29c298:

nit: I would suggest

NOTE: this method must ONLY allow time-based or height-based relative timelocks, except for the value zero which is valid as both a height or a time. Allowing both could cause miniscript to consruct an invalid witness for descriptors which mix height- and time-based timelocks. (We do not consider such descriptors to be "sane" but they can still be processed by this library, e.g. by using `parse_insane` to parse them.)

And same below (but change "relative" to "absolute").

But I'm not certain about the zero comment. Can somebody more familiar with timelocks check on that?

My main concern with the existing comment is that it doesn't make it clear that absolute and relative timelocks are considered separately; so you can have a height-based relative timelock and a time-based absolute timelock, for example.

Also, these comments are currently on Satisfier::check_older and Satisfier::check_after. Should they also be on the equivalent methods in AssetProvider?

apoelstra · 2023-09-21T21:19:54Z

src/plan.rs

+#[derive(Debug, Clone)]
+pub struct Plan {
+    /// This plan's witness template
+    pub(crate) template: Vec<Placeholder<DefiniteDescriptorKey>>,


In d29c298:

Maybe this was discussed in the other thread, but should we make this generic over Pk rather than using a DefiniteDescriptorKey?

apoelstra · 2023-09-21T21:28:04Z

a358076 looks amazing! I appreciate the detailed type-guided API with structs like TaprootCanSign.

I haven't tried to use this yet so there may be pain points but reading the code, this looks great. My comments are only nits.

apoelstra

ACK a358076

sanket1729

ACK a358076

I have quite a bit with this PR in Summer of bitcoin project with @Harshil-Jani who wrote some example programs using this API in #559. Thanks a lot for sticking with this PR for over a year :)

tcharding · 2023-09-22T02:54:13Z

Party! This one was open for a long time huh.

…ted PR) a358076cb88fe51952abf79d5072eb2f30aeadd3 test: absolute/relative timelocks in satisfaction (Daniela Brozzoni) cb9a76995821fd6fc5c4043b91f26b96bc54ed9b tests: plan capabilities (Daniela Brozzoni) d29c2984e60e41e3872ac23b8b59e8b25fea88a2 Add plan capabilities to miniscript (Alekos Filini) fc20eb074855554667bd77d9f7184fdb1f63e401 Fix test_cpp (sanket1729) 448fbd8d3a72ef0a2314697a644a6870adea712a Add full_derivation_paths on DescriptorPublicKey (Daniela Brozzoni) 7ca9ba178799c4d21d3cd389e0a9b54280815051 Add relative and absolute timelock in Satisfaction (Alekos Filini) Pull request description: This PR builds on top of #481, fixing all the review comments. I didn't squash my last commits on purpose to make review easier, I can squash them before merging if preferred. ACKs for top commit: apoelstra: ACK a358076cb88fe51952abf79d5072eb2f30aeadd3 sanket1729: ACK a358076cb88fe51952abf79d5072eb2f30aeadd3 Tree-SHA512: 32e547eedaf56d7ddb9ab8069ab394b655f46f6eae7b971d521befc800abadb785335a84c977875b050bcb202517381aba0fb9d8f2d418cd59a1f87147491d67

danielabrozzoni force-pushed the feat/planning-module branch 2 times, most recently from b0c2fd4 to 4400f85 Compare August 18, 2023 12:07

danielabrozzoni mentioned this pull request Aug 18, 2023

Add the "planning" module #481

Closed

tcharding approved these changes Aug 18, 2023

View reviewed changes

src/plan.rs Show resolved Hide resolved

src/plan.rs Outdated Show resolved Hide resolved

danielabrozzoni force-pushed the feat/planning-module branch from 4400f85 to d097867 Compare September 12, 2023 14:18

danielabrozzoni force-pushed the feat/planning-module branch from d097867 to 320c5ef Compare September 12, 2023 18:53

danielabrozzoni force-pushed the feat/planning-module branch from 320c5ef to 0bb4ba1 Compare September 15, 2023 13:42

afilini and others added 6 commits September 18, 2023 10:11

Add full_derivation_paths on DescriptorPublicKey

448fbd8

Fix test_cpp

fc20eb0

tests: plan capabilities

cb9a769

Co-authored-by: Alekos Filini <alekos.filini@gmail.com>

test: absolute/relative timelocks in satisfaction

a358076

Co-authored-by: Alekos Filini <alekos.filini@gmail.com>

danielabrozzoni force-pushed the feat/planning-module branch from 0bb4ba1 to a358076 Compare September 18, 2023 08:12

apoelstra reviewed Sep 21, 2023

View reviewed changes

apoelstra approved these changes Sep 21, 2023

View reviewed changes

sanket1729 approved these changes Sep 22, 2023

View reviewed changes

sanket1729 merged commit 780dbc6 into rust-bitcoin:master Sep 22, 2023

notmandatory mentioned this pull request Oct 2, 2023

Socratic Seminar #12 Topics NashBitDevs/nashbitdevs.org#14

Closed

This was referenced Mar 7, 2024

Feature request: witness size estimator that understands key availability #55

Closed

Figure out how I can satisfy a descriptor without actually generating signatures #453

Closed

apoelstra mentioned this pull request Jun 6, 2025

plan: remove unused AssetProvider::provider_lookup_tap_control_block_map #826

Merged

Conversation

danielabrozzoni commented Aug 18, 2023

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tcharding commented Sep 7, 2023

Uh oh!

danielabrozzoni commented Sep 12, 2023

Uh oh!

tcharding commented Sep 12, 2023

Uh oh!

danielabrozzoni commented Sep 12, 2023

Uh oh!

apoelstra commented Sep 14, 2023

Uh oh!

danielabrozzoni commented Sep 15, 2023

Uh oh!

danielabrozzoni commented Sep 18, 2023

Uh oh!

apoelstra Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

tcharding Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Sep 21, 2023

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!

tcharding commented Sep 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants