feat: address system contract consensus issues and comments

[Thegaram]

1. Purpose or design rationale of this PR

Bug fixes:

• Fix VerifyHeaders in UpgradableEngine. There were multiple issues: We'd return clique and system config values out-of-order. Failure to use a buffered channel, and failure to correctly iterate over results led to a frozen node.

Not fixed yet:

• Incorrect behavior during transition. Right before the transition the Clique engine would create a block with post-EuclidV2 timestamp that will fail SystemContract verification. Can be solved by manually restarting miner (miner.stop(); miner.start()).
• Same complication with the "back-timestamping" added in feat(MessageQueueV2): add reading of MessageQueueV2 and transition logic from V1 to V2 for EuclidV2 #1108.

Improvements:

• Expose APIs scroll_getAuthorizedSigner and scroll_getLocalSigner.
• Add more detailed logs.
• Update comments.

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:

• fix: A bug fix

3. Deployment tag versioning

Has the version in params/version.go been updated?

• Yes

4. Breaking change label

Does this PR have the breaking-change label?

• This PR is not a breaking change

Summary by CodeRabbit

• New Features

  • Introduced API endpoints to retrieve individual signer addresses.
  • Added new web3 extension properties for accessing local and authorized signers.

• Improvements

  • Enhanced logging and error reporting for consensus engine operations and network events.
  • Refined consensus configuration display to better reflect upgradable setups.

• Chores

  • Updated the patch version to the latest release.

[coderabbitai]

Walkthrough

The changes update logging, error messages, API structures, and documentation across multiple consensus-related packages. A new log statement has been added in the clique engine initialization. The system contract API now replaces the signer list method with distinct local and authorized signer retrieval methods. Additional error messages and updated API definitions have been introduced. Modifications also include improved logging in consensus wrappers, enhanced comments in block encoding, new web3 properties for signer access, minor documentation improvements in the miner worker, and a bump in the version patch.

Changes

