Validator: compare computed end state against expected in JIT and Arbitrator#4563
Open
pmikolajczyk41 wants to merge 12 commits intomasterfrom
Open
Validator: compare computed end state against expected in JIT and Arbitrator#4563pmikolajczyk41 wants to merge 12 commits intomasterfrom
pmikolajczyk41 wants to merge 12 commits intomasterfrom
Conversation
…tsAt with entry.End
pmikolajczyk41
commented
Mar 26, 2026
| return server_api.InputJSON{}, err | ||
| } | ||
| return *server_api.ValidationInputToJson(input), nil | ||
| jason := server_api.ValidationInputToJson(input) |
Member
Author
There was a problem hiding this comment.
to avoid shadowing global package name
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4563 +/- ##
==========================================
- Coverage 36.05% 34.29% -1.76%
==========================================
Files 498 498
Lines 58976 58978 +2
==========================================
- Hits 21264 20228 -1036
- Misses 33889 35176 +1287
+ Partials 3823 3574 -249 |
Contributor
❌ 12 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
bragaigor
previously approved these changes
Mar 26, 2026
Contributor
bragaigor
left a comment
bragaigor
left a comment
There was a problem hiding this comment.
LGTM just one question. The other thing is, we don't have any tests or assurances that if we remove this "field" ExpectedEndState it would go unnoticed. But I guess we don't care?
KolbyML
previously approved these changes
Mar 26, 2026
looks good
crates/prover/src/main.rs
Outdated
| } | ||
|
|
||
| let file = File::open(path)?; | ||
| let req: ExpectedState = serde_json::from_reader(BufReader::new(file))?; |
Member
There was a problem hiding this comment.
Suggested change
| let req: ExpectedState = serde_json::from_reader(BufReader::new(file))?; | |
| let req = serde_json::from_reader::<ExpectedState>(BufReader::new(file))?; |
Also is req? a request?
crates/prover/src/main.rs
Outdated
Comment on lines
+477
to
+483
| let gs = mach.get_global_state(); | ||
| let actual = GoGlobalState { | ||
| block_hash: gs.bytes32_vals[0], | ||
| send_root: gs.bytes32_vals[1], | ||
| batch: gs.u64_vals[0], | ||
| pos_in_batch: gs.u64_vals[1], | ||
| }; |
Member
There was a problem hiding this comment.
Does it make sense to just implement the From trait for this type conversion.
Member
There was a problem hiding this comment.
#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)]
#[repr(C)]
pub struct GlobalState {
pub bytes32_vals: [Bytes32; GLOBAL_STATE_BYTES32_NUM],
pub u64_vals: [u64; GLOBAL_STATE_U64_NUM],
}
^ this is gross but maybe it is this way due to the c ffi? This GlobalState code is 4+ years old so definitly not related to this PR, but it is indeed gross, so just thinking out loud this comment doesn't block this pr or anything
Member
Author
There was a problem hiding this comment.
yeah, I think it is required for C FFI; anyway, implemented From conversion in e657275
Member
There was a problem hiding this comment.
if it is required, I wonder if it is possible to use getters and setters and make the fields non-public then, just to avoid people from indexing these throughout the codebase, we can contain them more.
But this isn't related to this PR of course
Member
Author
There was a problem hiding this comment.
indexing these fields is a common pain point raised already in other PRs :/ (e.g. #4521 (comment)) - will take care of it at some point, I promise 😅
KolbyML
previously approved these changes
Mar 26, 2026
crates/jit/src/main.rs
Outdated
Comment on lines
+70
to
+84
| fn get_expected_state(opts: &Opts) -> Result<Option<GoGlobalState>> { | ||
| match &opts.input_mode { | ||
| jit::InputMode::Json { inputs } => { | ||
| let file = File::open(inputs)?; | ||
|
|
||
| // Use a temporary struct with the only interesting field, to avoid parsing all other data. | ||
| #[derive(Deserialize)] | ||
| #[serde(rename_all = "PascalCase")] | ||
| struct ExpectedState { | ||
| #[serde(default)] | ||
| pub expected_end_state: Option<GoGlobalState>, | ||
| } | ||
|
|
||
| let req: ExpectedState = serde_json::from_reader(&mut BufReader::new(file))?; | ||
| Ok(req.expected_end_state) |
Member
There was a problem hiding this comment.
is it possible to do the same thing to this function as well? anyways this doesn't block this PR
KolbyML
approved these changes
Mar 26, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Adds an optional
ExpectedEndStatefield to InputJSON. When present, both the JIT and Arbitrator prover binaries compare the computedGoGlobalStateagainst it after execution and exit non-zero on mismatch.ValidationInputsAtnow populates this field fromentry.End, so block input JSON files exported via the debug API carry the ground-truth end state. CI is updated to pass a testdata file containingExpectedEndStateand assert that both binaries print"Computed state matches the expected one".The field is intentionally not added to the Rust
ValidationRequeststruct — that struct is used in contexts (e.g. the validation server, machine construction) where the field is meaningless and would only cause confusion. Instead, each binary performs a lightweight partial parse of the JSON at startup to extractExpectedEndStatebefore the normal machine-initialization path, keeping the deeper layers unmodified.This catches divergence between the Go execution path and the Rust/WASM execution path — the core correctness property of the validator.
closes NIT-4248