Skip to content

Commit 609d4fb

Browse files
apoelstraapoelstra
committed
Merge #871: Remove a bit of validation-related cruft
bd8fca6 miniscript: replace decode_with_ext(., Ext::allow_all) with decode_consensus (Andrew Poelstra) 3dd006a miniscript: remove Ctx::check_witness (Andrew Poelstra) Pull request description: This PR contains two mostly-unrelated commits. The first removes the `Ctx::check_witness` function, which is a no-op for the overwhelmingly common case that users are working with sane Miniscripts, and probably wrong in uncommon cases. The second one removes the `decode_insane` method, replacing it with `decode_consensus` which allows much more stuff. The premise here is that "insane" Miniscripts are about allowing well-formed scripts that might not be analyzable because they violate some sanity rules, while "consensus" Miniscripts are anything the library can parse, no matter how half-baked or incomplete it is. When parsing strings we want "insane", when parsing on-chain scripts we want "consensus". The difference, for now, is that "consensus" Miniscripts can have raw pkhs while "insane" ones cannot. More details in the commit messages. ACKs for top commit: tcharding: ACK bd8fca6 - My review does not hold much weight. sanket1729: ACK bd8fca6 Tree-SHA512: c889fc66debde8bc8942ad1db5522ece660cded6962a7b3833d5465759a69063f57ea1283fccb89ad66836ec58114fb7ffb57586440e31c0917caf7b658c8fb9
2 parents 6c723ed + bd8fca6 commit 609d4fb

File tree

4 files changed

+52
-129
lines changed

4 files changed

+52
-129
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/context.rs‎

Lines changed: 0 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ use crate::miniscript::limits::{
1515
};
1616
use crate::miniscript::types;
1717
use crate::prelude::*;
18-
use crate::util::witness_to_scriptsig;
1918
use crate::{hash256, Error, ForEachKey, Miniscript, MiniscriptKey, Terminal};
2019

2120
/// Error for Script Context
@@ -189,16 +188,6 @@ where
189188
_frag: &Terminal<Pk, Self>,
190189
) -> Result<(), ScriptContextError>;
191190

192-
/// Check whether the given satisfaction is valid under the ScriptContext
193-
/// For example, segwit satisfactions may fail if the witness len is more
194-
/// 3600 or number of stack elements are more than 100.
195-
fn check_witness(_witness: &[Vec<u8>]) -> Result<(), ScriptContextError> {
196-
// Only really need to do this for segwitv0 and legacy
197-
// Bare is already restricted by standardness rules
198-
// and would reach these limits.
199-
Ok(())
200-
}
201-
202191
/// Each context has slightly different rules on what Pks are allowed in descriptors
203192
/// Legacy/Bare does not allow x_only keys
204193
/// Segwit does not allow uncompressed keys and x_only keys
@@ -389,19 +378,6 @@ impl ScriptContext for Legacy {
389378
}
390379
}
391380

392-
fn check_witness(witness: &[Vec<u8>]) -> Result<(), ScriptContextError> {
393-
// In future, we could avoid by having a function to count only
394-
// len of script instead of converting it.
395-
let script_sig = witness_to_scriptsig(witness);
396-
if script_sig.len() > MAX_SCRIPTSIG_SIZE {
397-
return Err(ScriptContextError::MaxScriptSigSizeExceeded {
398-
actual: script_sig.len(),
399-
limit: MAX_SCRIPTSIG_SIZE,
400-
});
401-
}
402-
Ok(())
403-
}
404-
405381
fn check_global_consensus_validity<Pk: MiniscriptKey>(
406382
ms: &Miniscript<Pk, Self>,
407383
) -> Result<(), ScriptContextError> {
@@ -506,16 +482,6 @@ impl ScriptContext for Segwitv0 {
506482
}
507483
}
508484

509-
fn check_witness(witness: &[Vec<u8>]) -> Result<(), ScriptContextError> {
510-
if witness.len() > MAX_STANDARD_P2WSH_STACK_ITEMS {
511-
return Err(ScriptContextError::MaxWitnessItemsExceeded {
512-
actual: witness.len(),
513-
limit: MAX_STANDARD_P2WSH_STACK_ITEMS,
514-
});
515-
}
516-
Ok(())
517-
}
518-
519485
fn check_global_consensus_validity<Pk: MiniscriptKey>(
520486
ms: &Miniscript<Pk, Self>,
521487
) -> Result<(), ScriptContextError> {
@@ -627,17 +593,6 @@ impl ScriptContext for Tap {
627593
}
628594
}
629595

