From fa2e8b41735a2433984d98a0caf23beda57d7ab5 Mon Sep 17 00:00:00 2001 From: sanket1729 Date: Thu, 1 Dec 2022 17:32:18 -0800 Subject: [PATCH 1/4] Cleanup Miniscript constructors Miniscript should only be called with two constructors. from_ast and from_components_unchecked --- src/miniscript/mod.rs | 85 +++++++++++++++++++----------------------- src/policy/compiler.rs | 26 ++++--------- 2 files changed, 46 insertions(+), 65 deletions(-) diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index 2b58a6c02..dc774fd5d 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -57,7 +57,7 @@ pub struct Miniscript { ///Additional information helpful for extra analysis. pub ext: types::extra_props::ExtData, /// Context PhantomData. Only accessible inside this crate - pub(crate) phantom: PhantomData, + phantom: PhantomData, } /// `PartialOrd` of `Miniscript` must depend only on node and not the type information. @@ -116,6 +116,24 @@ impl Miniscript { phantom: PhantomData, }) } + + /// Create a new `Miniscript` from a `Terminal` node and a `Type` annotation + /// This does not check the typing rules. The user is responsible for ensuring + /// that the type provided is correct. + /// + /// You should almost always use `Miniscript::from_ast` instead of this function. + pub fn from_components_unchecked( + node: Terminal, + ty: types::Type, + ext: types::extra_props::ExtData, + ) -> Miniscript { + Miniscript { + node, + ty, + ext, + phantom: PhantomData, + } + } } impl fmt::Display for Miniscript { @@ -311,15 +329,7 @@ impl Miniscript { T: Translator, { let inner = self.node.real_translate_pk(t)?; - let ms = Miniscript { - //directly copying the type and ext is safe because translating public - //key should not change any properties - ty: self.ty, - ext: self.ext, - node: inner, - phantom: PhantomData, - }; - Ok(ms) + Ok(Miniscript::from_ast(inner).expect("This will be removed in the next commit")) } } @@ -428,12 +438,7 @@ impl_from_tree!( /// should not be called directly; rather go through the descriptor API. fn from_tree(top: &expression::Tree) -> Result, Error> { let inner: Terminal = expression::FromTree::from_tree(top)?; - Ok(Miniscript { - ty: Type::type_check(&inner, |_| None)?, - ext: ExtData::type_check(&inner, |_| None)?, - node: inner, - phantom: PhantomData, - }) + Miniscript::from_ast(inner) } ); @@ -663,30 +668,22 @@ mod tests { .unwrap(); let hash = hash160::Hash::from_byte_array([17; 20]); - let pkk_ms: Miniscript = Miniscript { - node: Terminal::Check(Arc::new(Miniscript { - node: Terminal::PkK(String::from("")), - ty: Type::from_pk_k::(), - ext: types::extra_props::ExtData::from_pk_k::(), - phantom: PhantomData, - })), - ty: Type::cast_check(Type::from_pk_k::()).unwrap(), - ext: ExtData::cast_check(ExtData::from_pk_k::()).unwrap(), + let pk_node = Terminal::Check(Arc::new(Miniscript { + node: Terminal::PkK(String::from("")), + ty: Type::from_pk_k::(), + ext: types::extra_props::ExtData::from_pk_k::(), phantom: PhantomData, - }; + })); + let pkk_ms: Miniscript = Miniscript::from_ast(pk_node).unwrap(); dummy_string_rtt(pkk_ms, "[B/onduesm]c:[K/onduesm]pk_k(\"\")", "pk()"); - let pkh_ms: Miniscript = Miniscript { - node: Terminal::Check(Arc::new(Miniscript { - node: Terminal::PkH(String::from("")), - ty: Type::from_pk_h::(), - ext: types::extra_props::ExtData::from_pk_h::(), - phantom: PhantomData, - })), - ty: Type::cast_check(Type::from_pk_h::()).unwrap(), - ext: ExtData::cast_check(ExtData::from_pk_h::()).unwrap(), + let pkh_node = Terminal::Check(Arc::new(Miniscript { + node: Terminal::PkH(String::from("")), + ty: Type::from_pk_h::(), + ext: types::extra_props::ExtData::from_pk_h::(), phantom: PhantomData, - }; + })); + let pkh_ms: Miniscript = Miniscript::from_ast(pkh_node).unwrap(); let expected_debug = "[B/nduesm]c:[K/nduesm]pk_h(\"\")"; let expected_display = "pkh()"; @@ -701,17 +698,13 @@ mod tests { assert_eq!(display, expected); } - let pkk_ms: Segwitv0Script = Miniscript { - node: Terminal::Check(Arc::new(Miniscript { - node: Terminal::PkK(pk), - ty: Type::from_pk_k::(), - ext: types::extra_props::ExtData::from_pk_k::(), - phantom: PhantomData, - })), - ty: Type::cast_check(Type::from_pk_k::()).unwrap(), - ext: ExtData::cast_check(ExtData::from_pk_k::()).unwrap(), + let pkk_node = Terminal::Check(Arc::new(Miniscript { + node: Terminal::PkK(pk), + ty: Type::from_pk_k::(), + ext: types::extra_props::ExtData::from_pk_k::(), phantom: PhantomData, - }; + })); + let pkk_ms: Segwitv0Script = Miniscript::from_ast(pkk_node).unwrap(); script_rtt( pkk_ms, diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs index a1b7dd676..5509bc8e7 100644 --- a/src/policy/compiler.rs +++ b/src/policy/compiler.rs @@ -7,7 +7,6 @@ //! use core::convert::From; -use core::marker::PhantomData; use core::{cmp, f64, fmt, hash, mem}; #[cfg(feature = "std")] use std::error; @@ -496,12 +495,7 @@ impl AstElemExt { let ext = types::ExtData::type_check(&ast, |_| None)?; let comp_ext_data = CompilerExtData::type_check(&ast, lookup_ext)?; Ok(AstElemExt { - ms: Arc::new(Miniscript { - ty, - ext, - node: ast, - phantom: PhantomData, - }), + ms: Arc::new(Miniscript::from_components_unchecked(ast, ty, ext)), comp_ext_data, }) } @@ -524,12 +518,7 @@ impl AstElemExt { let ext = types::ExtData::type_check(&ast, |_| None)?; let comp_ext_data = CompilerExtData::type_check(&ast, lookup_ext)?; Ok(AstElemExt { - ms: Arc::new(Miniscript { - ty, - ext, - node: ast, - phantom: PhantomData, - }), + ms: Arc::new(Miniscript::from_components_unchecked(ast, ty, ext)), comp_ext_data, }) } @@ -547,12 +536,11 @@ struct Cast { impl Cast { fn cast(&self, ast: &AstElemExt) -> Result, ErrorKind> { Ok(AstElemExt { - ms: Arc::new(Miniscript { - ty: (self.ast_type)(ast.ms.ty)?, - ext: (self.ext_data)(ast.ms.ext)?, - node: (self.node)(Arc::clone(&ast.ms)), - phantom: PhantomData, - }), + ms: Arc::new(Miniscript::from_components_unchecked( + (self.node)(Arc::clone(&ast.ms)), + (self.ast_type)(ast.ms.ty)?, + (self.ext_data)(ast.ms.ext)?, + )), comp_ext_data: (self.comp_ext_data)(ast.comp_ext_data)?, }) } From ee8de9a0ab85f05e07d23490919cca96298dae1e Mon Sep 17 00:00:00 2001 From: sanket1729 Date: Fri, 2 Dec 2022 01:30:59 -0800 Subject: [PATCH 2/4] Cleanly check validity rules for DescriptorKeys Each context has slightly different rules on what Pks are allowed in descriptors Legacy/Bare does not allow x_only keys SegwitV0 does not allow uncompressed keys and x_only keys Tapscript does not allow uncompressed keys Note that String types are allowed everywhere --- src/descriptor/bare.rs | 14 +++-- src/descriptor/mod.rs | 56 +++++++++++++++++- src/descriptor/segwitv0.rs | 11 ++-- src/descriptor/tr.rs | 16 ++--- src/miniscript/context.rs | 118 ++++++++++++++++++++++--------------- src/psbt/finalizer.rs | 2 +- 6 files changed, 145 insertions(+), 72 deletions(-) diff --git a/src/descriptor/bare.rs b/src/descriptor/bare.rs index acb57d7ab..03e488dd3 100644 --- a/src/descriptor/bare.rs +++ b/src/descriptor/bare.rs @@ -15,7 +15,7 @@ use bitcoin::{Address, Network, ScriptBuf}; use super::checksum::{self, verify_checksum}; use crate::expression::{self, FromTree}; -use crate::miniscript::context::ScriptContext; +use crate::miniscript::context::{ScriptContext, ScriptContextError}; use crate::policy::{semantic, Liftable}; use crate::prelude::*; use crate::util::{varint_len, witness_to_scriptsig}; @@ -205,9 +205,12 @@ pub struct Pkh { impl Pkh { /// Create a new Pkh descriptor - pub fn new(pk: Pk) -> Self { + pub fn new(pk: Pk) -> Result { // do the top-level checks - Self { pk } + match BareCtx::check_pk(&pk) { + Ok(()) => Ok(Pkh { pk }), + Err(e) => Err(e), + } } /// Get a reference to the inner key @@ -336,7 +339,7 @@ impl_from_tree!( if top.name == "pkh" && top.args.len() == 1 { Ok(Pkh::new(expression::terminal(&top.args[0], |pk| { Pk::from_str(pk) - })?)) + })?)?) } else { Err(Error::Unexpected(format!( "{}({} args) while parsing pkh descriptor", @@ -374,6 +377,7 @@ where where T: Translator, { - Ok(Pkh::new(t.pk(&self.pk)?)) + let res = Pkh::new(t.pk(&self.pk)?); + Ok(res.expect("Expect will be fixed in next commit")) } } diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index ede2edfaf..2b7ab5e70 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -177,8 +177,8 @@ impl Descriptor { } /// Create a new PkH descriptor - pub fn new_pkh(pk: Pk) -> Self { - Descriptor::Pkh(Pkh::new(pk)) + pub fn new_pkh(pk: Pk) -> Result { + Ok(Descriptor::Pkh(Pkh::new(pk)?)) } /// Create a new Wpkh descriptor @@ -1297,7 +1297,7 @@ mod tests { ); assert_eq!(bare.unsigned_script_sig(), bitcoin::ScriptBuf::new()); - let pkh = Descriptor::new_pkh(pk); + let pkh = Descriptor::new_pkh(pk).unwrap(); pkh.satisfy(&mut txin, &satisfier).expect("satisfaction"); assert_eq!( txin, @@ -2021,4 +2021,54 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))"; Descriptor::::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/<0;1>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/<0;1;2;3;4>/*)))").unwrap_err(); Descriptor::::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/<0;1;2;3>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/<0;1;2>/*)))").unwrap_err(); } + + #[test] + fn test_context_pks() { + let comp_key = bitcoin::PublicKey::from_str( + "02015e4cb53458bf813db8c79968e76e10d13ed6426a23fa71c2f41ba021c2a7ab", + ) + .unwrap(); + let x_only_key = bitcoin::key::XOnlyPublicKey::from_str( + "015e4cb53458bf813db8c79968e76e10d13ed6426a23fa71c2f41ba021c2a7ab", + ) + .unwrap(); + let uncomp_key = bitcoin::PublicKey::from_str("04015e4cb53458bf813db8c79968e76e10d13ed6426a23fa71c2f41ba021c2a7ab0d46021e9e69ef061eb25eab41ae206187b2b05e829559df59d78319bd9267b4").unwrap(); + + type Desc = Descriptor; + + // Legacy tests, x-only keys are not supported + Desc::from_str(&format!("sh(pk({}))", comp_key)).unwrap(); + Desc::from_str(&format!("sh(pk({}))", uncomp_key)).unwrap(); + Desc::from_str(&format!("sh(pk({}))", x_only_key)).unwrap_err(); + + // bare tests, x-only keys not supported + Desc::from_str(&format!("pk({})", comp_key)).unwrap(); + Desc::from_str(&format!("pk({})", uncomp_key)).unwrap(); + Desc::from_str(&format!("pk({})", x_only_key)).unwrap_err(); + + // pkh tests, x-only keys not supported + Desc::from_str(&format!("pkh({})", comp_key)).unwrap(); + Desc::from_str(&format!("pkh({})", uncomp_key)).unwrap(); + Desc::from_str(&format!("pkh({})", x_only_key)).unwrap_err(); + + // wpkh tests, uncompressed and x-only keys not supported + Desc::from_str(&format!("wpkh({})", comp_key)).unwrap(); + Desc::from_str(&format!("wpkh({})", uncomp_key)).unwrap_err(); + Desc::from_str(&format!("wpkh({})", x_only_key)).unwrap_err(); + + // Segwitv0 tests, uncompressed and x-only keys not supported + Desc::from_str(&format!("wsh(pk({}))", comp_key)).unwrap(); + Desc::from_str(&format!("wsh(pk({}))", uncomp_key)).unwrap_err(); + Desc::from_str(&format!("wsh(pk({}))", x_only_key)).unwrap_err(); + + // Tap tests, key path + Desc::from_str(&format!("tr({})", comp_key)).unwrap(); + Desc::from_str(&format!("tr({})", uncomp_key)).unwrap_err(); + Desc::from_str(&format!("tr({})", x_only_key)).unwrap(); + + // Tap tests, script path + Desc::from_str(&format!("tr({},pk({}))", x_only_key, comp_key)).unwrap(); + Desc::from_str(&format!("tr({},pk({}))", x_only_key, uncomp_key)).unwrap_err(); + Desc::from_str(&format!("tr({},pk({}))", x_only_key, x_only_key)).unwrap(); + } } diff --git a/src/descriptor/segwitv0.rs b/src/descriptor/segwitv0.rs index 974befa7e..6234603ba 100644 --- a/src/descriptor/segwitv0.rs +++ b/src/descriptor/segwitv0.rs @@ -303,14 +303,11 @@ pub struct Wpkh { impl Wpkh { /// Create a new Wpkh descriptor - pub fn new(pk: Pk) -> Result { + pub fn new(pk: Pk) -> Result { // do the top-level checks - if pk.is_uncompressed() { - Err(Error::ContextError(ScriptContextError::CompressedOnly( - pk.to_string(), - ))) - } else { - Ok(Self { pk }) + match Segwitv0::check_pk(&pk) { + Ok(_) => Ok(Wpkh { pk }), + Err(e) => Err(e), } } diff --git a/src/descriptor/tr.rs b/src/descriptor/tr.rs index c9f0cc6e8..b61e12376 100644 --- a/src/descriptor/tr.rs +++ b/src/descriptor/tr.rs @@ -19,7 +19,8 @@ use crate::policy::Liftable; use crate::prelude::*; use crate::util::{varint_len, witness_size}; use crate::{ - errstr, Error, ForEachKey, MiniscriptKey, Satisfier, Tap, ToPublicKey, TranslatePk, Translator, + errstr, Error, ForEachKey, MiniscriptKey, Satisfier, ScriptContext, Tap, ToPublicKey, + TranslatePk, Translator, }; /// A Taproot Tree representation. @@ -165,6 +166,7 @@ impl fmt::Debug for TapTree { impl Tr { /// Create a new [`Tr`] descriptor from internal key and [`TapTree`] pub fn new(internal_key: Pk, tree: Option>) -> Result { + Tap::check_pk(&internal_key)?; let nodes = tree.as_ref().map(|t| t.taptree_height()).unwrap_or(0); if nodes <= TAPROOT_CONTROL_MAX_NODE_COUNT { @@ -631,14 +633,12 @@ where where T: Translator, { - let translate_desc = Tr { - internal_key: translate.pk(&self.internal_key)?, - tree: match &self.tree { - Some(tree) => Some(tree.translate_helper(translate)?), - None => None, - }, - spend_info: Mutex::new(None), + let tree = match &self.tree { + Some(tree) => Some(tree.translate_helper(translate)?), + None => None, }; + let translate_desc = Tr::new(translate.pk(&self.internal_key)?, tree) + .expect("This will be removed in future"); Ok(translate_desc) } } diff --git a/src/miniscript/context.rs b/src/miniscript/context.rs index 42b0cff51..139b9ffd5 100644 --- a/src/miniscript/context.rs +++ b/src/miniscript/context.rs @@ -208,6 +208,12 @@ where Ok(()) } + /// Each context has slightly different rules on what Pks are allowed in descriptors + /// Legacy/Bare does not allow x_only keys + /// Segwit does not allow uncompressed keys and x_only keys + /// Tapscript does not allow uncompressed keys + fn check_pk(pk: &Pk) -> Result<(), ScriptContextError>; + /// Depending on script context, the size of a satifaction witness may slightly differ. fn max_satisfaction_size(ms: &Miniscript) -> Option; /// Depending on script Context, some of the Terminals might not @@ -355,6 +361,18 @@ impl ScriptContext for Legacy { } } + // Only compressed and uncompressed public keys are allowed in Legacy context + fn check_pk(pk: &Pk) -> Result<(), ScriptContextError> { + if pk.is_x_only_key() { + Err(ScriptContextError::XOnlyKeysNotAllowed( + pk.to_string(), + Self::name_str(), + )) + } else { + Ok(()) + } + } + fn check_witness(witness: &[Vec]) -> Result<(), ScriptContextError> { // In future, we could avoid by having a function to count only // len of script instead of converting it. @@ -372,31 +390,21 @@ impl ScriptContext for Legacy { } match ms.node { - Terminal::PkK(ref key) if key.is_x_only_key() => { - return Err(ScriptContextError::XOnlyKeysNotAllowed( - key.to_string(), - Self::name_str(), - )) - } + Terminal::PkK(ref pk) => Self::check_pk(pk), Terminal::Multi(_k, ref pks) => { if pks.len() > MAX_PUBKEYS_PER_MULTISIG { return Err(ScriptContextError::CheckMultiSigLimitExceeded); } for pk in pks.iter() { - if pk.is_x_only_key() { - return Err(ScriptContextError::XOnlyKeysNotAllowed( - pk.to_string(), - Self::name_str(), - )); - } + Self::check_pk(pk)?; } + Ok(()) } Terminal::MultiA(..) => { return Err(ScriptContextError::MultiANotAllowed); } - _ => {} + _ => Ok(()), } - Ok(()) } fn check_local_consensus_validity( @@ -460,6 +468,20 @@ impl ScriptContext for Segwitv0 { Ok(()) } + // No x-only keys or uncompressed keys in Segwitv0 context + fn check_pk(pk: &Pk) -> Result<(), ScriptContextError> { + if pk.is_uncompressed() { + Err(ScriptContextError::UncompressedKeysNotAllowed) + } else if pk.is_x_only_key() { + Err(ScriptContextError::XOnlyKeysNotAllowed( + pk.to_string(), + Self::name_str(), + )) + } else { + Ok(()) + } + } + fn check_witness(witness: &[Vec]) -> Result<(), ScriptContextError> { if witness.len() > MAX_STANDARD_P2WSH_STACK_ITEMS { return Err(ScriptContextError::MaxWitnessItemssExceeded { @@ -478,30 +500,13 @@ impl ScriptContext for Segwitv0 { } match ms.node { - Terminal::PkK(ref pk) => { - if pk.is_uncompressed() { - return Err(ScriptContextError::CompressedOnly(pk.to_string())); - } else if pk.is_x_only_key() { - return Err(ScriptContextError::XOnlyKeysNotAllowed( - pk.to_string(), - Self::name_str(), - )); - } - Ok(()) - } + Terminal::PkK(ref pk) => Self::check_pk(pk), Terminal::Multi(_k, ref pks) => { if pks.len() > MAX_PUBKEYS_PER_MULTISIG { return Err(ScriptContextError::CheckMultiSigLimitExceeded); } for pk in pks.iter() { - if pk.is_uncompressed() { - return Err(ScriptContextError::CompressedOnly(pk.to_string())); - } else if pk.is_x_only_key() { - return Err(ScriptContextError::XOnlyKeysNotAllowed( - pk.to_string(), - Self::name_str(), - )); - } + Self::check_pk(pk)?; } Ok(()) } @@ -582,6 +587,15 @@ impl ScriptContext for Tap { Ok(()) } + // No uncompressed keys in Tap context + fn check_pk(pk: &Pk) -> Result<(), ScriptContextError> { + if pk.is_uncompressed() { + Err(ScriptContextError::UncompressedKeysNotAllowed) + } else { + Ok(()) + } + } + fn check_witness(witness: &[Vec]) -> Result<(), ScriptContextError> { // Note that tapscript has a 1000 limit compared to 100 of segwitv0 if witness.len() > MAX_STACK_SIZE { @@ -606,9 +620,10 @@ impl ScriptContext for Tap { } match ms.node { - Terminal::PkK(ref pk) => { - if pk.is_uncompressed() { - return Err(ScriptContextError::UncompressedKeysNotAllowed); + Terminal::PkK(ref pk) => Self::check_pk(pk), + Terminal::MultiA(_, ref keys) => { + for pk in keys.iter() { + Self::check_pk(pk)?; } Ok(()) } @@ -693,6 +708,18 @@ impl ScriptContext for BareCtx { Ok(()) } + // No x-only keys in Bare context + fn check_pk(pk: &Pk) -> Result<(), ScriptContextError> { + if pk.is_x_only_key() { + Err(ScriptContextError::XOnlyKeysNotAllowed( + pk.to_string(), + Self::name_str(), + )) + } else { + Ok(()) + } + } + fn check_global_consensus_validity( ms: &Miniscript, ) -> Result<(), ScriptContextError> { @@ -700,23 +727,13 @@ impl ScriptContext for BareCtx { return Err(ScriptContextError::MaxWitnessScriptSizeExceeded); } match ms.node { - Terminal::PkK(ref key) if key.is_x_only_key() => { - return Err(ScriptContextError::XOnlyKeysNotAllowed( - key.to_string(), - Self::name_str(), - )) - } + Terminal::PkK(ref key) => Self::check_pk(key), Terminal::Multi(_k, ref pks) => { if pks.len() > MAX_PUBKEYS_PER_MULTISIG { return Err(ScriptContextError::CheckMultiSigLimitExceeded); } for pk in pks.iter() { - if pk.is_x_only_key() { - return Err(ScriptContextError::XOnlyKeysNotAllowed( - pk.to_string(), - Self::name_str(), - )); - } + Self::check_pk(pk)?; } Ok(()) } @@ -787,6 +804,11 @@ impl ScriptContext for NoChecks { Ok(()) } + // No checks in NoChecks + fn check_pk(_pk: &Pk) -> Result<(), ScriptContextError> { + Ok(()) + } + fn check_global_policy_validity( _ms: &Miniscript, ) -> Result<(), ScriptContextError> { diff --git a/src/psbt/finalizer.rs b/src/psbt/finalizer.rs index fd88d1bac..c0bf447c9 100644 --- a/src/psbt/finalizer.rs +++ b/src/psbt/finalizer.rs @@ -146,7 +146,7 @@ fn get_descriptor(psbt: &Psbt, index: usize) -> Result, In *script_pubkey == addr.script_pubkey() }); match partial_sig_contains_pk { - Some((pk, _sig)) => Ok(Descriptor::new_pkh(*pk)), + Some((pk, _sig)) => Descriptor::new_pkh(*pk).map_err(|e| InputError::from(e)), None => Err(InputError::MissingPubkey), } } else if script_pubkey.is_v0_p2wpkh() { From be07a9c9c5283d171ed86b677d601881c7032e89 Mon Sep 17 00:00:00 2001 From: sanket1729 Date: Fri, 2 Dec 2022 02:48:15 -0800 Subject: [PATCH 3/4] Update TranslatePk again This fixes of the long last standing bug in rust-miniscript that allowed creating unsound miniscript using the translate APIs --- bitcoind-tests/tests/setup/test_util.rs | 6 +- src/descriptor/bare.rs | 15 +++-- src/descriptor/mod.rs | 77 ++++++++----------------- src/descriptor/segwitv0.rs | 14 +++-- src/descriptor/sh.rs | 4 +- src/descriptor/sortedmulti.rs | 11 ++-- src/descriptor/tr.rs | 10 ++-- src/lib.rs | 53 ++++++++++++++++- src/miniscript/astelem.rs | 9 ++- src/miniscript/context.rs | 32 +++++++++- src/miniscript/mod.rs | 22 +++++-- src/policy/compiler.rs | 15 +++-- src/psbt/mod.rs | 7 ++- 13 files changed, 177 insertions(+), 98 deletions(-) diff --git a/bitcoind-tests/tests/setup/test_util.rs b/bitcoind-tests/tests/setup/test_util.rs index 7542d1c3d..3b77047a0 100644 --- a/bitcoind-tests/tests/setup/test_util.rs +++ b/bitcoind-tests/tests/setup/test_util.rs @@ -279,8 +279,10 @@ pub fn parse_test_desc( let desc = subs_hash_frag(desc, pubdata); let desc = Descriptor::::from_str(&desc)?; let mut translator = StrDescPubKeyTranslator(0, pubdata); - let desc: Result<_, ()> = desc.translate_pk(&mut translator); - Ok(desc.expect("Translate must succeed")) + let desc = desc + .translate_pk(&mut translator) + .expect("Translation failed"); + Ok(desc) } // substitute hash fragments in the string as the per rules diff --git a/src/descriptor/bare.rs b/src/descriptor/bare.rs index 03e488dd3..074a21b61 100644 --- a/src/descriptor/bare.rs +++ b/src/descriptor/bare.rs @@ -20,8 +20,8 @@ use crate::policy::{semantic, Liftable}; use crate::prelude::*; use crate::util::{varint_len, witness_to_scriptsig}; use crate::{ - BareCtx, Error, ForEachKey, Miniscript, MiniscriptKey, Satisfier, ToPublicKey, TranslatePk, - Translator, + BareCtx, Error, ForEachKey, Miniscript, MiniscriptKey, Satisfier, ToPublicKey, TranslateErr, + TranslatePk, Translator, }; /// Create a Bare Descriptor. That is descriptor that is @@ -188,11 +188,11 @@ where { type Output = Bare; - fn translate_pk(&self, t: &mut T) -> Result + fn translate_pk(&self, t: &mut T) -> Result, TranslateErr> where T: Translator, { - Ok(Bare::new(self.ms.translate_pk(t)?).expect("Translation cannot fail inside Bare")) + Ok(Bare::new(self.ms.translate_pk(t)?).map_err(TranslateErr::OuterError)?) } } @@ -373,11 +373,14 @@ where { type Output = Pkh; - fn translate_pk(&self, t: &mut T) -> Result + fn translate_pk(&self, t: &mut T) -> Result> where T: Translator, { let res = Pkh::new(t.pk(&self.pk)?); - Ok(res.expect("Expect will be fixed in next commit")) + match res { + Ok(pk) => Ok(pk), + Err(e) => Err(TranslateErr::OuterError(Error::from(e))), + } } } diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index 2b7ab5e70..f90838910 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -26,7 +26,7 @@ use crate::miniscript::{Legacy, Miniscript, Segwitv0}; use crate::prelude::*; use crate::{ expression, hash256, miniscript, BareCtx, Error, ForEachKey, MiniscriptKey, Satisfier, - ToPublicKey, TranslatePk, Translator, + ToPublicKey, TranslateErr, TranslatePk, Translator, }; mod bare; @@ -519,7 +519,7 @@ where type Output = Descriptor; /// Converts a descriptor using abstract keys to one using specific keys. - fn translate_pk(&self, t: &mut T) -> Result + fn translate_pk(&self, t: &mut T) -> Result> where T: Translator, { @@ -581,7 +581,10 @@ impl Descriptor { translate_hash_clone!(DescriptorPublicKey, DescriptorPublicKey, ConversionError); } - self.translate_pk(&mut Derivator(index)) + self.translate_pk(&mut Derivator(index)).map_err(|e| { + e.try_into_translator_err() + .expect("No Context errors while translating") + }) } #[deprecated(note = "use at_derivation_index instead")] @@ -694,9 +697,13 @@ impl Descriptor { } let descriptor = Descriptor::::from_str(s)?; - let descriptor = descriptor - .translate_pk(&mut keymap_pk) - .map_err(|e| Error::Unexpected(e.to_string()))?; + let descriptor = descriptor.translate_pk(&mut keymap_pk).map_err(|e| { + Error::Unexpected( + e.try_into_translator_err() + .expect("No Outer context errors") + .to_string(), + ) + })?; Ok((descriptor, keymap_pk.0)) } @@ -823,49 +830,16 @@ impl Descriptor { for (i, desc) in descriptors.iter_mut().enumerate() { let mut index_choser = IndexChoser(i); - *desc = desc.translate_pk(&mut index_choser)?; + *desc = desc.translate_pk(&mut index_choser).map_err(|e| { + e.try_into_translator_err() + .expect("No Context errors possible") + })?; } Ok(descriptors) } } -impl Descriptor { - /// Whether this descriptor is a multipath descriptor that contains any 2 multipath keys - /// with a different number of derivation paths. - /// Such a descriptor is invalid according to BIP389. - pub fn multipath_length_mismatch(&self) -> bool { - // (Ab)use `for_each_key` to record the number of derivation paths a multipath key has. - #[derive(PartialEq)] - enum MultipathLenChecker { - SinglePath, - MultipathLen(usize), - LenMismatch, - } - - let mut checker = MultipathLenChecker::SinglePath; - self.for_each_key(|key| { - match key.num_der_paths() { - 0 | 1 => {} - n => match checker { - MultipathLenChecker::SinglePath => { - checker = MultipathLenChecker::MultipathLen(n); - } - MultipathLenChecker::MultipathLen(len) => { - if len != n { - checker = MultipathLenChecker::LenMismatch; - } - } - MultipathLenChecker::LenMismatch => {} - }, - } - true - }); - - checker == MultipathLenChecker::LenMismatch - } -} - impl Descriptor { /// Convert all the public keys in the descriptor to [`bitcoin::PublicKey`] by deriving them or /// otherwise converting them. All [`bitcoin::secp256k1::XOnlyPublicKey`]s are converted to by adding a @@ -909,8 +883,14 @@ impl Descriptor { translate_hash_clone!(DefiniteDescriptorKey, bitcoin::PublicKey, ConversionError); } - let derived = self.translate_pk(&mut Derivator(secp))?; - Ok(derived) + let derived = self.translate_pk(&mut Derivator(secp)); + match derived { + Ok(derived) => Ok(derived), + Err(e) => match e.try_into_translator_err() { + Ok(e) => Err(e), + Err(_) => unreachable!("No Context errors when deriving keys"), + }, + } } } @@ -944,10 +924,6 @@ impl_from_str!( expression::FromTree::from_tree(&top) }?; - if desc.multipath_length_mismatch() { - return Err(Error::MultipathDescLenMismatch); - } - Ok(desc) } ); @@ -1992,7 +1968,6 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))"; // We can parse a multipath descriptors, and make it into separate single-path descriptors. let desc = Descriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/<7';8h;20>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/4567/<0;1;987>/*)))").unwrap(); assert!(desc.is_multipath()); - assert!(!desc.multipath_length_mismatch()); assert_eq!(desc.into_single_descriptors().unwrap(), vec![ Descriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/7'/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/4567/0/*)))").unwrap(), Descriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/8h/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/4567/1/*)))").unwrap(), @@ -2002,7 +1977,6 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))"; // Even if only one of the keys is multipath. let desc = Descriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/<0;1>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/4567/*)))").unwrap(); assert!(desc.is_multipath()); - assert!(!desc.multipath_length_mismatch()); assert_eq!(desc.into_single_descriptors().unwrap(), vec![ Descriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/0/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/4567/*)))").unwrap(), Descriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/1/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/4567/*)))").unwrap(), @@ -2011,7 +1985,6 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))"; // We can detect regular single-path descriptors. let notmulti_desc = Descriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/4567/*)))").unwrap(); assert!(!notmulti_desc.is_multipath()); - assert!(!notmulti_desc.multipath_length_mismatch()); assert_eq!( notmulti_desc.clone().into_single_descriptors().unwrap(), vec![notmulti_desc] diff --git a/src/descriptor/segwitv0.rs b/src/descriptor/segwitv0.rs index 6234603ba..6b50d5559 100644 --- a/src/descriptor/segwitv0.rs +++ b/src/descriptor/segwitv0.rs @@ -18,8 +18,8 @@ use crate::policy::{semantic, Liftable}; use crate::prelude::*; use crate::util::varint_len; use crate::{ - Error, ForEachKey, Miniscript, MiniscriptKey, Satisfier, Segwitv0, ToPublicKey, TranslatePk, - Translator, + Error, ForEachKey, Miniscript, MiniscriptKey, Satisfier, Segwitv0, ToPublicKey, TranslateErr, + TranslatePk, Translator, }; /// A Segwitv0 wsh descriptor #[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)] @@ -282,7 +282,7 @@ where { type Output = Wsh; - fn translate_pk(&self, t: &mut T) -> Result + fn translate_pk(&self, t: &mut T) -> Result> where T: Translator, { @@ -480,10 +480,14 @@ where { type Output = Wpkh; - fn translate_pk(&self, t: &mut T) -> Result + fn translate_pk(&self, t: &mut T) -> Result> where T: Translator, { - Ok(Wpkh::new(t.pk(&self.pk)?).expect("Uncompressed keys in Wpkh")) + let res = Wpkh::new(t.pk(&self.pk)?); + match res { + Ok(pk) => Ok(pk), + Err(e) => Err(TranslateErr::OuterError(Error::from(e))), + } } } diff --git a/src/descriptor/sh.rs b/src/descriptor/sh.rs index 94e02e4d8..a421058a7 100644 --- a/src/descriptor/sh.rs +++ b/src/descriptor/sh.rs @@ -23,7 +23,7 @@ use crate::prelude::*; use crate::util::{varint_len, witness_to_scriptsig}; use crate::{ push_opcode_size, Error, ForEachKey, Legacy, Miniscript, MiniscriptKey, Satisfier, Segwitv0, - ToPublicKey, TranslatePk, Translator, + ToPublicKey, TranslateErr, TranslatePk, Translator, }; /// A Legacy p2sh Descriptor @@ -437,7 +437,7 @@ where { type Output = Sh; - fn translate_pk(&self, t: &mut T) -> Result + fn translate_pk(&self, t: &mut T) -> Result> where T: Translator, { diff --git a/src/descriptor/sortedmulti.rs b/src/descriptor/sortedmulti.rs index a6304f3df..96a2e43d0 100644 --- a/src/descriptor/sortedmulti.rs +++ b/src/descriptor/sortedmulti.rs @@ -18,7 +18,7 @@ use crate::miniscript::limits::MAX_PUBKEYS_PER_MULTISIG; use crate::prelude::*; use crate::{ errstr, expression, miniscript, policy, script_num_size, Error, ForEachKey, Miniscript, - MiniscriptKey, Satisfier, ToPublicKey, Translator, + MiniscriptKey, Satisfier, ToPublicKey, TranslateErr, Translator, }; /// Contents of a "sortedmulti" descriptor @@ -87,17 +87,14 @@ impl SortedMultiVec { pub fn translate_pk( &self, t: &mut T, - ) -> Result, FuncError> + ) -> Result, TranslateErr> where T: Translator, Q: MiniscriptKey, { let pks: Result, _> = self.pks.iter().map(|pk| t.pk(pk)).collect(); - Ok(SortedMultiVec { - k: self.k, - pks: pks?, - phantom: PhantomData, - }) + let res = SortedMultiVec::new(self.k, pks?).map_err(TranslateErr::OuterError)?; + Ok(res) } } diff --git a/src/descriptor/tr.rs b/src/descriptor/tr.rs index b61e12376..4e0b6a475 100644 --- a/src/descriptor/tr.rs +++ b/src/descriptor/tr.rs @@ -20,7 +20,7 @@ use crate::prelude::*; use crate::util::{varint_len, witness_size}; use crate::{ errstr, Error, ForEachKey, MiniscriptKey, Satisfier, ScriptContext, Tap, ToPublicKey, - TranslatePk, Translator, + TranslateErr, TranslatePk, Translator, }; /// A Taproot Tree representation. @@ -129,9 +129,9 @@ impl TapTree { } // Helper function to translate keys - fn translate_helper(&self, t: &mut T) -> Result, Error> + fn translate_helper(&self, t: &mut T) -> Result, TranslateErr> where - T: Translator, + T: Translator, Q: MiniscriptKey, { let frag = match self { @@ -629,7 +629,7 @@ where { type Output = Tr; - fn translate_pk(&self, translate: &mut T) -> Result + fn translate_pk(&self, translate: &mut T) -> Result> where T: Translator, { @@ -638,7 +638,7 @@ where None => None, }; let translate_desc = Tr::new(translate.pk(&self.internal_key)?, tree) - .expect("This will be removed in future"); + .map_err(|e| TranslateErr::OuterError(e))?; Ok(translate_desc) } } diff --git a/src/lib.rs b/src/lib.rs index 07a37a311..28475bf1c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -368,6 +368,57 @@ where fn hash160(&mut self, hash160: &P::Hash160) -> Result; } +/// An enum for representing translation errors +pub enum TranslateErr { + /// Error inside in the underlying key translation + TranslatorErr(E), + /// Error in the final translated structure. In some cases, the translated + /// structure might not be valid under the given context. For example, translating + /// from string keys to x-only keys in wsh descriptors. + OuterError(Error), +} + +impl TranslateErr { + /// Enum used to capture errors from the [`Translator`] trait as well as + /// context errors from the translated structure. + /// The errors occurred in translation are captured in the [`TranslateErr::TranslatorErr`] + /// while the errors in the translated structure are captured in the [`TranslateErr::OuterError`] + /// + /// As of taproot upgrade: The following rules apply to the translation of descriptors: + /// - Legacy/Bare does not allow x_only keys + /// - SegwitV0 does not allow uncompressed keys and x_only keys + /// - Tapscript does not allow uncompressed keys + /// - Translating into multi-path descriptors should have same number of path + /// for all the keys in the descriptor + /// + /// # Errors + /// + /// This function will return an error if the Error is OutError. + pub fn try_into_translator_err(self) -> Result { + if let Self::TranslatorErr(v) = self { + Ok(v) + } else { + Err(self) + } + } +} + +impl From for TranslateErr { + fn from(v: E) -> Self { + Self::TranslatorErr(v) + } +} + +// Required for unwrap +impl fmt::Debug for TranslateErr { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Self::TranslatorErr(e) => write!(f, "TranslatorErr({:?})", e), + Self::OuterError(e) => write!(f, "OuterError({:?})", e), + } + } +} + /// Converts a descriptor using abstract keys to one using specific keys. Uses translator `t` to do /// the actual translation function calls. pub trait TranslatePk @@ -380,7 +431,7 @@ where /// Translates a struct from one generic to another where the translations /// for Pk are provided by the given [`Translator`]. - fn translate_pk(&self, translator: &mut T) -> Result + fn translate_pk(&self, translator: &mut T) -> Result> where T: Translator; } diff --git a/src/miniscript/astelem.rs b/src/miniscript/astelem.rs index aabb0629a..d0388554c 100644 --- a/src/miniscript/astelem.rs +++ b/src/miniscript/astelem.rs @@ -22,7 +22,7 @@ use crate::prelude::*; use crate::util::MsKeyBuilder; use crate::{ errstr, expression, script_num_size, AbsLockTime, Error, ForEachKey, Miniscript, MiniscriptKey, - Terminal, ToPublicKey, TranslatePk, Translator, + Terminal, ToPublicKey, TranslateErr, TranslatePk, Translator, }; impl Terminal { @@ -55,7 +55,7 @@ where type Output = Terminal; /// Converts an AST element with one public key type to one of another public key type. - fn translate_pk(&self, translate: &mut T) -> Result + fn translate_pk(&self, translate: &mut T) -> Result> where T: Translator, { @@ -102,7 +102,10 @@ impl Terminal { } } - pub(super) fn real_translate_pk(&self, t: &mut T) -> Result, E> + pub(super) fn real_translate_pk( + &self, + t: &mut T, + ) -> Result, TranslateErr> where Q: MiniscriptKey, CtxQ: ScriptContext, diff --git a/src/miniscript/context.rs b/src/miniscript/context.rs index 139b9ffd5..3d820de96 100644 --- a/src/miniscript/context.rs +++ b/src/miniscript/context.rs @@ -17,7 +17,7 @@ use crate::miniscript::limits::{ use crate::miniscript::types; use crate::prelude::*; use crate::util::witness_to_scriptsig; -use crate::{hash256, Error, Miniscript, MiniscriptKey, Terminal}; +use crate::{hash256, Error, ForEachKey, Miniscript, MiniscriptKey, Terminal}; /// Error for Script Context #[derive(Clone, PartialEq, Eq, Debug)] @@ -294,6 +294,36 @@ where if ms.ty.corr.base != types::Base::B { return Err(Error::NonTopLevel(format!("{:?}", ms))); } + // (Ab)use `for_each_key` to record the number of derivation paths a multipath key has. + #[derive(PartialEq)] + enum MultipathLenChecker { + SinglePath, + MultipathLen(usize), + LenMismatch, + } + + let mut checker = MultipathLenChecker::SinglePath; + ms.for_each_key(|key| { + match key.num_der_paths() { + 0 | 1 => {} + n => match checker { + MultipathLenChecker::SinglePath => { + checker = MultipathLenChecker::MultipathLen(n); + } + MultipathLenChecker::MultipathLen(len) => { + if len != n { + checker = MultipathLenChecker::LenMismatch; + } + } + MultipathLenChecker::LenMismatch => {} + }, + } + true + }); + + if checker == MultipathLenChecker::LenMismatch { + return Err(Error::MultipathDescLenMismatch); + } Ok(()) } diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs index dc774fd5d..c73a7bd4e 100644 --- a/src/miniscript/mod.rs +++ b/src/miniscript/mod.rs @@ -22,6 +22,7 @@ use bitcoin::taproot::{LeafVersion, TapLeafHash}; use self::analyzable::ExtParams; pub use self::context::{BareCtx, Legacy, Segwitv0, Tap}; use crate::prelude::*; +use crate::TranslateErr; pub mod analyzable; pub mod astelem; @@ -109,12 +110,14 @@ impl Miniscript { /// `AstElem` fragment. Dependent on display and clone because of Error /// Display code of type_check. pub fn from_ast(t: Terminal) -> Result, Error> { - Ok(Miniscript { + let res = Miniscript { ty: Type::type_check(&t, |_| None)?, ext: ExtData::type_check(&t, |_| None)?, node: t, phantom: PhantomData, - }) + }; + Ctx::check_global_consensus_validity(&res)?; + Ok(res) } /// Create a new `Miniscript` from a `Terminal` node and a `Type` annotation @@ -306,7 +309,7 @@ where /// Translates a struct from one generic to another where the translation /// for Pk is provided by [`Translator`] - fn translate_pk(&self, translate: &mut T) -> Result + fn translate_pk(&self, translate: &mut T) -> Result> where T: Translator, { @@ -322,14 +325,14 @@ impl Miniscript { pub(super) fn real_translate_pk( &self, t: &mut T, - ) -> Result, FuncError> + ) -> Result, TranslateErr> where Q: MiniscriptKey, CtxQ: ScriptContext, T: Translator, { let inner = self.node.real_translate_pk(t)?; - Ok(Miniscript::from_ast(inner).expect("This will be removed in the next commit")) + Miniscript::from_ast(inner).map_err(TranslateErr::OuterError) } } @@ -1130,4 +1133,13 @@ mod tests { let ms_str = TapMs::from_str_insane("j:multi_a(1,A,B,C)"); assert!(ms_str.is_err()); } + + #[test] + fn translate_tests() { + let ms = Miniscript::::from_str("pk(A)").unwrap(); + let mut t = StrKeyTranslator::new(); + let uncompressed = bitcoin::PublicKey::from_str("0400232a2acfc9b43fa89f1b4f608fde335d330d7114f70ea42bfb4a41db368a3e3be6934a4097dd25728438ef73debb1f2ffdb07fec0f18049df13bdc5285dc5b").unwrap(); + t.pk_map.insert(String::from("A"), uncompressed); + ms.translate_pk(&mut t).unwrap_err(); + } } diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs index 5509bc8e7..9b29ee6ba 100644 --- a/src/policy/compiler.rs +++ b/src/policy/compiler.rs @@ -986,15 +986,14 @@ where } let ast = Terminal::Thresh(k, sub_ast); - let ast_ext = AstElemExt { - ms: Arc::new( - Miniscript::from_ast(ast) + if let Ok(ms) = Miniscript::from_ast(ast) { + let ast_ext = AstElemExt { + ms: Arc::new(ms), + comp_ext_data: CompilerExtData::threshold(k, n, |i| Ok(sub_ext_data[i])) .expect("threshold subs, which we just compiled, typeck"), - ), - comp_ext_data: CompilerExtData::threshold(k, n, |i| Ok(sub_ext_data[i])) - .expect("threshold subs, which we just compiled, typeck"), - }; - insert_wrap!(ast_ext); + }; + insert_wrap!(ast_ext); + } let key_vec: Vec = subs .iter() diff --git a/src/psbt/mod.rs b/src/psbt/mod.rs index bb5bd4a3b..af6c0a275 100644 --- a/src/psbt/mod.rs +++ b/src/psbt/mod.rs @@ -1236,7 +1236,12 @@ fn update_item_with_descriptor_helper( derived } else { let mut bip32_derivation = KeySourceLookUp(BTreeMap::new(), Secp256k1::verification_only()); - let derived = descriptor.translate_pk(&mut bip32_derivation)?; + let derived = descriptor + .translate_pk(&mut bip32_derivation) + .map_err(|e| { + e.try_into_translator_err() + .expect("No Outer Context errors in translations") + })?; if let Some(check_script) = check_script { if check_script != &derived.script_pubkey() { From b90cdda3136636485e69bdea4699563f7f74401b Mon Sep 17 00:00:00 2001 From: sanket1729 Date: Mon, 22 May 2023 13:45:42 -0700 Subject: [PATCH 4/4] Change try_into_translator_err to expect_translator_err --- src/descriptor/mod.rs | 21 +++++++-------------- src/lib.rs | 10 +++++----- src/psbt/mod.rs | 5 +---- 3 files changed, 13 insertions(+), 23 deletions(-) diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index f90838910..99d89f381 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -581,10 +581,8 @@ impl Descriptor { translate_hash_clone!(DescriptorPublicKey, DescriptorPublicKey, ConversionError); } - self.translate_pk(&mut Derivator(index)).map_err(|e| { - e.try_into_translator_err() - .expect("No Context errors while translating") - }) + self.translate_pk(&mut Derivator(index)) + .map_err(|e| e.expect_translator_err("No Context errors while translating")) } #[deprecated(note = "use at_derivation_index instead")] @@ -699,8 +697,7 @@ impl Descriptor { let descriptor = Descriptor::::from_str(s)?; let descriptor = descriptor.translate_pk(&mut keymap_pk).map_err(|e| { Error::Unexpected( - e.try_into_translator_err() - .expect("No Outer context errors") + e.expect_translator_err("No Outer context errors") .to_string(), ) })?; @@ -830,10 +827,9 @@ impl Descriptor { for (i, desc) in descriptors.iter_mut().enumerate() { let mut index_choser = IndexChoser(i); - *desc = desc.translate_pk(&mut index_choser).map_err(|e| { - e.try_into_translator_err() - .expect("No Context errors possible") - })?; + *desc = desc + .translate_pk(&mut index_choser) + .map_err(|e| e.expect_translator_err("No Context errors possible"))?; } Ok(descriptors) @@ -886,10 +882,7 @@ impl Descriptor { let derived = self.translate_pk(&mut Derivator(secp)); match derived { Ok(derived) => Ok(derived), - Err(e) => match e.try_into_translator_err() { - Ok(e) => Err(e), - Err(_) => unreachable!("No Context errors when deriving keys"), - }, + Err(e) => Err(e.expect_translator_err("No Context errors when deriving keys")), } } } diff --git a/src/lib.rs b/src/lib.rs index 28475bf1c..69a551018 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -391,14 +391,14 @@ impl TranslateErr { /// - Translating into multi-path descriptors should have same number of path /// for all the keys in the descriptor /// - /// # Errors + /// # Panics /// - /// This function will return an error if the Error is OutError. - pub fn try_into_translator_err(self) -> Result { + /// This function will panic if the Error is OutError. + pub fn expect_translator_err(self, msg: &str) -> E { if let Self::TranslatorErr(v) = self { - Ok(v) + v } else { - Err(self) + panic!("{}", msg) } } } diff --git a/src/psbt/mod.rs b/src/psbt/mod.rs index af6c0a275..668669fd5 100644 --- a/src/psbt/mod.rs +++ b/src/psbt/mod.rs @@ -1238,10 +1238,7 @@ fn update_item_with_descriptor_helper( let mut bip32_derivation = KeySourceLookUp(BTreeMap::new(), Secp256k1::verification_only()); let derived = descriptor .translate_pk(&mut bip32_derivation) - .map_err(|e| { - e.try_into_translator_err() - .expect("No Outer Context errors in translations") - })?; + .map_err(|e| e.expect_translator_err("No Outer Context errors in translations"))?; if let Some(check_script) = check_script { if check_script != &derived.script_pubkey() {