feat: update ere#59
Conversation
51e363a to
07f290d
Compare
jsign
left a comment
There was a problem hiding this comment.
Nice! LGTM! Left some comments for your consideration, but nothing blocking.
| @@ -0,0 +1,8 @@ | |||
| #[unsafe(no_mangle)] | |||
| extern "C" fn native_keccak256(bytes: *const u8, len: usize, output: *mut u8) { | |||
There was a problem hiding this comment.
Just to check if I understand correctly: this is just to adjust a naming mismatch between what extern keccak func Reth wants in their Crypto trait with the naming that we use in zkevm_interface (and SP1 sticks to). No?
There was a problem hiding this comment.
This is a leaking from the Reth that zkvm-standards didn't cover, in alloy-primitives if we don't want to patch the crate (sha3 or tiny-keccak), we need to provide this native_keccak256 so alloy-primitives routes the keccak256 to it, and SP1's zkvm-interface impl doesn't contains that, so we need to patch it here (ZisK doesn't need one because ziskos exposes one)
If stateless repo eventually publishes a rellocatable ELF, this should be move to there so the ELF doesn't depend on symbol outside zkvm-standards
| }) | ||
| } | ||
|
|
||
| /// Rewrites the schema prefix of `ElectraFulu` variant payload to `0x0000`. |
There was a problem hiding this comment.
Uhm, I see. Technically in the spec 0x0000 is not a valid value (was done intentionally to avoid mistakes i.e. avoiding default values just in case).
But I guess Nethermind assumed 0x0000 was for Fulu (since we started in the spec in Glamsterdam).
Is this correct?
I could meditate change 0x0002 for Glamsterdam and leave 0x0001 for Fulu so we can prove mainnet blocks.
Not a huge deal now. LMK your thoughts and if I'm understanding the situation correctly.
There was a problem hiding this comment.
But I guess Nethermind assumed 0x0000 was for Fulu (since we started in the spec in Glamsterdam).
Yes
I could meditate change 0x0002 for Glamsterdam and leave 0x0001 for Fulu so we can prove mainnet blocks.
I'm not sure in which case we'd bump the schema id, but if it's going to be bumped for each fork, leaving 0x0001 for Fulu and lifting Glamsterdam to 0x0002 sounds good to me (tho not sure if it's worth doing so since the Glamsterdam will land soon and Fulu support might not be necessary)
There was a problem hiding this comment.
Uhm, yeah agree. Maybe better keep Glamsterdam = 0x0001 and keep schema-id as a structural signal of things rather than forks. At least that was my original intention, since for forks we already have ChainConfig.
So a bit two orthogonal things -- no need to get distracted by this 0x0000 ad-hoc value usage.
There was a problem hiding this comment.
Yeah agree, even without the schema id hint, I think the guest can still decode the ExecutionPayload correctly by the fork passed in (I did that in e0567c3).
There was a problem hiding this comment.
Yeah, maybe that's simpler for guest programs instead of the SSZ "magic" of reading some fields first as that commit.
So schema-id might end up being fork attached (plus potentially encoding attached, but doubt usage of this dimension will happen soon really; mostly a future proof thing that we won't be touching much reg encodings or extra stateless input fields for same fork)
statelesstoparadigmxyz/statelessethrextov19.0.0ereto haveopenvm@v2.0with 256 bytes public values (configured byere), andsp1withzkvm-standardsimplethrexandrethguests now outputstateless_output_bytesinstead of hash, sinceopenvmandziskthey both support up to 256 bytes now (andsp1doesn't have limitation). Andzesuandnethermindboth outputstateless_output_bytes, so the released guests are at least compatible on V4 payload fixtures (Amsterdam).airbenderandrisc0guestsethrex-openvmguestzkvm-interfacecrypto impl forsp1guestszesu-zisktozisk@v1.0.0-alpha(only support V4 payload).nethermind-zisk(support V3 payload by tweaking the schema id to0x0000)artifact-registry.jsonas the single source of truth to do therepublish(read the json and generate job matrix).The blockers of linking guest lib and zkVM lib:
zisk- The lib currently does allocation, but will be fixed in the next release (fixed by Feature/ziskos isolated allocator 0xPolygonHermez/zisk#1157)sp1- Same and will require some more changes in upstream to avoid the allocation.So after
zisk@v1.0.0-betais released, we should be able to linkzesuwithziskwith a small shim (which relaxes the zkVM version matching a bit, and we only need to select the right EL version).