Skip to content

typeck: clean up data structures #859

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

typeck: clean up data structures #859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f57cac0
typeck: pull satisfaction/dissatisfaction limits into struct
apoelstra May 12, 2025
5f0072c
typeck: move opcode limits into sat/dissat limits
apoelstra May 13, 2025
1e6aead
add unit tests for recent typeck bug fixes
apoelstra Sep 29, 2025
f3ea32c
typeck: fix mistake in previous correction to thresh exec_stack_count
apoelstra Sep 29, 2025
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
27 changes: 12 additions & 15 deletions src/miniscript/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ impl ScriptContext for Legacy {
fn check_local_consensus_validity<Pk: MiniscriptKey>(
ms: &Miniscript<Pk, Self>,
) -> Result<(), ScriptContextError> {
match ms.ext.ops.op_count() {
match ms.ext.sat_op_count() {
None => Err(ScriptContextError::ImpossibleSatisfaction),
Some(op_count) if op_count > MAX_OPS_PER_SCRIPT => {
Err(ScriptContextError::MaxOpCountExceeded {
Expand Down Expand Up @@ -467,8 +467,7 @@ impl ScriptContext for Legacy {
}

fn max_satisfaction_size<Pk: MiniscriptKey>(ms: &Miniscript<Pk, Self>) -> Option<usize> {
// The scriptSig cost is the second element of the tuple
ms.ext.max_sat_size.map(|x| x.1)
ms.ext.sat_data.map(|data| data.max_script_sig_size)
}

fn pk_len<Pk: MiniscriptKey>(pk: &Pk) -> usize {
Expand Down Expand Up @@ -551,7 +550,7 @@ impl ScriptContext for Segwitv0 {
fn check_local_consensus_validity<Pk: MiniscriptKey>(
ms: &Miniscript<Pk, Self>,
) -> Result<(), ScriptContextError> {
match ms.ext.ops.op_count() {
match ms.ext.sat_op_count() {
None => Err(ScriptContextError::ImpossibleSatisfaction),
Some(op_count) if op_count > MAX_OPS_PER_SCRIPT => {
Err(ScriptContextError::MaxOpCountExceeded {
Expand Down Expand Up @@ -595,8 +594,7 @@ impl ScriptContext for Segwitv0 {
}

fn max_satisfaction_size<Pk: MiniscriptKey>(ms: &Miniscript<Pk, Self>) -> Option<usize> {
// The witness stack cost is the first element of the tuple
ms.ext.max_sat_size.map(|x| x.0)
ms.ext.sat_data.map(|data| data.max_witness_stack_size)
}

fn pk_len<Pk: MiniscriptKey>(_pk: &Pk) -> usize { 34 }
Expand Down Expand Up @@ -688,11 +686,10 @@ impl ScriptContext for Tap {
// will have it's corresponding 64 bytes signature.
// sigops budget = witness_script.len() + witness.size() + 50
// Each signature will cover it's own cost(64 > 50) and thus will will never exceed the budget
if let (Some(s), Some(h)) = (ms.ext.exec_stack_elem_count_sat, ms.ext.stack_elem_count_sat)
{
if s + h > MAX_STACK_SIZE {
if let Some(data) = ms.ext.sat_data {
if data.max_witness_stack_count + data.max_exec_stack_count > MAX_STACK_SIZE {
return Err(ScriptContextError::StackSizeLimitExceeded {
actual: s + h,
actual: data.max_witness_stack_count + data.max_exec_stack_count,
limit: MAX_STACK_SIZE,
});
}
Expand All @@ -714,8 +711,7 @@ impl ScriptContext for Tap {
}

fn max_satisfaction_size<Pk: MiniscriptKey>(ms: &Miniscript<Pk, Self>) -> Option<usize> {
// The witness stack cost is the first element of the tuple
ms.ext.max_sat_size.map(|x| x.0)
ms.ext.sat_data.map(|data| data.max_witness_stack_size)
}

fn sig_type() -> SigType { SigType::Schnorr }
Expand Down Expand Up @@ -787,7 +783,7 @@ impl ScriptContext for BareCtx {
fn check_local_consensus_validity<Pk: MiniscriptKey>(
ms: &Miniscript<Pk, Self>,
) -> Result<(), ScriptContextError> {
match ms.ext.ops.op_count() {
match ms.ext.sat_op_count() {
None => Err(ScriptContextError::ImpossibleSatisfaction),
Some(op_count) if op_count > MAX_OPS_PER_SCRIPT => {
Err(ScriptContextError::MaxOpCountExceeded {
Expand All @@ -812,8 +808,9 @@ impl ScriptContext for BareCtx {
}

fn max_satisfaction_size<Pk: MiniscriptKey>(ms: &Miniscript<Pk, Self>) -> Option<usize> {
// The witness stack cost is the first element of the tuple
ms.ext.max_sat_size.map(|x| x.1)
// For bare outputs the script appears in the scriptpubkey; its cost
// is the same as for a legacy scriptsig.
ms.ext.sat_data.map(|data| data.max_script_sig_size)
}

fn pk_len<Pk: MiniscriptKey>(pk: &Pk) -> usize {
Expand Down
53 changes: 50 additions & 3 deletions src/miniscript/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,8 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
/// impossible to satisfy
pub fn max_satisfaction_witness_elements(&self) -> Result<usize, Error> {
self.ext
.stack_elem_count_sat
.map(|x| x + 1)
.sat_data
.map(|data| data.max_witness_stack_count + 1)
.ok_or(Error::ImpossibleSatisfaction)
}

Expand Down Expand Up @@ -1193,7 +1193,7 @@ mod tests {
assert_eq!(format!("{:x}", ms.encode()), expected_hex);
assert_eq!(ms.ty.mall.non_malleable, non_mal);
assert_eq!(ms.ty.mall.safe, need_sig);
assert_eq!(ms.ext.ops.op_count().unwrap(), ops);
assert_eq!(ms.ext.static_ops + ms.ext.sat_data.unwrap().max_exec_op_count, ops);
}
(Err(_), false) => {}
_ => unreachable!(),
Expand Down Expand Up @@ -1883,6 +1883,53 @@ mod tests {
Tapscript::decode_insane(&script.into_script()).unwrap_err();
}

#[test]
fn test_or_d_exec_stack_count_fix() {
// Test for the or_d dissat_data.max_exec_stack_count fix
// The old code incorrectly added +1 to the exec stack count for or_d dissatisfaction
let ms_str = "or_d(pk(A),pk(B))";
let ms = Miniscript::<String, Segwitv0>::from_str_insane(ms_str).unwrap();

// With the fix, or_d dissatisfaction should not have the extra +1
// Both branches have exec_stack_count of 1, so dissat should be max(1,1) = 1, not 2
if let Some(dissat_data) = ms.ext.dissat_data {
assert_eq!(dissat_data.max_exec_stack_count, 1);
} else {
panic!("Expected dissat_data to be Some");
}
}

#[test]
fn test_threshold_exec_stack_count_max_not_sum() {
// Test for the threshold max_exec_stack_count fix
// The old code incorrectly summed exec stack counts, new code takes max
let ms_str = "thresh(2,pk(A),s:pk(B),s:pk(C))";
let ms = Miniscript::<String, Segwitv0>::from_str_insane(ms_str).unwrap();

// Each pk has exec_stack_count of 1, plus an extra stack element for the thresh accumulator.
// With the fix, threshold should take max(1,1,1) + 1 = 2, not sum 1+1+1 = 3
if let Some(sat_data) = ms.ext.sat_data {
assert_eq!(sat_data.max_exec_stack_count, 2);
} else {
panic!("Expected sat_data to be Some");
}

// Test with a more complex threshold, where the first child has a strictly higher
// exec_stack_count. This time, we take the maximum *without* adding +1 for the
// accumulator, since on the first child of `thresh` there is no accumulator yet
// (its initial value is the output value for the first child).
let complex_ms_str = "thresh(1,and_b(pk(A),s:pk(B)),s:pk(C))";
let complex_ms = Miniscript::<String, Segwitv0>::from_str_insane(complex_ms_str).unwrap();

// and_v has exec_stack_count of 2, pk has 1
// With the fix: max(2,1) = 2, old code would sum to 3
if let Some(sat_data) = complex_ms.ext.sat_data {
assert_eq!(sat_data.max_exec_stack_count, 2);
} else {
panic!("Expected sat_data to be Some");
}
}

#[test]
fn test_context_global_consensus() {
// Test from string tests
Expand Down
Loading
Loading