Test Epoch3.4 and Clarity5#6912
Test Epoch3.4 and Clarity5#6912BowTiedRadone wants to merge 29 commits intostacks-network:developfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6912 +/- ##
============================================
- Coverage 77.73% 61.00% -16.74%
============================================
Files 412 412
Lines 218667 218667
Branches 338 338
============================================
- Hits 169981 133387 -36594
- Misses 48686 85280 +36594 see 295 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
ec722ea to
7b53b4f
Compare
Eight proptests covering the version-dependent hashing change. Validates primitive/VM agreement on accept and reject paths, confirms ClarityVersion governs hashing (not epoch), and fuzzes arbitrary inputs for crash resistance.
…proptests Three new property-based tests for Clarity5 consensus deserialization: - Round-trip: serialize then from-consensus-buff? recovers original value - Type mismatch: wrong top-level constructors always returns none - Truncation: slicing a valid encoding at any interior byte returns none
7b53b4f to
ccc98ca
Compare
ccc98ca to
a02f406
Compare
b51347c to
2c24755
Compare
| #[test] | ||
| fn prop_from_consensus_buff_empty_buffer_returns_none( |
There was a problem hiding this comment.
It would be nice to include test tagging for all the proptest tests can be easily discovered by CI automations.
| #[test] | |
| fn prop_from_consensus_buff_empty_buffer_returns_none( | |
| #[tag(t_prop)] | |
| #[test] | |
| fn prop_from_consensus_buff_empty_buffer_returns_none( |
| tempfile = "3.3" | ||
| pinny = { workspace = true } | ||
| proptest = { version = "1.6.0", default-features = false, features = ["std"] } | ||
| madhouse = { git = "https://github.com/stacks-network/madhouse-rs.git", tag = "0.2.1" } |
There was a problem hiding this comment.
Minor: we are currently referencing two versions of madhouse-rs:
0.2.1instackslib0.2.0instacks-node
if the two versions are compatibles, It might be worth adding madhouse-rs as a workspace dependency and referencing it from the crates that use it, to ensure version consistency.
| name = "madhouse" | ||
| version = "0.2.0" | ||
| source = "git+https://github.com/stacks-network/madhouse-rs.git?tag=0.2.1#5d4d51bedf2d56c8ef0643d891fb085df1615c91" | ||
| dependencies = [ |
There was a problem hiding this comment.
nit: version reported in madhouse-rs/Cargo.tml is not aligned with tag.
There was a problem hiding this comment.
Thanks! PR opened upstream: stacks-network/madhouse-rs#33.
| /// Expected return value from a successful ping call: `(ok true)`. | ||
| fn ok_true() -> Value { | ||
| Value::Response(ResponseData { | ||
| committed: true, | ||
| data: Box::new(Value::Bool(true)), | ||
| }) | ||
| } |
There was a problem hiding this comment.
nit: this helper is repeated over 3 modules. Maybe it can be moved under commands/mod.rs
| let call_stack_limit = if state.chain_epoch() >= StacksEpochId::Epoch34 { | ||
| 128 | ||
| } else { | ||
| 64 | ||
| }; | ||
| let parser_limit = call_stack_limit + AST_DEPTH_BUFFER; |
There was a problem hiding this comment.
Could we leverage StackDepthLimits::for_epoch here? Then we could drop all the constants: 64, 128 and AST_DEPTH_BUFFER.
Or is the goal to replicate the stack depth computation to guard against changes in the underlying implementation? (and in this case we could just write some unit tests specifics for StackDepthLimits)
There was a problem hiding this comment.
It's the latter: the goal is to replicate the computation and guard against silent changes in the underlying implementation. If max_call_stack_depth_for_epoch or AST_CALL_STACK_DEPTH_BUFFER were accidentally modified, tests that delegate to StackDepthLimits::for_epoch would be tautological (testing the implementation against itself). The hardcoded values act as independent witnesses of the expected behavior.
Adding dedicated unit tests for StackDepthLimits would just duplicate what these commands already cover with the same hardcoded assertions, so I'd rather not add them unless you feel strongly.
| let chain_epoch = state.chain_epoch(); | ||
| assert_eq!( | ||
| state.current_epoch, chain_epoch, | ||
| "CallRestrictWithStxCombinedExceeds: model epoch {:?} disagrees with chain {:?}", | ||
| state.current_epoch, chain_epoch, | ||
| ); |
There was a problem hiding this comment.
sanity-check: I noticed that CallRestrictWithStxCombinedExceeds and CallAsContractWithStxCombinedExceeds have epoch check, while CallRestrictWithStxSafe and CallAsContractWithStxSafe haven't. Should they have it?
There was a problem hiding this comment.
It is intentional. The safe variants expect (ok true) regardless of epoch, so there's no epoch-dependent branch that a model/chain mismatch could silently mislead. The exceeds variants branch on is_epoch34() (block rejection vs (err u0)), so the epoch check is a precondition to ensure the branch is evaluating the right case.
| TransactionPostConditionMode::Originator, | ||
| vec![], | ||
| ); | ||
| state.nft_next_id += 1; |
There was a problem hiding this comment.
nit: this feels a bit inconsistent with other commands. Should the NFT ID be incremented before the transaction is created, as done elsewhere?
| pub fn consensus_buff_type_strategy() -> BoxedStrategy<String> { | ||
| let len_range = 1u32..=128; | ||
|
|
||
| prop_oneof![ | ||
| Just("int".to_string()), | ||
| Just("uint".to_string()), | ||
| Just("bool".to_string()), | ||
| Just("principal".to_string()), | ||
| len_range.clone().prop_map(|n| format!("(buff {n})")), | ||
| len_range | ||
| .clone() | ||
| .prop_map(|n| format!("(string-ascii {n})")), | ||
| len_range.clone().prop_map(|n| format!("(string-utf8 {n})")), | ||
| Just("(optional int)".to_string()), | ||
| len_range.prop_map(|n| format!("(list {n} int)")), | ||
| Just("(response int int)".to_string()), | ||
| ] | ||
| .boxed() |
There was a problem hiding this comment.
sanity-check: should tuple also be considered here?
| #[test] | ||
| fn test_default_for_epoch_is_monotonic() { | ||
| // No Clarity in Epoch10. | ||
| let clarity_epochs = &StacksEpochId::ALL[1..]; |
There was a problem hiding this comment.
nit: readability could be improved using StacksEpochId::since()
| let clarity_epochs = &StacksEpochId::ALL[1..]; | |
| let clarity_epochs = StacksEpochId::since(StacksEpochId::Epoch20); |
Coverage Report for CI Build 24504371565Coverage increased (+0.02%) to 85.734%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions3004 previously-covered lines in 76 files lost coverage.
Coverage Stats
💛 - Coveralls |
dfd390f to
80dd989
Compare
This PR applies stateless and stateful property-based testing techniques for Epoch 3.4 / Clarity 5 changes.
Stateless (proptest-rs)
secp256r1-verify: digest roundtrip, tamper/wrong-key/malformed-key cases, and version-governance checksStateful (madhouse-rs)
Infrastructure
MADHOUSE=1randomized modecargo test -p stackslib --lib chainstate::tests::madhouseMADHOUSE=1 PROPTEST_CASES=50 cargo test -p stackslib --lib chainstate::tests::madhouse