Repository navigation
evm: execute CGTv2 through direct upgrade deposits - #241
Conversation
ae9bba1 to
652c900
Compare
|
As built, the Upgrade18 install leaves the 0x…11 fee vault unconfigured, and anyone could take it over afterwards. Whatever we decide about Karst, this needs fixing.
The parity check missed this because I see two fixes. One is to drop 0x…11 from the install, so the live vault keeps its immutable config until Karst. The other is to also write slot 1, slot 2 and the init slot with each network's live values. Which one depends on whether we need |
Rebase PR #241 onto main at 8b21915. Keep the timestamp-based migration and the existing activation artifact. Use revm's database commit hook to report migration changes once, and forward the current refund snapshot APIs through CeloBlockExecutor. Preserve the inner conditional-commit override so declined candidates restore the producer's refund policy. The resolved executor source is celo-org/optimism at c9d5a6d69e1ec7efce7d453cc4cd9a424804bb9a. Celo owns a separate wrapper, so the new conditional-commit override must be forwarded explicitly. Reuse its ScriptedRefundPolicy regression to exercise that delegation. Keep main's per-EVM CIP-64 receipt storage and upstream Kona Driver. Restore the missing method-not-found helper for the PR's raw-RPC fallback.
1b6de36 to
827d31d
Compare
Rebase PR #241 onto main at 8b21915. Keep the timestamp-based migration and the existing activation artifact. Use revm's database commit hook to report migration changes once, and forward the current refund snapshot APIs through CeloBlockExecutor. Preserve the inner conditional-commit override so declined candidates restore the producer's refund policy. The resolved executor source is celo-org/optimism at c9d5a6d69e1ec7efce7d453cc4cd9a424804bb9a. Celo owns a separate wrapper, so the new conditional-commit override must be forwarded explicitly. Reuse its ScriptedRefundPolicy regression to exercise that delegation. Keep main's per-EVM CIP-64 receipt storage and upstream Kona Driver. Restore the missing method-not-found helper for the PR's raw-RPC fallback.
827d31d to
d01493b
Compare
Rebase PR #241 onto main at 8b21915. Keep the timestamp-based migration and the existing activation artifact. Use revm's database commit hook to report migration changes once, and forward the current refund snapshot APIs through CeloBlockExecutor. Preserve the inner conditional-commit override so declined candidates restore the producer's refund policy. The resolved executor source is celo-org/optimism at c9d5a6d69e1ec7efce7d453cc4cd9a424804bb9a. Celo owns a separate wrapper, so the new conditional-commit override must be forwarded explicitly. Reuse its ScriptedRefundPolicy regression to exercise that delegation. Keep main's per-EVM CIP-64 receipt storage and upstream Kona Driver. Restore the missing method-not-found helper for the PR's raw-RPC fallback.
d01493b to
24fe3b4
Compare
9d38b03 to
f0d25de
Compare
Rebase PR #241 onto main at 8b21915. Keep the timestamp-based migration and the existing activation artifact. Use revm's database commit hook to report migration changes once, and forward the current refund snapshot APIs through CeloBlockExecutor. Preserve the inner conditional-commit override so declined candidates restore the producer's refund policy. The resolved executor source is celo-org/optimism at c9d5a6d69e1ec7efce7d453cc4cd9a424804bb9a. Celo owns a separate wrapper, so the new conditional-commit override must be forwarded explicitly. Reuse its ScriptedRefundPolicy regression to exercise that delegation. Keep main's per-EVM CIP-64 receipt storage and upstream Kona Driver. Restore the missing method-not-found helper for the PR's raw-RPC fallback.
b2f0f7d to
6ccb72b
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
karlb
left a comment
There was a problem hiding this comment.
A few comments still describe earlier revisions of this PR instead of the current code:
.github/workflows/upgrade18_genesis_parity.yml:1: "Preserve the existing job identity while replacing the retired state-override gate". The file is new on main, so there is no existing job or retired gate. The file and job names (upgrade18_genesis_parity/genesis-parity) no longer match what it checks either ("CGTv2 upgrade fixture integrity").crates/kona/executor/src/test_utils.rs:315: "how the Upgrade 18 activation-artifact param overrides reach the executor". No such overrides exist; CGTv2 params come only from the chain ID.crates/kona/executor/src/test_utils.rs:29: "without fixture timestamp or reserve overrides" only makes sense next to the removed design.bin/execution-verifier/src/main.rs:67: "Celo-only fields such as the Upgrade 18 settings".CeloRollupConfighas onlyop_rollup_configandespresso.crates/alloy-celo-evm/src/block/mod.rs:79,executor.rs:240,crates/celo-reth/src/lib.rs:502, 537: these say an executor built outside the factory would "skip the Upgrade 18 transition". The migration now runs as ordinary deposits, so it would only skip CGTv2 group validation. That still splits consensus, but for a different reason.executor.rs:240's "silently fall back" would actually be a compile error.
| Ok(Err(e)) => { | ||
| tracing::error!("Block verification task failed: {e:#}"); | ||
| failures += 1; | ||
| } | ||
| Err(e) => { | ||
| tracing::error!("Block verification task panicked: {e}"); | ||
| failures += 1; | ||
| } |
There was a problem hiding this comment.
verify_block_range now ends with ensure!(failures == 0), which is right for range mode. When --start-block is given without --end-block, though, the backfill up to the first head runs as a task next to verify_new_heads. Its Err reaches the Ok(Err(e)) arm at L331, which cancels the shared cancel_token (stopping verify_new_heads) and returns. So one transient debug_executionWitness failure or one mismatch during the backfill also stops head-following and exits the process. Before this PR it was only logged. With --state-file, every restart takes this path.
Suggestion: return the error only when --end-block is set, and just log it in the backfill branch, as head-following does.
There was a problem hiding this comment.
Fixed in 9bdd459. Explicit --end-block range mode still returns verification errors. Background backfill now logs failures and restores head-following concurrency without failing the shared task group.
Added a regression test that injects a block RPC failure and checks that the backfill task completes without cancelling head-following or advancing verified progress.
| if step.is_some() && !op.inner.result.result.is_success() { | ||
| return Err(invalid_cgt_signal("required CGTv2 upgrade deposit reverted or halted")); | ||
| } |
There was a problem hiding this comment.
If any required step reverts or halts, the block is rejected. The deposits-only retry contains the same group, so derivation stops at the activation origin until a client release ships.
Shouldn't we undo the whole state change on any failure, so that we can retry the migration at a later time?
Use Celo-typed fixtures and preserve their rollup configuration. Fetch execution-witness preimages for fixture generation and block replay, and support explicit rollup configs and nonzero verifier failure exit codes. Let the proof host fall back to standard header/receipt RPC only when the raw-debug method is unavailable. Verify re-encoded header hashes.
Decode reviewed network bundles and construct reserve funding from fixed total supply minus the migrated amount. Validate exact required fields, source hashes, order and checked gas allowances. Keep production placeholders separate from the synthetic fixture and freeze the common NUT, RLP, prestate and provenance vectors.
Execute the required upgrade deposits through the ordinary EVM. Advance group state only for committed successful candidates and reject missing, failed, repeated or interrupted groups without client-side state overrides. Route execution through one wrapped factory, including reth post-exec paths. Cover rollback, database errors, reserve bounds, initialization guards and fee-vault state preservation.
Recognize activation deposits in node and proof payloads. Disable pool selection and reject forced user transactions on either side of the group; resume ordinary pool selection on the next block.
Check frozen fixture digests, gas allowances, prestate copies and optional Go/Rust byte parity. Validate and import reviewed per-network NUT bundles into Go JSON and Rust RLP without RPC calls or broadcasts. Add fixture/importer CI checks and document unresolved production inputs and release validation.
6ccb72b to
11d3ff2
Compare
|
The executor re-checks the whole CGTv2 group. Since celo-reth and the Kona proof only run blocks built by derivation, these checks can only fire if the two disagree, which halts the chain. Upstream doesn't check NUTs in the executor, and the L1 pause already lets us inspect a failed install before unpausing. Could we drop this and leave the executor CGTv2-agnostic? |
I'd prefer to keep the current all-or-nothing behavior. Yes, upstream doesn't check NUTs in the executor, but upstream executes the main predeploy upgrades atomically through I suggest rejecting and discarding the entire block candidate if any required step fails, then fixing the cause and retrying that activation block. This accepts a possible chain halt to prevent partial migration. If keeping the chain moving is the top priority, we could consider skipping the migration on failure. However, L1 migration would already be complete, so the L1 bridge would need to remain paused until L2 migration succeeds. We would also need to define how to roll back the migration while accepting the block, and a deterministic rule for which later block retries it. This would require a separate protocol design. Before activation, we should thoroughly test the final production bundle against mainnet state through op-node, celo-reth and Kona, including preceding origin deposits, initializer and |
Are we confident that we could quickly fix problems? Technically, I also prefer discarding the entire block (prevent getting ugly special cases into our chain history), but I'm not sure how much priority we want to put into avoiding unlikely cases of downtime.
I didn't really think through how this would work, so I have no idea yet how nice or ugly it would be.
If we get these tests realistic enough, we can probably rely on it and avoid further complication. I won't block the PR on this, I just wanted to point out the risk of a chain stall and make sure we consciously think about the trade off. |
|
@karlb How about merging this PR with the current all-or-nothing behavior, then reviewing a skip-and-retry design as the next step? We can investigate whether a failed migration could be fully rolled back while allowing the block and chain to continue, and how all nodes would deterministically retry the migration in a later block. We would decide whether to adopt that design before activation. Realistic mainnet-fork and cross-client tests would remain release requirements either way. |
Yes, that's good. You'll have to do the approve in this case, since I originally created the PR. |
I'm addressing the other reviews, so will ping you once it's ready, please take another look then. I'll approve after that. |
Keep bounded range failures fatal, but log background catch-up errors without failing the live head-following task group. Restore head concurrency when catch-up finishes, including after verification errors. Exercise a real range failure through a mocked RPC transport and verify that background completion succeeds without advancing verified progress. Review: #241 (comment)
Describe chain-ID parameters and upgrade-group validation accurately. Rename the fixture integrity workflow and job, updating both trigger paths to match the new filename. Review: #241 (review)
|
@karlb Addressed the outdated comments in this review in 9bdd459 and b70ae68. The workflow is now |
Execute CGTv2 through a required group of ordinary upgrade deposits shared by celo-reth and Kona. The direct contract upgrades are generated in celo-org/optimism#570 and derived by op-node in celo-org/optimism#567. The Kona derivation/prover integration is stacked in #364. Tracked in celo-org/celo-blockchain-planning#1466.
Activation ordering: all Portal deposits from the activation L1 origin, including deposits emitted after
EscrowSeeded, execute against the pre-migration L2 state. The complete CGTv2 group executes after all of that origin's deposits. Ordinary L2 user transactions are excluded from the activation block and resume in the next block.The shared executor validates the 15 static steps plus reserve funding, including exact transaction fields, required order and successful receipts. Failed, omitted, repeated or discarded steps invalidate the candidate; only committed candidates advance the group. Existing fee-vault settings, balances and
totalProcessedare preserved, without initializer resets. Execution checks reserve bounds and funding shape; op-node and Kona derivation authenticate the exactEscrowSeededamount used fortotalSupply - migratedAmount.Each active network selects its own compiled bundle. The current production artifacts are empty placeholders and fail explicitly until reviewed bundles and final bridge addresses are supplied. Final deployment inputs, gas calibration and production node/prover parity remain release requirements. The integrity CI checks the upgrade-deposit artifacts and production bundle importer.
Local validation on
6ccb72bbf9e628ae084df15ef09ee0ad42a801e9, rebased onto main41e9d9ee497280b49eee53e1f559c860c58b4315:just testpasses 792 tests (2 skipped),just lint-nativepasses, and the importer passes 5 Python tests. Frozen fixture integrity and Go/Rust byte parity pass. Nextest reports one LEAK warning in the full run; the affected test passes without that warning when run alone.