fix: proptest underflow + wrong Lean witnesses + counterexample values for named constants#10
Conversation
abishekk92
left a comment
There was a problem hiding this comment.
Thanks — all three fixes look correct and the gates pass clean (fmt / clippy -D warnings / 241 tests). Before merging, can we pin each fix with a unit test? The existing proptest_gen::tests / lean_gen::tests / check::tests modules are the right homes — each fix is one or two lines to exercise:
proptest_gen.rs — directly call the function, no spec needed:
#[test]
fn strategy_for_field_boundary_small_bound_avoids_underflow() {
let spec = ParsedSpec::default();
let s = strategy_for_field("U64", &spec, StrategyMode::Boundary, Some("2")).unwrap();
assert_eq!(s, "0u64..=2u64");
assert!(!s.contains("- 3"), "must not emit `(b - 3)` for b < 3");
}lean_gen.rs — a minimal WitnessState with constants = [("ZERO", "0")], apply a handler whose effect is counter := ZERO, assert the resulting state field renders as 0 not ZERO.
check.rs — a build_counterexample call where the effect triple is ("counter", "set", "ZERO") and constants = [("ZERO".into(), "0".into())], assert the post-state arithmetic uses 0, not the fallback 1.
Without these, a future refactor of the resolution order could quietly re-break any of the three paths. Happy to merge once they're in.
8653f7e to
f556259
Compare
|
Three real bugs, three tight fixes — nice catch on the proptest underflow in particular (that one only surfaces for small bounds, easy to miss). Unit tests cover each fix well. One ask before merge: could you add
The Rust unit tests prove the functions behave correctly; the spec fixtures prove the end-to-end Happy to merge once those land. |
added the three unit tests (proptest_gen, lean_gen, check.rs) and the three .qedspec fixtures under examples/regressions/const-resolution/. Should be good to go. |
|
Spent some time running the fixtures end-to-end and found two of three don't actually exercise the fix on their advertised symptom. Sharing the findings so we can decide between widening the patch or narrowing the fixtures.
Generated def initTransition (s : State) (signer : Pubkey) : Option State :=
if s.status = .Uninitialized then
some { s with counter := ZERO, status := .Active }
else noneThe fix to
Two ways forward:
(1) is the higher-value option since the underlying bugs are real on the cited symptoms. Happy to iterate on either approach. Also: please rebase onto |
abishekk92
left a comment
There was a problem hiding this comment.
I've left a comment requesting changes
7421e9a to
4d2d4aa
Compare
Fix three correctness bugs triggered by named spec constants.
(b-3)..=bwhenb<3,causing u64 underflow panic at test runtime
WitnessState::resolve_valueskipped spec constants,producing wrong post-state witnesses →
lake buildfailures on cover proofsbuild_counterexampleshowed wrong arithmetic in lint outputwhen effect values referenced named constants