Skip to content

Commit 276cf94

Browse files
committed
sortedmulti: simplify constructor and improve unit test
Update the constructor to avoid calling Miniscript::from_ast, which can return many kinds of errors, none of which are reachable. Instead call Miniscript::multi and then do a validation check once this is constructed. There is a comment in the constructor explaining why the validation is needed: it may be that an otherwise-valid checkmultisig script could exceed the 520-byte limit for p2sh outputs. HOWEVER, we have no test for this behavior, and when trying to test it, I found several issues. One, that our accounting for uncompressed keys is wrong, I fixed in the previous commits. The others are: 1. In the existing unit test we try to create a sortedmulti with 21 keys, violating the constructor for Threshold. This is a fine test... 2. ...except that we set k == 0 which makes the constructor fail no matter what n is, so we're not actually testing "too many keys". 3. Furthermore, we have no test at all that tries to exceed the P2SH limits, and when I added one I was able to exceed these limits because of the type system bugs fixed in the last two commits.
1 parent 3250e40 commit 276cf94

File tree

1 file changed

+24
-13
lines changed

1 file changed

+24
-13
lines changed

src/descriptor/sortedmulti.rs

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,10 @@ pub struct SortedMultiVec<Pk: MiniscriptKey, Ctx: ScriptContext> {
3333

3434
impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
3535
fn constructor_check(mut self) -> Result<Self, Error> {
36+
let ms = Miniscript::<Pk, Ctx>::multi(self.inner);
3637
// Check the limits before creating a new SortedMultiVec
3738
// For example, under p2sh context the scriptlen can only be
3839
// upto 520 bytes.
39-
let term: Terminal<Pk, Ctx> = Terminal::Multi(self.inner);
40-
let ms = Miniscript::from_ast(term)?;
41-
// This would check all the consensus rules for p2sh/p2wsh and
42-
// even tapscript in future
4340
Ctx::check_local_validity(&ms)?;
4441
if let Terminal::Multi(inner) = ms.node {
4542
self.inner = inner;
@@ -229,31 +226,45 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> fmt::Display for SortedMultiVec<Pk,
229226
mod tests {
230227
use core::str::FromStr as _;
231228

232-
use bitcoin::secp256k1::PublicKey;
229+
use bitcoin::PublicKey;
233230

234231
use super::*;
235-
use crate::miniscript::context::Legacy;
232+
use crate::miniscript::context::{Legacy, ScriptContextError};
236233

237234
#[test]
238235
fn too_many_pubkeys() {
239-
// Arbitrary pubic key.
236+
// Arbitrary 33-byte public key (34 with length prefix).
240237
let pk = PublicKey::from_str(
241238
"02e6642fd69bd211f93f7f1f36ca51a26a5290eb2dd1b0d8279a87bb0d480c8443",
242239
)
243240
.unwrap();
244241

245-
let over = 1 + MAX_PUBKEYS_PER_MULTISIG;
242+
let pks = vec![pk; 1 + MAX_PUBKEYS_PER_MULTISIG];
243+
let res: Result<SortedMultiVec<PublicKey, Legacy>, Error> = SortedMultiVec::new(1, pks);
244+
let error = res.expect_err("constructor should err");
246245

247-
let mut pks = Vec::new();
248-
for _ in 0..over {
249-
pks.push(pk);
246+
match error {
247+
Error::Threshold(_) => {} // ok
248+
other => panic!("unexpected error: {:?}", other),
250249
}
250+
}
251251

252-
let res: Result<SortedMultiVec<PublicKey, Legacy>, Error> = SortedMultiVec::new(0, pks);
252+
#[test]
253+
fn too_many_pubkeys_for_p2sh() {
254+
// Arbitrary 65-byte public key (66 with length prefix).
255+
let pk = PublicKey::from_str(
256+
"0400232a2acfc9b43fa89f1b4f608fde335d330d7114f70ea42bfb4a41db368a3e3be6934a4097dd25728438ef73debb1f2ffdb07fec0f18049df13bdc5285dc5b",
257+
)
258+
.unwrap();
259+
260+
// This is legal for CHECKMULTISIG, but the 8 keys consume the whole 520 bytes
261+
// allowed by P2SH, meaning that the full script goes over the limit.
262+
let pks = vec![pk; 8];
263+
let res: Result<SortedMultiVec<PublicKey, Legacy>, Error> = SortedMultiVec::new(2, pks);
253264
let error = res.expect_err("constructor should err");
254265

255266
match error {
256-
Error::Threshold(_) => {} // ok
267+
Error::ContextError(ScriptContextError::MaxRedeemScriptSizeExceeded { .. }) => {} // ok
257268
other => panic!("unexpected error: {:?}", other),
258269
}
259270
}

0 commit comments

Comments
 (0)