Skip to content

(WIP): Feat/500ms benchmark #1173

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

(WIP): Feat/500ms benchmark #1173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6ea4689
mock signer address
georgehao Apr 25, 2025
c44e300
change peroid from s -> ms
georgehao Apr 25, 2025
ff0fa92
update
georgehao Apr 25, 2025
1e03b6c
calcTIme+1
georgehao Apr 25, 2025
4c5ec01
update +2
georgehao Apr 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions consensus/system_contract/consensus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,15 +226,23 @@ func (s *SystemContract) VerifyUncles(chain consensus.ChainReader, block *types.
}

func (s *SystemContract) CalcTimestamp(parent *types.Header) uint64 {
timestamp := parent.Time + s.config.Period
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
}
Comment on lines +229 to 246
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplified timestamp calculation using block number parity

The timestamp calculation has been completely revised to use a much simpler rule:

  1. The timestamp starts as the parent block's timestamp
  2. 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


// Prepare initializes the consensus fields of a block header according to the
Expand Down
58 changes: 29 additions & 29 deletions consensus/system_contract/system_contract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package system_contract

import (
"context"
"fmt"
"sync"
"time"

Expand Down Expand Up @@ -89,34 +88,35 @@ func (s *SystemContract) Start() {
}

func (s *SystemContract) fetchAddressFromL1() error {
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())
}
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())
//}
Comment on lines +91 to +119
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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:

  1. Removes ability to dynamically update signers via L1
  2. Eliminates error handling for storage retrieval failures
  3. 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.

Suggested change
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")


return nil
}
Expand Down
3 changes: 2 additions & 1 deletion miner/scroll_worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,14 +552,15 @@ func (w *worker) newWork(now time.Time, parentHash common.Hash, reorging bool, r
vmConfig.Debug = true
vmConfig.Tracer = cccLogger
}

deadline := time.Unix(int64(header.Time), 0)
if w.chainConfig.Clique != nil && w.chainConfig.Clique.RelaxedPeriod {
// clique with relaxed period uses time.Now() as the header.Time, calculate the deadline
deadline = time.Unix(int64(header.Time+w.chainConfig.Clique.Period), 0)
}
if w.chainConfig.SystemContract != nil && w.chainConfig.SystemContract.RelaxedPeriod {
// system contract with relaxed period uses time.Now() as the header.Time, calculate the deadline
deadline = time.Unix(int64(header.Time+w.chainConfig.SystemContract.Period), 0)
deadline = time.UnixMilli(int64(header.Time*1000 + 500*((header.Number.Uint64()+1)%2) + w.chainConfig.SystemContract.Period))
}

w.current = &work{
Expand Down
Loading