Add the planning module (updated PR) #592
Conversation
danielabrozzoni
force-pushed
the
feat/planning-module
branch
2 times, most recently
from
b0c2fd4
to
4400f85
Compare
|
Looks like this is waiting for review please @sanket1729 and/or @apoelstra |
danielabrozzoni
force-pushed
the
feat/planning-module
branch
from
4400f85
to
d097867
Compare
|
d097867 - rebased and fixed review comments. CI is failing due to (I think) unrelated reasons, it's broken on master as well. |
|
#597 should fix CI |
danielabrozzoni
force-pushed
the
feat/planning-module
branch
from
d097867
to
320c5ef
Compare
|
Nice, rebased! |
|
Can you run through each commit and make sure that |
danielabrozzoni
force-pushed
the
feat/planning-module
branch
from
320c5ef
to
0bb4ba1
Compare
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 <[email protected]>
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 <[email protected]>
Co-authored-by: Alekos Filini <[email protected]>
Co-authored-by: Alekos Filini <[email protected]>
danielabrozzoni
force-pushed
the
feat/planning-module
branch
from
0bb4ba1
to
a358076
Compare
|
a358076 - rebased on master |
| Descriptor::Wsh(ref wsh) => wsh.plan_satisfaction(provider), | ||
| Descriptor::Sh(ref sh) => sh.plan_satisfaction(provider), | ||
| Descriptor::Tr(ref tr) => tr.plan_satisfaction(provider), | ||
| }; |
There was a problem hiding this comment.
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.
| /// | ||
| /// 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also, these comments are currently on Satisfier::check_older and Satisfier::check_after. Should they also be on the equivalent methods in AssetProvider?
| #[derive(Debug, Clone)] | ||
| pub struct Plan { | ||
| /// This plan's witness template | ||
| pub(crate) template: Vec<Placeholder<DefiniteDescriptorKey>>, |
There was a problem hiding this comment.
In d29c298:
Maybe this was discussed in the other thread, but should we make this generic over Pk rather than using a DefiniteDescriptorKey?
|
a358076 looks amazing! I appreciate the detailed type-guided API with structs like 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. |
There was a problem hiding this comment.
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 :)
|
Party! This one was open for a long time huh. |
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.