630-
fn check_witness(witness: &[Vec<u8>]) -> Result<(), ScriptContextError> {
631-
// Note that tapscript has a 1000 limit compared to 100 of segwitv0
632-
if witness.len() > MAX_STACK_SIZE {
633-
return Err(ScriptContextError::MaxWitnessItemsExceeded {
634-
actual: witness.len(),
635-
limit: MAX_STACK_SIZE,
636-
});
637-
}
638-
Ok(())
639-
}
640-
641596
fn check_global_consensus_validity<Pk: MiniscriptKey>(
642597
ms: &Miniscript<Pk, Self>,
643598
) -> Result<(), ScriptContextError> {
@@ -882,13 +837,6 @@ impl ScriptContext for NoChecks {
882837
"NochecksEcdsa"
883838
}
884839

885-
fn check_witness(_witness: &[Vec<u8>]) -> Result<(), ScriptContextError> {
886-
// Only really need to do this for segwitv0 and legacy
887-
// Bare is already restricted by standardness rules
888-
// and would reach these limits.
889-
Ok(())
890-
}
891-
892840
fn check_global_validity<Pk: MiniscriptKey>(
893841
ms: &Miniscript<Pk, Self>,
894842
) -> Result<(), ScriptContextError> {

‎src/miniscript/mod.rs‎

Lines changed: 41 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -471,10 +471,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
471471
Pk: ToPublicKey,
472472
{
473473
match satisfaction.stack {
474-
satisfy::Witness::Stack(stack) => {
475-
Ctx::check_witness(&stack)?;
476-
Ok(stack)
477-
}
474+
satisfy::Witness::Stack(stack) => Ok(stack),
478475
satisfy::Witness::Unavailable | satisfy::Witness::Impossible => {
479476
Err(Error::CouldNotSatisfy)
480477
}
@@ -522,24 +519,17 @@ impl Miniscript<<Tap as ScriptContext>::Key, Tap> {
522519
}
523520

524521
impl<Ctx: ScriptContext> Miniscript<Ctx::Key, Ctx> {
525-
/// Attempt to parse an insane(scripts don't clear sanity checks)
526-
/// script into a Miniscript representation.
527-
/// Use this to parse scripts with repeated pubkeys, timelock mixing, malleable
528-
/// scripts without sig or scripts that can exceed resource limits.
529-
/// Some of the analysis guarantees of miniscript are lost when dealing with
530-
/// insane scripts. In general, in a multi-party setting users should only
531-
/// accept sane scripts.
532-
pub fn decode_insane(script: &script::Script) -> Result<Miniscript<Ctx::Key, Ctx>, Error> {
533-
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())
534530
}
535531

536-
/// Attempt to parse an miniscript with extra features that not yet specified in the spec.
537-
/// Users should not use this function unless they scripts can/will change in the future.
538-
/// Currently, this function supports the following features:
539-
/// - Parsing all insane scripts
540-
/// - Parsing miniscripts with raw pubkey hashes
541-
///
542-
/// 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.
543533
pub fn decode_with_ext(
544534
script: &script::Script,
545535
ext: &ExtParams,
@@ -564,7 +554,7 @@ impl<Ctx: ScriptContext> Miniscript<Ctx::Key, Ctx> {
564554
/// Attempt to parse a Script into Miniscript representation.
565555
///
566556
/// This function will fail parsing for scripts that do not clear the
567-
/// [`Miniscript::sanity_check`] checks. Use [`Miniscript::decode_insane`] to
557+
/// [`Miniscript::sanity_check`] checks. Use [`Miniscript::decode_consensus`] to
568558
/// parse such scripts.
569559
///
570560
/// ## Decode/Parse a miniscript from script hex
@@ -1164,8 +1154,8 @@ mod tests {
11641154
assert_eq!(format!("{:x}", bitcoin_script), expected);
11651155
}
11661156
// Parse scripts with all extensions
1167-
let roundtrip = Segwitv0Script::decode_with_ext(&bitcoin_script, &ExtParams::allow_all())
1168-
.expect("parse string serialization");
1157+
let roundtrip =
1158+
Segwitv0Script::decode_consensus(&bitcoin_script).expect("parse string serialization");
11691159
assert_eq!(roundtrip, script);
11701160
}
11711161

@@ -1174,7 +1164,8 @@ mod tests {
11741164
let ser = tree.encode();
11751165
assert_eq!(ser.len(), tree.script_size());
11761166
assert_eq!(ser.to_string(), s);
1177-
let deser = Segwitv0Script::decode_insane(&ser).expect("deserialize result of serialize");
1167+
let deser =
1168+
Segwitv0Script::decode_consensus(&ser).expect("deserialize result of serialize");
11781169
assert_eq!(*tree, deser);
11791170
}
11801171

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

13201311
let ms = "and_v(v:sha256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),or_d(sha256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),older(16)))";
13211312
let ms: Segwitv0Script = Miniscript::from_str_insane(ms).unwrap();
1322-
assert_eq!(ms, Segwitv0Script::decode_insane(&ms.encode()).unwrap());
1313+
assert_eq!(ms, Segwitv0Script::decode_consensus(&ms.encode()).unwrap());
13231314

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

13281319
let ms = "and_v(v:hash256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),or_d(sha256(96de8fc8c256fa1e1556d41af431cace7dca68707c78dd88c3acab8b17164c47),older(16)))";
13291320
let ms: Segwitv0Script = Miniscript::from_str_insane(ms).unwrap();
1330-
assert_eq!(ms, Segwitv0Script::decode_insane(&ms.encode()).unwrap());
1321+
assert_eq!(ms, Segwitv0Script::decode_consensus(&ms.encode()).unwrap());
13311322
}
13321323

13331324
#[test]
@@ -1518,21 +1509,21 @@ mod tests {
15181509
#[test]
15191510
fn deserialize() {
15201511
// Most of these came from fuzzing, hence the increasing lengths
1521-
assert!(Segwitv0Script::decode_insane(&hex_script("")).is_err()); // empty
1522-
assert!(Segwitv0Script::decode_insane(&hex_script("00")).is_ok()); // FALSE
1523-
assert!(Segwitv0Script::decode_insane(&hex_script("51")).is_ok()); // TRUE
1524-
assert!(Segwitv0Script::decode_insane(&hex_script("69")).is_err()); // VERIFY
1525-
assert!(Segwitv0Script::decode_insane(&hex_script("0000")).is_err()); //and_v(FALSE,FALSE)
1526-
assert!(Segwitv0Script::decode_insane(&hex_script("1001")).is_err()); // incomplete push
1527-
assert!(Segwitv0Script::decode_insane(&hex_script("03990300b2")).is_err()); // non-minimal #
1528-
assert!(Segwitv0Script::decode_insane(&hex_script("8559b2")).is_err()); // leading bytes
1529-
assert!(Segwitv0Script::decode_insane(&hex_script("4c0169b2")).is_err()); // non-minimal push
1530-
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
15311522

15321523
// misc fuzzer problems
1533-
assert!(Segwitv0Script::decode_insane(&hex_script("0000000000af")).is_err());
1534-
assert!(Segwitv0Script::decode_insane(&hex_script("04009a2970af00")).is_err()); // giant CMS key num
1535-
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(
15361527
"2102ffffffffffffffefefefefefefefefefefef394c0fe5b711179e124008584753ac6900"
15371528
))
15381529
.is_err());
@@ -1572,22 +1563,22 @@ mod tests {
15721563

15731564
//---------------- test script <-> miniscript ---------------
15741565
// Test parsing from scripts: x-only fails decoding in segwitv0 ctx
1575-
Segwitv0Script::decode_insane(&hex_script(
1566+
Segwitv0Script::decode_consensus(&hex_script(
15761567
"202788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac",
15771568
))
15781569
.unwrap_err();
15791570
// x-only succeeds in tap ctx
1580-
Tapscript::decode_insane(&hex_script(
1571+
Tapscript::decode_consensus(&hex_script(
15811572
"202788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac",
15821573
))
15831574
.unwrap();
15841575
// tapscript fails decoding with compressed
1585-
Tapscript::decode_insane(&hex_script(
1576+
Tapscript::decode_consensus(&hex_script(
15861577
"21022788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac",
15871578
))
15881579
.unwrap_err();
15891580
// Segwitv0 succeeds decoding with tapscript.
1590-
Segwitv0Script::decode_insane(&hex_script(
1581+
Segwitv0Script::decode_consensus(&hex_script(
15911582
"21022788ee41e76f4f3af603da5bc8fa22997bc0344bb0f95666ba6aaff0242baa99ac",
15921583
))
15931584
.unwrap();
@@ -1623,7 +1614,7 @@ mod tests {
16231614
.unwrap();
16241615
// script rtt test
16251616
assert_eq!(
1626-
Miniscript::<XOnlyPublicKey, Tap>::decode_insane(&tap_ms.encode()).unwrap(),
1617+
Miniscript::<XOnlyPublicKey, Tap>::decode_consensus(&tap_ms.encode()).unwrap(),
16271618
tap_ms
16281619
);
16291620
assert_eq!(tap_ms.script_size(), 104);
@@ -1664,7 +1655,7 @@ mod tests {
16641655
.unwrap();
16651656
let ms_trans = ms.translate_pk(&mut StrKeyTranslator::new()).unwrap();
16661657
let enc = ms_trans.encode();
1667-
let ms = Miniscript::<bitcoin::PublicKey, Segwitv0>::decode_insane(&enc).unwrap();
1658+
let ms = Miniscript::<bitcoin::PublicKey, Segwitv0>::decode_consensus(&enc).unwrap();
16681659
assert_eq!(ms_trans.encode(), ms.encode());
16691660
}
16701661

@@ -1687,8 +1678,8 @@ mod tests {
16871678
let script = ms.encode();
16881679
// The same test, but parsing from script
16891680
SegwitMs::decode(&script).unwrap_err();
1690-
SegwitMs::decode_insane(&script).unwrap_err();
1691-
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();
16921683

16931684
// Try replacing the raw_pkh with a pkh
16941685
let mut map = BTreeMap::new();
@@ -1880,7 +1871,7 @@ mod tests {
18801871
for _ in 0..10000 {
18811872
script = script.push_opcode(bitcoin::opcodes::all::OP_0NOTEQUAL);
18821873
}
1883-
Tapscript::decode_insane(&script.into_script()).unwrap_err();
1874+
Tapscript::decode_consensus(&script.into_script()).unwrap_err();
18841875
}
18851876

18861877
#[test]

0 commit comments

Comments
 (0)