Skip to content

Commit bd8fca6

Browse files
apoelstraapoelstra
committed
miniscript: replace decode_with_ext(., Ext::allow_all) with decode_consensus
Currently we have decode() and decode_insane() (which were both called parse_* before PR #845). In practice though, throughout the codebase we see that when decoding from script we want to relax more than sanity. In particular, we frequently with ExtParams::allow_all, which beyond relaxing the sanity rules, also allows raw pubkeyhashes. We do this in the script interpreter and in PSBT, on the basis that once we're working with a script, we should just deal with whatever the library is capable of dealing with. (Is this the right decision? IMO yes. It could be argued. But regardless, it's the decision we've made for the past many versions of rust-miniscript.) I also propose to backport this with deprecation, telling users that if they really want parse_insane, they now need to call decode_with_ext and specify ExtParams::insane. But if their goal was "parse anything that might be allowed", they actually want decode_consensus. There is now an asymmetry between parsing from string and parsing from script: with strings we have parse() and parse_insane(). With script we have decode() and decode_consensus(). I think this is correct, and reflects the fact that somebody "breaking the rules" with a string is likely trying to use syntactically valid Miniscripts without the library whining at him, while somebody "breaking the rules" with a Script is probably trying to deal with some immutable on-chain thing and wants the library to work if at-all possible. Right now the distinction is simply "do we support raw pkh or not" but I think we could accept more-or-less arbitrary extensions to Miniscript in this library that worked the same way (allowed when decoded from script but disallowed from strings since there's no serialization).
1 parent 3dd006a commit bd8fca6

File tree

3 files changed

+51
-73
lines changed

3 files changed

+51
-73
lines changed

‎src/interpreter/inner.rs‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use bitcoin::Witness;
88
use super::{stack, BitcoinKey, Error, Stack};
99
use crate::miniscript::context::{NoChecks, ScriptContext, SigType};
1010
use crate::prelude::*;
11-
use crate::{BareCtx, ExtParams, Legacy, Miniscript, Segwitv0, Tap, ToPublicKey, Translator};
11+
use crate::{BareCtx, Legacy, Miniscript, Segwitv0, Tap, ToPublicKey, Translator};
1212

1313
/// Attempts to parse a slice as a Bitcoin public key, checking compressedness
1414
/// if asked to, but otherwise dropping it
@@ -43,8 +43,7 @@ fn script_from_stack_elem<Ctx: ScriptContext>(
4343
) -> Result<Miniscript<Ctx::Key, Ctx>, Error> {
4444
match *elem {
4545
stack::Element::Push(sl) => {
46-
Miniscript::decode_with_ext(bitcoin::Script::from_bytes(sl), &ExtParams::allow_all())
47-
.map_err(Error::from)
46+
Miniscript::decode_consensus(bitcoin::Script::from_bytes(sl)).map_err(Error::from)
4847
}
4948
stack::Element::Satisfied => Ok(Miniscript::TRUE),
5049
stack::Element::Dissatisfied => Ok(Miniscript::FALSE),
@@ -327,10 +326,7 @@ pub(super) fn from_txdata<'txin>(
327326
} else {
328327
if wit_stack.is_empty() {
329328
// Bare script parsed in BareCtx
330-
let miniscript = Miniscript::<bitcoin::PublicKey, BareCtx>::decode_with_ext(
331-
spk,
332-
&ExtParams::allow_all(),
333-
)?;
329+
let miniscript = Miniscript::<bitcoin::PublicKey, BareCtx>::decode_consensus(spk)?;
334330
let miniscript = miniscript.to_no_checks_ms();
335331
Ok((Inner::Script(miniscript, ScriptType::Bare), ssig_stack, Some(spk.to_owned())))
336332
} else {
@@ -678,8 +674,7 @@ mod tests {
678674
}
679675

680676
fn ms_inner_script(ms: &str) -> (Miniscript<BitcoinKey, NoChecks>, bitcoin::ScriptBuf) {
681-
let ms = Miniscript::<bitcoin::PublicKey, Segwitv0>::from_str_ext(ms, &ExtParams::insane())
682-
.unwrap();
677+
let ms = Miniscript::<bitcoin::PublicKey, Segwitv0>::from_str_insane(ms).unwrap();
683678
let spk = ms.encode();
684679
let miniscript = ms.to_no_checks_ms();
685680
(miniscript, spk)

‎src/miniscript/mod.rs‎

Lines changed: 40 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -519,24 +519,17 @@ impl Miniscript<<Tap as ScriptContext>::Key, Tap> {
519519
}
520520

521521
impl<Ctx: ScriptContext> Miniscript<Ctx::Key, Ctx> {
522-
/// Attempt to parse an insane(scripts don't clear sanity checks)
523-
/// script into a Miniscript representation.
524-
/// Use this to parse scripts with repeated pubkeys, timelock mixing, malleable
525-
/// scripts without sig or scripts that can exceed resource limits.
526-
/// Some of the analysis guarantees of miniscript are lost when dealing with
527-
/// insane scripts. In general, in a multi-party setting users should only
528-
/// accept sane scripts.
529-
pub fn decode_insane(script: &script::Script) -> Result<Miniscript<Ctx::Key, Ctx>, Error> {
530-
Miniscript::decode_with_ext(script, &ExtParams::insane())
522+
/// Attempt to decode a Miniscript from Script, checking only for consensus compatibility,
523+
/// and no other checks.
524+
///
525+
/// It may make sense to use this method when parsing Script that is already
526+
/// embedded in the chain. While it is inadvisable to use insane Miniscripts,
527+
/// once it's on the chain you don't have much choice anymore.
528+
pub fn decode_consensus(script: &script::Script) -> Result<Miniscript<Ctx::Key, Ctx>, Error> {
529+
Miniscript::decode_with_ext(script, &ExtParams::allow_all())
531530
}
532531

533-
/// Attempt to parse an miniscript with extra features that not yet specified in the spec.
534-
/// Users should not use this function unless they scripts can/will change in the future.
535-
/// Currently, this function supports the following features:
536-
/// - Parsing all insane scripts
537-
/// - Parsing miniscripts with raw pubkey hashes
538-
///
539-
/// Allowed extra features can be specified by the ext [`ExtParams`] argument.
532+
/// Attempt to decode a Miniscript from Script, specifying which validation parameters to apply.
540533
pub fn decode_with_ext(
541534
script: &script::Script,
542535
ext: &ExtParams,
@@ -561,7 +554,7 @@ impl<Ctx: ScriptContext> Miniscript<Ctx::Key, Ctx> {
561554
/// Attempt to parse a Script into Miniscript representation.
562555
///
563556
/// This function will fail parsing for scripts that do not clear the
564-
/// [`Miniscript::sanity_check`] checks. Use [`Miniscript::decode_insane`] to
557+
/// [`Miniscript::sanity_check`] checks. Use [`Miniscript::decode_consensus`] to
565558
/// parse such scripts.
566559
///
567560
/// ## Decode/Parse a miniscript from script hex
@@ -1161,8 +1154,8 @@ mod tests {
11611154
assert_eq!(format!("{:x}", bitcoin_script), expected);
11621155
}
11631156
// Parse scripts with all extensions
1164-
let roundtrip = Segwitv0Script::decode_with_ext(&bitcoin_script, &ExtParams::allow_all())
1165-
.expect("parse string serialization");
1157+
let roundtrip =
1158+
Segwitv0Script::decode_consensus(&bitcoin_script).expect("parse string serialization");
11661159
assert_eq!(roundtrip, script);
11671160
}
11681161

@@ -1171,7 +1164,8 @@ mod tests {
11711164
let ser = tree.encode();
11721165
assert_eq!(ser.len(), tree.script_size());
11731166
assert_eq!(ser.to_string(), s);
1174-
let deser = Segwitv0Script::decode_insane(&ser).expect("deserialize result of serialize");
1167+
let deser =
1168+
Segwitv0Script::decode_consensus(&ser).expect("deserialize result of serialize");
11751169
assert_eq!(*tree, deser);
11761170
}
11771171

@@ -1312,19 +1306,19 @@ mod tests {
13121306
fn verify_parse() {
13131307
let ms = "and_v(v:hash160(20195b5a3d650c17f0f29f91c33f8f6335193d07),or_d(sha256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),older(16)))";
13141308
let ms: Segwitv0Script = Miniscript::from_str_insane(ms).unwrap();
1315-
assert_eq!(ms, Segwitv0Script::decode_insane(&ms.encode()).unwrap());
1309+
assert_eq!(ms, Segwitv0Script::decode_consensus(&ms.encode()).unwrap());
13161310

13171311
let ms = "and_v(v:sha256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),or_d(sha256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),older(16)))";
13181312
let ms: Segwitv0Script = Miniscript::from_str_insane(ms).unwrap();
1319-
assert_eq!(ms, Segwitv0Script::decode_insane(&ms.encode()).unwrap());
1313+
assert_eq!(ms, Segwitv0Script::decode_consensus(&ms.encode()).unwrap());
13201314

13211315
let ms = "and_v(v:ripemd160(20195b5a3d650c17f0f29f91c33f8f6335193d07),or_d(sha256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),older(16)))";
13221316
let ms: Segwitv0Script = Miniscript::from_str_insane(ms).unwrap();
1323-
assert_eq!(ms, Segwitv0Script::decode_insane(&ms.encode()).unwrap());
1317+
assert_eq!(ms, Segwitv0Script::decode_consensus(&ms.encode()).unwrap());
13241318

13251319
let ms = "and_v(v:hash256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),or_d(sha256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),older(16)))";
13261320
let ms: Segwitv0Script = Miniscript::from_str_insane(ms).unwrap();
1327-
assert_eq!(ms, Segwitv0Script::decode_insane(&ms.encode()).unwrap());
1321+
assert_eq!(ms, Segwitv0Script::decode_consensus(&ms.encode()).unwrap());
13281322
}
13291323

13301324
#[test]
@@ -1515,21 +1509,21 @@ mod tests {
15151509
#[test]
15161510
fn deserialize() {
15171511
// Most of these came from fuzzing, hence the increasing lengths
1518-
assert!(Segwitv0Script::decode_insane(&hex_script("")).is_err()); // empty
1519-
assert!(Segwitv0Script::decode_insane(&hex_script("00")).is_ok()); // FALSE
1520-
assert!(Segwitv0Script::decode_insane(&hex_script("51")).is_ok()); // TRUE
1521-
assert!(Segwitv0Script::decode_insane(&hex_script("69")).is_err()); // VERIFY
1522-
assert!(Segwitv0Script::decode_insane(&hex_script("0000")).is_err()); //and_v(FALSE,FALSE)
1523-
assert!(Segwitv0Script::decode_insane(&hex_script("1001")).is_err()); // incomplete push
1524-
assert!(Segwitv0Script::decode_insane(&hex_script("03990300b2")).is_err()); // non-minimal #
1525-
assert!(Segwitv0Script::decode_insane(&hex_script("8559b2")).is_err()); // leading bytes
1526-
assert!(Segwitv0Script::decode_insane(&hex_script("4c0169b2")).is_err()); // non-minimal push
1527-
assert!(Segwitv0Script::decode_insane(&hex_script("0000af0000ae85")).is_err()); // OR not BOOLOR
1512+
assert!(Segwitv0Script::decode_consensus(&hex_script("")).is_err()); // empty
1513+
assert!(Segwitv0Script::decode_consensus(&hex_script("00")).is_ok()); // FALSE
1514+
assert!(Segwitv0Script::decode_consensus(&hex_script("51")).is_ok()); // TRUE
1515+
assert!(Segwitv0Script::decode_consensus(&hex_script("69")).is_err()); // VERIFY
1516+
assert!(Segwitv0Script::decode_consensus(&hex_script("0000")).is_err()); //and_v(FALSE,FALSE)
1517+
assert!(Segwitv0Script::decode_consensus(&hex_script("1001")).is_err()); // incomplete push
1518+
assert!(Segwitv0Script::decode_consensus(&hex_script("03990300b2")).is_err()); // non-minimal #
1519+
assert!(Segwitv0Script::decode_consensus(&hex_script("8559b2")).is_err()); // leading bytes
1520+
assert!(Segwitv0Script::decode_consensus(&hex_script("4c0169b2")).is_err()); // non-minimal push
1521+
assert!(Segwitv0Script::decode_consensus(&hex_script("0000af0000ae85")).is_err()); // OR not BOOLOR
15281522

15291523
// misc fuzzer problems
1530-
assert!(Segwitv0Script::decode_insane(&hex_script("0000000000af")).is_err());
1531-
assert!(Segwitv0Script::decode_insane(&hex_script("04009a2970af00")).is_err()); // giant CMS key num
1532-
assert!(Segwitv0Script::decode_insane(&hex_script(
1524+
assert!(Segwitv0Script::decode_consensus(&hex_script("0000000000af")).is_err());
1525+
assert!(Segwitv0Script::decode_consensus(&hex_script("04009a2970af00")).is_err()); // giant CMS key num
1526+
assert!(Segwitv0Script::decode_consensus(&hex_script(
15331527
"2102ffffffffffffffefefefefefefefefefefef394c0fe5b711179e124008584753ac6900"
15341528
))
15351529
.is_err());
@@ -1569,22 +1563,22 @@ mod tests {
15691563

15701564
//---------------- test script <-> miniscript ---------------
15711565
// Test parsing from scripts: x-only fails decoding in segwitv0 ctx
1572-
Segwitv0Script::decode_insane(&hex_script(
1566+
Segwitv0Script::decode_consensus(&hex_script(
15731567
"202788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac",
15741568
))
15751569
.unwrap_err();
15761570
// x-only succeeds in tap ctx
1577-
Tapscript::decode_insane(&hex_script(
1571+
Tapscript::decode_consensus(&hex_script(
15781572
"202788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac",
15791573
))
15801574
.unwrap();
15811575
// tapscript fails decoding with compressed
1582-
Tapscript::decode_insane(&hex_script(
1576+
Tapscript::decode_consensus(&hex_script(
15831577
"21022788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac",
15841578
))
15851579
.unwrap_err();
15861580
// Segwitv0 succeeds decoding with tapscript.
1587-
Segwitv0Script::decode_insane(&hex_script(
1581+
Segwitv0Script::decode_consensus(&hex_script(
15881582
"21022788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac",
15891583
))
15901584
.unwrap();
@@ -1620,7 +1614,7 @@ mod tests {
16201614
.unwrap();
16211615
// script rtt test
16221616
assert_eq!(
1623-
Miniscript::<XOnlyPublicKey, Tap>::decode_insane(&tap_ms.encode()).unwrap(),
1617+
Miniscript::<XOnlyPublicKey, Tap>::decode_consensus(&tap_ms.encode()).unwrap(),
16241618
tap_ms
16251619
);
16261620
assert_eq!(tap_ms.script_size(), 104);
@@ -1661,7 +1655,7 @@ mod tests {
16611655
.unwrap();
16621656
let ms_trans = ms.translate_pk(&mut StrKeyTranslator::new()).unwrap();
16631657
let enc = ms_trans.encode();
1664-
let ms = Miniscript::<bitcoin::PublicKey, Segwitv0>::decode_insane(&enc).unwrap();
1658+
let ms = Miniscript::<bitcoin::PublicKey, Segwitv0>::decode_consensus(&enc).unwrap();
16651659
assert_eq!(ms_trans.encode(), ms.encode());
16661660
}
16671661

@@ -1684,8 +1678,8 @@ mod tests {
16841678
let script = ms.encode();
16851679
// The same test, but parsing from script
16861680
SegwitMs::decode(&script).unwrap_err();
1687-
SegwitMs::decode_insane(&script).unwrap_err();
1688-
SegwitMs::decode_with_ext(&script, &ExtParams::allow_all()).unwrap();
1681+
SegwitMs::decode_with_ext(&script, &ExtParams::insane()).unwrap_err();
1682+
SegwitMs::decode_consensus(&script).unwrap();
16891683

16901684
// Try replacing the raw_pkh with a pkh
16911685
let mut map = BTreeMap::new();
@@ -1877,7 +1871,7 @@ mod tests {
18771871
for _ in 0..10000 {
18781872
script = script.push_opcode(bitcoin::opcodes::all::OP_0NOTEQUAL);
18791873
}
1880-
Tapscript::decode_insane(&script.into_script()).unwrap_err();
1874+
Tapscript::decode_consensus(&script.into_script()).unwrap_err();
18811875
}
18821876

18831877
#[test]

‎src/psbt/finalizer.rs‎

Lines changed: 7 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@ use super::{sanity_check, Error, InputError, Psbt, PsbtInputSatisfier};
2424
use crate::prelude::*;
2525
use crate::util::witness_size;
2626
use crate::{
27-
interpreter, BareCtx, Descriptor, ExtParams, Legacy, Miniscript, Satisfier, Segwitv0, SigType,
28-
Tap, ToPublicKey,
27+
interpreter, BareCtx, Descriptor, Legacy, Miniscript, Satisfier, Segwitv0, SigType, Tap,
28+
ToPublicKey,
2929
};
3030

3131
// Satisfy the taproot descriptor. It is not possible to infer the complete
@@ -71,10 +71,7 @@ fn construct_tap_witness(
7171
// We don't know how to satisfy non default version scripts yet
7272
continue;
7373
}
74-
let ms = match Miniscript::<XOnlyPublicKey, Tap>::decode_with_ext(
75-
script,
76-
&ExtParams::allow_all(),
77-
) {
74+
let ms = match Miniscript::<XOnlyPublicKey, Tap>::decode_consensus(script) {
7875
Ok(ms) => ms.substitute_raw_pkh(&map),
7976
Err(..) => continue, // try another script
8077
};
@@ -213,10 +210,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result<Descriptor<PublicKey>, In
213210
p2wsh_expected: script_pubkey.clone(),
214211
});
215212
}
216-
let ms = Miniscript::<bitcoin::PublicKey, Segwitv0>::decode_with_ext(
217-
witness_script,
218-
&ExtParams::allow_all(),
219-
)?;
213+
let ms = Miniscript::<bitcoin::PublicKey, Segwitv0>::decode_consensus(witness_script)?;
220214
Ok(Descriptor::new_wsh(ms.substitute_raw_pkh(&map))?)
221215
} else {
222216
Err(InputError::MissingWitnessScript)
@@ -240,9 +234,8 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result<Descriptor<PublicKey>, In
240234
p2wsh_expected: redeem_script.clone(),
241235
});
242236
}
243-
let ms = Miniscript::<bitcoin::PublicKey, Segwitv0>::decode_with_ext(
237+
let ms = Miniscript::<bitcoin::PublicKey, Segwitv0>::decode_consensus(
244238
witness_script,
245-
&ExtParams::allow_all(),
246239
)?;
247240
Ok(Descriptor::new_sh_wsh(ms.substitute_raw_pkh(&map))?)
248241
} else {
@@ -272,9 +265,8 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result<Descriptor<PublicKey>, In
272265
return Err(InputError::NonEmptyWitnessScript);
273266
}
274267
if let Some(ref redeem_script) = inp.redeem_script {
275-
let ms = Miniscript::<bitcoin::PublicKey, Legacy>::decode_with_ext(
268+
let ms = Miniscript::<bitcoin::PublicKey, Legacy>::decode_consensus(
276269
redeem_script,
277-
&ExtParams::allow_all(),
278270
)?;
279271
Ok(Descriptor::new_sh(ms)?)
280272
} else {
@@ -291,10 +283,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result<Descriptor<PublicKey>, In
291283
if inp.redeem_script.is_some() {
292284
return Err(InputError::NonEmptyRedeemScript);
293285
}
294-
let ms = Miniscript::<bitcoin::PublicKey, BareCtx>::decode_with_ext(
295-
&script_pubkey,
296-
&ExtParams::allow_all(),
297-
)?;
286+
let ms = Miniscript::<bitcoin::PublicKey, BareCtx>::decode_consensus(&script_pubkey)?;
298287
Ok(Descriptor::new_bare(ms.substitute_raw_pkh(&map))?)
299288
}
300289
}

0 commit comments

Comments
 (0)