Skip to content

Commit e57cd43

Browse files
committed
fix(test): assert exact T1 clock in DYNCALL overflow-addr regression test
Address huitseeker review comment #3006679141 (PR 0xMiden#2904 review 4027183231): the previous assert_ne!(recorded_overflow_addr, ZERO) passes even on the buggy path because both T1 and T2 are nonzero when there are ≥2 overflow entries. Fix: scan all PUSH rows that precede the DYNCALL row in the execution trace, take the second-to-last (T1 = clock of push(0)) and last (T2 = clock of push(HASH_ADDR)) rows, and assert that recorded_overflow_addr == T1. A sanity check asserts T2 == T1 + ONE (they are in the same 8-op group). This exact equality clearly distinguishes clk_after_pop_in_current_ctx() (returns T1) from the buggy last_update_clk_in_current_ctx() (returns T2). Also address adr1anh nit (review 4028964881, comment 3008362338): add the missing blank lines before #### Changes and #### Bug fixes in CHANGELOG.md.
1 parent 0ad83a7 commit e57cd43

File tree

2 files changed

+34
-10
lines changed

2 files changed

+34
-10
lines changed

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#### Bug Fixes
66

77
- Reverted `InvokeKind::ProcRef` back to `InvokeKind::Exec` in `visit_mut_procref` and added an explanatory comment (#2893).
8+
89
#### Changes
910

1011
- [BREAKING] Sync execution and proving APIs now require `SyncHost`; async `Host`, `execute`, and `prove` remain available ([#2865](https://github.com/0xMiden/miden-vm/pull/2865)).
@@ -13,6 +14,7 @@
1314
- [BREAKING] Removed the deprecated `FastProcessor::execute_for_trace_sync()` and `execute_for_trace()` wrappers; use `execute_trace_inputs_sync()` or `execute_trace_inputs()` instead ([#2865](https://github.com/0xMiden/miden-vm/pull/2865)).
1415
- [BREAKING] Removed the deprecated unbound `TraceBuildInputs::new()` and `TraceBuildInputs::from_program()` constructors; use `execute_trace_inputs_sync()` or `execute_trace_inputs()` instead ([#2865](https://github.com/0xMiden/miden-vm/pull/2865)).
1516
- Added `prove_from_trace_sync(...)` for proving from pre-executed trace inputs ([#2865](https://github.com/0xMiden/miden-vm/pull/2865)).
17+
1618
#### Bug fixes
1719

1820
- Fixed `ExecutionTracer` DYNCALL stack-depth off-by-one at `MIN_STACK_DEPTH` ([#2813](https://github.com/0xMiden/miden-vm/issues/2813)).

processor/src/trace/tests/decoder.rs

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1076,17 +1076,39 @@ fn decoder_dyncall_with_multiple_overflow_entries_records_correct_overflow_addr(
10761076
"parent_stack_depth should be 17 (= pre-DYNCALL depth 18 minus 1)"
10771077
);
10781078

1079-
// There are 2 overflow entries at DYNCALL time (clocks T1 < T2).
1080-
// clk_after_pop_in_current_ctx() returns T1 (the second-to-last entry's clock).
1079+
// Independently determine T1 (clock of push(0)) and T2 (clock of push(HASH_ADDR)) by
1080+
// scanning the trace for all PUSH rows that appear before the DYNCALL row.
1081+
//
1082+
// The preamble BasicBlockNode contains 8 ops in order:
1083+
// [push(HASH_ADDR), mstorew, drop×4, push(0), push(HASH_ADDR)]
1084+
// All 8 fit in a single op-group, so they run at consecutive clocks within the span.
1085+
// The last two PUSH rows before DYNCALL are therefore:
1086+
// push_rows[n-2] → push(0) at clock T1
1087+
// push_rows[n-1] → push(HASH_ADDR) at clock T2 = T1 + 1
1088+
let push_opcode = Felt::from_u8(opcodes::PUSH);
1089+
let push_rows_before_dyncall: Vec<_> = main
1090+
.row_iter()
1091+
.filter(|&i| i < dyncall_row && main.get_op_code(i) == push_opcode)
1092+
.collect();
1093+
let n = push_rows_before_dyncall.len();
1094+
assert!(n >= 2, "expected at least 2 PUSH rows before DYNCALL, found {n}");
1095+
let t1_row = push_rows_before_dyncall[n - 2]; // push(0) → overflow[0]
1096+
let t2_row = push_rows_before_dyncall[n - 1]; // push(HASH_ADDR) → overflow[1]
1097+
let t1 = main.clk(t1_row);
1098+
let t2 = main.clk(t2_row);
1099+
// Sanity: push(0) and push(HASH_ADDR) execute at consecutive clocks in the same op-group.
1100+
assert_eq!(t2, t1 + ONE, "push(0) and push(HASH_ADDR) must be at consecutive clocks");
1101+
1102+
// clk_after_pop_in_current_ctx() returns T1 (the second-to-last overflow entry's clock).
10811103
// The buggy last_update_clk_in_current_ctx() would return T2 (the top entry's clock).
1082-
// Both T1 and T2 are nonzero (pushed during program execution, not at clock 0).
1083-
// Asserting ≠ ZERO verifies we got the correct second-to-last clock, not ZERO (which
1084-
// would indicate no overflow entry remained after the pop).
1085-
assert_ne!(
1086-
recorded_overflow_addr, ZERO,
1087-
"parent_next_overflow_addr must be the second-to-last overflow entry's clock \
1088-
(nonzero); ZERO would indicate either a missing overflow entry or the buggy \
1089-
last_update_clk_in_current_ctx() path when there is only 1 entry"
1104+
// Asserting exact equality with T1 (not just != ZERO) distinguishes the two code paths
1105+
// even when both T1 and T2 are nonzero.
1106+
assert_eq!(
1107+
recorded_overflow_addr,
1108+
t1,
1109+
"parent_next_overflow_addr must equal T1 (second-to-last overflow clock = {t1}); \
1110+
T2 (top overflow clock = {t2}) would indicate the buggy \
1111+
last_update_clk_in_current_ctx() path"
10901112
);
10911113
}
10921114

0 commit comments

Comments
 (0)