Address review feedback

sanket1729 · sanket1729 · commit 8fce86d2b44f · 2021-10-05T00:56:18.000-07:00
diff --git a/src/descriptor/segwitv0.rs b/src/descriptor/segwitv0.rs
@@ -293,7 +293,9 @@ impl<Pk: MiniscriptKey> Wpkh<Pk> {
     pub fn new(pk: Pk) -> Result<Self, Error> {
         // do the top-level checks
         if pk.is_uncompressed() {
-            Err(Error::ContextError(ScriptContextError::CompressedOnly))
+            Err(Error::ContextError(ScriptContextError::CompressedOnly(
+                pk.to_string(),
+            )))
         } else {
             Ok(Self { pk: pk })
         }
@@ -376,7 +378,9 @@ where
 impl<Pk: MiniscriptKey> DescriptorTrait<Pk> for Wpkh<Pk> {
     fn sanity_check(&self) -> Result<(), Error> {
         if self.pk.is_uncompressed() {
-            Err(Error::ContextError(ScriptContextError::CompressedOnly))
+            Err(Error::ContextError(ScriptContextError::CompressedOnly(
+                self.pk.to_string(),
+            )))
         } else {
             Ok(())
         }
diff --git a/src/miniscript/context.rs b/src/miniscript/context.rs
@@ -29,7 +29,7 @@ use super::decode::ParseableKey;
 use {Miniscript, MiniscriptKey, Terminal};
 
 /// Error for Script Context
-#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+#[derive(Clone, PartialEq, Eq, Debug)]
 pub enum ScriptContextError {
     /// Script Context does not permit PkH for non-malleability
     /// It is not possible to estimate the pubkey size at the creation
@@ -43,13 +43,13 @@ pub enum ScriptContextError {
     MalleableDupIf,
     /// Only Compressed keys allowed under current descriptor
     /// Segwitv0 fragments do not allow uncompressed pubkeys
-    CompressedOnly,
+    CompressedOnly(String),
     /// Tapscript descriptors cannot contain uncompressed keys
     /// Tap context can contain compressed or xonly
     UncompressedKeysNotAllowed,
     /// At least one satisfaction path in the Miniscript fragment has more than
     /// `MAX_STANDARD_P2WSH_STACK_ITEMS` (100) witness elements.
-    MaxWitnessItemssExceeded,
+    MaxWitnessItemssExceeded { actual: usize, limit: usize },
     /// At least one satisfaction path in the Miniscript fragment contains more
     /// than `MAX_OPS_PER_SCRIPT`(201) opcodes.
     MaxOpCountExceeded,
@@ -66,7 +66,7 @@ pub enum ScriptContextError {
     /// No Multi Node in Taproot context
     TaprootMultiDisabled,
     /// Stack size exceeded in script execution
-    StackSizeLimitExceeded,
+    StackSizeLimitExceeded { actual: usize, limit: usize },
 }
 
 impl fmt::Display for ScriptContextError {
@@ -77,23 +77,24 @@ impl fmt::Display for ScriptContextError {
             ScriptContextError::MalleableDupIf => {
                 write!(f, "DupIf is malleable under Legacy rules")
             }
-            ScriptContextError::CompressedOnly => {
+            ScriptContextError::CompressedOnly(ref pk) => {
                 write!(
                     f,
-                    "Only Compressed pubkeys are allowed in segwit context. X-only and uncompressed keys are forbidden"
+                    "Only Compressed pubkeys are allowed in segwit context. Found {}",
+                    pk
                 )
             }
             ScriptContextError::UncompressedKeysNotAllowed => {
                 write!(
                     f,
-                    "Only x-only keys are allowed in tapscript checksig. \
-                    Compressed keys maybe specified in descriptor."
+                    "uncompressed keys cannot be used in Taproot descriptors."
                 )
             }
-            ScriptContextError::MaxWitnessItemssExceeded => write!(
+            ScriptContextError::MaxWitnessItemssExceeded { actual, limit } => write!(
                 f,
-                "At least one spending path in the Miniscript fragment has more \
-                 witness items than MAX_STANDARD_P2WSH_STACK_ITEMS.",
+                "At least one spending path in the Miniscript fragment has {} more \
+                 witness items than limit {}.",
+                actual, limit
             ),
             ScriptContextError::MaxOpCountExceeded => write!(
                 f,
@@ -122,12 +123,13 @@ impl fmt::Display for ScriptContextError {
                 )
             }
             ScriptContextError::TaprootMultiDisabled => {
-                write!(f, "No Multi node in taproot context")
+                write!(f, "Invalid use of Multi node in taproot context")
             }
-            ScriptContextError::StackSizeLimitExceeded => {
+            ScriptContextError::StackSizeLimitExceeded { actual, limit } => {
                 write!(
                     f,
-                    "Stack limit can exceed in atleast one script path during script execution"
+                    "Stack limit {} can exceed the allowed limit {} in at least one script path during script execution",
+                    actual, limit
                 )
             }
         }
@@ -293,7 +295,7 @@ where
     fn pk_len<Pk: MiniscriptKey>(pk: &Pk) -> usize;
 
     /// Local helper function to display error messages with context
-    fn to_str() -> &'static str;
+    fn name_str() -> &'static str;
 }
 
 /// Legacy ScriptContext
@@ -378,7 +380,7 @@ impl ScriptContext for Legacy {
         }
     }
 
-    fn to_str() -> &'static str {
+    fn name_str() -> &'static str {
         "Legacy/p2sh"
     }
 }
@@ -399,7 +401,10 @@ impl ScriptContext for Segwitv0 {
         witness: &[Vec<u8>],
     ) -> Result<(), ScriptContextError> {
         if witness.len() > MAX_STANDARD_P2WSH_STACK_ITEMS {
-            return Err(ScriptContextError::MaxWitnessItemssExceeded);
+            return Err(ScriptContextError::MaxWitnessItemssExceeded {
+                actual: witness.len(),
+                limit: MAX_STANDARD_P2WSH_STACK_ITEMS,
+            });
         }
         Ok(())
     }
@@ -414,13 +419,15 @@ impl ScriptContext for Segwitv0 {
         match ms.node {
             Terminal::PkK(ref pk) => {
                 if pk.is_uncompressed() {
-                    return Err(ScriptContextError::CompressedOnly);
+                    return Err(ScriptContextError::CompressedOnly(pk.to_string()));
                 }
                 Ok(())
             }
             Terminal::Multi(_k, ref pks) => {
-                if pks.iter().any(|pk| pk.is_uncompressed()) {
-                    return Err(ScriptContextError::CompressedOnly);
+                for pk in pks.iter() {
+                    if pk.is_uncompressed() {
+                        return Err(ScriptContextError::CompressedOnly(pk.to_string()));
+                    }
                 }
                 Ok(())
             }
@@ -459,7 +466,10 @@ impl ScriptContext for Segwitv0 {
             // No possible satisfactions
             Err(_e) => Err(ScriptContextError::ImpossibleSatisfaction),
             Ok(max_witness_items) if max_witness_items > MAX_STANDARD_P2WSH_STACK_ITEMS => {
-                Err(ScriptContextError::MaxWitnessItemssExceeded)
+                Err(ScriptContextError::MaxWitnessItemssExceeded {
+                    actual: max_witness_items,
+                    limit: MAX_STANDARD_P2WSH_STACK_ITEMS,
+                })
             }
             _ => Ok(()),
         }
@@ -476,7 +486,7 @@ impl ScriptContext for Segwitv0 {
         34
     }
 
-    fn to_str() -> &'static str {
+    fn name_str() -> &'static str {
         "Segwitv0"
     }
 }
@@ -498,8 +508,12 @@ impl ScriptContext for Tap {
     fn check_witness<Pk: MiniscriptKey, Ctx: ScriptContext>(
         witness: &[Vec<u8>],
     ) -> Result<(), ScriptContextError> {
+        // Note that tapscript has a 1000 limit compared to 100 of segwitv0
         if witness.len() > MAX_STACK_SIZE {
-            return Err(ScriptContextError::MaxWitnessItemssExceeded);
+            return Err(ScriptContextError::MaxWitnessItemssExceeded {
+                actual: witness.len(),
+                limit: MAX_STACK_SIZE,
+            });
         }
         Ok(())
     }
@@ -526,8 +540,6 @@ impl ScriptContext for Tap {
             Terminal::Multi(..) => {
                 return Err(ScriptContextError::TaprootMultiDisabled);
             }
-            // What happens to the Multi node in tapscript? Do we use it, create
-            // a new fragment?
             _ => Ok(()),
         }
     }
@@ -549,7 +561,10 @@ impl ScriptContext for Tap {
             ms.ext.stack_elem_count_sat,
         ) {
             if s + h > MAX_STACK_SIZE {
-                return Err(ScriptContextError::StackSizeLimitExceeded);
+                return Err(ScriptContextError::StackSizeLimitExceeded {
+                    actual: s + h,
+                    limit: MAX_STACK_SIZE,
+                });
             }
         }
         Ok(())
@@ -583,7 +598,7 @@ impl ScriptContext for Tap {
         33
     }
 
-    fn to_str() -> &'static str {
+    fn name_str() -> &'static str {
         "TapscriptCtx"
     }
 }
@@ -657,7 +672,7 @@ impl ScriptContext for BareCtx {
         }
     }
 
-    fn to_str() -> &'static str {
+    fn name_str() -> &'static str {
         "BareCtx"
     }
 }
@@ -712,9 +727,9 @@ impl ScriptContext for NoChecks {
         panic!("Tried to compute a pk len bound on a no-checks miniscript")
     }
 
-    fn to_str() -> &'static str {
+    fn name_str() -> &'static str {
         // Internally used code
-        "NochecksCtx"
+        "Nochecks"
     }
 }
 
