(WIP): Feat/500ms benchmark #1173
Conversation
WalkthroughThis set of changes simplifies the logic for timestamp calculation in the consensus system contract, modifies how the L1 signer address is obtained by hardcoding it, and updates the mining work deadline computation to use a millisecond-based approach with an alternating offset. The previous dynamic and configurable logic for both timestamp and address retrieval is commented out but left in the code, while the new implementations use fixed or simplified rules. No changes were made to the signatures of exported or public entities. Changes
Sequence Diagram(s)sequenceDiagram
participant Miner
participant SystemContract
participant BlockHeader
Miner->>SystemContract: CalcTimestamp(parentHeader)
SystemContract->>SystemContract: Set timestamp = parentHeader.timestamp
alt parentHeader.number is even
SystemContract->>SystemContract: Increment timestamp by 1
end
SystemContract-->>Miner: Return timestamp
Miner->>SystemContract: fetchAddressFromL1()
SystemContract->>SystemContract: Assign hardcoded address
SystemContract-->>Miner: Return
Miner->>BlockHeader: Compute mining work deadline
BlockHeader-->>Miner: Provide header.time and number
Miner->>Miner: Calculate deadline using ms and 500ms offset
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
consensus/system_contract/consensus.go(1 hunks)consensus/system_contract/system_contract.go(1 hunks)miner/scroll_worker.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
consensus/system_contract/system_contract.go (1)
common/types.go (1)
HexToAddress(218-218)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
miner/scroll_worker.go (1)
563-563:❓ Verification inconclusive
Updated deadline calculation uses millisecond precision with alternating offset
The deadline calculation is now more precise, using millisecond-level granularity with a 500ms offset that alternates based on block number parity.
This change appears to be coordinated with the timestamp calculation update in
consensus/system_contract/consensus.go. Verify that this change works correctly with the new timestamp logic:
🏁 Script executed:
#!/bin/bash # Description: Check for any tests or comments related to deadline calculation rg -A 3 -B 3 "deadline.*UnixMilli" --type go rg -A 3 -B 3 "500.*\%2" --type goLength of output: 666
Verify deadline calculation alignment with updated consensus timestamp logic
We attempted to confirm the implementation via:
rg -C3 'deadline.*UnixMilli' --type go rg -C3 '%2' --type go # regex search errored due to escaping rg -C3 'UnixMilli' consensus/system_contract/consensus.goSince the
%2search failed, please manually verify that:
- miner/scroll_worker.go (line 563) correctly applies the alternating 500 ms offset
- the timestamp logic in consensus/system_contract/consensus.go now uses millisecond granularity and matches this calculation
- no other code paths or tests depend on the old second-level deadline calculation
| s.signerAddressL1 = common.HexToAddress("0x756EA06BDEe36de11F22DCca45a31d8a178eF3c6") | ||
| //address, err := s.client.StorageAt(s.ctx, s.config.SystemContractAddress, s.config.SystemContractSlot, nil) | ||
| //if err != nil { | ||
| // return fmt.Errorf("failed to get signer address from L1 System Contract: %w", err) | ||
| //} | ||
| //bAddress := common.BytesToAddress(address) | ||
| // | ||
| //s.lock.Lock() | ||
| //defer s.lock.Unlock() | ||
| // | ||
| //// Validate the address is not empty | ||
| //if bAddress == (common.Address{}) { | ||
| // log.Debug("Retrieved empty signer address from L1 System Contract", "contract", s.config.SystemContractAddress.Hex(), "slot", s.config.SystemContractSlot.Hex()) | ||
| // | ||
| // // Not initialized yet -- we don't consider this an error | ||
| // if s.signerAddressL1 == (common.Address{}) { | ||
| // log.Warn("System Contract signer address not initialized") | ||
| // return nil | ||
| // } | ||
| // | ||
| // return fmt.Errorf("retrieved empty signer address from L1 System Contract") | ||
| //} | ||
| // | ||
| //log.Debug("Read address from system contract", "address", bAddress.Hex()) | ||
| // | ||
| //if s.signerAddressL1 != bAddress { | ||
| // s.signerAddressL1 = bAddress | ||
| // log.Info("Updated new signer from L1 system contract", "signer", bAddress.Hex()) | ||
| //} |
There was a problem hiding this comment.
Hardcoded signer address replaces dynamic L1 retrieval
The implementation now uses a fixed address (0x756EA06BDEe36de11F22DCca45a31d8a178eF3c6) instead of dynamically retrieving the signer address from L1. All validation, error handling, and logging logic has been commented out.
While this simplifies the code, it creates the following concerns:
- Removes ability to dynamically update signers via L1
- Eliminates error handling for storage retrieval failures
- Removes validation that would prevent using an empty address
Consider whether this is intended as a temporary change for testing or if it's a permanent architectural decision. If permanent, add a comment explaining the rationale.
-s.signerAddressL1 = common.HexToAddress("0x756EA06BDEe36de11F22DCca45a31d8a178eF3c6")
+// Hardcoded address for 500ms benchmark testing
+// TODO: Revert to dynamic L1 retrieval after testing is complete
+s.signerAddressL1 = common.HexToAddress("0x756EA06BDEe36de11F22DCca45a31d8a178eF3c6")📝 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.
| s.signerAddressL1 = common.HexToAddress("0x756EA06BDEe36de11F22DCca45a31d8a178eF3c6") | |
| //address, err := s.client.StorageAt(s.ctx, s.config.SystemContractAddress, s.config.SystemContractSlot, nil) | |
| //if err != nil { | |
| // return fmt.Errorf("failed to get signer address from L1 System Contract: %w", err) | |
| //} | |
| //bAddress := common.BytesToAddress(address) | |
| // | |
| //s.lock.Lock() | |
| //defer s.lock.Unlock() | |
| // | |
| //// Validate the address is not empty | |
| //if bAddress == (common.Address{}) { | |
| // log.Debug("Retrieved empty signer address from L1 System Contract", "contract", s.config.SystemContractAddress.Hex(), "slot", s.config.SystemContractSlot.Hex()) | |
| // | |
| // // Not initialized yet -- we don't consider this an error | |
| // if s.signerAddressL1 == (common.Address{}) { | |
| // log.Warn("System Contract signer address not initialized") | |
| // return nil | |
| // } | |
| // | |
| // return fmt.Errorf("retrieved empty signer address from L1 System Contract") | |
| //} | |
| // | |
| //log.Debug("Read address from system contract", "address", bAddress.Hex()) | |
| // | |
| //if s.signerAddressL1 != bAddress { | |
| // s.signerAddressL1 = bAddress | |
| // log.Info("Updated new signer from L1 system contract", "signer", bAddress.Hex()) | |
| //} | |
| // Hardcoded address for 500ms benchmark testing | |
| // TODO: Revert to dynamic L1 retrieval after testing is complete | |
| s.signerAddressL1 = common.HexToAddress("0x756EA06BDEe36de11F22DCca45a31d8a178eF3c6") |
| timestamp := parent.Time | ||
|
|
||
| // If RelaxedPeriod is enabled, always set the header timestamp to now (ie the time we start building it) as | ||
| // we don't know when it will be sealed | ||
| if s.config.RelaxedPeriod || timestamp < uint64(time.Now().Unix()) { | ||
| timestamp = uint64(time.Now().Unix()) | ||
| if parent.Number.Uint64()%2 == 0 { | ||
| timestamp += 1 | ||
| } | ||
|
|
||
| return timestamp | ||
|
|
||
| //timestamp := parent.Time + s.config.Period | ||
| // | ||
| //// If RelaxedPeriod is enabled, always set the header timestamp to now (ie the time we start building it) as | ||
| //// we don't know when it will be sealed | ||
| //if s.config.RelaxedPeriod || timestamp < uint64(time.Now().Unix()) { | ||
| // timestamp = uint64(time.Now().Unix()) | ||
| //} | ||
| // | ||
| //return timestamp | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplified timestamp calculation using block number parity
The timestamp calculation has been completely revised to use a much simpler rule:
- The timestamp starts as the parent block's timestamp
- If the parent block number is even, increment timestamp by 1
This replaces the previous approach that used configured periods and current time checks.
While this simplification makes the timestamp calculation more deterministic and predictable, it's lacking documentation to explain the rationale and potential impacts. This change works in conjunction with the deadline calculation update in miner/scroll_worker.go.
timestamp := parent.Time
if parent.Number.Uint64()%2 == 0 {
+ // Increment timestamp by 1 on even parent blocks to ensure
+ // a regular, deterministic pattern for block timestamps
+ // This works with the 500ms deadline offset in miner/scroll_worker.go
timestamp += 1
}
return timestamp
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