[Don't Merge] feat: add param to skip system config signer check #1272
Conversation
📝 WalkthroughWalkthroughAdds a new boolean CLI flag Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Parser
participant SetCfg as SetEthConfig
participant Config as ETH Config
participant Engine as Consensus Engine Creator
participant SC as SystemContract
participant Verify as Verification Logic
User->>CLI: supply --skipsignercheck
CLI->>SetCfg: parse flag
SetCfg->>Config: cfg.SkipSignerCheck = true
Config->>Engine: CreateConsensusEngine(..., skipSignerCheck)
Engine->>SC: New(ctx, cfg, client, skipSignerCheck)
SC->>SC: s.skipSignerCheck = skipSignerCheck
rect rgba(100, 200, 150, 0.5)
Note over SC,Verify: During block verification
SC->>Verify: verifyHeader / verifyCascadingFields
Verify->>Verify: if s.skipSignerCheck == true
Verify->>Verify: bypass signature presence & authorization checks
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
frisitano
left a comment
frisitano
left a comment
There was a problem hiding this comment.
Do we need to add some tests for this, or have you already tested it ad hoc?
Not tested yet, planing to do it in a minute. |
|
Tested in my local environment, found an issue and fixed, should works well now. |
|
I overwrote the original image, so the image |
![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
🤖 Fix all issues with AI agents
In `@cmd/utils/flags.go`:
- Around line 1855-1858: The SkipSignerCheckFlag handling must be guarded:
modify the block that reads ctx.GlobalIsSet(SkipSignerCheckFlag.Name) /
ctx.GlobalBool(...) and cfg.SkipSignerCheck so that it emits a WARN (use
log.Warn) whenever the flag is used, and refuse to enable it when the active
network is mainnet or Scroll mainnet (detect network via your existing
network/env/config API); alternatively require an explicit unsafe
acknowledgement flag (e.g., UnsafeAllowSkipSignerCheck) before setting
cfg.SkipSignerCheck. Ensure the change references SkipSignerCheckFlag,
cfg.SkipSignerCheck, ctx.GlobalIsSet/GlobalBool, and log.* so callers cannot
accidentally enable signer skipping in production.
🧹 Nitpick comments (2)
cmd/utils/flags.go (1)
922-925: Make the help text explicitly warn about insecurity.This flag disables signer verification; the usage string should clearly mark it as unsafe/testing-only.
♻️ Suggested wording tweak
- Usage: "Skip signer check for the SystemContract engine", + Usage: "INSECURE (testing only): skip signer check for the SystemContract engine",consensus/system_contract/system_contract.go (1)
36-55: Add a loud warning whenskipSignerCheckis enabled.Since this bypasses signer verification, emit a warning so operators can’t enable it silently.
♻️ Proposed change
func New(ctx context.Context, config *params.SystemContractConfig, client sync_service.EthClient, skipSignerCheck bool) *SystemContract { log.Info("Initializing system_contract consensus engine", "config", config) + if skipSignerCheck { + log.Warn("SystemContract signer check disabled; consensus verification bypassed") + } ctx, cancel := context.WithCancel(ctx)
1. Purpose or design rationale of this PR
This PR adds param to skip system config signer check.
To use: add
--skipsignercheckThis PR is tailored for l2reth testing architecture, we probably don't want to merge it.
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
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.