diff --git a/src/miniscript/decode.rs b/src/miniscript/decode.rs
@@ -290,19 +290,19 @@ pub fn parse<Ctx: ScriptContext>(
                     // pubkey
                     Tk::Bytes33(pk) => {
                         let ret = Ctx::Key::from_slice(pk)
-                            .map_err(|e| Error::PubKeyCtxError(e, Ctx::to_str()))?;
+                            .map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?;
                         term.reduce0(Terminal::PkK(ret))?
                     },
                     Tk::Bytes65(pk) => {
                         let ret = Ctx::Key::from_slice(pk)
-                            .map_err(|e| Error::PubKeyCtxError(e, Ctx::to_str()))?;
+                            .map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?;
                         term.reduce0(Terminal::PkK(ret))?
                     },
                     // Note this does not collide with hash32 because they always followed by equal
                     // and would be parsed in different branch. If we get a naked Bytes32, it must be
                     // a x-only key
                     Tk::Bytes32(pk) => {
-                        let ret = Ctx::Key::from_slice(pk).map_err(|e| Error::PubKeyCtxError(e, Ctx::to_str()))?;
+                        let ret = Ctx::Key::from_slice(pk).map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?;
                         term.reduce0(Terminal::PkK(ret))?
                     },
                     // checksig
@@ -464,9 +464,9 @@ pub fn parse<Ctx: ScriptContext>(
                             match_token!(
                                 tokens,
                                 Tk::Bytes33(pk) => keys.push(<Ctx::Key>::from_slice(pk)
-                                    .map_err(|e| Error::PubKeyCtxError(e, Ctx::to_str()))?),
+                                    .map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?),
                                 Tk::Bytes65(pk) => keys.push(<Ctx::Key>::from_slice(pk)
-                                    .map_err(|e| Error::PubKeyCtxError(e, Ctx::to_str()))?),
+                                    .map_err(|e| Error::PubKeyCtxError(e, Ctx::name_str()))?),
                             );
                         }
                         let k = match_token!(

Original file line number	Diff line number	Diff line change
`@@ -29,7 +29,7 @@ use super::decode::ParseableKey;`
`29`	`29`	`use {Miniscript, MiniscriptKey, Terminal};`
`30`	`30`
`31`	`31`	`/// Error for Script Context`
`32`		`-#[derive(Copy, Clone, PartialEq, Eq, Debug)]`
	`32`	`+#[derive(Clone, PartialEq, Eq, Debug)]`
`33`	`33`	`pub enum ScriptContextError {`
`34`	`34`	`/// Script Context does not permit PkH for non-malleability`
`35`	`35`	`/// It is not possible to estimate the pubkey size at the creation`
`@@ -43,13 +43,13 @@ pub enum ScriptContextError {`
`43`	`43`	`MalleableDupIf,`
`44`	`44`	`/// Only Compressed keys allowed under current descriptor`
`45`	`45`	`/// Segwitv0 fragments do not allow uncompressed pubkeys`
`46`		`- CompressedOnly,`
	`46`	`+ CompressedOnly(String),`
`47`	`47`	`/// Tapscript descriptors cannot contain uncompressed keys`
`48`	`48`	`/// Tap context can contain compressed or xonly`
`49`	`49`	`UncompressedKeysNotAllowed,`
`50`	`50`	`/// At least one satisfaction path in the Miniscript fragment has more than`
`51`	`51`	/// `MAX_STANDARD_P2WSH_STACK_ITEMS` (100) witness elements.
`52`		`- MaxWitnessItemssExceeded,`
	`52`	`+ MaxWitnessItemssExceeded { actual: usize, limit: usize },`
`53`	`53`	`/// At least one satisfaction path in the Miniscript fragment contains more`
`54`	`54`	/// than `MAX_OPS_PER_SCRIPT`(201) opcodes.
`55`	`55`	`MaxOpCountExceeded,`
`@@ -66,7 +66,7 @@ pub enum ScriptContextError {`
`66`	`66`	`/// No Multi Node in Taproot context`
`67`	`67`	`TaprootMultiDisabled,`
`68`	`68`	`/// Stack size exceeded in script execution`
`69`		`- StackSizeLimitExceeded,`
	`69`	`+ StackSizeLimitExceeded { actual: usize, limit: usize },`
`70`	`70`	`}`
`71`	`71`
`72`	`72`	`impl fmt::Display for ScriptContextError {`
`@@ -77,23 +77,24 @@ impl fmt::Display for ScriptContextError {`
`77`	`77`	`ScriptContextError::MalleableDupIf => {`
`78`	`78`	`write!(f, "DupIf is malleable under Legacy rules")`
`79`	`79`	`}`
`80`		`- ScriptContextError::CompressedOnly => {`
	`80`	`+ ScriptContextError::CompressedOnly(ref pk) => {`
`81`	`81`	`write!(`
`82`	`82`	`f,`
`83`		`- "Only Compressed pubkeys are allowed in segwit context. X-only and uncompressed keys are forbidden"`
	`83`	`+ "Only Compressed pubkeys are allowed in segwit context. Found {}",`
	`84`	`+ pk`
`84`	`85`	`)`
`85`	`86`	`}`
`86`	`87`	`ScriptContextError::UncompressedKeysNotAllowed => {`
`87`	`88`	`write!(`
`88`	`89`	`f,`
`89`		`- "Only x-only keys are allowed in tapscript checksig. \`
`90`		`- Compressed keys maybe specified in descriptor."`
	`90`	`+ "uncompressed keys cannot be used in Taproot descriptors."`
`91`	`91`	`)`
`92`	`92`	`}`
`93`		`- ScriptContextError::MaxWitnessItemssExceeded => write!(`
	`93`	`+ ScriptContextError::MaxWitnessItemssExceeded { actual, limit } => write!(`
`94`	`94`	`f,`
`95`		`- "At least one spending path in the Miniscript fragment has more \`
`96`		`- witness items than MAX_STANDARD_P2WSH_STACK_ITEMS.",`
	`95`	`+ "At least one spending path in the Miniscript fragment has {} more \`
	`96`	`+ witness items than limit {}.",`
	`97`	`+ actual, limit`
`97`	`98`	`),`
`98`	`99`	`ScriptContextError::MaxOpCountExceeded => write!(`
`99`	`100`	`f,`
`@@ -122,12 +123,13 @@ impl fmt::Display for ScriptContextError {`
`122`	`123`	`)`
`123`	`124`	`}`
`124`	`125`	`ScriptContextError::TaprootMultiDisabled => {`
`125`		`- write!(f, "No Multi node in taproot context")`
	`126`	`+ write!(f, "Invalid use of Multi node in taproot context")`
`126`	`127`	`}`
`127`		`- ScriptContextError::StackSizeLimitExceeded => {`
	`128`	`+ ScriptContextError::StackSizeLimitExceeded { actual, limit } => {`
`128`	`129`	`write!(`
`129`	`130`	`f,`
`130`		`- "Stack limit can exceed in atleast one script path during script execution"`
	`131`	`+ "Stack limit {} can exceed the allowed limit {} in at least one script path during script execution",`
	`132`	`+ actual, limit`
`131`	`133`	`)`
`132`	`134`	`}`
`133`	`135`	`}`
`@@ -293,7 +295,7 @@ where`
`293`	`295`	`fn pk_len<Pk: MiniscriptKey>(pk: &Pk) -> usize;`
`294`	`296`
`295`	`297`	`/// Local helper function to display error messages with context`
`296`		`- fn to_str() -> &'static str;`
	`298`	`+ fn name_str() -> &'static str;`
`297`	`299`	`}`
`298`	`300`
`299`	`301`	`/// Legacy ScriptContext`
`@@ -378,7 +380,7 @@ impl ScriptContext for Legacy {`
`378`	`380`	`}`
`379`	`381`	`}`
`380`	`382`
`381`		`- fn to_str() -> &'static str {`
	`383`	`+ fn name_str() -> &'static str {`
`382`	`384`	`"Legacy/p2sh"`
`383`	`385`	`}`
`384`	`386`	`}`
`@@ -399,7 +401,10 @@ impl ScriptContext for Segwitv0 {`
`399`	`401`	`witness: &[Vec<u8>],`
`400`	`402`	`) -> Result<(), ScriptContextError> {`
`401`	`403`	`if witness.len() > MAX_STANDARD_P2WSH_STACK_ITEMS {`
`402`		`- return Err(ScriptContextError::MaxWitnessItemssExceeded);`
	`404`	`+ return Err(ScriptContextError::MaxWitnessItemssExceeded {`
	`405`	`+ actual: witness.len(),`
	`406`	`+ limit: MAX_STANDARD_P2WSH_STACK_ITEMS,`
	`407`	`+ });`
`403`	`408`	`}`
`404`	`409`	`Ok(())`
`405`	`410`	`}`
`@@ -414,13 +419,15 @@ impl ScriptContext for Segwitv0 {`
`414`	`419`	`match ms.node {`
`415`	`420`	`Terminal::PkK(ref pk) => {`
`416`	`421`	`if pk.is_uncompressed() {`
`417`		`- return Err(ScriptContextError::CompressedOnly);`
	`422`	`+ return Err(ScriptContextError::CompressedOnly(pk.to_string()));`
`418`	`423`	`}`
`419`	`424`	`Ok(())`
`420`	`425`	`}`
`421`	`426`	`Terminal::Multi(_k, ref pks) => {`
`422`		`- if pks.iter().any(\|pk\| pk.is_uncompressed()) {`
`423`		`- return Err(ScriptContextError::CompressedOnly);`
	`427`	`+ for pk in pks.iter() {`
	`428`	`+ if pk.is_uncompressed() {`
	`429`	`+ return Err(ScriptContextError::CompressedOnly(pk.to_string()));`
	`430`	`+ }`
`424`	`431`	`}`
`425`	`432`	`Ok(())`
`426`	`433`	`}`
`@@ -459,7 +466,10 @@ impl ScriptContext for Segwitv0 {`
`459`	`466`	`// No possible satisfactions`
`460`	`467`	`Err(_e) => Err(ScriptContextError::ImpossibleSatisfaction),`
`461`	`468`	`Ok(max_witness_items) if max_witness_items > MAX_STANDARD_P2WSH_STACK_ITEMS => {`
`462`		`- Err(ScriptContextError::MaxWitnessItemssExceeded)`
	`469`	`+ Err(ScriptContextError::MaxWitnessItemssExceeded {`
	`470`	`+ actual: max_witness_items,`
	`471`	`+ limit: MAX_STANDARD_P2WSH_STACK_ITEMS,`
	`472`	`+ })`
`463`	`473`	`}`
`464`	`474`	`_ => Ok(()),`
`465`	`475`	`}`
`@@ -476,7 +486,7 @@ impl ScriptContext for Segwitv0 {`
`476`	`486`	`34`
`477`	`487`	`}`
`478`	`488`
`479`		`- fn to_str() -> &'static str {`
	`489`	`+ fn name_str() -> &'static str {`
`480`	`490`	`"Segwitv0"`
`481`	`491`	`}`
`482`	`492`	`}`
`@@ -498,8 +508,12 @@ impl ScriptContext for Tap {`
`498`	`508`	`fn check_witness<Pk: MiniscriptKey, Ctx: ScriptContext>(`
`499`	`509`	`witness: &[Vec<u8>],`
`500`	`510`	`) -> Result<(), ScriptContextError> {`
	`511`	`+ // Note that tapscript has a 1000 limit compared to 100 of segwitv0`
`501`	`512`	`if witness.len() > MAX_STACK_SIZE {`
`502`		`- return Err(ScriptContextError::MaxWitnessItemssExceeded);`
	`513`	`+ return Err(ScriptContextError::MaxWitnessItemssExceeded {`
	`514`	`+ actual: witness.len(),`
	`515`	`+ limit: MAX_STACK_SIZE,`
	`516`	`+ });`
`503`	`517`	`}`
`504`	`518`	`Ok(())`
`505`	`519`	`}`
`@@ -526,8 +540,6 @@ impl ScriptContext for Tap {`
`526`	`540`	`Terminal::Multi(..) => {`
`527`	`541`	`return Err(ScriptContextError::TaprootMultiDisabled);`
`528`	`542`	`}`
`529`		`- // What happens to the Multi node in tapscript? Do we use it, create`
`530`		`- // a new fragment?`
`531`	`543`	`_ => Ok(()),`
`532`	`544`	`}`
`533`	`545`	`}`
`@@ -549,7 +561,10 @@ impl ScriptContext for Tap {`
`549`	`561`	`ms.ext.stack_elem_count_sat,`
`550`	`562`	`) {`
`551`	`563`	`if s + h > MAX_STACK_SIZE {`
`552`		`- return Err(ScriptContextError::StackSizeLimitExceeded);`
	`564`	`+ return Err(ScriptContextError::StackSizeLimitExceeded {`
	`565`	`+ actual: s + h,`
	`566`	`+ limit: MAX_STACK_SIZE,`
	`567`	`+ });`
`553`	`568`	`}`
`554`	`569`	`}`
`555`	`570`	`Ok(())`
`@@ -583,7 +598,7 @@ impl ScriptContext for Tap {`
`583`	`598`	`33`
`584`	`599`	`}`
`585`	`600`
`586`		`- fn to_str() -> &'static str {`
	`601`	`+ fn name_str() -> &'static str {`
`587`	`602`	`"TapscriptCtx"`
`588`	`603`	`}`
`589`	`604`	`}`
`@@ -657,7 +672,7 @@ impl ScriptContext for BareCtx {`
`657`	`672`	`}`
`658`	`673`	`}`
`659`	`674`
`660`		`- fn to_str() -> &'static str {`
	`675`	`+ fn name_str() -> &'static str {`
`661`	`676`	`"BareCtx"`
`662`	`677`	`}`
`663`	`678`	`}`
`@@ -712,9 +727,9 @@ impl ScriptContext for NoChecks {`
`712`	`727`	`panic!("Tried to compute a pk len bound on a no-checks miniscript")`
`713`	`728`	`}`
`714`	`729`
`715`		`- fn to_str() -> &'static str {`
	`730`	`+ fn name_str() -> &'static str {`
`716`	`731`	`// Internally used code`
`717`		`- "NochecksCtx"`
	`732`	`+ "Nochecks"`
`718`	`733`	`}`
`719`	`734`	`}`
`720`	`735`