Skip to content

Commit 9eb4375

Browse files
apoelstraapoelstra
committed
Merge #674: Use Threshold type in concrete policy and in Terminal::multi/multi_a
79d3f7e sortedmulti: eliminate allocation in constructor_check (Andrew Poelstra) f62322d miniscript: use new Threshold type for multi/multi_a (Andrew Poelstra) 1ced03b policy: use Threshold in concrete policy (Andrew Poelstra) Pull request description: Some more large but mostly-mechanical diffs. This is able to eliminate a bunch of error paths, though the actual error variants can't be removed until we also convert Terminal::thresh in the next PR(s). At that point we will start to see the real benefits of this type because fallible functions will become fallible and massive amounts of compiler error-checking can just go away entirely. Also removes the allocation in `SortedMulti::constructor_check` that was pointed out in #660. ACKs for top commit: sanket1729: ACK 79d3f7e tcharding: ACK 79d3f7e Tree-SHA512: 98828910c6fea91608b9f7dc15f68999ac4555e2e8eac28c04bd3e70f7c5e74dfeb7902805e606a3ad5a7ba36f77d4dc0c47f189b7ff591a8c4fd7abcccbefc7
2 parents f83eb64 + 79d3f7e commit 9eb4375

File tree

14 files changed

+258
-334
lines changed

14 files changed

+258
-334
lines changed

‎src/descriptor/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1492,7 +1492,7 @@ mod tests {
14921492
#[test]
14931493
fn roundtrip_tests() {
14941494
let descriptor = Descriptor::<bitcoin::PublicKey>::from_str("multi");
1495-
assert_eq!(descriptor.unwrap_err().to_string(), "unexpected «no arguments given»")
1495+
assert_eq!(descriptor.unwrap_err().to_string(), "expected threshold, found terminal",);
14961496
}
14971497

14981498
#[test]

‎src/descriptor/sortedmulti.rs‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -32,15 +32,21 @@ pub struct SortedMultiVec<Pk: MiniscriptKey, Ctx: ScriptContext> {
3232
}
3333

3434
impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
35-
fn constructor_check(&self) -> Result<(), Error> {
35+
fn constructor_check(mut self) -> Result<Self, Error> {
3636
// Check the limits before creating a new SortedMultiVec
3737
// For example, under p2sh context the scriptlen can only be
3838
// upto 520 bytes.
39-
let term: Terminal<Pk, Ctx> = Terminal::Multi(self.inner.k(), self.inner.data().to_owned());
39+
let term: Terminal<Pk, Ctx> = Terminal::Multi(self.inner);
4040
let ms = Miniscript::from_ast(term)?;
4141
// This would check all the consensus rules for p2sh/p2wsh and
4242
// even tapscript in future
43-
Ctx::check_local_validity(&ms).map_err(From::from)
43+
Ctx::check_local_validity(&ms)?;
44+
if let Terminal::Multi(inner) = ms.node {
45+
self.inner = inner;
46+
Ok(self)
47+
} else {
48+
unreachable!()
49+
}
4450
}
4551

4652
/// Create a new instance of `SortedMultiVec` given a list of keys and the threshold
@@ -49,8 +55,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
4955
pub fn new(k: usize, pks: Vec<Pk>) -> Result<Self, Error> {
5056
let ret =
5157
Self { inner: Threshold::new(k, pks).map_err(Error::Threshold)?, phantom: PhantomData };
52-
ret.constructor_check()?;
53-
Ok(ret)
58+
ret.constructor_check()
5459
}
5560

5661
/// Parse an expression tree into a SortedMultiVec
@@ -66,8 +71,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
6671
.translate_by_index(|i| expression::terminal(&tree.args[i + 1], Pk::from_str))?,
6772
phantom: PhantomData,
6873
};
69-
ret.constructor_check()?;
70-
Ok(ret)
74+
ret.constructor_check()
7175
}
7276

