Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize policy compilation for N-of-N tresholds #113

Merged
merged 2 commits into from
Aug 27, 2020

Conversation

darosior
Copy link
Contributor

This adds a special case to the compilation of the thresh(k, subs) policy to
miniscript: when all sub-policies are keys and k == subs.len() this
compiles to a combination of and()s instead of a thresh().

The first commit adds some tests for the policy->miniscript compilation of different combinations of k and pubkeys under the thresh() policy.

The second commit special cases k == pubkeys.len() (when > 20) to eventually produce slightly more efficient Bitcoin Script:

<pubkey1> CHECKSIGVERIFY <pubkey2> CHECKSIGVERIFY  <pubkey3> CHECKSIGVERIFY ... <pubkeyN> CHECKSIG

Instead of

<pubkey1> CHECKSIG SWAP <pubkey2> CHECKSIG ADD SWAP  <pubkey3> CHECKSIG ADD SWAP ... <pubkeyN> CHECKSIG ADD N EQUAL

As mentioned by sipa on IRC we can also special-case k==1 to a combination of or()s, but I left a FIXME as I intend to do this in a follow-up (can also push a commit here, as you prefer).

src/policy/compiler.rs Outdated Show resolved Hide resolved
src/policy/compiler.rs Outdated Show resolved Hide resolved
src/policy/compiler.rs Outdated Show resolved Hide resolved
@sanket1729
Copy link
Member

sanket1729 commented Jul 22, 2020

The complexity of the compiler is exponential to the number of nested or's. I am a bit concerned in transforming Thresh(1, subs) into a chain of ors for higher value of k. cc @apoelstra

@darosior darosior force-pushed the optimize_nofn branch 2 times, most recently from 70954fb to d5086e3 Compare July 23, 2020 13:25
@darosior
Copy link
Contributor Author

Amended to not use inclusive range syntax..

@darosior darosior force-pushed the optimize_nofn branch 3 times, most recently from b562e31 to 6c716f4 Compare July 23, 2020 17:19
@sanket1729 sanket1729 mentioned this pull request Jul 31, 2020
src/policy/compiler.rs Outdated Show resolved Hide resolved
policy = Concrete::And(vec![sub.clone(), policy]);
}

ret = best_compilations(policy_cache, &policy, sat_prob, dissat_prob)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is probably better to compare the best_compilations returned here to the ones we computed using the threshold logic, and actually make sure that we're taking the best ones. It seems intuitively true that nested ands will beat thresh every time but I'm not sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that best_compilations makes more sense here. But the current code always produces the most optimal compilation. Consider a particular compilation with all AND's replaced as BOOLAND.
We would have

[E] [W] BOOLAND [W] BOOLAND .....

The corresponding thresh would be:
[E] [W] ADD [W] ADD [W] ADD.... k EQUAL.

Therefore, there exists at least one compilation that is better than the thresh one. In any case, this deserves a comment explaining this scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've expanded a bit more on the comment just above (was not sure how to phrase it though):

// Not a threshold, it's always more optimal to translate it to and()s as we save the
// resulting threshold check (N EQUAL) in any case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this phrasing is fine

This tests the compilation of the `tresh(k, [pubkeys])` policy for different
numbers of public keys and different values of k.

The next commit will change part of this compilation.

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
This adds a special case to the compilation of the `thresh(k, subs)` policy to
miniscript: when k == subs.len() this compiles to a combination of and()s
instead of a thresh().

This results in slightly more efficient Bitcoin Scripts. For example
with an N-of-N:
```
<pubkey1> CHECKSIGVERIFY <pubkey2> CHECKSIGVERIFY  <pubkey3> CHECKSIGVERIFY ... <pubkeyN> CHECKSIG
```
Instead of
```
<pubkey1> CHECKSIG SWAP <pubkey2> CHECKSIG ADD SWAP  <pubkey3> CHECKSIG ADD SWAP ... <pubkeyN> CHECKSIG ADD N EQUAL
```

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
@sanket1729
Copy link
Member

tAck 688b0a8 . Will merge once #118 is merged so that the current invariant fuzz tests
policy -> compile -> lift == policy -> lift is preserved.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat, lgtm. I'm not sure we want a "FIXME" comment for a future optimization we're unsure how to do, but we can address that later

@apoelstra apoelstra merged commit c2cfb12 into rust-bitcoin:master Aug 27, 2020
@darosior darosior deleted the optimize_nofn branch August 28, 2020 10:00
@darosior
Copy link
Contributor Author

Will open an issue regarding the uncertainty of fixing the FIXME, this'll end up with either me implementing it or removing the FIXME.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants