fix(system-contract): Check coinbase during header verification #1128
Conversation
WalkthroughThis pull request updates the consensus block header validation in the system contract. It removes the Changes
Sequence Diagram(s)sequenceDiagram
participant B as Block Header
participant VH as verifyHeader
B->>VH: Pass block header
VH->>VH: Check if coinbase is empty
alt Non-empty coinbase
VH-->>B: Return errInvalidCoinbase
else Valid coinbase
VH-->>B: Continue header validation
end
Possibly related PRs
Suggested labels
Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
consensus/system_contract/consensus.go (1)
230-233: Consider adding a comment explaining the consensus requirement.While the code correctly initializes these fields to zero values, it might be helpful to add a comment explaining why these fields must be empty, especially mentioning the Data Availability layer constraints for future maintainers.
// Make sure unused fields are empty + // These fields must be empty as they are not posted to the Data Availability layer header.Coinbase = common.Address{} header.Nonce = types.BlockNonce{} header.MixDigest = common.Hash{}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
consensus/system_contract/consensus.go(2 hunks)params/version.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- params/version.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (4)
consensus/system_contract/consensus.go (4)
40-41: Added new error for rejecting non-empty coinbase.This error definition is correctly implemented to enforce the requirement that blocks must have an empty coinbase field.
122-125: LGTM: Implemented proper coinbase verification.The implementation correctly enforces that the coinbase field must be empty by comparing it against an empty address. This aligns with the PR objective to ensure peers reject blocks containing a non-zero coinbase field, as the coinbase field is not posted to the Data Availability layer.
230-231: Verify the initialization is consistent with validation.The
Preparemethod already initializes the coinbase to an empty address, which is consistent with the new validation check. This ensures that blocks created by this node will always pass the new validation rule.
40-44: Ensure error naming conventions are consistent.The new
errInvalidCoinbaseerror follows a similar pattern toerrInvalidNonce. Both errors enforce empty values for their respective fields, which is consistent with the consensus requirements.
1. Purpose or design rationale of this PR
Adds a strict check so that peers reject blocks with non-zero coinbase field yet. This is necessary because this field is currently not posted to DA.
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
Bug Fixes
Chores