Skip to content

Commit

Permalink
compiler: improve logic for deciding between thresholds and ands
Browse files Browse the repository at this point in the history 
In our threshold logic we have some special cases to handle multi,
multi_a, and conjunctions. However, we were not choosing between
these special cases correctly. Our logic was roughly that we would
try to use multi_a if all children were pk()s, OR try to use multi
if all children were pk() and there weren't too many, OR try to
use a conjunction if k == n.

The correct logic is: if all children are keys, and there aren't
too many, try to use multi or multi_a. ALSO, if k == n, try to
use a conjunction.

With this fix, the compiler correctly finds that conjunctions are more
efficient than CHECKMULTISIG when k == n and n < 3. When n == 3 the
two cases have equal cost, but it seems to prefer the conjunction. It
also correctly finds that when k == n, it is always more efficient to
use a conjunction than to use CHECKSIGADD.

This change necessitates changing some tests.
  • Loading branch information
@apoelstra
apoelstra committed Mar 12, 2024
1 parent bc3b8dc commit 200991d
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 43 deletions.
12 changes: 6 additions & 6 deletions examples/taproot.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ fn main() {
let desc = pol.compile_tr(Some("UNSPENDABLE_KEY".to_string())).unwrap();

let expected_desc =
Descriptor::<String>::from_str("tr(Ca,{and_v(v:pk(In),older(9)),multi_a(2,hA,S)})")
Descriptor::<String>::from_str("tr(Ca,{and_v(v:pk(In),older(9)),and_v(v:pk(hA),pk(S))})")
.unwrap();
assert_eq!(desc, expected_desc);

Expand Down Expand Up @@ -73,7 +73,7 @@ fn main() {
);
assert_eq!(
iter.next().unwrap(),
(1u8, &Miniscript::<String, Tap>::from_str("multi_a(2,hA,S)").unwrap())
(1u8, &Miniscript::<String, Tap>::from_str("and_v(v:pk(hA),pk(S))").unwrap())
);
assert_eq!(iter.next(), None);
}
Expand All @@ -97,19 +97,19 @@ fn main() {
let real_desc = desc.translate_pk(&mut t).unwrap();

// Max satisfaction weight for compilation, corresponding to the script-path spend
// `multi_a(2,PUBKEY_1,PUBKEY_2) at tap tree depth 1, having:
// `and_v(PUBKEY_1,PUBKEY_2) at tap tree depth 1, having:
//
// max_witness_size = varint(control_block_size) + control_block size +
// varint(script_size) + script_size + max_satisfaction_size
// = 1 + 65 + 1 + 70 + 132 = 269
// = 1 + 65 + 1 + 68 + 132 = 269
let max_sat_wt = real_desc.max_weight_to_satisfy().unwrap();
assert_eq!(max_sat_wt, 269);
assert_eq!(max_sat_wt, 267);

// Compute the bitcoin address and check if it matches
let network = Network::Bitcoin;
let addr = real_desc.address(network).unwrap();
let expected_addr = bitcoin::Address::from_str(
"bc1pcc8ku64slu3wu04a6g376d2s8ck9y5alw5sus4zddvn8xgpdqw2swrghwx",
"bc1p4l2xzq7js40965s5w0fknd287kdlmt2dljte37zsc5a34u0h9c4q85snyd",
)
.unwrap()
.assume_checked();
Expand Down
8 changes: 3 additions & 5 deletions src/miniscript/decode.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,10 @@ use core::marker::PhantomData;
use std::error;

use bitcoin::hashes::{hash160, ripemd160, sha256, Hash};
use bitcoin::Weight;
use sync::Arc;

use crate::miniscript::lex::{Token as Tk, TokenIter};
use crate::miniscript::limits::MAX_PUBKEYS_PER_MULTISIG;
use crate::miniscript::limits::{MAX_PUBKEYS_IN_CHECKSIGADD, MAX_PUBKEYS_PER_MULTISIG};
use crate::miniscript::types::extra_props::ExtData;
use crate::miniscript::types::Type;
use crate::miniscript::ScriptContext;
Expand Down Expand Up @@ -451,10 +450,9 @@ pub fn parse<Ctx: ScriptContext>(
},
// MultiA
Tk::NumEqual, Tk::Num(k) => {
let max = Weight::MAX_BLOCK.to_wu() / 32;
// Check size before allocating keys
if k as u64 > max {
return Err(Error::MultiATooManyKeys(max))
if k as usize > MAX_PUBKEYS_IN_CHECKSIGADD {
return Err(Error::MultiATooManyKeys(MAX_PUBKEYS_IN_CHECKSIGADD as u64))
}
let mut keys = Vec::with_capacity(k as usize); // atleast k capacity
while tokens.peek() == Some(&Tk::CheckSigAdd) {
Expand Down
2 changes: 2 additions & 0 deletions src/miniscript/limits.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,5 @@ pub const MAX_BLOCK_WEIGHT: usize = 4000000;
/// Maximum pubkeys as arguments to CHECKMULTISIG
// https://github.com/bitcoin/bitcoin/blob/6acda4b00b3fc1bfac02f5de590e1a5386cbc779/src/script/script.h#L30
pub const MAX_PUBKEYS_PER_MULTISIG: usize = 20;
/// Maximum pubkeys in a CHECKSIGADD construction.
pub const MAX_PUBKEYS_IN_CHECKSIGADD: usize = (bitcoin::Weight::MAX_BLOCK.to_wu() / 32) as usize;
61 changes: 38 additions & 23 deletions src/policy/compiler.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use std::error;
use sync::Arc;

use crate::miniscript::context::SigType;
use crate::miniscript::limits::MAX_PUBKEYS_PER_MULTISIG;
use crate::miniscript::limits::{MAX_PUBKEYS_IN_CHECKSIGADD, MAX_PUBKEYS_PER_MULTISIG};
use crate::miniscript::types::{self, ErrorKind, ExtData, Type};
use crate::miniscript::ScriptContext;
use crate::policy::Concrete;
Expand Down Expand Up @@ -1065,24 +1065,26 @@ where
})
.collect();

match Ctx::sig_type() {
SigType::Schnorr if key_vec.len() == subs.len() => {
insert_wrap!(AstElemExt::terminal(Terminal::MultiA(k, key_vec)))
}
SigType::Ecdsa
if key_vec.len() == subs.len() && subs.len() <= MAX_PUBKEYS_PER_MULTISIG =>
{
insert_wrap!(AstElemExt::terminal(Terminal::Multi(k, key_vec)))
if key_vec.len() == subs.len() {
match Ctx::sig_type() {
SigType::Schnorr => {
if key_vec.len() <= MAX_PUBKEYS_IN_CHECKSIGADD {
insert_wrap!(AstElemExt::terminal(Terminal::MultiA(k, key_vec)))
}
}
SigType::Ecdsa => {
if key_vec.len() <= MAX_PUBKEYS_PER_MULTISIG {
insert_wrap!(AstElemExt::terminal(Terminal::Multi(k, key_vec)))
}
}
}
_ if k == subs.len() => {
let mut it = subs.iter();
let mut policy = it.next().expect("No sub policy in thresh() ?").clone();
policy =
it.fold(policy, |acc, pol| Concrete::And(vec![acc, pol.clone()]).into());
}
if k == subs.len() {
let mut it = subs.iter();
let mut policy = it.next().expect("No sub policy in thresh() ?").clone();
policy = it.fold(policy, |acc, pol| Concrete::And(vec![acc, pol.clone()]).into());

ret = best_compilations(policy_cache, policy.as_ref(), sat_prob, dissat_prob)?;
}
_ => {}
ret = best_compilations(policy_cache, policy.as_ref(), sat_prob, dissat_prob)?;
}

// FIXME: Should we also optimize thresh(1, subs) ?
Expand Down Expand Up @@ -1501,13 +1503,19 @@ mod tests {
fn compile_thresh() {
let (keys, _) = pubkeys_and_a_sig(21);

// Up until 20 keys, thresh should be compiled to a multi no matter the value of k
// For 3 < n <= 20, thresh should be compiled to a multi no matter the value of k
for k in 1..4 {
let small_thresh: BPolicy =
policy_str!("thresh({},pk({}),pk({}),pk({}))", k, keys[0], keys[1], keys[2]);
let small_thresh: BPolicy = policy_str!(
"thresh({},pk({}),pk({}),pk({}),pk({}))",
k,
keys[0],
keys[1],
keys[2],
keys[3]
);
let small_thresh_ms: SegwitMiniScript = small_thresh.compile().unwrap();
let small_thresh_ms_expected: SegwitMiniScript =
ms_str!("multi({},{},{},{})", k, keys[0], keys[1], keys[2]);
ms_str!("multi({},{},{},{},{})", k, keys[0], keys[1], keys[2], keys[3]);
assert_eq!(small_thresh_ms, small_thresh_ms_expected);
}

Expand Down Expand Up @@ -1640,8 +1648,15 @@ mod tests {
let small_thresh: Concrete<String> =
policy_str!("{}", &format!("thresh({},pk(B),pk(C),pk(D))", k));
let small_thresh_ms: Miniscript<String, Tap> = small_thresh.compile().unwrap();
let small_thresh_ms_expected: Miniscript<String, Tap> = ms_str!("multi_a({},B,C,D)", k);
assert_eq!(small_thresh_ms, small_thresh_ms_expected);
// When k == 3 it is more efficient to use and_v than multi_a
if k == 3 {
assert_eq!(
small_thresh_ms,
ms_str!("and_v(v:and_v(vc:pk_k(B),c:pk_k(C)),c:pk_k(D))")
);
} else {
assert_eq!(small_thresh_ms, ms_str!("multi_a({},B,C,D)", k));
}
}
}
}
Expand Down
19 changes: 10 additions & 9 deletions src/policy/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -505,12 +505,12 @@ mod tests {
Tr::<String>::from_str(
"tr(UNSPEND ,{
{
{multi_a(7,B,C,D,E,F,G,H),multi_a(7,A,C,D,E,F,G,H)},
{multi_a(7,A,B,D,E,F,G,H),multi_a(7,A,B,C,E,F,G,H)}
{and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:pk(B),pk(C)),pk(D)),pk(E)),pk(F)),pk(G)),pk(H)),and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:pk(A),pk(C)),pk(D)),pk(E)),pk(F)),pk(G)),pk(H))},
{and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:pk(A),pk(B)),pk(D)),pk(E)),pk(F)),pk(G)),pk(H)),and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:pk(A),pk(B)),pk(C)),pk(E)),pk(F)),pk(G)),pk(H))}
},
{
{multi_a(7,A,B,C,D,F,G,H),multi_a(7,A,B,C,D,E,G,H)}
,{multi_a(7,A,B,C,D,E,F,H),multi_a(7,A,B,C,D,E,F,G)}
{and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:pk(A),pk(B)),pk(C)),pk(D)),pk(F)),pk(G)),pk(H)),and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:pk(A),pk(B)),pk(C)),pk(D)),pk(E)),pk(G)),pk(H))},
{and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:pk(A),pk(B)),pk(C)),pk(D)),pk(E)),pk(F)),pk(H)),and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:and_v(v:pk(A),pk(B)),pk(C)),pk(D)),pk(E)),pk(F)),pk(G))}
}})"
.replace(&['\t', ' ', '\n'][..], "")
.as_str(),
Expand All @@ -526,18 +526,19 @@ mod tests {
let desc = pol
.compile_tr_private_experimental(Some(unspendable_key.clone()))
.unwrap();
println!("{}", desc);
let expected_desc = Descriptor::Tr(
Tr::<String>::from_str(
"tr(UNSPEND,
{{
{multi_a(3,A,D,E),multi_a(3,A,C,E)},
{multi_a(3,A,C,D),multi_a(3,A,B,E)}\
{and_v(v:and_v(v:pk(A),pk(D)),pk(E)),and_v(v:and_v(v:pk(A),pk(C)),pk(E))},
{and_v(v:and_v(v:pk(A),pk(C)),pk(D)),and_v(v:and_v(v:pk(A),pk(B)),pk(E))}
},
{
{multi_a(3,A,B,D),multi_a(3,A,B,C)},
{and_v(v:and_v(v:pk(A),pk(B)),pk(D)),and_v(v:and_v(v:pk(A),pk(B)),pk(C))},
{
{multi_a(3,C,D,E),multi_a(3,B,D,E)},
{multi_a(3,B,C,E),multi_a(3,B,C,D)}
{and_v(v:and_v(v:pk(C),pk(D)),pk(E)),and_v(v:and_v(v:pk(B),pk(D)),pk(E))},
{and_v(v:and_v(v:pk(B),pk(C)),pk(E)),and_v(v:and_v(v:pk(B),pk(C)),pk(D))}
}}})"
.replace(&['\t', ' ', '\n'][..], "")
.as_str(),
Expand Down

0 comments on commit 200991d

Please sign in to comment.