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.

Sign up for GitHub

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

Jump to bottom

compiler: improve logic when deciding between conjunctions and multi/multi_a #657

Merged
merged 5 commits into from
Mar 12, 2024
Merged

compiler: improve logic when deciding between conjunctions and multi/multi_a #657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b2ec4a8
clippy: fix some nits introduced by #651
apoelstra Mar 7, 2024
0666aef
error: remove some unused variants
apoelstra Mar 7, 2024
78db616
consolidate some FIXMEs
apoelstra Mar 10, 2024
bc3b8dc
policy: rename Threshold variant to Thresh
apoelstra Mar 7, 2024
200991d
compiler: improve logic for deciding between thresholds and ands
apoelstra Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions examples/taproot.rs
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion src/descriptor/sortedmulti.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {

impl<Pk: MiniscriptKey, Ctx: ScriptContext> policy::Liftable<Pk> for SortedMultiVec<Pk, Ctx> {
fn lift(&self) -> Result<policy::semantic::Policy<Pk>, Error> {
let ret = policy::semantic::Policy::Threshold(
let ret = policy::semantic::Policy::Thresh(
self.k,
self.pks
.iter()
Expand Down
4 changes: 2 additions & 2 deletions src/descriptor/tr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,7 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for TapTree<Pk> {
fn lift(&self) -> Result<Policy<Pk>, Error> {
fn lift_helper<Pk: MiniscriptKey>(s: &TapTree<Pk>) -> Result<Policy<Pk>, Error> {
match *s {
TapTree::Tree { ref left, ref right, height: _ } => Ok(Policy::Threshold(
TapTree::Tree { ref left, ref right, height: _ } => Ok(Policy::Thresh(
1,
vec![Arc::new(lift_helper(left)?), Arc::new(lift_helper(right)?)],
)),
Expand All @@ -632,7 +632,7 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for TapTree<Pk> {
impl<Pk: MiniscriptKey> Liftable<Pk> for Tr<Pk> {
fn lift(&self) -> Result<Policy<Pk>, Error> {
match &self.tree {
Some(root) => Ok(Policy::Threshold(
Some(root) => Ok(Policy::Thresh(
1,
vec![
Arc::new(Policy::Key(self.internal_key.clone())),
Expand Down
44 changes: 0 additions & 44 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,8 +443,6 @@ pub enum Error {
Unexpected(String),
/// Name of a fragment contained `:` multiple times
MultiColon(String),
/// Name of a fragment contained `@` multiple times
MultiAt(String),
/// Name of a fragment contained `@` but we were not parsing an OR
AtOutsideOr(String),
/// Encountered a wrapping character that we don't recognize
Expand All @@ -453,16 +451,8 @@ pub enum Error {
NonTopLevel(String),
/// Parsed a miniscript but there were more script opcodes after it
Trailing(String),
/// Failed to parse a push as a public key
BadPubkey(bitcoin::key::Error),
/// Could not satisfy a script (fragment) because of a missing hash preimage
MissingHash(sha256::Hash),
/// Could not satisfy a script (fragment) because of a missing signature
MissingSig(bitcoin::PublicKey),
/// Could not satisfy, relative locktime not met
RelativeLocktimeNotMet(u32),
/// Could not satisfy, absolute locktime not met
AbsoluteLocktimeNotMet(u32),
/// General failure to satisfy
CouldNotSatisfy,
/// Typechecking failed
Expand All @@ -482,8 +472,6 @@ pub enum Error {
ContextError(miniscript::context::ScriptContextError),
/// Recursion depth exceeded when parsing policy/miniscript from string
MaxRecursiveDepthExceeded,
/// Script size too large
ScriptSizeTooLarge,
/// Anything but c:pk(key) (P2PK), c:pk_h(key) (P2PKH), and thresh_m(k,...)
/// up to n=3 is invalid by standardness (bare)
NonStandardBareScript,
Expand All @@ -495,12 +483,8 @@ pub enum Error {
BareDescriptorAddr,
/// PubKey invalid under current context
PubKeyCtxError(miniscript::decode::KeyParseError, &'static str),
/// Attempted to call function that requires PreComputed taproot info
TaprootSpendInfoUnavialable,
/// No script code for Tr descriptors
TrNoScriptCode,
/// No explicit script for Tr descriptors
TrNoExplicitScript,
/// At least two BIP389 key expressions in the descriptor contain tuples of
/// derivation indexes of different lengths.
MultipathDescLenMismatch,
Expand All @@ -512,8 +496,6 @@ pub enum Error {

// https://github.com/sipa/miniscript/pull/5 for discussion on this number
const MAX_RECURSION_DEPTH: u32 = 402;
// https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki
const MAX_SCRIPT_SIZE: u32 = 10000;

impl fmt::Display for Error {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
Expand All @@ -531,23 +513,12 @@ impl fmt::Display for Error {
Error::UnexpectedStart => f.write_str("unexpected start of script"),
Error::Unexpected(ref s) => write!(f, "unexpected «{}»", s),
Error::MultiColon(ref s) => write!(f, "«{}» has multiple instances of «:»", s),
Error::MultiAt(ref s) => write!(f, "«{}» has multiple instances of «@»", s),
Error::AtOutsideOr(ref s) => write!(f, "«{}» contains «@» in non-or() context", s),
Error::UnknownWrapper(ch) => write!(f, "unknown wrapper «{}:»", ch),
Error::NonTopLevel(ref s) => write!(f, "non-T miniscript: {}", s),
Error::Trailing(ref s) => write!(f, "trailing tokens: {}", s),
Error::MissingHash(ref h) => write!(f, "missing preimage of hash {}", h),
Error::MissingSig(ref pk) => write!(f, "missing signature for key {:?}", pk),
Error::RelativeLocktimeNotMet(n) => {
write!(f, "required relative locktime CSV of {} blocks, not met", n)
}
Error::AbsoluteLocktimeNotMet(n) => write!(
f,
"required absolute locktime CLTV of {} blocks, not met",
n
),
Error::CouldNotSatisfy => f.write_str("could not satisfy"),
Error::BadPubkey(ref e) => fmt::Display::fmt(e, f),
Error::TypeCheck(ref e) => write!(f, "typecheck: {}", e),
Error::BadDescriptor(ref e) => write!(f, "Invalid descriptor: {}", e),
Error::Secp(ref e) => fmt::Display::fmt(e, f),
Expand All @@ -561,11 +532,6 @@ impl fmt::Display for Error {
"Recursive depth over {} not permitted",
MAX_RECURSION_DEPTH
),
Error::ScriptSizeTooLarge => write!(
f,
"Standardness rules imply bitcoin than {} bytes",
MAX_SCRIPT_SIZE
),
Error::NonStandardBareScript => write!(
f,
"Anything but c:pk(key) (P2PK), c:pk_h(key) (P2PKH), and thresh_m(k,...) \
Expand All @@ -579,9 +545,7 @@ impl fmt::Display for Error {
write!(f, "Pubkey error: {} under {} scriptcontext", pk, ctx)
}
Error::MultiATooManyKeys(k) => write!(f, "MultiA too many keys {}", k),
Error::TaprootSpendInfoUnavialable => write!(f, "Taproot Spend Info not computed."),
Error::TrNoScriptCode => write!(f, "No script code for Tr descriptors"),
Error::TrNoExplicitScript => write!(f, "No script code for Tr descriptors"),
Error::MultipathDescLenMismatch => write!(f, "At least two BIP389 key expressions in the descriptor contain tuples of derivation indexes of different lengths"),
Error::AbsoluteLockTime(ref e) => e.fmt(f),
Error::RelativeLockTime(ref e) => e.fmt(f),
Expand All @@ -605,30 +569,22 @@ impl error::Error for Error {
| UnexpectedStart
| Unexpected(_)
| MultiColon(_)
| MultiAt(_)
| AtOutsideOr(_)
| UnknownWrapper(_)
| NonTopLevel(_)
| Trailing(_)
| MissingHash(_)
| MissingSig(_)
| RelativeLocktimeNotMet(_)
| AbsoluteLocktimeNotMet(_)
| CouldNotSatisfy
| TypeCheck(_)
| BadDescriptor(_)
| MaxRecursiveDepthExceeded
| ScriptSizeTooLarge
| NonStandardBareScript
| ImpossibleSatisfaction
| BareDescriptorAddr
| TaprootSpendInfoUnavialable
| TrNoScriptCode
| TrNoExplicitScript
| MultipathDescLenMismatch => None,
Script(e) => Some(e),
AddrError(e) => Some(e),
BadPubkey(e) => Some(e),
Secp(e) => Some(e),
#[cfg(feature = "compiler")]
CompilerError(e) => Some(e),
Expand Down
6 changes: 3 additions & 3 deletions src/miniscript/astelem.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ fn fmt_1<D: fmt::Debug + fmt::Display>(
a: &D,
is_debug: bool,
) -> fmt::Result {
f.write_str(&name)?;
f.write_str(name)?;
conditional_fmt(f, a, is_debug)?;
f.write_str(")")
}
Expand All @@ -150,7 +150,7 @@ fn fmt_2<D: fmt::Debug + fmt::Display>(
b: &D,
is_debug: bool,
) -> fmt::Result {
f.write_str(&name)?;
f.write_str(name)?;
conditional_fmt(f, a, is_debug)?;
f.write_str(",")?;
conditional_fmt(f, b, is_debug)?;
Expand All @@ -163,7 +163,7 @@ fn fmt_n<D: fmt::Debug + fmt::Display>(
list: &[D],
is_debug: bool,
) -> fmt::Result {
f.write_str(&name)?;
f.write_str(name)?;
write!(f, "{}", first)?;
for el in list {
f.write_str(",")?;
Expand Down
8 changes: 3 additions & 5 deletions src/miniscript/decode.rs
View file
Open in desktop
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
Open in desktop
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;
16 changes: 1 addition & 15 deletions src/miniscript/types/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ impl Type {

/// Constructor for the type of the `a:` fragment.
pub const fn cast_alt(self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
// FIXME need to do manual `?` because ? is not supported in constfns. (Also below.)
Ok(Type {
corr: match Correctness::cast_alt(self.corr) {
Ok(x) => x,
Expand All @@ -232,7 +232,6 @@ impl Type {

/// Constructor for the type of the `s:` fragment.
pub const fn cast_swap(self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
Ok(Type {
corr: match Correctness::cast_swap(self.corr) {
Ok(x) => x,
Expand All @@ -255,7 +254,6 @@ impl Type {

/// Constructor for the type of the `d:` fragment.
pub const fn cast_dupif(self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
Ok(Type {
corr: match Correctness::cast_dupif(self.corr) {
Ok(x) => x,
Expand All @@ -267,7 +265,6 @@ impl Type {

/// Constructor for the type of the `v:` fragment.
pub const fn cast_verify(self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
Ok(Type {
corr: match Correctness::cast_verify(self.corr) {
Ok(x) => x,
Expand All @@ -279,7 +276,6 @@ impl Type {

/// Constructor for the type of the `j:` fragment.
pub const fn cast_nonzero(self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
Ok(Type {
corr: match Correctness::cast_nonzero(self.corr) {
Ok(x) => x,
Expand All @@ -291,7 +287,6 @@ impl Type {

/// Constructor for the type of the `n:` fragment.
pub const fn cast_zeronotequal(self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
Ok(Type {
corr: match Correctness::cast_zeronotequal(self.corr) {
Ok(x) => x,
Expand All @@ -303,7 +298,6 @@ impl Type {

/// Constructor for the type of the `t:` fragment.
pub const fn cast_true(self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
Ok(Type {
corr: match Correctness::cast_true(self.corr) {
Ok(x) => x,
Expand All @@ -315,7 +309,6 @@ impl Type {

/// Constructor for the type of the `u:` fragment.
pub const fn cast_unlikely(self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
Ok(Type {
corr: match Correctness::cast_or_i_false(self.corr) {
Ok(x) => x,
Expand All @@ -327,7 +320,6 @@ impl Type {

/// Constructor for the type of the `l:` fragment.
pub const fn cast_likely(self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
Ok(Type {
corr: match Correctness::cast_or_i_false(self.corr) {
Ok(x) => x,
Expand All @@ -339,7 +331,6 @@ impl Type {

/// Constructor for the type of the `and_b` fragment.
pub const fn and_b(left: Self, right: Self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
Ok(Type {
corr: match Correctness::and_b(left.corr, right.corr) {
Ok(x) => x,
Expand All @@ -351,7 +342,6 @@ impl Type {

/// Constructor for the type of the `and_v` fragment.
pub const fn and_v(left: Self, right: Self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
Ok(Type {
corr: match Correctness::and_v(left.corr, right.corr) {
Ok(x) => x,
Expand All @@ -363,7 +353,6 @@ impl Type {

/// Constructor for the type of the `or_b` fragment.
pub const fn or_b(left: Self, right: Self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
Ok(Type {
corr: match Correctness::or_b(left.corr, right.corr) {
Ok(x) => x,
Expand All @@ -375,7 +364,6 @@ impl Type {

/// Constructor for the type of the `or_b` fragment.
pub const fn or_d(left: Self, right: Self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
Ok(Type {
corr: match Correctness::or_d(left.corr, right.corr) {
Ok(x) => x,
Expand All @@ -387,7 +375,6 @@ impl Type {

/// Constructor for the type of the `or_c` fragment.
pub const fn or_c(left: Self, right: Self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
Ok(Type {
corr: match Correctness::or_c(left.corr, right.corr) {
Ok(x) => x,
Expand All @@ -410,7 +397,6 @@ impl Type {

/// Constructor for the type of the `and_or` fragment.
pub const fn and_or(a: Self, b: Self, c: Self) -> Result<Self, ErrorKind> {
// FIXME need to do manual `?` because ? is not supported in constfns.
Ok(Type {
corr: match Correctness::and_or(a.corr, b.corr, c.corr) {
Ok(x) => x,
Expand Down
Loading
Loading