Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: check the node syntax before size limit #747

Merged
merged 2 commits into from
Sep 12, 2024
Merged

refactor: check the node syntax before size limit #747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da806be
refactor: check the node syntax before size limit
ChrisCho-H Sep 11, 2024
3cdc711
test: ensure multi fragment panic before size limit panic
ChrisCho-H Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 57 additions & 24 deletions src/miniscript/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,11 +394,8 @@ impl ScriptContext for Legacy {
fn check_global_consensus_validity<Pk: MiniscriptKey>(
ms: &Miniscript<Pk, Self>,
) -> Result<(), ScriptContextError> {
if ms.ext.pk_cost > MAX_SCRIPT_ELEMENT_SIZE {
return Err(ScriptContextError::MaxRedeemScriptSizeExceeded);
}

match ms.node {
// 1. Check the node first, throw an error on the language itself
let node_checked = match ms.node {
Terminal::PkK(ref pk) => Self::check_pk(pk),
Terminal::Multi(ref thresh) => {
for pk in thresh.iter() {
Expand All @@ -408,6 +405,17 @@ impl ScriptContext for Legacy {
}
Terminal::MultiA(..) => Err(ScriptContextError::MultiANotAllowed),
_ => Ok(()),
};
// 2. After fragment and param check, validate the script size finally
match node_checked {
Ok(_) => {
if ms.ext.pk_cost > MAX_SCRIPT_ELEMENT_SIZE {
Err(ScriptContextError::MaxRedeemScriptSizeExceeded)
} else {
Ok(())
}
}
Err(_) => node_checked,
}
}

Expand Down Expand Up @@ -492,11 +500,8 @@ impl ScriptContext for Segwitv0 {
fn check_global_consensus_validity<Pk: MiniscriptKey>(
ms: &Miniscript<Pk, Self>,
) -> Result<(), ScriptContextError> {
if ms.ext.pk_cost > MAX_SCRIPT_SIZE {
return Err(ScriptContextError::MaxWitnessScriptSizeExceeded);
}

match ms.node {
// 1. Check the node first, throw an error on the language itself
let node_checked = match ms.node {
Terminal::PkK(ref pk) => Self::check_pk(pk),
Terminal::Multi(ref thresh) => {
for pk in thresh.iter() {
Expand All @@ -506,6 +511,17 @@ impl ScriptContext for Segwitv0 {
}
Terminal::MultiA(..) => Err(ScriptContextError::MultiANotAllowed),
_ => Ok(()),
};
// 2. After fragment and param check, validate the script size finally
match node_checked {
Ok(_) => {
if ms.ext.pk_cost > MAX_SCRIPT_SIZE {
Err(ScriptContextError::MaxWitnessScriptSizeExceeded)
} else {
Ok(())
}
}
Err(_) => node_checked,
}
}

Expand Down Expand Up @@ -598,16 +614,8 @@ impl ScriptContext for Tap {
fn check_global_consensus_validity<Pk: MiniscriptKey>(
ms: &Miniscript<Pk, Self>,
) -> Result<(), ScriptContextError> {
// No script size checks for global consensus rules
// Should we really check for block limits here.
// When the transaction sizes get close to block limits,
// some guarantees are not easy to satisfy because of knapsack
// constraints
if ms.ext.pk_cost as u64 > Weight::MAX_BLOCK.to_wu() {
return Err(ScriptContextError::MaxWitnessScriptSizeExceeded);
}

match ms.node {
// 1. Check the node first, throw an error on the language itself
let node_checked = match ms.node {
Terminal::PkK(ref pk) => Self::check_pk(pk),
Terminal::MultiA(ref thresh) => {
for pk in thresh.iter() {
Expand All @@ -617,6 +625,22 @@ impl ScriptContext for Tap {
}
Terminal::Multi(..) => Err(ScriptContextError::TaprootMultiDisabled),
_ => Ok(()),
};
// 2. After fragment and param check, validate the script size finally
match node_checked {
Ok(_) => {
// No script size checks for global consensus rules
// Should we really check for block limits here.
// When the transaction sizes get close to block limits,
// some guarantees are not easy to satisfy because of knapsack
// constraints
if ms.ext.pk_cost as u64 > Weight::MAX_BLOCK.to_wu() {
Err(ScriptContextError::MaxWitnessScriptSizeExceeded)
} else {
Ok(())
}
}
Err(_) => node_checked,
}
}

Expand Down Expand Up @@ -700,10 +724,8 @@ impl ScriptContext for BareCtx {
fn check_global_consensus_validity<Pk: MiniscriptKey>(
ms: &Miniscript<Pk, Self>,
) -> Result<(), ScriptContextError> {
if ms.ext.pk_cost > MAX_SCRIPT_SIZE {
return Err(ScriptContextError::MaxWitnessScriptSizeExceeded);
}
match ms.node {
// 1. Check the node first, throw an error on the language itself
let node_checked = match ms.node {
Terminal::PkK(ref key) => Self::check_pk(key),
Terminal::Multi(ref thresh) => {
for pk in thresh.iter() {
Expand All @@ -713,6 +735,17 @@ impl ScriptContext for BareCtx {
}
Terminal::MultiA(..) => Err(ScriptContextError::MultiANotAllowed),
_ => Ok(()),
};
// 2. After fragment and param check, validate the script size finally
match node_checked {
Ok(_) => {
if ms.ext.pk_cost > MAX_SCRIPT_SIZE {
Err(ScriptContextError::MaxWitnessScriptSizeExceeded)
} else {
Ok(())
}
}
Err(_) => node_checked,
}
}

Expand Down
65 changes: 64 additions & 1 deletion src/miniscript/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,7 +829,9 @@ mod tests {
use crate::policy::Liftable;
use crate::prelude::*;
use crate::test_utils::{StrKeyTranslator, StrXOnlyKeyTranslator};
use crate::{hex_script, Error, ExtParams, RelLockTime, Satisfier, ToPublicKey};
use crate::{
hex_script, BareCtx, Error, ExtParams, Legacy, RelLockTime, Satisfier, ToPublicKey,
};

type Segwitv0Script = Miniscript<bitcoin::PublicKey, Segwitv0>;
type Tapscript = Miniscript<bitcoin::secp256k1::XOnlyPublicKey, Tap>;
Expand Down Expand Up @@ -1634,6 +1636,67 @@ mod tests {
}
Tapscript::parse_insane(&script.into_script()).unwrap_err();
}

#[test]
fn test_context_global_consensus() {
// Test from string tests
type LegacyMs = Miniscript<String, Legacy>;
type Segwitv0Ms = Miniscript<String, Segwitv0>;
type BareMs = Miniscript<String, BareCtx>;

// multisig script of 20 pubkeys exceeds 520 bytes
let pubkey_vec_20: Vec<String> = (0..20).map(|x| x.to_string()).collect();
// multisig script of 300 pubkeys exceeds 10,000 bytes
let pubkey_vec_300: Vec<String> = (0..300).map(|x| x.to_string()).collect();

// wrong multi_a for non-tapscript, while exceeding consensus size limit
let legacy_multi_a_ms =
LegacyMs::from_str(&format!("multi_a(20,{})", pubkey_vec_20.join(",")));
let segwit_multi_a_ms =
Segwitv0Ms::from_str(&format!("multi_a(300,{})", pubkey_vec_300.join(",")));
let bare_multi_a_ms =
BareMs::from_str(&format!("multi_a(300,{})", pubkey_vec_300.join(",")));

// Should panic for wrong multi_a, even if it exceeds the max consensus size
assert_eq!(
legacy_multi_a_ms.unwrap_err().to_string(),
"Multi a(CHECKSIGADD) only allowed post tapscript"
);
assert_eq!(
segwit_multi_a_ms.unwrap_err().to_string(),
"Multi a(CHECKSIGADD) only allowed post tapscript"
);
assert_eq!(
bare_multi_a_ms.unwrap_err().to_string(),
"Multi a(CHECKSIGADD) only allowed post tapscript"
);

// multisig script of 20 pubkeys exceeds 520 bytes
let multi_ms = format!("multi(20,{})", pubkey_vec_20.join(","));
// other than legacy, and_v to build 15 nested 20-of-20 multisig script
// to exceed 10,000 bytes without violation of threshold limit(max: 20)
let and_v_nested_multi_ms =
format!("and_v(v:{},", multi_ms).repeat(14) + &multi_ms + "))))))))))))))";

// correct multi for non-tapscript, but exceeding consensus size limit
let legacy_multi_ms = LegacyMs::from_str(&multi_ms);
let segwit_multi_ms = Segwitv0Ms::from_str(&and_v_nested_multi_ms);
let bare_multi_ms = BareMs::from_str(&and_v_nested_multi_ms);

// Should panic for exceeding the max consensus size, as multi properly used
assert_eq!(
legacy_multi_ms.unwrap_err().to_string(),
"The Miniscript corresponding Script would be larger than MAX_SCRIPT_ELEMENT_SIZE bytes."
);
assert_eq!(
segwit_multi_ms.unwrap_err().to_string(),
"The Miniscript corresponding Script would be larger than MAX_STANDARD_P2WSH_SCRIPT_SIZE bytes."
);
assert_eq!(
bare_multi_ms.unwrap_err().to_string(),
"The Miniscript corresponding Script would be larger than MAX_STANDARD_P2WSH_SCRIPT_SIZE bytes."
);
}
}

#[cfg(bench)]
Expand Down
Loading