Update AssembleL2Block#259
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughChanges convert block timestamp handling from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
eth/catalyst/gen_l2blockparams.go (1)
15-24: Generated JSON uses plain numeric timestamp; fix via override, not manual editsThe generated helper structs here use
Timestamp *uint64and assign it directly, sotimestampis encoded as a JSON number. Given the// DO NOT EDITheader and the hexutil override defined for this type, the real fix should be inassembleL2BlockParamsMarshaling(seeeth/catalyst/api_types.go), followed by re-runninggo generate, rather than editing this file directly.Also applies to: 35-49
🧹 Nitpick comments (1)
eth/catalyst/l2_api.go (1)
84-90: Timestamp default/override wiring looks correctDefaulting
timestamptouint64(time.Now().Unix())and conditionally overriding withparams.Timestampbefore callingBuildBlockis sound and matches the new uint64 API. If you ever need stricter control, you could clamp the finaltimestamphere to be at least the parent header timestamp (e.g.,max(parent.Time+1, timestamp)) instead of relying solely on downstream checks, but that’s optional for this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
eth/catalyst/api_types.go(1 hunks)eth/catalyst/gen_l2blockparams.go(3 hunks)eth/catalyst/l2_api.go(1 hunks)eth/catalyst/l2_api_test.go(3 hunks)ethclient/authclient/engine.go(2 hunks)miner/miner.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
eth/catalyst/l2_api_test.go (1)
common/mclock/mclock.go (1)
Now(34-36)
miner/miner.go (2)
core/types/transaction.go (1)
Transactions(615-615)miner/worker.go (1)
NewBlockResult(73-77)
eth/catalyst/gen_l2blockparams.go (2)
eth/catalyst/api_types.go (1)
AssembleL2BlockParams(41-45)common/hexutil/json.go (1)
Uint64(339-339)
ethclient/authclient/engine.go (2)
eth/catalyst/api_types.go (1)
ExecutableL2Data(91-113)eth/catalyst/gen_l2_ed.go (2)
ExecutableL2Data(18-33)ExecutableL2Data(59-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
eth/catalyst/l2_api_test.go (3)
111-117: Adaptation to uint64 timestamp in tests is correctPassing
uint64(time.Now().Unix())intoBuildBlockmatches the new signature and preserves prior semantics.
171-176: NewL2Block test updated consistentlyThis
BuildBlockcall is updated to the uint64 timestamp form and remains consistent with the other tests and the miner API.
224-229: NewSafeL2Block test timestamp change is aligned with APIUsing
uint64(time.Now().Unix())here correctly follows the newBuildBlocksignature without altering the test’s intent.ethclient/authclient/engine.go (1)
15-31: Client-side AssembleL2Block wiring and tx marshaling look solidMarshaling each
types.Transactionwith indexed error reporting and threading the optionaltimeStamppointer through toAssembleL2BlockParams.Timestampcleanly match the updated engine API. This method remains straightforward and consistent with the rest of the auth client.miner/miner.go (1)
170-177: BuildBlock API change to uint64 timestamp is correctly implemented across all call sitesThe change to
BuildBlockacceptinguint64timestamp instead oftime.Timeis consistent with howgetPendingusesuint64(time.Now().Unix()), simplifying the API to match the header's native time representation. All call sites in the codebase correctly passuint64timestamps: the catalyst API forwards a properly constructeduint64value, and test code correctly convertstime.Now().Unix()touint64.
* feat: change zktrie to mpt * fix: add diskroot mapping for consensus api new L2 block * fix: fixed alttxfee stroage trace in mpt mode * refactor: remove useless block trace struct fields * refactor: retain executionResults in blocktrace but remove structlog * fix: fixed ci code scanning alerts * feat: add eth_config with morph extension * feat: add mptForkTime override flag * fix: fixed getblockTrace stateroot mismatch * Update AssembleL2Block (#259) * update AssembleL2BlockParams --------- Co-authored-by: corey <corey.zhang@bitget.com> * feat:Add migration-checker (#271) * update AssembleL2BlockParams * update ec clent * add migration-checker * add gen preimages * clean changes * optimize“ * update readme“ * clean readme“ * clean trie changes --------- Co-authored-by: corey <corey.zhang@bitget.com> * limit AssembleL2Block (#285) * limit AssembleL2Block * fmt --------- Co-authored-by: corey <corey.zhang@bitget.com> * fix(zktrie): add max recursion depth limit to countLeaves (#286) Add maxZkTrieDepth (256) constant and depth check in countLeaves function to prevent potential stack overflow, node crashes, and DoS attacks. The theoretical maximum depth of zkTrie is 256, so this limit ensures safe recursion while covering all valid use cases. Co-authored-by: corey <corey.zhang@bitget.com> * Add Progress bar (#280) * add progress bar * clean default value * fix --------- Co-authored-by: corey <corey.zhang@bitget.com> --------- Co-authored-by: corey <coreyx1992@gmail.com> Co-authored-by: corey <corey.zhang@bitget.com>
1. Purpose or design rationale of this PR
...
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Changes
✏️ Tip: You can customize this high-level summary in your review settings.