Detect miniscripts that have a satisfaction with a combination of time locks and height locks #121

sanket1729 · 2020-08-06T22:18:47Z

No description provided.

apoelstra · 2020-08-27T21:15:13Z

src/miniscript/types/extra_props.rs

+    /// contains_timelock[1] = csv with times
+    /// contains_timelock[2] = cltv with heights
+    /// contains_timelock[3] = cltv with times
+    /// contains_timelock[4] = combination of any heightlocks and timelocks


Please create a structure for this instead of using a [bool; 5] :)

You can impl Default on it so you can construct it quickly

To be clear, I can't review this PR any further until this is fixed :)

@apoelstra, This meaning the logic condition? I can change that quickly

apoelstra · 2020-08-27T21:53:04Z

src/policy/mod.rs

+        // disjunction
+        assert!(ConcretePol::from_str("or(after(1000000000),after(100))").is_ok());
+        // conjunction
+        assert!(ConcretePol::from_str("and(after(1000000000),after(100))").is_err());


Why should this fail?

A conjunction of height lock and timelock?

Oh, right, I forgot that timelocks are defined implicitly by having a value >500000000.

I really don't like this, we shuold modify Miniscript to use a different syntax for them.

Any suggestions? For starters, I like the way shesek did it for minsc. https://min.sc/#time-durations

apoelstra · 2020-09-10T15:48:41Z

src/miniscript/satisfy.rs

