typeck: clean up data structures #859
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Right now in our ExtData type-checking structure there are four properties that we measure for satisfactions: maximum witness stack element count, maximum witness size, maximum scriptsig size, and maximum number of stack elements during execution. We measure the same variables for dissatisfaction. These are always either all present or none present; for some reason we have consolidated these four values into three separate Option types, and we debug_assert that either they are all present or none are. Furthermore, in the one case that we've consolidated stuff, we used a tuple. So we have a (max witness size, max scriptsig size) pair, and code comments everywhere explaining which one is .0 and which one is .1. Pull all these into a struct with named fields, and improve the field names to be more consistent. This means that we now have one Option for satisfaction properties, and one for dissatisfaction properties. No need for debug assertions. There are two oddities worth highlighting: 1. The `or_d` dissat_data.max_exec_stack_count (line 693) used to have a +1 in its formula. I think this was a copy/paste error from or_b. I removed it. No tests break. 2. In `threshold` we sum up the satisfaction and dissatisfaction numbers for the four variables we're measuring. But for max_exec_stack_count I think we should be maxing, not summing. I also fixed this, and again no tests break.
This consolidates two Option fields into existing Option fields, unifying a bunch of code. It also deletes some crufty utility functions. This one actually has pretty good tests.
tcharding
previously approved these changes
Sep 29, 2025
|
FWIW |
These unit tests were written with the help of Claude 4 (it wrong the
tests and I just fixed its miniscript syntax by adding s wrappers).
If you want to try reordering this commit to two commits back (where it
will fail), you need to tweak it a bit because the data structures were
rearranged. This diff will work.
diff --git a/src/miniscript/mod.rs b/src/miniscript/mod.rs
index 2435e50f..cb0b3530 100644
--- a/src/miniscript/mod.rs
+++ b/src/miniscript/mod.rs
@@ -1892,8 +1892,8 @@ mod tests {
// 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);
+ if let Some(dissat_data) = ms.ext.exec_stack_elem_count_dissat{
+ assert_eq!(dissat_data, 1);
} else {
panic!("Expected dissat_data to be Some");
}
@@ -1908,8 +1908,8 @@ mod tests {
// Each pk has exec_stack_count of 1
// With the fix, threshold should take max(1,1,1) = 1, not sum 1+1+1 = 3
- if let Some(sat_data) = ms.ext.sat_data {
- assert_eq!(sat_data.max_exec_stack_count, 1);
+ if let Some(sat_data) = ms.ext.exec_stack_elem_count_sat{
+ assert_eq!(sat_data, 1);
} else {
panic!("Expected sat_data to be Some");
}
@@ -1920,8 +1920,8 @@ mod tests {
// 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);
+ if let Some(sat_data) = complex_ms.ext.exec_stack_elem_count_sat{
+ assert_eq!(sat_data, 2);
} else {
panic!("Expected sat_data to be Some");
}
|
Added a commit with unit tests that show the new changes. @sanket1729 when you review this, I think it will be easiest to review the new tests first. They were written by Claude but they are small and have clear comments explaining the change in behavior (and have example descriptors that you can compute these values by hand to confirm the new ones arecorrect). |
|
After studying the new The reason is that in addition to the stack elements inherited from each threshold child, we also have the accumulator value which is |
|
Yeah, I'm fairly convinced. Added a commit with this fix. This PR should be good to go now. |
I am putting this in its own commit because it's kinda hard to reason about and I wanted the change to be separate, even though it changes the same code that previous commits changed.
apoelstra
force-pushed
the
2025-09/typeck-cleanup
branch
from
c7c5ca6 to
f3ea32c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These two simple commits just collect some related data into common data structures, allowing satisfaction/dissatisfaction data to all be
Optional at once without any debug_asserts, and simplifying a bunch of code.This makes a couple obscure fixes that are not detected by tests and we have never received reports about. See the commit message for the first commit for details.