Tentative canonical guest program input support for Ethrex#44
Conversation
…ming with versioning support
… JSON fixture path collection
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Co-authored-by: Copilot <copilot@github.com>
…pdated to conform to the specs
jsign
left a comment
There was a problem hiding this comment.
@han0110, I think this is ready for review without rush to gain a bit of time.
Still, for this PR to be merged we need to wait to see what happens with lambdaclass/ethrex#6550. Maybe that happens next week.
So mainly kind of half-opening for review. :)
| pub fn get_stateless_validator_output( | ||
| block_hash: B256, | ||
| success: bool, | ||
| chain_id: u64, |
There was a problem hiding this comment.
Technically is needed for the spec StatelessValidationResult.
This part of the API is a bit unstable, so not sure yet if this will be the final form. Opened a convo to explore this a bit more with this doc.
For now sticking to the current specs -- we can easily adjust whenever that convo comes to some conclusion and we change the specs.
There was a problem hiding this comment.
This got already resolved in the latest release -- instead of chain_id : u64 we have a bit more comprehensive struct as input/output.
I think will take a bit of time for ELs to support this latest release, since it indirectly forces them to now support all changes in bal-devnet-7.
Leaving this as is now since it is the way that the latest v0.3.4 release works and most people are using/passing.
I soon can add a more focused PR supporting hte latest thing, or maybe if we are ambitious enough remove all this guest program wrappers and expect all ELs to return the output bytes themselves. Not sure yet how agressive we can be in shor term.
| HashMap::from([ | ||
| ( | ||
| b256!("e4bd1c4dc22a58a0a9a8e789e2c54b4ace2d1ebc16a605c3976723b52fc011f1"), | ||
| b256!("45328434f812b65daa21b4e8a3d6440d0da95fbd95a6c10b0a28f081cab53bd5"), |
There was a problem hiding this comment.
These things had to be updated since MAX_WITHDRAWALS_PER_PAYLOAD had the wrong value compared to the specs. The specs define a non-consensus compliant value for some testing framework problems.
Until those are fixed, we should stick to what the spec choose to allow running EEST tests properly.
So, since MAX_WITHDRAWALS_PER_PAYLOAD changes, that affects hash_tree_root results, thus why these updates are here.
| /// Chain ID from the stateless validation chain configuration. | ||
| pub chain_id: u64, |
There was a problem hiding this comment.
|
|
||
| /// Limits | ||
| pub const MAX_WITHDRAWALS_PER_PAYLOAD: usize = 16; | ||
| pub const MAX_WITHDRAWALS_PER_PAYLOAD: usize = 65536; // Re-defined in EIP-8025 at least temporarily. |
There was a problem hiding this comment.
There was a problem hiding this comment.
I also resolved this problem in latest zkevm@v0.4.0 -- for now leaving it compatible with v0.3.4 since it is the thing most people are using. As said before, I can update this soon to the original value, or maybe we can nuke everything here and expect this be a responsibility of the EL.
There was a problem hiding this comment.
TL;DR is that stateless-validator-debug now supports two fixtures:
- The usual workload fixture -- nothing changed (called
Legacynow) - Literal fixtures from EEST releases
For the later, basically each EEST test block has a statelessInputBytes and statelessOutputBytes. We use literally both to run guest and check correctness.
Ideally, asap, we can simply always use this canonical format which would be great. Simple format (just bytes), no manipulation in ere-guests, and all ELs receive the same. Meanwhile, we still support both.
| if let Some(expected_output_bytes) = &summary.expected_output_bytes { | ||
| return handle_output_mismatch( | ||
| summary, | ||
| fixture_path, | ||
| expected_output_bytes, | ||
| allow_fixture_mismatch, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Reg checking the guest output -- if we have output bytes it means we ran an EEST fixture. Compare against the fixture statelessOutputBytes literally. If not, do what we did before.
| stateless_input: &StatelessInput, | ||
| valid_block: bool, | ||
| ) -> anyhow::Result<Vec<u8>> { | ||
| pub fn build_eip8025_input(source: Eip8025InputSource<'_>) -> anyhow::Result<Vec<u8>> { |
There was a problem hiding this comment.
Still single method to build Ethrex input, just that now we have these variants.
For workload/zkboost, they would just need to use Legacy as usual, and all should be okay. I think at some point we could make some helper to construct he canonical form from the eth_getBlock+debug_executionWitness and we could switch over everything to Canonical.
But trying to go a bit in steps to avoid complicating things too agressively.
| )) | ||
| } | ||
|
|
||
| fn to_ethrex_chain_config(cfg: &alloy_genesis::ChainConfig) -> ChainConfig { |
There was a problem hiding this comment.
This is just extracted from below.
There was a problem hiding this comment.
Explained a bit in the PR description. Can also be useful to see lambdaclass/ethrex#6550 but just "the expected thing" there.
han0110
left a comment
There was a problem hiding this comment.
So far LGTM! Will check again when the Ethrex PR is merged
|
|
||
| fn decode_from_slice(slice: &[u8]) -> Result<Self, Self::Error> { | ||
| if slice.len() < STATELESS_VALIDATOR_OUTPUT_SIZE { | ||
| if slice.len() != STATELESS_VALIDATOR_OUTPUT_SIZE { |
There was a problem hiding this comment.
The check was loose to support ZisK that has fixed size output 256 bytes, but since we always hash the output so this never gets called.
I think we can leave it stricter as is, and to be compatible with our zkVM standard IO interface in the future, I'm thinking we can probably force the output to be [u8; 32] so this Decode impl can be even removed.
| pub fn get_stateless_validator_output( | ||
| block_hash: B256, | ||
| success: bool, | ||
| chain_id: u64, |
There was a problem hiding this comment.
This got already resolved in the latest release -- instead of chain_id : u64 we have a bit more comprehensive struct as input/output.
I think will take a bit of time for ELs to support this latest release, since it indirectly forces them to now support all changes in bal-devnet-7.
Leaving this as is now since it is the way that the latest v0.3.4 release works and most people are using/passing.
I soon can add a more focused PR supporting hte latest thing, or maybe if we are ambitious enough remove all this guest program wrappers and expect all ELs to return the output bytes themselves. Not sure yet how agressive we can be in shor term.
|
|
||
| /// Limits | ||
| pub const MAX_WITHDRAWALS_PER_PAYLOAD: usize = 16; | ||
| pub const MAX_WITHDRAWALS_PER_PAYLOAD: usize = 65536; // Re-defined in EIP-8025 at least temporarily. |
There was a problem hiding this comment.
I also resolved this problem in latest zkevm@v0.4.0 -- for now leaving it compatible with v0.3.4 since it is the thing most people are using. As said before, I can update this soon to the original value, or maybe we can nuke everything here and expect this be a responsibility of the EL.
| /// Chain config sourced from the fixture. | ||
| chain_config: &'a alloy_genesis::ChainConfig, |
There was a problem hiding this comment.
As mentioned before, in zkevm@v0.4.0, now chain_config will be part of ssz_input, so this will get simpler soon when more people start targeting that release 😄
|
@han0110, added some boring extra commits compared to last review, plus some extra review comments (not that interesting either, just signaling good stuff happened pushing things in this repo to be simpler). As we try to push ELs to stick to the latest standard, I think we can keep iterating on this changes topic to polish things better. |
This PR adds tentative support for the canonical EEST
statelessInputBytespayload as a guest program input to the Ethrex stateless validator, alongside the existing legacyStatelessInputflow.The goal is to start exercising the Ethrex guest with the canonical format that EEST's
blockchain_testfixtures emit, without breaking the existing legacy fixture path used by the rest of the stack.Ideally in the future, we would like to use this new
Canonicalstateless input inere-guestssince basically simplifies a lot of gymnastics we do today reg input preparation. ThisCanonicalstyle is literally receiving fully prepared SSZ inputs from EEST fixtures, and mostly passing them directly to the guest program.It is only implemented for Ethrex since I created a PR to support it lambdaclass/ethrex#6550.
Notes:
Legacyinput style.Canonicalin their current style would simply add more tech debt to our repo that should eventually be removed after.What changed
Host-side input construction (
stateless-validator-ethrex)Eip8025InputSourceenum with two variants:Legacy { stateless_input, valid_block }— the existing path, unchanged in behaviourCanonical { ssz_input, chain_config }— accepts EESTstatelessInputBytesdirectlybuild_eip8025_input(...)now dispatches on the variant; the legacy path is moved tobuild_legacy(...)and a newbuild_canonical(...)rkyv-encodes the chain config and frames it together with the SSZ inputalloy_genesis::ChainConfig -> ethrex ChainConfigconversion into a sharedto_ethrex_chain_config(...)helper so both paths use the same logicencode_eip8025_canonical(ssz_input, rkyv_chain_config_bytes)producing[ssz_len: u32 LE][ssz_bytes][cfg_len: u32 LE][rkyv_chain_config_bytes]. This is a tentative format I created, but I think is quite solid. (ChainConfigisn't part of the specs, so this is why is passed "separately", but at least all the rest is proxied directly from the generated EEST fixture stateless input)Fixture loading (
stateless-validator-debug)fixtures.rsmoduleStatelessValidatorFixturenow wraps aFixtureInput::{Legacy, Canonical}enumload_fixtures(path)auto-detects the JSON shape:{ name, stateless_input, success }→ one fixtureblockchain_testmap → one canonical fixture per(test, block)with a populatedstatelessInputByteschain_config_for_test(...)builds analloy_genesis::ChainConfigfrom the EESTnetwork+chainid+blobScheduleusingreth_chainspec::create_chain_configand theef-testsForkSpecstatelessrevision doesn't expose it yet — temporary workaround until upstream catches up. This allows to try make progress inere-guests@maininstead of other branches.reth guest does not yet accept EEST canonical SSZ input); only Ethrex consumes it