fix(node): restore mainnet validator hash compatibility#978
Conversation
Update go-ethereum dependency (#962)
Preserve the height-aware invalid BLS key handling from the Emerald fork so nodes replaying mainnet history compute the expected validator set hash around height 18409547. Constraint: Keep behavior gated by the existing --mainnet flag to match the original mainnet-only compatibility path. Confidence: high Scope-risk: narrow
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughThis PR adds historical BLS key validation at a configurable fork height. A mainnet fork constant and config field enable the feature; the executor stores and uses the fork height to filter invalid BLS keys from validator sets via new helper methods that check key validity and cache reuse conditions. ChangesBLS Key Validation at Fork Height
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Keep the mainnet historical validator hash fix focused by collapsing fork-boundary and BLS-key checks into the two call-site predicates that need them. Constraint: Preserve the existing --mainnet-gated compatibility behavior Confidence: high Scope-risk: narrow
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@node/core/config.go`:
- Around line 152-155: The current guard uses
ctx.GlobalIsSet(flags.MainnetFlag.Name) which returns true even when
--mainnet=false; change the condition to check the boolean flag value by using
ctx.GlobalBool(flags.MainnetFlag.Name) instead so that the block setting
c.BlsKeyCheckForkHeight = MainnetBlsKeyCheckForkHeight and the logger.Info call
only run when the mainnet flag is actually true (update the condition where
GlobalIsSet is used, e.g., replace GlobalIsSet with GlobalBool for
flags.MainnetFlag.Name while leaving c.BlsKeyCheckForkHeight and logger.Info
as-is).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4a46bf0-5a24-47d6-841d-89410fb0aff3
📒 Files selected for processing (3)
node/core/config.gonode/core/executor.gonode/core/sequencers.go
| if ctx.GlobalIsSet(flags.MainnetFlag.Name) { | ||
| c.BlsKeyCheckForkHeight = MainnetBlsKeyCheckForkHeight | ||
| logger.Info("mainnet historical validator compatibility enabled", "BlsKeyCheckForkHeight", c.BlsKeyCheckForkHeight) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In urfave/cli v1.22.x, when a BoolFlag is invoked as --mainnet=false, does Context.GlobalIsSet("mainnet") return true while Context.GlobalBool("mainnet") returns false?
💡 Result:
Yes, in urfave/cli v1.22.x, if you invoke a BoolFlag as --mainnet=false, Context.GlobalIsSet("mainnet") will return true and Context.GlobalBool("mainnet") will return false [1][2]. This occurs because of how the library handles flag parsing and state tracking: 1. GlobalIsSet: This method determines whether a flag was explicitly provided by the user [1][3]. When you pass --mainnet=false, the underlying standard library flagset registers that the flag was set, causing GlobalIsSet to return true [1][2]. 2. GlobalBool: This method retrieves the current value of the boolean flag [2]. Since the value provided was false, GlobalBool correctly returns false [2]. The IsSet mechanism tracks whether a flag was provided during parsing (or via direct modification methods like Set), regardless of whether the resulting value is true or false [1][3]. Consequently, providing the flag explicitly results in it being marked as "set" even when the assigned value is false [1].
Citations:
- 1: https://github.com/urfave/cli/blob/v1.22.17/context.go
- 2: https://github.com/urfave/cli/blob/v1.22.17/flag_bool.go
- 3: https://github.com/urfave/cli/blob/v1.20.0/context.go
Use --mainnet flag value (GlobalBool) instead of “is set” (GlobalIsSet) for compatibility gating.
In urfave/cli v1.22.x, --mainnet=false still makes ctx.GlobalIsSet("mainnet") true, so mainnet compatibility can be enabled incorrectly; the guard should use the boolean value instead.
Suggested fix
- if ctx.GlobalIsSet(flags.MainnetFlag.Name) {
+ if ctx.GlobalBool(flags.MainnetFlag.Name) {
c.BlsKeyCheckForkHeight = MainnetBlsKeyCheckForkHeight
logger.Info("mainnet historical validator compatibility enabled", "BlsKeyCheckForkHeight", c.BlsKeyCheckForkHeight)
}📝 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.
| if ctx.GlobalIsSet(flags.MainnetFlag.Name) { | |
| c.BlsKeyCheckForkHeight = MainnetBlsKeyCheckForkHeight | |
| logger.Info("mainnet historical validator compatibility enabled", "BlsKeyCheckForkHeight", c.BlsKeyCheckForkHeight) | |
| } | |
| if ctx.GlobalBool(flags.MainnetFlag.Name) { | |
| c.BlsKeyCheckForkHeight = MainnetBlsKeyCheckForkHeight | |
| logger.Info("mainnet historical validator compatibility enabled", "BlsKeyCheckForkHeight", c.BlsKeyCheckForkHeight) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@node/core/config.go` around lines 152 - 155, The current guard uses
ctx.GlobalIsSet(flags.MainnetFlag.Name) which returns true even when
--mainnet=false; change the condition to check the boolean flag value by using
ctx.GlobalBool(flags.MainnetFlag.Name) instead so that the block setting
c.BlsKeyCheckForkHeight = MainnetBlsKeyCheckForkHeight and the logger.Info call
only run when the mainnet flag is actually true (update the condition where
GlobalIsSet is used, e.g., replace GlobalIsSet with GlobalBool for
flags.MainnetFlag.Name while leaving c.BlsKeyCheckForkHeight and logger.Info
as-is).
3 participants
Summary
18409547that was removed with the BLS signing cleanup.NextValidatorsHash.Test plan
go test ./core -count=1go test ./... -short -count=1go vet ./...git diff --checkSummary by CodeRabbit