feat: merge dev into main #416
Conversation
… to `ProtocolVault` (#387) * feat(contracts): withdraw remaining validator vault balance to protocol vault * feat(contracts): apply PR review
* feat: separate `KromaOutputV0` from `OutputV0` * feat: add `KromaMPTTime` and `L2GenesisKromaMPTTimeOffset` * feat: add withdrawal proving in MPT
* feat(contracts): save parent L1 block hash when creating challenge * feat(contracts): separate `ZKProofVerifier` and support zkVM proving * feat(contracts): modify deploy scripts for `Colosseum`, `ZKProofVerifier` * feat(validator): branch challenger's proving fault based on MPT time * refac: separate `proveFault` function * refac(contracts): move private function below external function * fix(contracts): change the order of params of `verifyZKVMProof` * refac(contracts): rename variables to pascal case
* feat(contracts): revive `getChallenge` method of `Colosseum` * feat(validator): prove fault with witness generator, zkVM prover * feat: verify zkVM program verification key * fix: launch kroma-challenger in devnet * chore(contracts): update bindings and snapshot * feat: use rpc client for proof fetcher, witness generator
This commit sets up Nx Cloud for your Nx workspace, enabling distributed caching and the Nx Cloud GitHub integration for fast CI and improved developer experience. You can access your Nx Cloud workspace by going to https://cloud.nx.app/orgs/672d775fe7535d520b298d46/workspaces/672d7b30b76a0e014484fab5 **Note:** This commit attempts to maintain formatting of the nx.json file, however you may need to correct formatting by running an nx format command and committing the changes.
* chore: rename `ColosseumTestData` to `ZkEvmTestData` * fix(e2e): add missing error check * test(contracts): add `proveFaultWithZkVm` test * test(e2e): add prove fault with zkVM tests * chore: bump kroma-geth to v0.5.4 * feat: apply PR reviews
* feat: separate DepositTx into OP version and Kroma version * test: resolve test failures due to different deposit tx types * feat: set suggested fee recipient for payload attributes * test: add differential testing for KromaDepositTx * fix: remove unnecessary KromaDepositTx conversion
* feat(contracts): implement restrict output submit, create challenge * feat(contracts): add test code * chore: adjust PR review * feat: Change to structure using immutable MPT_FIRST_OUTPUT_INDEX * chore: update deploy code * chore: update bindings, contracts-snapshot * feat: adjust PR reviews * feat: adjust second PR reviews * feat: adjust third PR reviews
* wip: change predeployed contract addresses * chore: adjust changes * feat: add test for kromaMPT upgrade * chore: update dependency * fix: fix reorg problem * chore: fix typo * chore: adjust PR reviews * feat: set noTxPool in MPT parent block * chore: adjust PR reviews - 2 * chore: adjust PR reviews - 3 * chore: adjust PR reviews - 4 * feat: modified to provide chainID-specific deployment bytecode * chore: adjust PR reviews - 5 * chore: adjust PR reviews - 6 --------- Co-authored-by: Pangsu <[email protected]>
* feat: upgrade GasPriceOracle in MPT parent block * feat: adjust PR reviews * feat: adjust second PR reviews
* test(e2e): add system test on withdrawal after MPT transition * feat: apply PR review
* feat: increase gas limit for GasPriceOracleMPT - actual consumption about 1.2 million gas * chore: update gas limit in test
* feat: separate KromaL1Block and L1Block contract * feat: apply PR review * feat: apply second PR review
* fix: prevent race condition on L1Block address * chore: bump geth version * chore: remove devops from codeowners --------- Co-authored-by: seolaoh <[email protected]>
* test(e2e): add fee distribution test in Kroma MPT * test(e2e): apply PR review * test(e2e): apply second PR review * fix(e2e): find available port to avoid timing issues * fix(e2e): get historical node port after setup
* test(e2e): add fee distribution test in Kroma MPT * test(e2e): apply PR review * test(e2e): apply second PR review * fix(e2e): find available port to avoid timing issues * fix(e2e): get historical node port after setup * feat(chain-ops): add check-kroma-mpt command * chore(hardhat-deploy-config): revise mpt and validator V1 configs in devnet * feat(chain-ops): apply PR review * chore(hardhat-deploy-config): revert validator V1 configs in devnet
* test(e2e): add a test for kromampt upgrade transactions * feat: apply first PR review * feat: apply second PR review * feat: change L1Block to KromaL1Block * feat: apply third PR review * feat: add fourth PR review
…st (#408) * chore(bindings): add bindings of SP1 Verifier * test(e2e): add deploy functions for SP1 verifier and ZK Proof verifier * test(e2e): enable zkVM fault proof test * chore: update bindings and devnetL1 template * test(e2e): remove unused zkVM challenges on actions test * test(contracts): replace with new zkVM testdata and enable zkVM challenge test * feat: apply first PR review * feat: apply second PR review
feat: implement MPT migration and zkVM fault proof transition
* feat(validator): request zkVM proof only when necessary * feat(validator): apply PR reviews * fix(validator): fix bugs in challenger * feat(validator): apply PR review * feat(validator): immediately return challenge loop after cancelChallenge
Signed-off-by: dashangcun <[email protected]>
WalkthroughThis pull request introduces comprehensive changes across multiple repositories and packages, focusing on the Kroma Merkle Patricia Tree (MPT) upgrade. The modifications span various components of the Ethereum layer 2 infrastructure, including bindings, chain operations, node configurations, and testing frameworks. Key areas of change include contract bindings, configuration updates, transaction handling, and the introduction of new proof verification mechanisms for zkEVM and zkVM challenges. Changes
Sequence DiagramsequenceDiagram
participant User
participant L1Portal
participant KromaL1Block
participant ValidatorManager
participant ZKProofVerifier
User->>L1Portal: Initiate Deposit
L1Portal->>KromaL1Block: Update Block Information
KromaL1Block-->>ValidatorManager: Notify Block Update
ValidatorManager->>ZKProofVerifier: Request Proof Verification
ZKProofVerifier-->>ValidatorManager: Verify Proof
ValidatorManager->>L1Portal: Confirm Transaction
This sequence diagram illustrates the high-level interaction between key components during a transaction in the Kroma MPT upgrade, showcasing the flow of deposit transactions, block updates, and proof verification. Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
View your CI Pipeline Execution ↗ for commit 2b9ef9e.
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
op-e2e/e2eutils/mock_proof_fetcher.go (1)
Line range hint
116-148: Properly utilize context inprovemethod to handle cancellations.The
provemethod initializes an error group witherrgroup.WithContext, but the returned context is discarded:g, _ := errgroup.WithContext(ctx)This means context cancellation signals are not propagated to the goroutines. Modify the code to use the returned context to ensure cancellations are properly handled.
Apply this diff to fix the issue:
-func (m *MockRPCWithData) prove(ctx context.Context) (*chal.ZkEVMProveResponse, error) { - g, _ := errgroup.WithContext(ctx) +func (m *MockRPCWithData) prove(ctx context.Context) (*chal.ZkEVMProveResponse, error) { + g, ctx := errgroup.WithContext(ctx)
🧹 Nitpick comments (38)
op-node/rollup/derive/attributes_queue_test.go (2)
76-79: Document the beacon root initialization logic.The parent beacon root initialization logic for Ecotone could benefit from a comment explaining why we default to zero hash when no beacon block root is available.
+ // Default to zero hash for Ecotone when parent beacon root is unavailable + // This maintains compatibility with the Ecotone upgrade requirements if cfg.IsEcotone(safeHead.Time+cfg.BlockTime) && parentBeaconRoot == nil { parentBeaconRoot = new(common.Hash) }
115-128: Consider adding edge case tests.While the basic post-MPT test case is good, consider adding tests for:
- Transition exactly at MPT time
- Error cases for invalid L1 info deposits
- Interaction between MPT and other protocol upgrades
op-e2e/e2eutils/mock_proof_fetcher.go (2)
34-65: Enhance error handling inCallContextmethod ofMockRPC.The
CallContextmethod handles specific RPC methods but returns a generic error message for unhandled methods. Consider providing more informative error messages or handling additional methods if necessary, to improve debugging and maintainability.
95-113: Consistent context handling inCallContextofMockRPCWithData.In
MockRPCWithData, theCallContextmethod mirrors that ofMockRPC. Ensure that context cancellation and errors are consistently handled, and consider refining error messages for unhandled methods to aid in debugging.op-node/node/api.go (1)
210-210: Implement the TODO to reuse the fetched proofThere's a TODO comment at line 210 indicating that the proof fetched in the
fetchKromaOutputAtBlockfunction should be reused. Implementing this refactor would improve efficiency by eliminating redundant proof fetching and reducing unnecessary network calls.Do you want me to help refactor the code to reuse the proof fetched in
fetchKromaOutputAtBlock, or should I open a GitHub issue to track this enhancement?kroma-validator/config.go (4)
265-289: Avoid Usingtime.Now()Directly for Configuration ChecksUsing
time.Now()directly inrollupConfig.IsKromaMPT(uint64(time.Now().Unix()))can lead to non-deterministic behavior and issues in testing or when the system clock is adjusted. Consider passing a timestamp or block number explicitly to ensure deterministic and testable code.Apply this diff to inject the current time as a parameter:
- if rollupConfig.IsKromaMPT(uint64(time.Now().Unix())) { + currentTime := uint64(time.Now().Unix()) + if rollupConfig.IsKromaMPT(currentTime) {Alternatively, accept a
currentTimeparameter in the function or struct to improve testability.
286-288: Enhance Error Message with Detailed Version MismatchThe error message
"SP1 version of zkVM prover and witness generator mismatched"lacks specific version details, which could aid in debugging. Consider including the mismatched versions in the error message.Apply this diff to include version details:
- return nil, errors.New("SP1 version of zkVM prover and witness generator mismatched") + return nil, fmt.Errorf("SP1 version mismatch: prover version %s, witness generator version %s", proverSpec.SP1Version, witnessGenSpec.SP1Version)
260-260: Address the TODO Comment RegardingzkEVMProofFetcherThe TODO comment suggests removing
zkEVMProofFetcherafter the zkVM transition is completed. To prevent this task from being overlooked, consider creating a tracking issue or task in your issue tracking system.Would you like me to create a GitHub issue to track the removal of
zkEVMProofFetcherafter the zkVM transition is complete?
242-252: Implement Retry Logic for RPC Client ConnectionsWhile the error handling when dialing RPC clients is good, consider adding retry logic or exponential backoff to handle transient network issues more gracefully, improving the robustness of your application.
op-e2e/testdata/challenge_test_data.go (3)
85-116: Parameterize Proof Type inSetPrevOutputWithProofResponseFunctionThe function
SetPrevOutputWithProofResponsecurrently hardcodes the proof type toZkEVMType, which limits flexibility. Consider adding aproofTypeparameter to allow for different proof types when calling this function.Apply this diff to parameterize the proof type:
-func SetPrevOutputWithProofResponse(o *eth.OutputWithProofResponse) error { - err := SetPrevOutputResponse(&o.OutputResponse, ZkEVMType) +func SetPrevOutputWithProofResponse(o *eth.OutputWithProofResponse, proofType ProofType) error { + err := SetPrevOutputResponse(&o.OutputResponse, proofType) if err != nil { return err } // Rest of the function...Update any calls to
SetPrevOutputWithProofResponseto pass the appropriateproofType.
165-197: Parameterize Proof Type inSetTargetOutputWithProofResponseFunctionSimilarly,
SetTargetOutputWithProofResponsehardcodes the proof type toZkEVMType. To increase the function's usability with different proof types, add aproofTypeparameter.Apply this diff:
-func SetTargetOutputWithProofResponse(o *eth.OutputWithProofResponse) error { - err := SetTargetOutputResponse(&o.OutputResponse, ZkEVMType) +func SetTargetOutputWithProofResponse(o *eth.OutputWithProofResponse, proofType ProofType) error { + err := SetTargetOutputResponse(&o.OutputResponse, proofType) if err != nil { return err } // Rest of the function...Ensure all calls to
SetTargetOutputWithProofResponsepass the desiredproofType.
Line range hint
39-83: Remove Unused Variables to Clean Up the CodeVariables such as
ChallengeL1Headand others related to the zkEVM challenge may not be used in the current implementation. Removing unused variables helps maintain code clarity and reduce potential confusion.op-e2e/e2eutils/setup.go (4)
282-283: Reintroducecontext.ContextParameter for Better ControlThe
ctxparameter has been removed from theRedeployValPoolToTerminatefunction. Passingcontext.Contextallows for cancellation and timeout control, which is important in deployment functions that may hang or take a long time. Consider reintroducingctxto enhance control over the deployment process.
318-332: Improve Error Handling inparseSegLenToBigIntsFunctionIn the
parseSegLenToBigIntsfunction, consider providing more descriptive error messages. If parsing fails, indicate which segment length caused the failure to aid in debugging.Apply this diff:
- return nil, fmt.Errorf("failed to convert %s to *big.Int", segLen) + return nil, fmt.Errorf("failed to parse segment length '%s' into *big.Int", segLen)
344-345: Wrap and Log Errors for Better DebuggingIn functions like
ReplaceWithMockColosseum, errors are returned without additional context. Consider wrapping errors with context and logging them to facilitate troubleshooting.Apply this diff:
- return + return nil, nil, fmt.Errorf("failed to create transaction options: %w", err)Repeat similar improvements in other functions where errors are returned.
382-445: Refactor Deployment Functions to Reduce Code DuplicationThe deployment functions (
DeploySP1Verifier,RedeployZKProofVerifier, etc.) share similar patterns. Refactoring common logic into helper functions can reduce code duplication and improve maintainability.op-node/node/server_test.go (1)
149-155: Avoid duplication by extracting the common hash into a variableThe hash value
0x8512bee03061475e4b069171f7b406097184f16b22c3f5c97c0abfc49591c524is used multiple times in this code segment. Consider extracting it into a variable to improve maintainability and reduce the risk of typos.Apply this diff to extract the hash into a variable:
+ blockHash := common.HexToHash("0x8512bee03061475e4b069171f7b406097184f16b22c3f5c97c0abfc49591c524") if isKromaMPT { - l2Client.ExpectOutputV0AtBlock(common.HexToHash("0x8512bee03061475e4b069171f7b406097184f16b22c3f5c97c0abfc49591c524"), output, nil) + l2Client.ExpectOutputV0AtBlock(blockHash, output, nil) } else { - l2Client.ExpectInfoByHash(common.HexToHash("0x8512bee03061475e4b069171f7b406097184f16b22c3f5c97c0abfc49591c524"), &info, nil) - l2Client.ExpectGetProof(predeploys.L2ToL1MessagePasserAddr, []common.Hash{}, "0x8512bee03061475e4b069171f7b406097184f16b22c3f5c97c0abfc49591c524", &result, nil) + l2Client.ExpectInfoByHash(blockHash, &info, nil) + l2Client.ExpectGetProof(predeploys.L2ToL1MessagePasserAddr, []common.Hash{}, blockHash.Hex(), &result, nil) }kroma-chain-ops/cmd/check-kroma-mpt/main.go (1)
561-569: Consider making the retry interval and maximum attempts configurableIn
execTx, the transaction confirmation loop retries every second for up to 30 times. Consider making the retry interval and the maximum number of attempts configurable to allow flexibility in different network conditions.kroma-validator/challenger.go (1)
1015-1019: Consider consistent parameter usage inProveFaultmethod callsIn the
ProveFaultmethod, the calls toproveFaultWithZkVmandproveFaultWithZkEvmpass different parameters:
proveFaultWithZkVmreceiveschallengeWithData.proveFaultWithZkEvmreceiveschallenge.Challenger.For better consistency and readability, consider aligning the parameters passed to these functions or documenting the reason for the difference.
op-e2e/op_geth_test.go (1)
96-99: Reduce code duplication when convertingDepositTxtoKromaDepositTxThe conversion from
DepositTxtoKromaDepositTxusingToKromaDepositTx()is repeated across multiple test cases. To improve maintainability and reduce code duplication, consider creating a helper function or utility method that handles this conversion, especially if the pattern is the same in each instance.op-e2e/setup.go (3)
Line range hint
632-660: Refactoring node initialization withinitializeNodefunctionThe introduction of the
initializeNodefunction improves code modularity and reusability by encapsulating the node initialization logic. This change enhances maintainability.Consider adding error handling for potential failures within
initializeNode, such as handling cases where node startup might fail.
996-1061: AddingsetupNodesForMPTfunctionThe
setupNodesForMPTfunction is introduced to configure nodes for the Kroma MPT migration. This function modifies genesis configurations and initializes the historical node required for migration.Enhance error handling by checking the error returned from
json.Marshalwhen deep copying the genesis configuration. Currently, the error is ignored.Apply this diff to handle the error:
dst := &core.Genesis{} b, err := json.Marshal(ethCfg.Genesis) +if err != nil { + return err +} err = json.Unmarshal(b, dst) if err != nil { return err }
Line range hint
1063-1240: ImplementingstartChallengeSystemandwaitTxsfunctionsThe
startChallengeSystemfunction encapsulates the logic for setting up the challenge system, including deploying necessary contracts and initializing validators. ThewaitTxsfunction is a helper to wait for transaction confirmations.Consider parameterizing the context timeout duration in
waitTxsto enhance flexibility and make it configurable based on different use cases.kroma-bindings/bindings/zkproofverifier.go (1)
1-482: AddingZKProofVerifiercontract bindingsThe new file
zkproofverifier.goprovides Go bindings for theZKProofVerifiersmart contract. This addition enables interaction with the contract from Go applications, facilitating verification of zkEVM and zkVM proofs.Consider adding unit tests for the
ZKProofVerifierbindings to ensure that all methods function correctly and handle errors appropriately.op-e2e/system_test.go (4)
13-13: Updating imports to include Kroma predeploysThe import alias
oppredeploysis added forgithub.com/ethereum-optimism/optimism/op-bindings/predeploys. Ensure that the alias is used consistently throughout the code to avoid confusion with Kroma predeploys.If both Optimism and Kroma predeploys are used, consider using distinct aliases to differentiate them clearly.
51-51: Reordering importsThe import for
testdatais repositioned. While this doesn't affect functionality, maintaining a consistent import order improves code readability.
Line range hint
181-210: Adding test case for L2 Output Submitter V2The test function
TestL2OutputSubmitterV2is added to validate the L2 output submission in version 2 of the validator system. This enhances test coverage for the updated validator logic.Ensure that all new test cases are documented and that any prerequisites are clearly defined to aid future maintenance.
2228-2228: Note on commented-out codeThere is a block of code commented out between
[Kroma: START]and[Kroma: END]regarding protocol version changes. Ensure that this commented code is either removed if not needed or properly handled if it will be used in the future.Consider removing commented-out code to keep the codebase clean, or add explanations for why it remains.
kroma-validator/challenge/proof_fetcher.go (2)
14-59: Consider adding retry mechanism for RPC calls.The
ZkEVMProofFetcherimplementation looks good, but RPC calls might fail due to network issues. Consider adding a retry mechanism with backoff for better reliability.+func (z *ZkEVMProofFetcher) callWithRetry(ctx context.Context, result interface{}, method string, args ...interface{}) error { + maxRetries := 3 + for i := 0; i < maxRetries; i++ { + err := z.rpc.CallContext(ctx, result, method, args...) + if err == nil { + return nil + } + if i < maxRetries-1 { + time.Sleep(time.Second * time.Duration(1<<uint(i))) + } + } + return fmt.Errorf("failed after %d retries", maxRetries) +}
47-59: Consider optimizing the byte swapping operation.The
decodefunction performs byte swapping for big-endian conversion. Consider using a more efficient approach or documenting why this specific implementation is needed.func decode(data []byte) []*big.Int { result := make([]*big.Int, len(data)/32) + // Add a comment explaining why we need to swap bytes for i := 0; i < len(data)/32; i++ { - // The best is data is given in Big Endian. + // Convert from little-endian to big-endian format for j := 0; j < 16; j++ { data[i*32+j], data[i*32+31-j] = data[i*32+31-j], data[i*32+j] } result[i] = new(big.Int).SetBytes(data[i*32 : (i+1)*32]) } return result }kroma-bindings/predeploys/addresses_test.go (1)
Line range hint
65-107: Consider adding test cases for invalid storage layouts.The new
TestKromaL1BlockSlotsfunction looks good but only tests the happy path. Consider adding test cases for invalid storage layouts to ensure proper error handling.+func TestKromaL1BlockSlotsWithInvalidLayout(t *testing.T) { + // Test with missing required fields + layout := &bindings.StorageLayout{ + Storage: []bindings.StorageItem{ + // Missing critical fields + }, + } + _, err := validateKromaL1BlockLayout(layout) + require.Error(t, err) + require.Contains(t, err.Error(), "missing required fields") +}op-e2e/e2eutils/mock_l2_rpc.go (1)
68-76: Consider adding logging for random hash generation.When generating random hashes for malicious responses, consider adding debug logging to help with troubleshooting.
if o, ok := result.(**eth.OutputResponse); ok { + log.Debug("Generating random malicious response", "blockNumber", blockNumber) (**o).OutputRoot = eth.Bytes32(testutils.RandomHash(rng)) (**o).WithdrawalStorageRoot = testutils.RandomHash(rng) (**o).StateRoot = testutils.RandomHash(rng)kroma-bindings/bindings/kromal1block_more.go (1)
18-26: Ensure thread-safe initialization of global maps.The
init()function modifies global maps. While Go guarantees thatinit()runs in a single thread, consider documenting that these maps should not be modified elsewhere.Add a comment above the global maps:
// These maps are populated during init() and should not be modified elsewhere var ( layouts map[string]*solc.StorageLayout deployedBytecodes map[string]string immutableReferences map[string]bool )kroma-validator/flags/flags.go (1)
169-179: Document flag dependencies and mutual exclusivity.The relationship between zkEVM and zkVM flags should be documented, especially since they're required at different times (before/after Kroma MPT).
Add comments explaining:
- Which flags are mutually exclusive
- When each flag is required
- Dependencies between flags
op-e2e/actions/l2_sequencer.go (1)
180-187: Enhance validation in ActBuildL2ToPreKromaMPT.The method correctly validates KromaMPT timing, but consider:
- Adding a check for potential overflow when subtracting BlockTime
- Adding a progress log for long-running block building
func (s *L2Sequencer) ActBuildL2ToPreKromaMPT(t Testing) { require.NotNil(t, s.rollupCfg.KromaMPTTime, "cannot activate KromaMPT when it is not scheduled") require.GreaterOrEqual(t, *s.rollupCfg.KromaMPTTime, s.rollupCfg.BlockTime, "KromaMPT activation time cannot be zero") + targetTime := *s.rollupCfg.KromaMPTTime - s.rollupCfg.BlockTime + require.Greater(t, *s.rollupCfg.KromaMPTTime, targetTime, "block time subtraction overflow") for s.L2Unsafe().Time < *s.rollupCfg.KromaMPTTime-s.rollupCfg.BlockTime { + log.Debug("Building block towards KromaMPT", "current", s.L2Unsafe().Time, "target", *s.rollupCfg.KromaMPTTime-s.rollupCfg.BlockTime) s.ActL2StartBlock(t) s.ActL2EndBlock(t) } }op-e2e/actions/l2_verifier.go (1)
283-293: Consider refactoring duplicate code into a helper function.The transaction conversion logic is duplicated in both
ActL2UnsafeGossipReceiveandActL2InsertUnsafePayload. Consider extracting it into a helper function to improve maintainability.+func convertToKromaDepositTxs(payload *eth.ExecutionPayload) error { + for i, otx := range payload.ExecutionPayload.Transactions { + if otx[0] != types.DepositTxType { + continue + } + txBytes, err := derive.ToKromaDepositBytes(otx) + if err != nil { + return err + } + payload.ExecutionPayload.Transactions[i] = txBytes + } + return nil +} func (s *L2Verifier) ActL2UnsafeGossipReceive(payload *eth.ExecutionPayloadEnvelope) Action { return func(t Testing) { - for i, otx := range payload.ExecutionPayload.Transactions { - if otx[0] != types.DepositTxType { - continue - } - var err error - txBytes, err := derive.ToKromaDepositBytes(otx) - require.NoError(t, err) - payload.ExecutionPayload.Transactions[i] = txBytes - } + require.NoError(t, convertToKromaDepositTxs(payload.ExecutionPayload)) s.derivation.AddUnsafePayload(payload) } } func (s *L2Verifier) ActL2InsertUnsafePayload(payload *eth.ExecutionPayloadEnvelope) Action { return func(t Testing) { - for i, otx := range payload.ExecutionPayload.Transactions { - if otx[0] != types.DepositTxType { - continue - } - var err error - payload.ExecutionPayload.Transactions[i], err = derive.ToKromaDepositBytes(otx) - require.NoError(t, err) - } + require.NoError(t, convertToKromaDepositTxs(payload.ExecutionPayload)) ref, err := derive.PayloadToBlockRef(s.rollupCfg, payload.ExecutionPayload) require.NoError(t, err) err = s.engine.InsertUnsafePayload(t.Ctx(), payload, ref) require.NoError(t, err) } }Also applies to: 301-310
op-e2e/actions/user_test.go (1)
12-14: Remove unnecessary import changes.The imports are removed and then re-added without any changes. This modification appears to be unnecessary.
op-node/rollup/derive/attributes_test.go (1)
252-313: Enhance test coverage for KromaMPT activationThe test cases cover basic KromaMPT activation scenarios, but could benefit from additional edge cases.
Consider adding test cases for:
- Concurrent activation of multiple forks
- Edge cases around activation boundaries
- Error conditions during activation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (37)
audits/2024_09_Validator_System_V2_ChainLight.pdfis excluded by!**/*.pdf,!**/*.pdfgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumkroma-bindings/artifacts.jsonis excluded by!**/*.jsonkroma-chain-ops/genesis/testdata/allocs-l1.jsonis excluded by!**/*.jsonkroma-chain-ops/genesis/testdata/deploy.jsonis excluded by!**/*.jsonkroma-chain-ops/genesis/testdata/l1-deployments.jsonis excluded by!**/*.jsonkroma-chain-ops/genesis/testdata/test-deploy-config-full.jsonis excluded by!**/*.jsonnx.jsonis excluded by!**/*.jsonops-devnet/docker-compose.ymlis excluded by!**/*.ymlpackages/contracts/contracts/vendor/ISP1Verifier.solis excluded by!**/vendor/**packages/contracts/contracts/vendor/PlonkVerifier.solis excluded by!**/vendor/**packages/contracts/contracts/vendor/SP1VerifierPlonk.solis excluded by!**/vendor/**packages/contracts/deploy-config/devnetL1-template.jsonis excluded by!**/*.jsonpackages/contracts/deploy-config/mainnet.jsonis excluded by!**/*.jsonpackages/contracts/deploy-config/sepolia.jsonis excluded by!**/*.jsonpackages/contracts/deployments/kroma/ValidatorRewardVault.jsonis excluded by!**/*.jsonpackages/contracts/deployments/kroma/solcInputs/c9375212ffd331436ce484e7026e4f56.jsonis excluded by!**/*.jsonpackages/contracts/deployments/kromaSepolia/ValidatorRewardVault.jsonis excluded by!**/*.jsonpackages/contracts/deployments/kromaSepolia/solcInputs/c9375212ffd331436ce484e7026e4f56.jsonis excluded by!**/*.jsonpackages/contracts/deployments/mainnet/AssetManager.jsonis excluded by!**/*.jsonpackages/contracts/deployments/mainnet/AssetManagerProxy.jsonis excluded by!**/*.jsonpackages/contracts/deployments/mainnet/Colosseum.jsonis excluded by!**/*.jsonpackages/contracts/deployments/mainnet/L2OutputOracle.jsonis excluded by!**/*.jsonpackages/contracts/deployments/mainnet/ValidatorManager.jsonis excluded by!**/*.jsonpackages/contracts/deployments/mainnet/ValidatorManagerProxy.jsonis excluded by!**/*.jsonpackages/contracts/deployments/mainnet/ValidatorPool.jsonis excluded by!**/*.jsonpackages/contracts/deployments/mainnet/solcInputs/6271f6f40156b835b824d3aeb153efe1.jsonis excluded by!**/*.jsonpackages/contracts/deployments/sepolia/AssetManager.jsonis excluded by!**/*.jsonpackages/contracts/deployments/sepolia/AssetManagerProxy.jsonis excluded by!**/*.jsonpackages/contracts/deployments/sepolia/Colosseum.jsonis excluded by!**/*.jsonpackages/contracts/deployments/sepolia/L2OutputOracle.jsonis excluded by!**/*.jsonpackages/contracts/deployments/sepolia/ValidatorManager.jsonis excluded by!**/*.jsonpackages/contracts/deployments/sepolia/ValidatorManagerProxy.jsonis excluded by!**/*.jsonpackages/contracts/deployments/sepolia/ValidatorPool.jsonis excluded by!**/*.jsonpackages/contracts/deployments/sepolia/solcInputs/6271f6f40156b835b824d3aeb153efe1.jsonis excluded by!**/*.jsonpackages/contracts/foundry.tomlis excluded by!**/*.toml
📒 Files selected for processing (82)
.github/CODEOWNERS(0 hunks)audits/README.md(1 hunks)kroma-bindings/bindings/colosseum_more.go(1 hunks)kroma-bindings/bindings/gaspriceoracle.go(3 hunks)kroma-bindings/bindings/gaspriceoracle_more.go(1 hunks)kroma-bindings/bindings/kromal1block.go(1 hunks)kroma-bindings/bindings/kromal1block_more.go(1 hunks)kroma-bindings/bindings/kromaportal.go(1 hunks)kroma-bindings/bindings/kromaportal_more.go(1 hunks)kroma-bindings/bindings/l1block.go(2 hunks)kroma-bindings/bindings/l1block_more.go(1 hunks)kroma-bindings/bindings/mockcolosseum_more.go(1 hunks)kroma-bindings/bindings/registry.go(1 hunks)kroma-bindings/bindings/sp1verifier.go(1 hunks)kroma-bindings/bindings/sp1verifier_more.go(1 hunks)kroma-bindings/bindings/types.go(2 hunks)kroma-bindings/bindings/validatormanager_more.go(1 hunks)kroma-bindings/bindings/validatorrewardvault.go(2 hunks)kroma-bindings/bindings/validatorrewardvault_more.go(1 hunks)kroma-bindings/bindings/zkproofverifier.go(1 hunks)kroma-bindings/bindings/zkproofverifier_more.go(1 hunks)kroma-bindings/predeploys/addresses.go(3 hunks)kroma-bindings/predeploys/addresses_test.go(2 hunks)kroma-chain-ops/cmd/check-kroma-mpt/main.go(1 hunks)kroma-chain-ops/genesis/config.go(11 hunks)kroma-chain-ops/genesis/config_test.go(1 hunks)kroma-chain-ops/genesis/genesis.go(2 hunks)kroma-chain-ops/genesis/layer_one.go(1 hunks)kroma-chain-ops/immutables/immutables.go(1 hunks)kroma-chain-ops/immutables/immutables_test.go(1 hunks)kroma-devnet/devnet/__init__.py(2 hunks)kroma-validator/challenge/fetcher.go(0 hunks)kroma-validator/challenge/proof_fetcher.go(1 hunks)kroma-validator/challenge/witness_generator.go(1 hunks)kroma-validator/challenger.go(15 hunks)kroma-validator/config.go(6 hunks)kroma-validator/flags/flags.go(3 hunks)op-batcher/batcher/channel_builder.go(2 hunks)op-batcher/batcher/channel_builder_test.go(3 hunks)op-batcher/batcher/channel_manager_test.go(1 hunks)op-chain-ops/cmd/check-ecotone/main.go(1 hunks)op-e2e/actions/ecotone_fork_test.go(4 hunks)op-e2e/actions/kromampt_fork_test.go(1 hunks)op-e2e/actions/l2_challenger.go(3 hunks)op-e2e/actions/l2_challenger_test.go(1 hunks)op-e2e/actions/l2_engine_test.go(3 hunks)op-e2e/actions/l2_runtime.go(5 hunks)op-e2e/actions/l2_sequencer.go(3 hunks)op-e2e/actions/l2_validator.go(4 hunks)op-e2e/actions/l2_verifier.go(1 hunks)op-e2e/actions/user.go(1 hunks)op-e2e/actions/user_test.go(2 hunks)op-e2e/bridge_test.go(1 hunks)op-e2e/e2eutils/mock_l2_rpc.go(4 hunks)op-e2e/e2eutils/mock_proof_fetcher.go(3 hunks)op-e2e/e2eutils/setup.go(5 hunks)op-e2e/e2eutils/setup_test.go(1 hunks)op-e2e/migration_test.go(1 hunks)op-e2e/op_geth.go(2 hunks)op-e2e/op_geth_test.go(13 hunks)op-e2e/setup.go(18 hunks)op-e2e/system_adminrpc_test.go(2 hunks)op-e2e/system_test.go(15 hunks)op-e2e/system_tob_test.go(2 hunks)op-e2e/testdata/challenge_test_data.go(4 hunks)op-e2e/tx_helper.go(2 hunks)op-node/chaincfg/chains.go(2 hunks)op-node/chaincfg/chains_test.go(2 hunks)op-node/flags/flags_test.go(1 hunks)op-node/node/api.go(7 hunks)op-node/node/server_test.go(5 hunks)op-node/rollup/derive/attributes.go(4 hunks)op-node/rollup/derive/attributes_queue_test.go(2 hunks)op-node/rollup/derive/attributes_test.go(4 hunks)op-node/rollup/derive/deposit_log.go(0 hunks)op-node/rollup/derive/deposit_log_tob_test.go(1 hunks)op-node/rollup/derive/ecotone_upgrade_transactions.go(5 hunks)op-node/rollup/derive/ecotone_upgrade_transactions_test.go(0 hunks)op-node/rollup/derive/engine_consolidate.go(1 hunks)op-node/rollup/derive/engine_controller.go(1 hunks)op-node/rollup/derive/engine_queue.go(0 hunks)op-node/rollup/derive/engine_queue_test.go(1 hunks)
⛔ Files not processed due to max files limit (25)
- op-node/rollup/derive/fuzz_parsers_test.go
- op-node/rollup/derive/kromampt_upgrade_transactions.go
- op-node/rollup/derive/kromampt_upgrade_transactions_test.go
- op-node/rollup/derive/l1_block_info.go
- op-node/rollup/derive/l1_block_info_test.go
- op-node/rollup/derive/span_channel_out.go
- op-node/rollup/derive/test/random.go
- op-node/rollup/driver/sequencer.go
- op-node/rollup/driver/sequencer_test.go
- op-node/rollup/driver/state.go
- op-node/rollup/output_root.go
- op-node/rollup/output_root_kroma.go
- op-node/rollup/types.go
- op-node/service.go
- op-node/withdrawals/proof.go
- op-node/withdrawals/utils.go
- op-service/client/l2/engineapi/l2_engine_api.go
- op-service/client/l2/engineapi/test/l2_engine_api_tests.go
- op-service/eth/output.go
- op-service/eth/output_kroma.go
- op-service/eth/output_test.go
- op-service/eth/types.go
- op-service/flags/flags.go
- op-service/sources/rollupclient.go
- op-service/testutils/deposits.go
💤 Files with no reviewable changes (5)
- .github/CODEOWNERS
- op-node/rollup/derive/ecotone_upgrade_transactions_test.go
- op-node/rollup/derive/engine_queue.go
- op-node/rollup/derive/deposit_log.go
- kroma-validator/challenge/fetcher.go
✅ Files skipped from review due to trivial changes (8)
- kroma-bindings/bindings/registry.go
- op-node/rollup/derive/engine_controller.go
- audits/README.md
- kroma-bindings/bindings/kromaportal_more.go
- kroma-bindings/bindings/validatormanager_more.go
- kroma-bindings/bindings/sp1verifier_more.go
- kroma-bindings/bindings/zkproofverifier_more.go
- kroma-bindings/bindings/mockcolosseum_more.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
op-e2e/migration_test.go
39-39: SA5001: should check error returned from cfg.Start() before deferring sys.Close()
(staticcheck)
op-batcher/batcher/channel_builder_test.go
830-830: ineffectual assignment to err
(ineffassign)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Kroma Validator Version dev-2b9ef9e
- GitHub Check: Kroma Node Version dev-2b9ef9e
- GitHub Check: Kroma Batcher Version dev-2b9ef9e
- GitHub Check: Run tests
🔇 Additional comments (130)
op-node/rollup/derive/attributes_queue_test.go (3)
14-14: LGTM: Import addition is appropriate.The new import for predeploys is necessary for accessing
SequencerFeeVaultAddrin the MPT-related test case.
99-113: LGTM: Pre-MPT test case is well structured.The test case properly validates the behavior before the MPT upgrade, including the conversion to Kroma deposit format.
62-67: Verify the omission of ValidatorRewardScalar in MPT config.The MPT system configuration excludes the
ValidatorRewardScalarfield that is present in the regular config. Please confirm if this is intentional.Let's check other MPT-related configurations in the codebase:
✅ Verification successful
Omission of ValidatorRewardScalar in MPT config is intentional
The codebase shows this is a deliberate design choice. The MPT implementation uses a different system configuration structure that intentionally excludes the ValidatorRewardScalar field, with its own dedicated serialization format.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other MPT system config definitions to verify the pattern rg -A 5 "SystemConfig.*MPT" --type goLength of output: 542
Script:
#!/bin/bash # Search for MPT-related system config files and tests rg -B 2 -A 5 "SystemConfig.*{" --type go # Also check for any documentation about MPT system configs fd . -e md -e txt | xargs rg -i "mpt.*system.*config|system.*config.*mpt"Length of output: 212
Script:
#!/bin/bash # Search for SystemConfig struct definitions ast-grep --pattern 'type SystemConfig struct { $$$ }' # Search for MPT-related test files rg -B 2 -A 5 'MPT.*SystemConfig|SystemConfig.*MPT' --type go # Search for validator reward scalar usage rg -B 2 -A 5 'ValidatorRewardScalar' --type goLength of output: 66720
Script:
#!/bin/bash # Search for MPT migration related code and comments rg -B 2 -A 5 "MPT.*migration|migration.*MPT" --type go # Look for any MPT-specific system config handling rg -B 2 -A 5 "KromaMPT.*SystemConfig|SystemConfig.*KromaMPT" --type goLength of output: 1448
Script:
#!/bin/bash # Search for MPT-specific L1 block info handling rg -B 2 -A 10 "func.*marshalBinaryKromaMPT|KromaMPT.*L1BlockInfo" --type go # Look for any comments explaining MPT differences fd . -e go | xargs rg -B 2 -A 5 "// .*MPT.*validator|// .*validator.*MPT"Length of output: 2042
op-node/rollup/derive/attributes.go (1)
111-121: Verify KromaMPT upgrade transactions inclusion.The added logic correctly appends KromaMPT network upgrade transactions when
IsKromaMPTParentBlockreturns true. Ensure thatKromaMPTNetworkUpgradeTransactionsgenerates the appropriate transactions and that any errors are handled correctly.kroma-chain-ops/genesis/layer_one.go (2)
Line range hint
197-215: Ensure correct governance token balance initialization.The logic initializes governance token balances for development accounts. Verify that:
- The value of
bigValaccurately represents the intended token amount.- The loop correctly sets the balance for each development account.
- There is proper error handling for any potential issues during balance updates.
193-197: Verify the setup ofBeaconDepositContractCode.The code sets the code for
BeaconDepositContractAddr:stateDB.SetCode(predeploys.BeaconDepositContractAddr, predeploys.BeaconDepositContractCode)Ensure that
BeaconDepositContractCodeis correctly initialized and that the contract code is properly set in the state.✅ Verification successful
BeaconDepositContract setup verified successfully
The contract code is correctly initialized using predefined constants from the predeploys package, with the bytecode being set at the correct address (0x4242...4242) in the genesis state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find predeploys package and BeaconDeposit related files fd -t f -e go beacon -iLength of output: 225
Script:
#!/bin/bash # Search for BeaconDepositContract constants and their usage rg -l "BeaconDepositContract" -t goLength of output: 116
Script:
#!/bin/bash # Look for test files related to BeaconDeposit setup fd -t f -e go test -i | rg -i "beacon|deposit"Length of output: 280
Script:
#!/bin/bash # Check the content of beacon_deposit.go cat kroma-bindings/predeploys/beacon_deposit.goLength of output: 688
Script:
#!/bin/bash # Look for genesis tests related to beacon deposit rg -A 5 -B 5 "BeaconDepositContract" "kroma-chain-ops/genesis" -t goLength of output: 1192
op-e2e/actions/kromampt_fork_test.go (1)
31-174: Comprehensive test for Kroma MPT network upgradeThe
TestKromaMPTNetworkUpgradeTransactionsfunction is well-structured and thoroughly tests the Kroma MPT network upgrade transactions. It effectively validates the deployment of new implementations, verifies code hashes, and ensures that the upgrade activates as expected. The test maintains best practices in test setup and assertions.kroma-validator/config.go (2)
53-55: Ensure Nil Checks for Proof Fetchers When Challenger Is DisabledWhen
ChallengerEnabledisfalse, theZkEVMProofFetcher,ZkVMProofFetcher, andWitnessGeneratorfields will benil. Ensure that any code using these fields properly checks fornilto avoidnilpointer dereferences.
46-46: Ensure Consistent Field Naming After RenamingThe field
ChallengePollIntervalhas been added to theConfigstruct. Please verify that all references to the oldChallengerPollIntervalhave been updated throughout the codebase to prevent any inconsistencies or build errors.Run the following script to check for any remaining references to
ChallengerPollInterval:op-e2e/e2eutils/setup.go (1)
382-445: Ensure Private Keys Are Handled SecurelyThe functions use private keys (
sysCfgOwner *ecdsa.PrivateKey) directly for transactions. Ensure that private keys are managed securely, and consider abstracting key management to avoid potential security risks.op-node/node/server_test.go (4)
24-24: Import statement is appropriateThe addition of the import for
github.com/kroma-network/kroma/kroma-bindings/predeploysis necessary for accessing the predeploy addresses used in the test.
28-44: Well-structured addition of TestOutputAtBlockThe new test function
TestOutputAtBlockwith subtests "pre-mpt" and "mpt" appropriately covers the different configurations for the Kroma MPT upgrade. The test cases are clearly separated and effectively simulate different rollup configurations.
160-163: Conditional expectations are correctly set based on Kroma MPT statusThe conditional addition of
ExpectBlockRefsWithStatuswhenisKromaMPTis false appropriately adjusts the mock expectations based on the rollup configuration.
324-328:⚠️ Potential issueEnsure nil safety when handling error pointers in mock methods
In
BlockRefsWithStatus, dereferencing*m[3].(*error)can lead to a nil pointer dereference ifm[3]isnil. To prevent potential panics, consider safely handling the error value.Apply this diff to check for
nilbefore dereferencing:func (c *mockDriverClient) BlockRefsWithStatus(ctx context.Context, num uint64) (eth.L2BlockRef, eth.L2BlockRef, *eth.SyncStatus, error) { m := c.Mock.MethodCalled("BlockRefsWithStatus", num) - return m[0].(eth.L2BlockRef), m[1].(eth.L2BlockRef), m[2].(*eth.SyncStatus), *m[3].(*error) + var err error + if m[3] != nil { + if e, ok := m[3].(*error); ok && e != nil { + err = *e + } + } + return m[0].(eth.L2BlockRef), m[1].(eth.L2BlockRef), m[2].(*eth.SyncStatus), err }Likely invalid or redundant comment.
kroma-bindings/bindings/l1block.go (2)
34-35: Updated ABI and binary reflect contract changesThe changes to
L1BlockMetaDatacorrectly update the ABI and binary to reflect the removal of thevalidatorRewardScalarfunction from the contract. This ensures that the bindings are consistent with the deployed contract.
608-626: Ensure consistency in method signatures in bindingsThe
SetL1BlockValuesmethod signatures inL1BlockTransactor,L1BlockSession, andL1BlockTransactorSessionhave been updated to match the new contract interface. This alignment is crucial for correct contract interactions.kroma-bindings/bindings/kromal1block.go (1)
1-679: Generated code reviewThis file is auto-generated. The bindings appear correctly generated and follow the expected structure for Go Ethereum contract bindings.
kroma-validator/challenger.go (2)
505-510: Use ofChallengeWithDatastructThe introduction of the
ChallengeWithDatastruct improves the encapsulation of challenge-related data, enhancing code readability and maintainability.
815-818: Address the TODO regarding deep copying the Merkle ProofThere's a TODO comment questioning whether a deep copy of the
MerkleProofis necessary in thePublicInputProoffunction. Sharing slices without copying can lead to unintended side effects if the underlying data is modified elsewhere.Would you like assistance in determining if a deep copy is required here to prevent potential issues with shared memory references? I can help analyze this further or open a GitHub issue to track this task.
op-e2e/op_geth_test.go (4)
271-273: Repeated conversion ofDepositTxtoKromaDepositTxThis code segment repeats the conversion logic addressed in a previous comment. Please refer to the earlier suggestion about creating a helper function to handle this conversion uniformly across tests.
320-322: Repeated conversion ofDepositTxtoKromaDepositTxAs before, consider refactoring this repeated conversion into a shared helper function to enhance code maintainability.
387-389: Repeated conversion ofDepositTxtoKromaDepositTxThis instance also repeats the conversion logic. Utilizing a helper function would streamline the code.
476-478: Repeated conversion ofDepositTxtoKromaDepositTxAgain, this conversion is repeated. A helper function could reduce redundancy.
op-e2e/setup.go (10)
7-7: Importingencoding/jsonThe
encoding/jsonpackage is imported to handle JSON marshaling and unmarshaling operations in the code. This import is necessary and correctly added.
70-70: Adding Kroma predeploys importThe import of
github.com/kroma-network/kroma/kroma-bindings/predeploysis necessary for accessing Kroma-specific predeployed contract addresses used within the code.
108-108: InitializingL2GenesisKromaMPTTimeOffsetSetting
deployConfig.L2GenesisKromaMPTTimeOffset = nilensures that the Kroma MPT time offset is initialized properly in the deployment configuration.
183-185: Setting defaultChallengeProofTypeThe
ChallengeProofTypefield is added toSystemConfigand initialized withtestdata.DefaultProofType. This provides a default proof type for challenges and is correctly set.
264-266: AddingChallengeProofTypetoSystemConfigThe new field
ChallengeProofTypein theSystemConfigstruct allows for specifying the proof type used in challenges. This enhances configurability for different proof mechanisms.
270-271: AddingSetupMPTMigrationflag toSystemConfigThe
SetupMPTMigrationboolean flag enables conditional setup for Kroma MPT migration. This addition allows the system to configure nodes appropriately when migrating to Kroma MPT.
591-591: SettingKromaMPTTimein Rollup ConfigAssigning
KromaMPTTimein the rollup configuration specifies the activation time for the Kroma MPT upgrade. This ensures that the rollup node is aware of the migration timing.
662-669: Conditional setup for MPT migrationWhen
SetupMPTMigrationis enabled, the code sets up nodes specifically for Kroma MPT migration usingsetupNodesForMPT. This conditional logic correctly handles the special setup required for migration.
Line range hint
721-742: UpdatingL1InfoFromStateto useKromaL1BlockThe function
L1InfoFromStatenow uses*bindings.KromaL1Blockinstead of*bindings.L1Block. This change aligns with the Kroma-specific L1 block contract and ensures the function retrieves the correct state information.
1150-1153: InitializingHonestL2RPCwithChallengeProofTypeEnsure that
cfg.ChallengeProofTypeis properly initialized before being passed toe2eutils.NewHonestL2RPC. This prevents potential nil pointer dereferences and ensures the correct proof type is used.op-e2e/system_test.go (9)
453-456: Ensuring L1 blocks are Dencun blocksIn
TestSystemE2EDencunAtGenesisWithBlobs, the test verifies that L1 is building Dencun blocks since genesis. This test checks the functionality when blobs are present on L1.
Line range hint
721-742: UpdatingL1InfoFromStateto useKromaL1BlockThe function
L1InfoFromStatenow accepts a*bindings.KromaL1Blockinstead of*bindings.L1Block, aligning with Kroma-specific implementations. This change ensures accurate retrieval of L1 block information in Kroma.
Line range hint
1147-1164: Refactoring L1 info retrieval logicThe
fillInfoListsfunction is modified to use the updatedL1InfoFromStatewithKromaL1Block. Ensure that this function handles both the Kroma and non-Kroma cases correctly.
1249-1275: AddingTestWithdrawalsfor Kroma MPTThe test function
TestWithdrawalsis updated to include a test case for Kroma MPT migration. This ensures that withdrawals are correctly processed during and after the migration.
Line range hint
1277-1315: Adjusting withdrawal test for Kroma MPTIn the
testWithdrawalsfunction, logic is added to handle the Kroma MPT migration case. The test waits for the MPT migration to complete before proceeding, ensuring accurate testing.
1434-1452: UpdatingTestFeesto include Kroma MPTThe
TestFeesfunction is extended to test fee calculations during the Kroma MPT migration. This addition verifies that fees are correctly handled across different protocol versions.
1478-1493: Validating fee recipient addresses in Kroma MPTIn the
testFeesfunction, the code checks that the coinbase address is set to the Sequencer Fee Vault during Kroma MPT. It also verifies the balances of fee recipient contracts.
Line range hint
1775-1821: RenamingTestChallengetoTestZkEVMChallengeThe test function is renamed to reflect that it specifically tests zkEVM challenges. This enhances clarity regarding the purpose of the test.
2117-2227: AddingTestZkVMChallengeA new test function
TestZkVMChallengeis added to test challenges in the zkVM context during the Kroma MPT migration. This expands test coverage for zkVM proof verification mechanisms.kroma-bindings/bindings/kromaportal.go (1)
34-35: Updated ABI and Bin fields are acceptableThe updated
ABIandBinfields inKromaPortalMetaDatareflect changes to the contract's interface and implementation. Ensure that any code interacting withKromaPortalis compatible with these updates.Consider verifying that all contract interactions are updated accordingly by running:
✅ Verification successful
Updated ABI and Bin fields are acceptable
The changes to the KromaPortal contract are non-breaking:
- Version bump from 0.1.0 to 1.0.0
- Documentation improvements
- No changes to function signatures, events, or storage layout
The updated ABI and Bin fields in kromaportal.go accurately reflect these changes and are safe to use with existing contract interactions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for outdated references to KromaPortal methods. rg 'KromaPortal' --glob '!*kromaportal.go'Length of output: 99698
Script:
#!/bin/bash # Search for KromaPortal contract implementation ast-grep --pattern 'contract KromaPortal { $$$ }'Length of output: 53
Script:
#!/bin/bash # Search for KromaPortal contract definition and interface rg -U "contract KromaPortal.*?\{" -A 10Length of output: 114437
op-e2e/e2eutils/setup_test.go (1)
38-38: Verify all references topredeploys.L1BlockAddrare updatedThe change from
predeploys.L1BlockAddrtopredeploys.KromaL1BlockAddrensures the correct predeploy address is used. Please verify that all instances ofpredeploys.L1BlockAddrhave been updated throughout the codebase to prevent inconsistencies.Run the following script to find any remaining references:
kroma-bindings/bindings/types.go (3)
34-34: LGTM! Field addition for L1 head tracking.The addition of
L1Headfield toTypesChallengestruct is appropriate for tracking the L1 head during challenges.
98-110: LGTM! Well-structured proof types.The new types
TypesZkEvmProofandTypesZkVmProofare well-structured for handling different types of zero-knowledge proofs:
TypesZkEvmProofincludes fields for public input proof and verification pairsTypesZkVmProofincludes fields for program verification key and proof data
42-42: Verify the impact of field renaming.The field has been renamed from
BlockHashtoLatestBlockhashinTypesOutputRootProof. While the change makes the field's purpose clearer, we should verify all usages.✅ Verification successful
Field rename has been properly implemented
The field rename from
BlockHashtoLatestBlockhashinTypesOutputRootProofhas been consistently applied across all usages in the codebase, including struct definitions, output root computations, and serialization logic. The change improves code clarity by better indicating the field's purpose.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old field name rg -l "BlockHash.*TypesOutputRootProof" # Search for all usages of TypesOutputRootProof to verify the update ast-grep --pattern 'TypesOutputRootProof { $$$BlockHash$$$ }'Length of output: 105
Script:
#!/bin/bash # Search for TypesOutputRootProof struct usage with context rg -A 3 "TypesOutputRootProof" # Search for any remaining BlockHash references rg "BlockHash" # Search for new LatestBlockhash references rg "LatestBlockhash"Length of output: 75722
Script:
#!/bin/bash # Search for TypesOutputRootProof struct usages with context rg -A 5 "TypesOutputRootProof.*{" # Search for any LatestBlockhash field references rg "LatestBlockhash.*TypesOutputRootProof|TypesOutputRootProof.*LatestBlockhash" # Search for any remaining BlockHash references in relevant files rg -g "*output*.go" -g "*types*.go" "BlockHash"Length of output: 23231
kroma-validator/challenge/proof_fetcher.go (1)
98-114: LGTM! Robust hex string handling.The
UnmarshalJSONimplementation forHexBytesproperly handles both quoted and unquoted hex strings, with and without "0x" prefix.op-node/flags/flags_test.go (1)
117-117: LGTM! Consistent with existing patterns.Adding "override.mpt" to
skippedFlagsis appropriate as it follows the same pattern as other flags that don't have a standard flag name to environment variable conversion.kroma-bindings/predeploys/addresses_test.go (2)
11-19: LGTM! Clean import update and address verification.The change to import
oppredeploysand the addition ofKromaL1BlockAddrverification is appropriate.
32-62: LGTM! Comprehensive slot verification for Ecotone.The updated
TestL1BlockSlotsfunction properly verifies all storage slots, including the new Ecotone-specific slots for blob fees and scalars.op-node/chaincfg/chains.go (2)
45-45: LGTM: Mainnet KromaMPTTime configurationThe KromaMPTTime is correctly set to nil for mainnet, indicating that the upgrade timing is not yet determined.
85-85: Verify the Sepolia KromaMPTTime timestampThe KromaMPTTime is set to 1737090000 (approximately July 17, 2025) for Sepolia testnet. Please ensure this timestamp aligns with the planned upgrade schedule.
op-e2e/bridge_test.go (1)
101-105: LGTM: Updated deposit transaction handlingThe conversion to KromaDepositTx with proper error handling is correctly implemented. The changes align with the Kroma MPT upgrade requirements.
op-e2e/actions/l2_challenger.go (4)
52-56: LGTM: Simplified output retrieval in ActBisectThe transition to GetL2Output simplifies the code while maintaining proper error handling and validation.
59-62: LGTM: Enhanced challenge handling in ActBisectThe challenge retrieval and processing is properly implemented with the new GetChallenge method.
86-89: LGTM: Consistent output handling in ActChallengerTimeoutThe output retrieval and validation logic is consistently implemented across methods.
106-112: LGTM: Enhanced fault proving with safety checksThe implementation correctly:
- Retrieves the challenge
- Creates ChallengeWithData with proper initialization
- Ensures safety by checking the Processing field
op-node/chaincfg/chains_test.go (2)
72-72: LGTM: Mainnet test configurationThe test configuration correctly mirrors the production configuration with KromaMPTTime set to nil.
112-112: LGTM: Sepolia test configurationThe test configuration correctly mirrors the production configuration with KromaMPTTime set to 1737090000.
kroma-bindings/bindings/l1block_more.go (1)
16-16: Verify the deployed bytecode matches the compiled contract.The L1BlockDeployedBin has been updated with new bytecode. Please ensure this matches the compiled contract's bytecode to prevent deployment issues.
Run the following script to verify the bytecode:
✅ Verification successful
Bytecode verification successful
The L1BlockDeployedBin in the binding matches exactly with the deployed contract bytecode from both Kroma mainnet and Sepolia networks. The bytecode is correctly updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the deployed bytecode matches the compiled contract # Search for any test files that verify the L1Block contract deployment rg -l "L1BlockDeployedBin.*test" --type go # Search for the contract compilation artifacts fd -g "*L1Block*.json"Length of output: 293
Script:
#!/bin/bash # Extract bytecode from deployment artifacts and compare echo "=== Kroma Mainnet Bytecode ===" jq -r '.bytecode' packages/contracts/deployments/kroma/L1Block.json echo -e "\n=== Kroma Sepolia Bytecode ===" jq -r '.bytecode' packages/contracts/deployments/kromaSepolia/L1Block.json # Extract the runtime bytecode as well, as sometimes that's what's stored in bindings echo -e "\n=== Kroma Mainnet Deployed Bytecode ===" jq -r '.deployedBytecode' packages/contracts/deployments/kroma/L1Block.json echo -e "\n=== Kroma Sepolia Deployed Bytecode ===" jq -r '.deployedBytecode' packages/contracts/deployments/kromaSepolia/L1Block.jsonLength of output: 17315
kroma-chain-ops/immutables/immutables_test.go (1)
42-42: Verify the L1Block to KromaL1Block rename consistency.The L1Block field has been renamed to KromaL1Block. This change needs to be consistent across the entire codebase.
Run the following script to verify the rename consistency:
✅ Verification successful
L1Block to KromaL1Block rename is consistent with upgrade pattern
The rename from L1Block to KromaL1Block is part of an intentional upgrade process, with proper separation between legacy and new code paths. Most remaining "L1Block" references are unrelated type/variable names, not contract references.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to L1Block that should be KromaL1Block # Search for L1Block references that might need updating rg "L1Block" --type go # Search for new KromaL1Block references rg "KromaL1Block" --type goLength of output: 72190
op-e2e/tx_helper.go (2)
27-27: LGTM! Function signature updated to support Kroma MPT.The addition of the variadic isKromaMPT parameter maintains backward compatibility while adding support for Kroma MPT.
53-58: LGTM! Proper implementation of Kroma MPT support.The implementation correctly:
- Handles the default case (empty isKromaMPT or false)
- Converts to KromaDepositTx when needed
- Includes proper error handling
kroma-chain-ops/genesis/config_test.go (1)
124-125: LGTM! Added verification for ZK proof verifier addresses.The added assertions properly verify that the ZKProofVerifier and ZKProofVerifierProxy addresses are not zero addresses, which is crucial for the system's security.
op-e2e/e2eutils/mock_l2_rpc.go (2)
25-29: Consistent error handling in constructors.Both constructors now properly validate the proof type before initialization. The error messages are clear and descriptive.
Also applies to: 100-104
52-64: Verify type assertions in CallContext method.The type assertions and error handling look good, but consider adding unit tests to verify the behavior with different proof types and invalid result types.
Also applies to: 127-139
kroma-validator/flags/flags.go (1)
116-135: Verify the network timeout value for zkEVM proof fetching.The default timeout of 4 hours for
ZkEVMNetworkTimeoutFlagseems high. Consider:
- Making this configurable per environment
- Adding validation for maximum allowed timeout
✅ Verification successful
The 4-hour default timeout for zkEVM proof fetching is appropriate
The default timeout is reasonable for zkEVM proof generation, which is computationally intensive. The value is already configurable per environment through both CLI flags (
--challenger.zkevm-network-timeout) and environment variables (CHALLENGER_ZKEVM_NETWORK_TIMEOUT). Adding a maximum timeout validation would unnecessarily restrict operators who might need longer timeouts in specific environments.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for timeout usage in the codebase rg "ZkEVMNetworkTimeout|4.*hour" --type goLength of output: 510
Script:
#!/bin/bash # Search for test files and documentation rg -t go "TestZkEVM|test.*timeout" kroma-validator/ fd -e md . | xargs rg -l "timeout|zkEVM"Length of output: 142
kroma-bindings/predeploys/addresses.go (1)
12-12: LGTM! Consistent renaming of L1Block to KromaL1Block.The renaming from
L1BlocktoKromaL1Blockis applied consistently across the constant, variable, and Predeploys map entry. This change improves clarity by making the Kroma-specific nature of these identifiers explicit.Also applies to: 39-39, 82-82
kroma-chain-ops/genesis/genesis.go (2)
65-66: LGTM! KromaMPTTime field added to chain configuration.The addition of
KromaMPTTimebeforeInteropTimein the chain configuration is correct and maintains proper field ordering.
75-82: LGTM! Proper configuration for KromaMPT activation.The conditional block correctly:
- Configures Optimism parameters when KromaMPT is active
- Disables Zktrie when KromaMPT is active
op-node/rollup/derive/deposit_log_tob_test.go (1)
64-71: LGTM! Added IsSystemTransaction field to test deposits.The addition of
IsSystemTransaction: falseto the DepositTx construction in tests is correct and consistent with the codebase changes.op-node/rollup/derive/engine_consolidate.go (1)
44-54: LGTM! Added proper handling of DepositTx in RPC responses.The changes correctly handle the conversion of DepositTx to KromaDepositTx when KromaMPT is not active. The code:
- Properly checks for DepositTxType
- Includes appropriate error handling
- Is well-documented with clear comments explaining the reason for conversion
kroma-bindings/bindings/validatorrewardvault_more.go (1)
16-16: Verify the bytecode update.The deployed bytecode for the
ValidatorRewardVaultcontract has been updated. This change appears to be related to the addition of thewithdrawToProtocolVaultfunctionality.Run the following script to verify the bytecode changes:
✅ Verification successful
Bytecode update verified.
The bytecode changes correspond to the addition of the
withdrawToProtocolVaultfunctionality in version 1.0.1 of the ValidatorRewardVault contract, along with proper version bumping and interface updates. The changes are well-tested and maintain contract security.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the bytecode changes in the ValidatorRewardVault contract. # Test: Search for the contract source file and verify it matches the bytecode. rg -A 5 $'contract ValidatorRewardVault' # Test: Search for any references to the protocol vault functionality. rg -A 5 $'withdrawToProtocolVault'Length of output: 127679
op-e2e/op_geth.go (2)
114-114: LGTM! Added Kroma MPT time support.The addition of the
KromaMPTTimeparameter toeth.BlockAsPayloadaligns with the Kroma MPT upgrade.
223-236: LGTM! Clean implementation of Kroma deposit transaction handling.The code properly handles the conversion of deposit transactions for pre-Kroma MPT blocks:
- Checks block timestamp against Kroma MPT activation time
- Converts only DepositTxType transactions
- Includes proper error handling
op-e2e/actions/l2_engine_test.go (2)
21-22: LGTM! Clean import organization.The engineapi imports are properly organized.
49-49: LGTM! Consistent Kroma MPT time support in tests.The addition of
KromaMPTTimeparameter toeth.BlockAsPayloadcalls maintains consistency with the implementation changes.Also applies to: 72-72
op-e2e/actions/l2_validator.go (4)
26-26: LGTM! Clean import addition.The challenge package import is properly added.
79-79: LGTM! Improved naming consistency.Renamed
ChallengerPollIntervaltoChallengePollIntervalfor better clarity.
89-91: LGTM! Enhanced modularity with proof fetchers.Added separate components for zkEVM proof fetching, zkVM proof fetching, and witness generation, improving modularity and testability.
192-193: LGTM! Improved error handling.Added proper error check for assetManagerContract initialization.
op-e2e/actions/user_test.go (1)
68-68: Verify the impact of disabling the fjord fork test.The fjord fork test has been commented out. Please verify if this is intentional and document the reasoning.
Run the following script to check for other fjord-related tests and configurations:
✅ Verification successful
Disabling the fjord fork test is intentional
The fjord fork appears to be a future network upgrade. The test is intentionally disabled while maintaining the supporting infrastructure, which is a common pattern when preparing for upcoming network upgrades that are not yet ready for testing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for fjord-related tests and configurations # Test: Search for fjord-related code. Expect: Understanding of fjord usage across the codebase # Check for fjord-related tests echo "Checking for fjord-related tests..." rg -i "fjord.*test" # Check for fjord configurations echo "Checking for fjord configurations..." rg -i "fjord.*config"Length of output: 492
Script:
#!/bin/bash # Get the full context of the test file and any fjord-related documentation echo "Checking the test implementation..." rg -B 5 -A 10 "fjord" op-e2e/actions/user_test.go echo "\nChecking for fjord documentation..." rg -i "fjord" -g "*.md" echo "\nChecking the setup configuration..." cat op-e2e/setup.go | grep -A 5 -B 5 "FjordTime"Length of output: 3111
kroma-chain-ops/immutables/immutables.go (1)
51-51: LGTM! Field rename aligns with Kroma naming convention.The rename from
L1BlocktoKromaL1Blockmaintains consistency with Kroma-specific implementations.kroma-bindings/bindings/gaspriceoracle_more.go (1)
16-16: Verify the source of the updated bytecode.This file is marked as generated (
// Code generated - DO NOT EDIT.), but contains manual changes to the deployed bytecode. Please ensure:
- The bytecode was generated from the correct source
- The changes are made in the source contract instead of this generated file
Run the following script to verify the bytecode source:
op-e2e/actions/ecotone_fork_test.go (4)
78-79: LGTM: Using Kroma-specific L1Block addressThe change correctly uses the Kroma-specific L1Block address for storage access.
128-129: LGTM: Consistent use of Kroma L1Block addressThe storage check correctly uses the same Kroma L1Block address as earlier in the code.
207-208: LGTM: Using Kroma-specific L1Block callerThe change consistently uses the Kroma L1Block contract caller.
94-101: Verify transaction conversion completenessThe code correctly converts deposit transactions to Kroma format, but we should ensure all transaction types are handled.
Run this script to verify no deposit transactions are missed:
✅ Verification successful
Deposit transaction conversion is properly implemented
The conversion from DepositTx to KromaDepositTx in the test follows the established pattern used consistently throughout the codebase, with proper type checking and error handling. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining DepositTx usage that might have been missed rg "types\.DepositTxType|types\.DepositTx" --type goLength of output: 8003
Script:
#!/bin/bash # Search for KromaDepositTx usage rg "KromaDepositTx" --type goLength of output: 7241
op-batcher/batcher/channel_builder.go (2)
23-25: LGTM: Well-documented error declarationThe new error is clearly documented with appropriate comments indicating Kroma-specific functionality.
180-183: Verify error handling for KromaMPT timing edge casesThe code handles the KromaMPT timing check, but we should verify edge cases.
✅ Verification successful
KromaMPT timing edge case handling verified
The code correctly handles the KromaMPT timing edge case by:
- Properly identifying the parent block of the KromaMPT transition
- Using appropriate error handling with ErrJustBeforeKromaMPTTime
- Coordinating with system-wide transition handling in the sequencer and other components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for comprehensive error handling in similar timing checks rg "KromaMPTTime|BlockTime" --type go -B 2 -A 2Length of output: 128631
Script:
#!/bin/bash # Check for KromaMPT related errors and timing checks rg "KromaMPT.*Error|ErrJustBeforeKromaMPT|IsKromaMPT" --type go -B 2 -A 2Length of output: 11593
op-node/rollup/derive/attributes_test.go (3)
116-119: LGTM: Proper error handling in transaction conversionThe code correctly handles potential errors during Kroma deposit conversion.
160-165: LGTM: Comprehensive transaction conversionThe loop properly converts all transactions while maintaining error checking.
192-195: LGTM: Consistent transaction conversionThe code maintains consistency with other conversion points in the test.
op-e2e/actions/l2_runtime.go (3)
136-137: LGTM: Improved error handling for RPC client creationThe code now properly handles potential errors during RPC client creation for both malicious and honest validators.
Also applies to: 142-143
284-284: LGTM: Updated method callThe code correctly uses the new
GetChallengemethod name.
169-169: Verify mock contract compatibilityUsing mock contract for testing is good practice, but verify interface compatibility.
✅ Verification successful
Mock contract maintains interface compatibility ✅
The MockColosseum contract properly inherits from the Colosseum contract and maintains full interface compatibility, including the
getChallengemethod. The mock only adds testing utilities without modifying the core contract behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check interface compatibility between mock and real contract ast-grep --pattern 'interface $name { $$$ getChallenge($$$) $$$ }'Length of output: 74
Script:
#!/bin/bash # Find Colosseum-related contract files fd -e sol -e go "Colosseum|colosseum" --type f # Search for contract definitions and method signatures rg -l "contract .*Colosseum" rg "func.*getChallenge" -A 5Length of output: 85569
Script:
#!/bin/bash # Find MockColosseum implementation and method signatures cat packages/contracts/contracts/test/mock/MockColosseum.solLength of output: 1314
kroma-devnet/devnet/__init__.py (3)
259-261: LGTM: Historical service deployment added.The deployment of the
l2-historicalservice is correctly implemented with proper environment variable handling.
266-267: LGTM: RPC server wait logic added.The wait logic for the historical service's RPC server is properly implemented using the existing wait functions.
283-283: LGTM: Service list updated.The list of services has been updated to include
kroma-node-historicalandkroma-challenger, maintaining consistency with the new architecture.op-node/rollup/derive/ecotone_upgrade_transactions.go (5)
57-57: LGTM: Transaction flags and formatting improved.The
IsSystemTransactionflag is correctly set tofalse, and the transaction parameters are well-formatted for better readability.Also applies to: 66-73
82-89: LGTM: L1 Block proxy update transaction properly configured.Transaction parameters are correctly set with appropriate gas limits and system transaction flags.
98-105: LGTM: Gas price oracle proxy update transaction properly configured.Transaction parameters are correctly set with appropriate gas limits and system transaction flags.
114-121: LGTM: Ecotone enable transaction properly configured.Transaction parameters are correctly set with appropriate gas limits and system transaction flags.
129-136: LGTM: EIP4788 deployment transaction properly configured.Transaction parameters are correctly set with the standard EIP-4788 gas limit (0x3d090) and system transaction flags.
op-e2e/actions/user.go (1)
384-395: LGTM: Enhanced deposit transaction handling.The code now properly handles Kroma-specific deposit transactions by:
- Attempting to find the receipt using the original transaction hash
- Converting to KromaDepositTx if the receipt is not found
- Adding appropriate error handling for the conversion process
op-batcher/batcher/channel_manager_test.go (3)
488-503: LGTM: Test setup properly configured.The test setup correctly initializes the channel manager with appropriate configuration for testing MPT block behavior:
- Random seed for reproducibility
- Correct block time configuration
- MPT time configuration
504-528: LGTM: Block creation and timing setup.The test properly creates and configures blocks with correct:
- Block timing relative to MPT time
- Block number sequence
- Parent hash relationships
529-548: LGTM: Test assertions verify critical behavior.The test correctly verifies:
- Channel behavior before MPT time
- Channel closure behavior
- Channel ID changes after closure
op-e2e/actions/l2_challenger_test.go (1)
14-14: LGTM: Import path updated to use Kroma's testdata package.The import path has been correctly updated to use Kroma's implementation.
op-batcher/batcher/channel_builder_test.go (1)
834-868: LGTM: Well-structured test for span batch checksum validation.The test comprehensively validates that:
- The span batch encoding includes a checksum
- The checksum is correctly calculated using Adler-32
- The checksum matches the expected value
op-chain-ops/cmd/check-ecotone/main.go (1)
735-735: LGTM: Updated to use Kroma's L1Block contract binding.The contract binding has been correctly updated to use
NewKromaL1Blockwith the appropriate predeploy address.op-e2e/system_tob_test.go (1)
142-149: LGTM: Added IsSystemTransaction field to DepositTx.The
IsSystemTransactionfield has been correctly added to track system transactions.kroma-bindings/bindings/gaspriceoracle.go (3)
34-35: LGTM: Updated contract metadataThe ABI and bytecode updates properly reflect the addition of new Kroma MPT-related functions.
515-544: LGTM: Well-structured IsKromaMPT implementationThe IsKromaMPT method is correctly implemented with:
- Proper error handling
- Consistent return types
- Method variants for different binding types (Caller, Session, CallerSession)
690-710: LGTM: Well-structured SetKromaMPT implementationThe SetKromaMPT method is correctly implemented with:
- Proper transaction handling
- Method variants for different binding types (Transactor, Session, TransactorSession)
op-node/rollup/derive/engine_queue_test.go (1)
971-976: LGTM: Proper handling of Kroma deposit transactionsThe conditional block correctly handles the conversion of transactions to Kroma deposit format based on configuration state. The error handling is appropriate.
kroma-bindings/bindings/sp1verifier.go (1)
1-356: LGTM: Well-structured SP1Verifier contract bindingsThe implementation follows best practices for Go contract bindings:
- Complete contract metadata and proper initialization
- Well-structured binding types with clear separation of concerns
- Comprehensive error handling
- Proper method implementations for all contract functions
kroma-bindings/bindings/validatorrewardvault.go (2)
34-35: LGTM: Updated contract metadataThe ABI and bytecode updates properly reflect the addition of the new WithdrawToProtocolVault function.
516-535: LGTM: Well-structured WithdrawToProtocolVault implementationThe WithdrawToProtocolVault method is correctly implemented with:
- Proper transaction handling
- Method variants for different binding types (Transactor, Session, TransactorSession)
kroma-bindings/bindings/colosseum_more.go (1)
Line range hint
1-1: Generated code - no manual changes needed.This is an auto-generated binding file. The changes reflect updates to the underlying Solidity contract structure.
kroma-chain-ops/genesis/config.go (8)
123-125: New field added for Kroma MPT hard fork timing.The
L2GenesisKromaMPTTimeOffsetfield is added to control the activation timing of the Kroma MPT hard fork, following the same pattern as other fork timing fields.
289-291: New field added for MPT transition control.The
ValidatorManagerMptFirstOutputIndexfield is added to control the first output index after the MPT transition, with restricted access for output submission and challenges.
344-348: New fields added for ZK proof verification.Two new fields are added:
ZKProofVerifierSP1Verifier: Address of the SP1VerifierGateway contractZKProofVerifierVKey: Verification key for the zkVM program
531-535: Updated fork order validation.Added validation to ensure the Kroma MPT fork is activated after Ecotone and before Fjord, maintaining the correct fork ordering.
566-568: Added validation for MPT transition index.Added check to ensure
ValidatorManagerMptFirstOutputIndexis not nil, which is crucial for the MPT transition.
644-649: Added validation for ZK proof verification fields.Added checks to ensure:
ZKProofVerifierSP1Verifieris not address(0)ZKProofVerifierVKeyis not zero hash
739-748: Added KromaMPTTime calculation method.Added method to calculate the Kroma MPT activation time based on the genesis time, following the same pattern as other fork timing methods.
813-813: Updated RollupConfig to include KromaMPTTime.Added KromaMPTTime to the rollup configuration, ensuring it's properly propagated through the system.
op-e2e/system_adminrpc_test.go (1)
201-202: Updated BlockAsPayload calls to include KromaMPTTime parameter.The method calls have been updated to include the new required KromaMPTTime parameter from the rollup configuration, maintaining consistency with the updated method signature.
Also applies to: 210-211
| // [Kroma: START] | ||
| suggestedFeeRecipient := common.Address{} | ||
| if ba.rollupCfg.IsKromaMPT(nextL2Time) { | ||
| suggestedFeeRecipient = oppredeploys.SequencerFeeVaultAddr | ||
| } else { | ||
| // In Kroma, the IsSystemTransaction field was deleted from DepositTx. | ||
| // After transitioning to MPT, we bring back the IsSystemTransaction field for compatibility with OP. | ||
| // Therefore, before MPT time, use KromaDepositTx struct to create deposit transactions without that field. | ||
| for i, otx := range txs { | ||
| if otx[0] != types.DepositTxType { | ||
| continue | ||
| } | ||
| tx, err := ToKromaDepositBytes(otx) | ||
| if err != nil { | ||
| return nil, NewCriticalError(err) | ||
| } | ||
| txs[i] = tx | ||
| } | ||
| } | ||
| // [Kroma: END] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid in-place modification of transactions slice during iteration.
Modifying the txs slice while iterating over it can lead to unexpected behavior. To prevent potential issues, consider one of the following solutions:
- Iterate over a copy of the slice indices.
- Collect modified transactions in a new slice.
Apply this diff to iterate safely:
- for i, otx := range txs {
+ for i := range txs {
if txs[i][0] != types.DepositTxType {
continue
}
tx, err := ToKromaDepositBytes(txs[i])
if err != nil {
return nil, NewCriticalError(err)
}
txs[i] = tx
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // [Kroma: START] | |
| suggestedFeeRecipient := common.Address{} | |
| if ba.rollupCfg.IsKromaMPT(nextL2Time) { | |
| suggestedFeeRecipient = oppredeploys.SequencerFeeVaultAddr | |
| } else { | |
| // In Kroma, the IsSystemTransaction field was deleted from DepositTx. | |
| // After transitioning to MPT, we bring back the IsSystemTransaction field for compatibility with OP. | |
| // Therefore, before MPT time, use KromaDepositTx struct to create deposit transactions without that field. | |
| for i, otx := range txs { | |
| if otx[0] != types.DepositTxType { | |
| continue | |
| } | |
| tx, err := ToKromaDepositBytes(otx) | |
| if err != nil { | |
| return nil, NewCriticalError(err) | |
| } | |
| txs[i] = tx | |
| } | |
| } | |
| // [Kroma: END] | |
| // [Kroma: START] | |
| suggestedFeeRecipient := common.Address{} | |
| if ba.rollupCfg.IsKromaMPT(nextL2Time) { | |
| suggestedFeeRecipient = oppredeploys.SequencerFeeVaultAddr | |
| } else { | |
| // In Kroma, the IsSystemTransaction field was deleted from DepositTx. | |
| // After transitioning to MPT, we bring back the IsSystemTransaction field for compatibility with OP. | |
| // Therefore, before MPT time, use KromaDepositTx struct to create deposit transactions without that field. | |
| for i := range txs { | |
| if txs[i][0] != types.DepositTxType { | |
| continue | |
| } | |
| tx, err := ToKromaDepositBytes(txs[i]) | |
| if err != nil { | |
| return nil, NewCriticalError(err) | |
| } | |
| txs[i] = tx | |
| } | |
| } | |
| // [Kroma: END] |
| cfg.DeployConfig.L2GenesisKromaMPTTimeOffset = &mptTimeOffset | ||
|
|
||
| sys, err := cfg.Start(t) | ||
| defer sys.Close() |
There was a problem hiding this comment.
Fix potential nil pointer dereference when deferring sys.Close()
The defer sys.Close() statement is executed before checking if cfg.Start(t) returned an error. If an error occurs during cfg.Start(t), sys may be nil, leading to a nil pointer dereference when sys.Close() is called. It's important to check for errors before deferring sys.Close() to prevent this issue.
Apply this diff to fix the issue:
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
39-39: SA5001: should check error returned from cfg.Start() before deferring sys.Close()
(staticcheck)
| return fmt.Errorf("GPO is not set to Kroma MPT: %w", err) | ||
| } |
There was a problem hiding this comment.
Remove unnecessary error wrapping when 'err' is nil
In the error message, err is likely nil at this point. Including %w with err in fmt.Errorf when err is nil will result in an error message with <nil> appended. Consider removing err from the error wrapping.
Apply this diff to correct the error handling:
if !isKromaMPT {
- return fmt.Errorf("GPO is not set to Kroma MPT: %w", err)
+ return fmt.Errorf("GPO is not set to Kroma MPT")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return fmt.Errorf("GPO is not set to Kroma MPT: %w", err) | |
| } | |
| return fmt.Errorf("GPO is not set to Kroma MPT") | |
| } |
| if challengeWithData.Processing { | ||
| challengeWithData.Processing = false | ||
| continue | ||
| } |
There was a problem hiding this comment.
Potential logic issue with Processing flag handling
In the handleChallenge function, after calling ProveFault, the Processing flag is reset to false immediately after checking if it's true. This might lead to the loop proceeding without ensuring that the required proof data is ready, potentially causing incorrect behavior.
Consider updating the logic to reset Processing only when the proof generation is confirmed to be complete, ensuring that the challenge handling waits appropriately for the necessary data.
| var output *RequestStatusType | ||
| err := w.rpc.CallContext(ctx, &output, "requestWitness", blockHash, l1Head) | ||
| return output, err |
There was a problem hiding this comment.
Initialize output variable to avoid nil pointer dereference
In the RequestWitness method, output is declared as a pointer but not initialized. This can lead to a nil pointer dereference. Declare output as a value and pass its address to CallContext.
Apply this diff to fix the issue:
func (w *WitnessGenerator) RequestWitness(ctx context.Context, blockHash string, l1Head string) (*RequestStatusType, error) {
- var output *RequestStatusType
+ var output RequestStatusType
err := w.rpc.CallContext(ctx, &output, "requestWitness", blockHash, l1Head)
- return output, err
+ return &output, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var output *RequestStatusType | |
| err := w.rpc.CallContext(ctx, &output, "requestWitness", blockHash, l1Head) | |
| return output, err | |
| var output RequestStatusType | |
| err := w.rpc.CallContext(ctx, &output, "requestWitness", blockHash, l1Head) | |
| return &output, err |
| var output *SpecResponse | ||
| err := w.rpc.CallContext(ctx, &output, "spec") | ||
| return output, err |
There was a problem hiding this comment.
Initialize output variable to avoid nil pointer dereference
In the Spec method, the output variable is declared as a pointer but not initialized, which can lead to a nil pointer dereference during unmarshalling. Declare output as a value and pass its address to CallContext.
Apply this diff to fix the issue:
func (w *WitnessGenerator) Spec(ctx context.Context) (*SpecResponse, error) {
- var output *SpecResponse
+ var output SpecResponse
err := w.rpc.CallContext(ctx, &output, "spec")
- return output, err
+ return &output, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var output *SpecResponse | |
| err := w.rpc.CallContext(ctx, &output, "spec") | |
| return output, err | |
| var output SpecResponse | |
| err := w.rpc.CallContext(ctx, &output, "spec") | |
| return &output, err |
| var output *WitnessResponse | ||
| err := w.rpc.CallContext(ctx, &output, "getWitness", blockHash, l1Head) | ||
| return output, err |
There was a problem hiding this comment.
Initialize output variable to avoid nil pointer dereference
In the GetWitness method, output is declared as a pointer but not initialized. This can lead to a nil pointer dereference. Declare output as a value and pass its address to CallContext.
Apply this diff to fix the issue:
func (w *WitnessGenerator) GetWitness(ctx context.Context, blockHash string, l1Head string) (*WitnessResponse, error) {
- var output *WitnessResponse
+ var output WitnessResponse
err := w.rpc.CallContext(ctx, &output, "getWitness", blockHash, l1Head)
- return output, err
+ return &output, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var output *WitnessResponse | |
| err := w.rpc.CallContext(ctx, &output, "getWitness", blockHash, l1Head) | |
| return output, err | |
| var output WitnessResponse | |
| err := w.rpc.CallContext(ctx, &output, "getWitness", blockHash, l1Head) | |
| return &output, err |
| func (z *ZkVMProofFetcher) RequestProve(ctx context.Context, blockHash string, l1Head string, witness string) (*RequestStatusType, error) { | ||
| var output *RequestStatusType | ||
| err := z.rpc.CallContext(ctx, &output, "requestProve", blockHash, l1Head, witness) | ||
| return output, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add context timeout for RPC calls.
The RequestProve and GetProof methods should have appropriate timeouts to prevent hanging on slow responses.
func (z *ZkVMProofFetcher) RequestProve(ctx context.Context, blockHash string, l1Head string, witness string) (*RequestStatusType, error) {
+ ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+ defer cancel()
var output *RequestStatusType
err := z.rpc.CallContext(ctx, &output, "requestProve", blockHash, l1Head, witness)
return output, err
}
func (z *ZkVMProofFetcher) GetProof(ctx context.Context, blockHash string, l1Head string) (*ZkVMProofResponse, error) {
+ ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+ defer cancel()
var output *ZkVMProofResponse
err := z.rpc.CallContext(ctx, &output, "getProof", blockHash, l1Head)
return output, err
}Also applies to: 91-95
Description
This PR is for v2.1.0 release.