@@ -94,7 +95,13 @@ pub struct Older(pub u32);

 impl<Pk: MiniscriptKey> Satisfier<Pk> for Older {
    fn check_older(&self, n: u32) -> bool {
-        n <= self.0
+        if !((n < HEIGHT_TIME_THRESHOLD && self.0 < HEIGHT_TIME_THRESHOLD)


It may be simpler to check either

(n < HEIGHT_TIME_THRESHOLD) == (self.0 < HEIGHT_TIME_THRESHOLD)

or just

n < HEIGHT_TIME_THRESHOLD && self.0 >= HEIGHT_TIME_THRESHOLD

My logic for these is trying to implement "as it is" written in the BIP.

Which BIP?

My suggestion is logically equivalent to what's written here, for what it's worth, it's just shorter.

https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki . I can change this too, just was a bit pedantic in trying to copy the logic conditions directly.

apoelstra · 2020-09-10T15:52:25Z

src/miniscript/satisfy.rs

@@ -104,7 +111,13 @@ pub struct After(pub u32);

 impl<Pk: MiniscriptKey> Satisfier<Pk> for After {
    fn check_after(&self, n: u32) -> bool {
-        n <= self.0
+        if !((n < HEIGHT_TIME_THRESHOLD && self.0 < HEIGHT_TIME_THRESHOLD)
+            || (n >= HEIGHT_TIME_THRESHOLD && self.0 >= HEIGHT_TIME_THRESHOLD))


apoelstra · 2020-09-10T15:53:17Z

src/policy/concrete.rs

@@ -115,6 +118,9 @@ impl fmt::Display for PolicyError {
                "Policy entailment only supports {} terminals",
                ENTAILMENT_MAX_TERMINALS
            ),
+            PolicyError::HeightTimeLockCombination => {
+                f.write_str("Cannot lift polcies that have a heightlock and timelock combination")


typo policies

apoelstra · 2020-10-01T12:34:52Z

src/policy/concrete.rs

+    // timelocks[1]: satisfaction contains csv timelock
+    // timelocks[2]: satisfaction contains cltv heightlock
+    // timelocks[3]: satisfaction contains cltv timelock
+    // timelocks[4]: satisfaction contains an invalid combination


delete this comment

apoelstra · 2020-10-01T12:41:05Z

src/miniscript/types/extra_props.rs

+    // handy function for combining `and` timelocks
+    // This can be operator overloaded in future
+    pub(crate) fn comb_and_timelocks(a: Self, b: Self) -> Self {
+        Self::combine_thresh_timelocks(2, vec![a, b].into_iter())


Can you do once(a).chain(once(b)) here rather than Vec::into_iter. (You need to use std::iter::once.) Saves the allocation.

apoelstra · 2020-10-01T12:48:25Z

src/miniscript/types/extra_props.rs

+    /// cltv with times
+    pub cltv_with_time: bool,
+    /// combination of any heightlocks and timelocks
+    pub contains_combination: bool,


Do we actually need this field? Or can we just check (csv_with_height && csv_with_time) || (cltv_with_height && cltv_with_time)?

Ah - it is there to cover the disjunction case, where you may have a heighlock branch and a timelock branch, but they are independent so it is ok.

apoelstra · 2020-10-01T12:58:30Z

src/miniscript/types/extra_props.rs

+        // 2) hieghtlocks and timelocks are distributed across two branches:
+        // and we check if any of the existing branches has correponding lock
+        // that cannot be combined
+        // If k == 1, then this is no combination possible.


I find this whole comment block confusing. I think you should replace it with

// Propagate all fields of `TimelockInfo` from each of the node's children to the node // itself (by taking the logical-or of all of them). In case `k == 1` (this is a disjunction) // this is all we need to do: the node may behave like any of its children, for purposes // of timelock accounting. // // If `k > 1` we have the additional consideration that if any two children have conflicting // timelock requirements, this represents an inaccessible spending branch. If there are // two such children we must set the `contains_combination` field.

apoelstra · 2020-10-01T12:59:52Z

src/miniscript/types/extra_props.rs

+                    || (timelock_info.csv_with_time && sub_timelock.csv_with_height)
+                    || (timelock_info.cltv_with_time && sub_timelock.cltv_with_height)
+                    || (timelock_info.cltv_with_height && sub_timelock.cltv_with_time);
+            }


You can move this block out of the loop and simplify it:

if k >= 2 { timelock_info.contains_combination |= timelock_info.csv_with_time && timelock_info.csv_with_height; timelock_info.contains_combination |= timelock_info.cltv_with_time && timelock_info.cltv_with_height; }

(or however you want to format it)

Once it is out of the loop you can replace the loop with fold, as

let mut ret = sub_timelocks.fold(TimelockInfo::default(), |acc, x| TimelockInfo { csv_with_time: acc.csv_with_time | x.csv_with_time, csv_with_height: acc.csv_with_height | x.csv_with_height, cltv_with_time: acc.cltv_with_time | x.cltv_with_time; cltv_with_height: acc.cltv_with_height | x.cltv_with_height; contains_combination: acc.contains_combination | x.contains_combination; }); if k > 1 { ret.contains_combination |= ret.csv_with_time && ret.csv_with_height; ret.contains_combination |= ret.cltv_with_time && ret.cltv_with_height; }

This is incorrect. As it will set contains_combination for the following policy too.
thresh(2,pk(),pk(),or(time,height))

Unforetunately, I think it is necessary to check k>2 inside of loop.

I think your existing code has the same issue.

Ah, no, I see -- the distinction is that only the one child has any timelocks at all. Interesting.

Ok, so I leave it up to you whether you want to bother using fold instead of a loop. But can you add a comment above the k >= 2 line saying

// If more than one branch may be taken, and some other branch has a requirement // that conflicts with this one, set `contains_combination`

or something

Changing to fold

apoelstra

ack a662105

sanket1729 force-pushed the heightlocks branch from 14a1e8b to eaa48d6 Compare August 7, 2020 03:41

apoelstra reviewed Aug 27, 2020

View reviewed changes

sanket1729 force-pushed the heightlocks branch 4 times, most recently from ca46d42 to bb4991a Compare August 28, 2020 23:44

apoelstra reviewed Sep 10, 2020

View reviewed changes

sanket1729 added 2 commits September 25, 2020 14:12

Update README: Remove stability section

aac0178

Refactor has_verify_form -> has_free_verify

3ce5ccc

sanket1729 force-pushed the heightlocks branch 3 times, most recently from 4f65119 to 1e55cad Compare September 25, 2020 19:37

apoelstra reviewed Oct 1, 2020

View reviewed changes

sanket1729 force-pushed the heightlocks branch from 1e55cad to aba41d0 Compare October 1, 2020 16:04

sanket1729 added 3 commits October 1, 2020 11:50

Added Timelock info

9098986

Update lift trait

d0a4f1a

Update signer

a662105

sanket1729 force-pushed the heightlocks branch from aba41d0 to a662105 Compare October 1, 2020 16:50

apoelstra approved these changes Oct 1, 2020

View reviewed changes

apoelstra merged commit f36e4f9 into rust-bitcoin:master Oct 1, 2020

sanket1729 mentioned this pull request Oct 1, 2020

Release 2.0.0 #143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect miniscripts that have a satisfaction with a combination of time locks and height locks #121

Detect miniscripts that have a satisfaction with a combination of time locks and height locks #121

sanket1729 commented Aug 6, 2020

apoelstra Aug 27, 2020

apoelstra Aug 27, 2020

apoelstra Sep 25, 2020

sanket1729 Sep 25, 2020

apoelstra Aug 27, 2020

sanket1729 Aug 27, 2020

apoelstra Aug 28, 2020 •

edited

Loading

sanket1729 Sep 8, 2020

apoelstra Sep 10, 2020

sanket1729 Sep 10, 2020 •

edited

Loading

apoelstra Sep 18, 2020

sanket1729 Sep 18, 2020

apoelstra Sep 10, 2020

apoelstra Sep 10, 2020

apoelstra Oct 1, 2020

apoelstra Oct 1, 2020

apoelstra Oct 1, 2020

apoelstra Oct 1, 2020

apoelstra Oct 1, 2020

apoelstra Oct 1, 2020 •

edited

Loading

apoelstra Oct 1, 2020 •

edited

Loading

sanket1729 Oct 1, 2020

sanket1729 Oct 1, 2020

apoelstra Oct 1, 2020

apoelstra Oct 1, 2020

apoelstra Oct 1, 2020

sanket1729 Oct 1, 2020

apoelstra left a comment •

edited

Loading

Detect miniscripts that have a satisfaction with a combination of time locks and height locks #121

Detect miniscripts that have a satisfaction with a combination of time locks and height locks #121

Conversation

sanket1729 commented Aug 6, 2020

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 Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 Sep 10, 2020 • edited Loading

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

Choose a reason for hiding this comment

apoelstra Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

apoelstra Oct 1, 2020 • edited Loading

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 • edited Loading

Choose a reason for hiding this comment

apoelstra Aug 28, 2020 •

edited

Loading

sanket1729 Sep 10, 2020 •

edited

Loading

apoelstra Oct 1, 2020 •

edited

Loading

apoelstra Oct 1, 2020 •

edited

Loading

apoelstra left a comment •

edited

Loading