Non-recursive bound checks for Miniscript fragments #150

darosior · 2020-10-08T13:06:11Z

This is based on #149 and keeps track of the maximum number of stack elements and the size used by both a satisfaction and a dissatisfaction directly in the Miniscript fragment extra data.
This avoids the recursive methods currently called each time we check for limits.

TODO:

implement it for satisfaction size as well (i'm using a tuple for the cost differing between witness and scriptSig)
replace callers to use these instead of recursive methods
fix the (likely compiler-related) error in Check for segwit standardness limits #149
cleanup the FIXMEs which are basically reminder for me to ask you

Moved from TODO to "can be done in a follow-up this one is large enough":

add new method to Miniscript to get both the dissatisfaction costs in size and stack elements

src/miniscript/types/extra_props.rs

src/descriptor/mod.rs

src/miniscript/context.rs

src/miniscript/decode.rs

src/miniscript/types/extra_props.rs

sanket1729 · 2020-10-09T15:42:42Z

src/miniscript/types/extra_props.rs

@@ -211,6 +231,8 @@ impl Property for ExtData {
            ops_count_nsat: None,
            stack_elem_count_sat: Some(1),
            stack_elem_count_dissat: Some(1),
+            max_sat_size: Some((33, 33)),
+            max_dissat_size: None,


Should be 33. Same for other hashes

I thought hashes were not dissatisfiable ? Sipa implemented this https://github.com/sipa/miniscript/blob/027b422495a8ee3ea3fa7787150389bb65d1b9de/bitcoin/script/miniscript.h#L535-L538 and iirc we did too.

There is, however, a mistake on the stack_elem_count_dissat in this case.

Made stack_elem_count_dissat None, as i really think the mistake is this way around.

I think that this is wrong in the sipa implementation. We should have that as 33 for reasons in the previous comment. @apoelstra

Okay, that is bug in the sipa implementation as he explicitly considers them being dissatifyable here:

https://github.com/sipa/miniscript/blob/027b422495a8ee3ea3fa7787150389bb65d1b9de/bitcoin/script/miniscript.h#L657 .

Heh, good catch. There is definitely no canonical dissatisfaction for an unwrapped hash.

@darosior I think we want the estimation functions to return the worst-case value that is possible under standardness rules. If the fragment is supposed not to be satisfiable (for malleability checks or other reasons) then we should use the d flag to indicate that. But if we make it imposible to even estimate these values, it will become harder to work with incomplete fragments or other malleable scripts.

@darosior I think we want the estimation functions to return the worst-case value that is possible under standardness rules.

Does this mean we want this not to be None even if the dissat is malleable ? If so we could have a dissat cost for virtually all fragments here..

But if we make it imposible to even estimate these values, it will become harder to work with incomplete fragments or other malleable scripts.

Hmm.. That's how it was before this PR though ? Just to be clear (i didn't follow the last IRC discussions very closely): do we want to not ignore malleable dissatisfaction anymore ?

Does this mean we want this not to be None even if the dissat is malleable ? If so we could have a dissat cost for virtually all fragments here..

I don't think there are any non-canonical ones that we are missing here. As you point out, it was a bug in the previous code too.

src/miniscript/types/extra_props.rs

sanket1729

Thanks for doing this :) There are some breaking API changes here, but they are worth it for sure.

sanket1729 · 2020-10-09T19:44:35Z

It looks like one of tests was broken :) . https://github.com/rust-bitcoin/rust-miniscript/pull/150/checks?check_run_id=1226708928

apoelstra · 2020-11-04T22:10:22Z

src/miniscript/types/extra_props.rs

+            } else {
+                // The thresh is dissatifiable iff all sub policies are dissatifiable
+                stack_elem_count_dissat = None;
+            }


sucks that we don't have Option::zip or Option::zip_with

apoelstra

ack 5881733

apoelstra · 2020-11-05T01:33:53Z

Actually can you add a commit which removes max_dissatisfaction_witness_elements etc from astelem.rs? These methods are no longer needed.