File(s)	Change Summary
consensus/clique/clique.go	Added a log statement in the New function to log the initialization of the clique consensus engine.
consensus/system_contract/*.go	* In api.go: Replaced chain field with system_contract pointer; removed GetSigners; added GetLocalSigner and GetAuthorizedSigner. * In consensus.go: Added new error messages and updated the APIs method (namespace changed to "scroll", added version "1.0" and public flag set to false). * In system_contract.go: Updated comments, logging, signer logic, and added localSignerAddress.
consensus/wrapper/consensus.go	Enhanced logging in engine initialization; modified header verification concurrency and channel naming; streamlined API aggregation; and updated error logging in the Close method.
core/types/block.go	Improved documentation comments for the Header struct fields, clarifying RLP encoding behavior and database storage details.
eth/protocols/eth/handlers.go	Added warning log statements before disconnecting peers due to network compatibility errors in handleNewBlock and handleBlockHeaders66.
internal/web3ext/web3ext.go	Introduced two new properties, authorizedSigner and localSigner, each linked to their respective getter methods (scroll_getAuthorizedSigner and scroll_getLocalSigner).
miner/scroll_worker.go	Added clarifying comments regarding blocking waits in the main loop and inserted a TODO comment in the pending L1 messages collection regarding a potential re-run of the Prepare function.
params/config.go & params/version.go	Updated the String() method in ChainConfig to represent consensus engine configurations more accurately and bumped the VersionPatch constant from 14 to 15.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Web3Ext
    participant API
    participant SystemContract

    Client->>Web3Ext: scroll_getAuthorizedSigner
    Web3Ext->>API: GetAuthorizedSigner()
    API->>SystemContract: currentSignerAddressL1()
    SystemContract-->>API: Authorized signer address
    API-->>Web3Ext: Return address
    Web3Ext-->>Client: Deliver authorized signer

Loading

sequenceDiagram
    participant Client
    participant Web3Ext
    participant API
    participant SystemContract

    Client->>Web3Ext: scroll_getLocalSigner
    Web3Ext->>API: GetLocalSigner()
    API->>SystemContract: localSignerAddress()
    SystemContract-->>API: Local signer address
    API-->>Web3Ext: Return address
    Web3Ext-->>Client: Deliver local signer

Loading

Suggested labels

bump-version

Suggested reviewers

• omerfirmak
• colinlyguo

Poem

I'm a busy bunny hopping through the code,
Logging each step on my lightening-fast road.
With signers fetched by magic methods new,
And error checks that keep things true.
Each patch and comment makes my heart glow—
A rabbit's cheer for improvements in our show! 🐇
Hop on and code, let the fixes flow!

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 365b829 and f32fb17.

📒 Files selected for processing (11)

• consensus/clique/clique.go (1 hunks)
• consensus/system_contract/api.go (1 hunks)
• consensus/system_contract/consensus.go (2 hunks)
• consensus/system_contract/system_contract.go (5 hunks)
• consensus/wrapper/consensus.go (5 hunks)
• core/types/block.go (2 hunks)
• eth/protocols/eth/handlers.go (2 hunks)
• internal/web3ext/web3ext.go (1 hunks)
• miner/scroll_worker.go (2 hunks)
• params/config.go (1 hunks)
• params/version.go (1 hunks)

✅ Files skipped from review due to trivial changes (3)

• params/version.go
• miner/scroll_worker.go
• consensus/clique/clique.go

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: test

🔇 Additional comments (26)

internal/web3ext/web3ext.go (1)

978-985: Great addition! Properly exposes new signer access methods.

These new web3 properties provide access to the important signer information through the JSON-RPC API, which aligns with the objective of addressing system contract consensus issues. The naming is consistent and clear.

core/types/block.go (4)

89-91: Good documentation improvement.

This comment clearly explains how RLP encoding handles optional fields, which is important context for the EuclidV2-related fields that follow.

---

92-95: Improved documentation for BlockSignature field.

The enhanced comment provides important clarification about when this field is used and transmitted, which is critical for understanding network interactions and potential consensus issues.

---

97-100: Improved documentation for IsEuclidV2 field.

Similar to the BlockSignature comment, this makes it clear that the field is for local use and not transmitted over the network.

---

118-121: Enhanced comment for the Requested field.

This comment properly clarifies the internal-only nature of this field, providing useful context about its purpose and lifecycle.

eth/protocols/eth/handlers.go (2)

273-273: Improved error logging when disconnecting peers.

This log message will aid in diagnosing network issues related to EuclidV2 compatibility, helping to address the consensus issues mentioned in the PR objectives.

---

313-313: Added consistent warning log for peer disconnection.

Following the same pattern as the other handler, this ensures consistent logging when peers are disconnected due to EuclidV2 field compatibility issues.

params/config.go (2)

796-797: Good addition for upgradable consensus engine representation.

This case properly identifies when both Clique and SystemContract engines are configured, labeling it as "upgradable" which clearly indicates the dual nature of the consensus mechanism.

---

800-801: Added missing case for SystemContract-only configuration.

This completes the engine representation logic to handle all possible combinations of consensus engines, making the configuration string representation more accurate.

consensus/system_contract/consensus.go (2)

39-58: Improved error handling for header verification.

The addition of specific error variables for different validation conditions (nonce, signature, mix digest, uncle hash, difficulty) provides clearer context when validation fails. The updated comment for errInvalidTimestamp removes the minimum block period reference, simplifying the error message.

---

359-366: API restructuring for better organization.

The API changes enhance the organization by:

1. Changing namespace from "system_contract" to "scroll"
2. Adding version information
3. Updating service reference to use the system_contract instance
4. Setting public access to false

This aligns with the modifications in api.go where methods were reorganized for specific signer retrieval.

consensus/system_contract/system_contract.go (6)

19-21: Enhanced documentation for SystemContract.

The expanded comment clarifies the purpose of the SystemContract consensus engine, specifically noting that it fetches the authorized signer from the SystemConfig contract starting from EuclidV2.

---

38-39: Added initialization logging.

Good addition of initialization logging that will help with debugging and understanding the system startup sequence.

---

59-60: Improved logging message clarity.

The updated log message more clearly indicates that the system contract consensus is being authorized.

---

99-100: Enhanced error message with context.

The improved error message now includes the contract address and slot when an empty signer is retrieved, which will significantly help with debugging.

---

104-110: Simplified locking mechanism.

The code now uses a direct write lock instead of the previous read-then-write lock approach. The condition for updating the signer address has been streamlined, and a helpful log message has been added when the signer is updated.

---

128-133: Added method for local signer retrieval.

The new localSignerAddress method properly retrieves the local signer address with appropriate thread safety using read locks. This method supports the new API structure in api.go.

consensus/system_contract/api.go (2)

10-11: Restructured API dependency.

Replaced the generic chain field with a specific system_contract pointer, allowing direct access to SystemContract methods without intermediaries.

---

14-21: Improved API method organization.

The refactoring from a single GetSigners method to separate GetLocalSigner and GetAuthorizedSigner methods provides:

1. Clearer separation of concerns
2. More explicit functionality
3. Better alignment with the PR objective of addressing system contract consensus issues

This change supports the new API namespace "scroll" defined in consensus.go.

consensus/wrapper/consensus.go (7)

20-21: Clarified isUpgraded function purpose.

The updated comment correctly specifies that the function evaluates block timestamps rather than block numbers to determine when to upgrade the engine.

---

32-33: Added engine initialization logging.

Good addition of initialization logging that will help with debugging and understanding the system startup sequence.

---

41-42: Updated comment for accuracy.

The comment now correctly specifies that the engine selection is based on the header's timestamp.

---

65-72: Improved channel naming and handling.

Renamed the output channel to the more descriptive results and fixed the channel closing logic for empty headers, which helps prevent potential synchronization issues.

---

103-149: Enhanced header verification with improved logging and synchronization.

The changes include:

1. Added comprehensive logging for the EuclidV2 transition verification process
2. Added a 2-second sleep to prevent "unknown ancestor" errors (with explanation)
3. Improved abort handling with proper channel closing
4. Better structured verification flow

These changes directly address the PR objective of fixing consensus issues in VerifyHeaders function to prevent node freezing.

---

186-187: Simplified API handling.

The API method has been simplified to directly append APIs from both engines, removing unnecessary complexity in determining which engine's APIs to return.

---

191-202: Improved error handling in Close method.

The enhanced Close method now properly logs errors from both engines and returns the appropriate error, improving error visibility and debugging.

✨ Finishing Touches

• 📝 Generate Docstrings

---

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?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.