7377
/// This will panic if fpk returns an uncompressed key when
@@ -85,8 +89,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
8589
inner: self.inner.translate_ref(|pk| t.pk(pk))?,
8690
phantom: PhantomData,
8791
};
88-
ret.constructor_check().map_err(TranslateErr::OuterError)?;
89-
Ok(ret)
92+
ret.constructor_check().map_err(TranslateErr::OuterError)
9093
}
9194

9295
/// The threshold value for the multisig.
@@ -113,11 +116,8 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
113116
/// utility function to sanity a sorted multi vec
114117
pub fn sanity_check(&self) -> Result<(), Error> {
115118
let ms: Miniscript<Pk, Ctx> =
116-
Miniscript::from_ast(Terminal::Multi(self.k(), self.pks().to_owned()))
117-
.expect("Must typecheck");
118-
// '?' for doing From conversion
119-
ms.sanity_check()?;
120-
Ok(())
119+
Miniscript::from_ast(Terminal::Multi(self.inner.clone())).expect("Must typecheck");
120+
ms.sanity_check().map_err(From::from)
121121
}
122122
}
123123

@@ -127,16 +127,16 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
127127
where
128128
Pk: ToPublicKey,
129129
{
130-
let mut pks = self.pks().to_owned();
130+
let mut thresh = self.inner.clone();
131131
// Sort pubkeys lexicographically according to BIP 67
132-
pks.sort_by(|a, b| {
132+
thresh.data_mut().sort_by(|a, b| {
133133
a.to_public_key()
134134
.inner
135135
.serialize()
136136
.partial_cmp(&b.to_public_key().inner.serialize())
137137
.unwrap()
138138
});
139-
Terminal::Multi(self.k(), pks)
139+
Terminal::Multi(thresh)
140140
}
141141

142142
/// Encode as a Bitcoin script

‎src/interpreter/mod.rs‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -845,9 +845,9 @@ where
845845
None => return Some(Err(Error::UnexpectedStackEnd)),
846846
}
847847
}
848-
Terminal::MultiA(k, ref subs) => {
849-
if node_state.n_evaluated == subs.len() {
850-
if node_state.n_satisfied == k {
848+
Terminal::MultiA(ref thresh) => {
849+
if node_state.n_evaluated == thresh.n() {
850+
if node_state.n_satisfied == thresh.k() {
851851
self.stack.push(stack::Element::Satisfied);
852852
} else {
853853
self.stack.push(stack::Element::Dissatisfied);
@@ -856,10 +856,10 @@ where
856856
// evaluate each key with as a pk
857857
// note that evaluate_pk will error on non-empty incorrect sigs
858858
// push 1 on satisfied sigs and push 0 on empty sigs
859-
match self
860-
.stack
861-
.evaluate_pk(&mut self.verify_sig, subs[node_state.n_evaluated])
862-
{
859+
match self.stack.evaluate_pk(
860+
&mut self.verify_sig,
861+
thresh.data()[node_state.n_evaluated],
862+
) {
863863
Some(Ok(x)) => {
864864
self.push_evaluation_state(
865865
node_state.node,
@@ -886,34 +886,34 @@ where
886886
}
887887
}
888888
}
889-
Terminal::Multi(ref k, ref subs) if node_state.n_evaluated == 0 => {
889+
Terminal::Multi(ref thresh) if node_state.n_evaluated == 0 => {
890890
let len = self.stack.len();
891-
if len < k + 1 {
891+
if len < thresh.k() + 1 {
892892
return Some(Err(Error::InsufficientSignaturesMultiSig));
893893
} else {
894894
//Non-sat case. If the first sig is empty, others k elements must
895895
//be empty.
896896
match self.stack.last() {
897897
Some(&stack::Element::Dissatisfied) => {
898898
//Remove the extra zero from multi-sig check
899-
let sigs = self.stack.split_off(len - (k + 1));
899+
let sigs = self.stack.split_off(len - (thresh.k() + 1));
900900
let nonsat = sigs
901901
.iter()
902902
.map(|sig| *sig == stack::Element::Dissatisfied)
903903
.filter(|empty| *empty)
904904
.count();
905-
if nonsat == *k + 1 {
905+
if nonsat == thresh.k() + 1 {
906906
self.stack.push(stack::Element::Dissatisfied);
907907
} else {
908908
return Some(Err(Error::MissingExtraZeroMultiSig));
909909
}
910910
}
911911
None => return Some(Err(Error::UnexpectedStackEnd)),
912912
_ => {
913-
match self
914-
.stack
915-
.evaluate_multi(&mut self.verify_sig, &subs[subs.len() - 1])
916-
{
913+
match self.stack.evaluate_multi(
914+
&mut self.verify_sig,
915+
&thresh.data()[thresh.n() - 1],
916+
) {
917917
Some(Ok(x)) => {
918918
self.push_evaluation_state(
919919
node_state.node,
@@ -933,20 +933,20 @@ where
933933
}
934934
}
935935
}
936-
Terminal::Multi(k, ref subs) => {
937-
if node_state.n_satisfied == k {
936+
Terminal::Multi(ref thresh) => {
937+
if node_state.n_satisfied == thresh.k() {
938938
//multi-sig bug: Pop extra 0
939939
if let Some(stack::Element::Dissatisfied) = self.stack.pop() {
940940
self.stack.push(stack::Element::Satisfied);
941941
} else {
942942
return Some(Err(Error::MissingExtraZeroMultiSig));
943943
}
944-
} else if node_state.n_evaluated == subs.len() {
944+
} else if node_state.n_evaluated == thresh.n() {
945945
return Some(Err(Error::MultiSigEvaluationError));
946946
} else {
947947
match self.stack.evaluate_multi(
948948
&mut self.verify_sig,
949-
&subs[subs.len() - node_state.n_evaluated - 1],
949+
&thresh.data()[thresh.n() - node_state.n_evaluated - 1],
950950
) {
951951
Some(Ok(x)) => {
952952
self.push_evaluation_state(

‎src/miniscript/astelem.rs‎

Lines changed: 32 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,20 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
8282
fmt_2(f, "or_i(", l, r, is_debug)
8383
}
8484
Terminal::Thresh(k, ref subs) => fmt_n(f, "thresh(", k, subs, is_debug),
85-
Terminal::Multi(k, ref keys) => fmt_n(f, "multi(", k, keys, is_debug),
86-
Terminal::MultiA(k, ref keys) => fmt_n(f, "multi_a(", k, keys, is_debug),
85+
Terminal::Multi(ref thresh) => {
86+
if is_debug {
87+
fmt::Debug::fmt(&thresh.debug("multi", true), f)
88+
} else {
89+
fmt::Display::fmt(&thresh.display("multi", true), f)
90+
}
91+
}
92+
Terminal::MultiA(ref thresh) => {
93+
if is_debug {
94+
fmt::Debug::fmt(&thresh.debug("multi_a", true), f)
95+
} else {
96+
fmt::Display::fmt(&thresh.display("multi_a", true), f)
97+
}
98+
}
8799
// wrappers
88100
_ => {
89101
if let Some((ch, sub)) = self.wrap_char() {
@@ -314,27 +326,16 @@ impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Termina
314326

315327
Ok(Terminal::Thresh(k, subs?))
316328
}
317-
("multi", n) | ("multi_a", n) => {
318-
if n == 0 {
319-
return Err(errstr("no arguments given"));
320-
}
321-
let k = expression::terminal(&top.args[0], expression::parse_num)? as usize;
322-
if k > n - 1 {
323-
return Err(errstr("higher threshold than there were keys in multi"));
324-
}
325-
326-
let pks: Result<Vec<Pk>, _> = top.args[1..]
327-
.iter()
328-
.map(|sub| expression::terminal(sub, Pk::from_str))
329-
.collect();
330-
331-
if frag_name == "multi" {
332-
pks.map(|pks| Terminal::Multi(k, pks))
333-
} else {
334-
// must be multi_a
335-
pks.map(|pks| Terminal::MultiA(k, pks))
336-
}
337-
}
329+
("multi", _) => top
330+
.to_null_threshold()
331+
.map_err(Error::ParseThreshold)?
332+
.translate_by_index(|i| expression::terminal(&top.args[1 + i], Pk::from_str))
333+
.map(Terminal::Multi),
334+
("multi_a", _) => top
335+
.to_null_threshold()
336+
.map_err(Error::ParseThreshold)?
337+
.translate_by_index(|i| expression::terminal(&top.args[1 + i], Pk::from_str))
338+
.map(Terminal::MultiA),
338339
_ => Err(Error::Unexpected(format!(
339340
"{}({} args) while parsing Miniscript",
340341
top.name,
@@ -483,27 +484,27 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Terminal<Pk, Ctx> {
483484
.push_int(k as i64)
484485
.push_opcode(opcodes::all::OP_EQUAL)
485486
}
486-
Terminal::Multi(k, ref keys) => {
487+
Terminal::Multi(ref thresh) => {
487488
debug_assert!(Ctx::sig_type() == SigType::Ecdsa);
488-
builder = builder.push_int(k as i64);
489-
for pk in keys {
489+
builder = builder.push_int(thresh.k() as i64);
490+
for pk in thresh.data() {
490491
builder = builder.push_key(&pk.to_public_key());
491492
}
492493
builder
493-
.push_int(keys.len() as i64)
494+
.push_int(thresh.n() as i64)
494495
.push_opcode(opcodes::all::OP_CHECKMULTISIG)
495496
}
496-
Terminal::MultiA(k, ref keys) => {
497+
Terminal::MultiA(ref thresh) => {
497498
debug_assert!(Ctx::sig_type() == SigType::Schnorr);
498499
// keys must be atleast len 1 here, guaranteed by typing rules
499-
builder = builder.push_ms_key::<_, Ctx>(&keys[0]);
500+
builder = builder.push_ms_key::<_, Ctx>(&thresh.data()[0]);
500501
builder = builder.push_opcode(opcodes::all::OP_CHECKSIG);
501-
for pk in keys.iter().skip(1) {
502+
for pk in thresh.iter().skip(1) {
502503
builder = builder.push_ms_key::<_, Ctx>(pk);
503504
builder = builder.push_opcode(opcodes::all::OP_CHECKSIGADD);
504505
}
505506
builder
506-
.push_int(k as i64)
507+
.push_int(thresh.k() as i64)
507508
.push_opcode(opcodes::all::OP_NUMEQUAL)
508509
}
509510
}

‎src/miniscript/context.rs‎

Lines changed: 11 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,8 @@ use bitcoin::Weight;
1010

1111
use super::decode::ParseableKey;
1212
use crate::miniscript::limits::{
13-
MAX_OPS_PER_SCRIPT, MAX_PUBKEYS_PER_MULTISIG, MAX_SCRIPTSIG_SIZE, MAX_SCRIPT_ELEMENT_SIZE,
14-
MAX_SCRIPT_SIZE, MAX_STACK_SIZE, MAX_STANDARD_P2WSH_SCRIPT_SIZE,
15-
MAX_STANDARD_P2WSH_STACK_ITEMS,
13+
MAX_OPS_PER_SCRIPT, MAX_SCRIPTSIG_SIZE, MAX_SCRIPT_ELEMENT_SIZE, MAX_SCRIPT_SIZE,
14+
MAX_STACK_SIZE, MAX_STANDARD_P2WSH_SCRIPT_SIZE, MAX_STANDARD_P2WSH_STACK_ITEMS,
1615
};
1716
use crate::miniscript::types;
1817
use crate::prelude::*;
@@ -61,8 +60,6 @@ pub enum ScriptContextError {
6160
TaprootMultiDisabled,
6261
/// Stack size exceeded in script execution
6362
StackSizeLimitExceeded { actual: usize, limit: usize },
64-
/// More than 20 keys in a Multi fragment
65-
CheckMultiSigLimitExceeded,
6663
/// MultiA is only allowed in post tapscript
6764
MultiANotAllowed,
6865
}
@@ -87,7 +84,6 @@ impl error::Error for ScriptContextError {
8784
| ImpossibleSatisfaction
8885
| TaprootMultiDisabled
8986
| StackSizeLimitExceeded { .. }
90-
| CheckMultiSigLimitExceeded
9187
| MultiANotAllowed => None,
9288
}
9389
}
@@ -149,9 +145,6 @@ impl fmt::Display for ScriptContextError {
149145
actual, limit
150146
)
151147
}
152-
ScriptContextError::CheckMultiSigLimitExceeded => {
153-
write!(f, "CHECkMULTISIG ('multi()' descriptor) only supports up to 20 pubkeys")
154-
}
155148
ScriptContextError::MultiANotAllowed => {
156149
write!(f, "Multi a(CHECKSIGADD) only allowed post tapscript")
157150
}
@@ -405,11 +398,8 @@ impl ScriptContext for Legacy {
405398

406399
match ms.node {
407400
Terminal::PkK(ref pk) => Self::check_pk(pk),
408-
Terminal::Multi(_k, ref pks) => {
409-
if pks.len() > MAX_PUBKEYS_PER_MULTISIG {
410-
return Err(ScriptContextError::CheckMultiSigLimitExceeded);
411-
}
412-
for pk in pks.iter() {
401+
Terminal::Multi(ref thresh) => {
402+
for pk in thresh.iter() {
413403
Self::check_pk(pk)?;
414404
}
415405
Ok(())
@@ -506,11 +496,8 @@ impl ScriptContext for Segwitv0 {
506496

507497
match ms.node {
508498
Terminal::PkK(ref pk) => Self::check_pk(pk),
509-
Terminal::Multi(_k, ref pks) => {
510-
if pks.len() > MAX_PUBKEYS_PER_MULTISIG {
511-
return Err(ScriptContextError::CheckMultiSigLimitExceeded);
512-
}
513-
for pk in pks.iter() {
499+
Terminal::Multi(ref thresh) => {
500+
for pk in thresh.iter() {
514501
Self::check_pk(pk)?;
515502
}
516503
Ok(())
@@ -620,8 +607,8 @@ impl ScriptContext for Tap {
620607

621608
match ms.node {
622609
Terminal::PkK(ref pk) => Self::check_pk(pk),
623-
Terminal::MultiA(_, ref keys) => {
624-
for pk in keys.iter() {
610+
Terminal::MultiA(ref thresh) => {
611+
for pk in thresh.iter() {
625612
Self::check_pk(pk)?;
626613
}
627614
Ok(())
@@ -716,11 +703,8 @@ impl ScriptContext for BareCtx {
716703
}
717704
match ms.node {
718705
Terminal::PkK(ref key) => Self::check_pk(key),
719-
Terminal::Multi(_k, ref pks) => {
720-
if pks.len() > MAX_PUBKEYS_PER_MULTISIG {
721-
return Err(ScriptContextError::CheckMultiSigLimitExceeded);
722-
}
723-
for pk in pks.iter() {
706+
Terminal::Multi(ref thresh) => {
707+
for pk in thresh.iter() {
724708
Self::check_pk(pk)?;
725709
}
726710
Ok(())
@@ -749,7 +733,7 @@ impl ScriptContext for BareCtx {
749733
Terminal::PkK(_pk) | Terminal::PkH(_pk) => Ok(()),
750734
_ => Err(Error::NonStandardBareScript),
751735
},
752-
Terminal::Multi(_k, subs) if subs.len() <= 3 => Ok(()),
736+
Terminal::Multi(ref thresh) if thresh.n() <= 3 => Ok(()),
753737
_ => Err(Error::NonStandardBareScript),
754738
}
755739
}

0 commit comments

Comments
 (0)