darosior · 2020-11-05T10:35:17Z

Added

sanket1729 · 2020-11-05T12:08:01Z

Based on the comment from @apoelstra, I will re-review the entire PR assuming we are allowing malleable Miniscripts. I think my first review assumed that we were dealing with malleable miniscripts.

sanket1729 · 2020-11-05T12:08:47Z

src/miniscript/types/extra_props.rs

@@ -217,6 +235,8 @@ impl Property for ExtData {
            ops_count_static: 4,
            ops_count_sat: Some(4),
            ops_count_nsat: None,
+            stack_elem_count_sat: Some(1),
+            stack_elem_count_dissat: None,


If we allow malleable dissat, this should be Some(1). Same thing for rest of the hashes

Note that hash fragment is

SIZE <32> EQUALVERIFY SHA256 <h> EQUAL . So any 32 bit vec will dissat it

sanket1729

Left a minor discussion comment about dissatisfying hash fragments.
If that requires more discussion, we can merge this PR and possibly have that as a small followup if it is required.

sanket1729

nit: the last commit gives a warning about unused std::cmp

apoelstra · 2020-11-05T19:40:12Z

Will need rebase on #169

This avoids recursion to check Segwit standardness limits. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

This is inherently bound to the context, and avoids for the caller to set the cost of 1 depending on wether they are using Segwitv0 or Legacy. This also makes it reeturn an Option, as previously for max_witness_elements(). Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

And some other small followups to rust-bitcoin#149 Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

…inal We don't need them anymore Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

darosior · 2020-11-06T11:53:03Z

Rebased on master, left a comment in the inner discussion regarding the dissat cost computation.

sanket1729 · 2020-11-09T16:35:55Z

@darosior , In case you missed I added a comment here. #150 (comment) . We should have hashes be dissatisfyable

darosior · 2020-11-09T17:48:01Z

We discussed it on IRC and you told me it should be part of another PR, so i discarded the patch... Did i misunderstand something ? Also, this patch iirc created some new tests failure that i didn't look into. Seemed a good idea to have it as part of another PR since it clearly is a different feature ? ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ Le lundi, novembre 9, 2020 5:36 PM, Sanket Kanjalkar <notifications@github.com> a écrit :

…

***@***.***(https://github.com/darosior) , In case you missed I added a comment here. [#150 (comment)](#150 (comment)) . We should have hashes be dissatisfyable — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](#150 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F3OIHJLO3ARJKZ4SITSPAK7XANCNFSM4SIXNRZA).

sanket1729 · 2020-11-09T19:46:12Z

@darosior , I meant we can fix the opcode stuff later. We should fix the stack size and sat cost as they a part of this PR.

<darosior> But checking right now and we don't :)
<darosior> Hmm so i should fill_ops_count_nsat as well..
<darosior> Will add a new commit rather than amending
<sanket1729_> we can keep that as separate PR. I think this is almost good to go
<darosior> Ok, will open a new PR with the commit then

apoelstra

ack eb5e04b

We can fix the dissat thing later

darosior · 2020-11-10T20:48:53Z

Sorry Sanket i never got to answer your comment, pretty busy right now...

<darosior> Hmm so i should fill_ops_count_nsat as well..
<darosior> Will add a new commit rather than amending
<sanket1729_> we can keep that as separate PR. I think this is almost good to go```

Yea, right. I misunderstood that it only concerned the ops.

sanket1729 · 2020-11-10T22:59:25Z

Sure, no worries :)

darosior commented Oct 8, 2020

View reviewed changes

src/miniscript/types/extra_props.rs Show resolved Hide resolved

darosior commented Oct 8, 2020

View reviewed changes

src/miniscript/types/extra_props.rs Outdated Show resolved Hide resolved

src/miniscript/types/extra_props.rs Outdated Show resolved Hide resolved

darosior force-pushed the avoid_recursion branch 2 times, most recently from c74fb84 to 90377d6 Compare October 8, 2020 14:21