Tibfox/review5 split - #3
Open
tibfox wants to merge 7 commits into
Open
Conversation
W4 Cluster B's main.go re-entry guard references ce.ErrState but the
contract/contracterrors package didn't define it. Build failed with
'undefined: ce.ErrState' until this const landed. Added as:
ErrState = ErrorSymbol("invalid_state_transition")
placed between ErrInitialization and ErrIntent (alphabetical-ish grouping
with the other state/init lifecycle errors).
Discovered during post-implementation build verification of the W4 Cluster
B mega-commit; logically belongs with the W4-B work but landed in its own
commit so the errors-package change is reviewable in isolation.
…r, HIGH #22 ELF hash handler Implements zk-header-verifier-side fixes from clauderfly's MILO-REVIEW-PACKAGE-2026-05-24 Wave 4 Cluster B (chain binding) and CRIT #25 (lastBlockHash anchor). CRIT #6 sites 5+6 (chain binding through proof outputs): - parseProvenFields returns 5 values (added provenChainId from slot 11) - PvFieldChainId=384, PvMinLenWithChainId=416; legacy PvMinLen=288 retained for migration window - submitProof binds provenChainId into storeHeader (matches account-mapping's 137-byte EthBlockHeader layout) CRIT #25 lastBlockHash anchor (was: prevHeader unbound across batches): - New state slot KeyLastBlockHash - InitContractParams extended (InitialBlockHash, InitialHeight, IsTestnet) - Re-entry guard at top of initContract (refuses re-init; uses ce.ErrState from the prior commit) - submitProof rejects with ErrInitialization 'anchor not set' if KeyLastBlockHash empty - parsed[0].ParentHash MUST match stored lastBlockHash; updated to last header's BlockHash on success - looksLikeBlockHash shape validator for the init param HIGH #22 ELF hash pinning (contract side only): - setExpectedElfHash admin handler with shape validation - KeyExpectedElfHash state slot - OPEN GAP: prover-side ELF SHA-256 read + compare + fatal-exit on mismatch is NOT in sp1-helios-magi/operator.rs yet. Without that, the on-chain handler is hollow. Flag for follow-up PR. Test file rewritten to cover new arity, boundary at PvMinLenWithChainId, looksLikeBlockHash shape, legacy corpus rejected. Build status: 'go build ./...' clean; 'go vet ./...' clean.
…ored only)
Team testnet review found Site 6 was a half-fix: parseProvenFields
returned provenChainId and storeHeader persisted it, but submitProof
never COMPARED it against an expected value. A valid SP1 proof generated
against the wrong chain (e.g. Sepolia proof submitted to a mainnet
verifier) would be accepted and its headers written as authoritative —
defeating the entire CRIT #6 chain-binding fix.
Enforcement added:
- InitContractParams gains required ExpectedChainId (non-zero) — the L1
chainId this verifier instance is bound to.
- init persists it to new state slot KeyExpectedChainId ("cid"). Immutable
after init (no setter — chain binding must not be admin-mutable, same
rationale as account-mapping dropping setChainId, CRIT #6 Site 12).
- submitProof now reads KeyExpectedChainId and reverts (ErrTransaction)
if provenChainId != expectedChainId. Also reverts (ErrInitialization)
if the slot was never set.
This is the consumer/enforcement half of Site 6. The storage half (from
the prior W4-B commit) is unchanged.
OPERATOR NOTE: initContract now REQUIRES expected_chain_id in the deploy
params (mainnet=1, Sepolia=11155111). Add to W0 P5 deploy runbook.
Build: 'go build ./...' + 'go vet ./...' clean.
…s_testnet) review5-split audit: the README init example listed only groth16_vk/vk_root/ sp1_vkey_hash. Following it verbatim hit the fail-closed abort at contract/main.go (now requires initial_height + initial_block_hash + expected_chain_id). Added the full example + a required-fields section explaining the CRIT #25 anchor, CRIT #6 Site 6 expected_chain_id, and the is_testnet sentinel. (#21 submitProof chainId-enforcement tests + #22 ELF-pin consumer tests: the enforcement is reachable only via wasmexport / Rust-fatal paths that need an SDK/process harness the repos' unit tests don't have; closing proof is TESTNET-LIVE-SIM / DEVNET per the test plans, not go test — which TESTING-MANIFEST excludes as a proof tier anyway.)
Follow-up review 2026-05-31. updateVkey and setExpectedElfHash were instant owner-only writes while the comments claimed a 400K-block timelock that didn't exist. Owner-key compromise could instantly rotate the SP1 vkey -> accept proofs from an arbitrary program -> forge L1 state -> drain. - Add propose/execute/cancelProposal/expireProposal lifecycle with TimelockLong=400_000 (ported from account-mapping admin/proposals.go). - updateVkey reachable ONLY via execute after the timelock (first vkey write still in init). - setExpectedElfHash: instant first pin (deploy ramp-up, nothing trusted to rotate from), rotations timelocked. - Delete the false "timelock" comments. Tested (real WasmEdge host) by the reviewer: execute at execHeight-1 rejected, exact-execHeight succeeds; first ELF pin instant, second direct call rejected; cancelled/expired proposals can't execute; expireProposal permissionless but inert. Patch credit: review5-fixes-for-tibfox 2026-05-31 F1.
review5-fixes-for-tibfox 2026-05-31 flagged pre-existing minor: README listed `updateVkey` as a direct Owner action, but F1 made it propose/execute- gated under the 400K-block timelock. setExpectedElfHash is now first-pin- direct + rotations propose/execute. Replaced the 3-row table with the full lifecycle: init, propose, execute, cancelProposal, expireProposal, setExpectedElfHash (with the first-pin exception explained), submitProof.
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
2 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.
This pull request updates the WASM contract and its documentation to support stricter proof validation, introduce timelocked admin actions, and improve test coverage for new consensus proof fields. The most important changes are the addition of new admin proposal flows, mandatory chain ID anchoring for proofs, and expanded tests to cover these changes and edge cases.
Contract interface and admin flows:
propose,execute,cancelProposal,expireProposal) for sensitive actions likeupdateVkeyandsetExpectedElfHash, instead of allowing direct execution. Theinitaction now sets additional parameters and is strictly one-shot. (README.md)updateVkeyis now only accessible via the proposal/timelock mechanism, increasing security for verification key updates. (README.md)Proof validation and anchoring:
initaction now requiresinitial_height,initial_block_hash,expected_chain_id, andis_testnet, enforcing strict chain binding and anchoring.submitProofnow checks that the proof's chain ID matches the expected value. (README.md)ErrStateerror symbol for invalid state transitions, improving error handling for new state logic. (contracterrors/errors.go)Testing and validation improvements:
main_test.go) [1] [2] [3] [4]looksLikeBlockHashutility to ensure only valid block hashes are accepted as anchors. (main_test.go)Documentation:
README.mdto reflect new contract requirements and admin flows, including detailed explanations of required fields and their security implications. (README.md)These changes collectively strengthen the contract's security posture, ensure proofs are strictly bound to the correct chain, and provide robust testing for new data structures and admin flows.