feat: support verify block signature signed by RN #1236
Conversation
WalkthroughNormalize the ECDSA recovery ID (V) inside Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SC as SystemContract
participant H as Header
participant EC as crypto.Ecrecover
SC->>H: Extract BlockSignature -> signature[0..64]
note right of SC: Ensure signature present as expected
alt signature[64] == 27 or 28
SC->>SC: signature[64] = signature[64] - 27 %% normalize V to 0/1
end
SC->>EC: Ecrecover(SealHash(header).Bytes(), signature)
EC-->>SC: pubkey / error
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
consensus/system_contract/consensus.go (3)
137-141: Consensus check currently rejects the compatibility case; allow signature in Extra when BlockSignature is empty.With the new fallback, VerifyHeader still enforces BlockSignature length == 65 for non-genesis blocks and will reject reth-style headers before we reach ecrecover. Relax this to accept either BlockSignature or Extra (when BlockSignature is empty).
- // Check that the BlockSignature contains signature if block is not requested - if header.Number.Cmp(big.NewInt(0)) != 0 && len(header.BlockSignature) != extraSeal { - return errMissingSignature - } + // Check that a signature exists for non-genesis blocks: either in BlockSignature + // or (for compatibility with reth) in Extra when BlockSignature is empty. + if header.Number.Cmp(big.NewInt(0)) != 0 { + if len(header.BlockSignature) == extraSeal { + // ok + } else if len(header.BlockSignature) == 0 && len(header.Extra) == extraSeal { + // compatibility path: signature is encoded in Extra + } else { + return errMissingSignature + } + }
158-161: Extra-data check contradicts the new behavior; permit 65-byte Extra only when BlockSignature is empty.As written, any non-empty Extra is rejected. This prevents reth-style blocks from passing VerifyHeader even if we fix the signature-length check above.
- if len(header.Extra) > 0 { - return errInvalidExtra - } + // Extra must be empty, except when it carries the signature for reth-compat + if len(header.Extra) > 0 { + if !(len(header.BlockSignature) == 0 && len(header.Extra) == extraSeal) { + return errInvalidExtra + } + }
393-412: Add unit tests to cover both signature locations.Please add tests for:
- Accept header with BlockSignature present, Extra empty.
- Accept header with BlockSignature empty, Extra length 65 carrying a valid signature.
- Reject header with both empty.
- Reject header with Extra non-empty but not 65 bytes.
I can draft table-driven tests in consensus/system_contract/consensus_test.go that build headers, sign them, and assert VerifyHeader/ecrecover behavior. Want me to push a patch?
🧹 Nitpick comments (3)
consensus/system_contract/consensus.go (3)
397-401: Fallback path added in ecrecover is good; broaden condition to cover invalid-length BlockSignature too.Right now fallback only triggers when BlockSignature length is 0. If a peer encodes an invalid-length signature (e.g., 0 or 64) in BlockSignature while placing the correct 65-byte signature in Extra, we still fail. Make the condition “not equal to SignatureLength” instead of “== 0”.
- // For compatibility with scroll's reth node, the signature is stored in the extra data field - // If the signature is not found in the block signature field, we try to extract it from the extra data field - if len(signature) == 0 && len(header.Extra) == crypto.SignatureLength { - signature = header.Extra[0:] - } + // For compatibility with Scroll's reth node, fall back to Extra when BlockSignature is missing or invalid-length. + if len(signature) != crypto.SignatureLength && len(header.Extra) == crypto.SignatureLength { + signature = header.Extra + }
36-65: Optional: update error wording to reflect new acceptance criteria.errMissingSignature currently says “extra-data 65 byte signature missing,” which no longer reflects the broader rule (signature can be in BlockSignature or Extra). Consider revising to avoid confusion in logs.
- // errMissingSignature is returned if a block's extra-data section doesn't seem - // to contain a 65 byte secp256k1 signature. - errMissingSignature = errors.New("extra-data 65 byte signature missing") + // errMissingSignature is returned if a block lacks a 65-byte secp256k1 signature + // in both BlockSignature and Extra (compatibility path). + errMissingSignature = errors.New("block signature missing (neither BlockSignature nor Extra contains a 65-byte signature)")
114-163: Consensus-sensitivity: this is a consensus rule change for this client. Coordinate rollout.Allowing 65-byte Extra on non-genesis blocks is a consensus validation change for this engine. Older nodes running this codebase will reject such blocks. Please coordinate upgrades (or behind-a-flag activation) to avoid partitions.
Would you like me to add a config-gated switch (e.g., Config.AllowSignatureInExtra) so networks can roll this out safely?
Also applies to: 169-219
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
consensus/system_contract/consensus.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
consensus/system_contract/consensus.go (1)
crypto/crypto.go (1)
SignatureLength(41-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-and-push-arm64-image (aarch64)
- GitHub Check: test
- GitHub Check: Analyze (go)
- GitHub Check: build-and-push
🔇 Additional comments (3)
consensus/system_contract/consensus.go (3)
276-304: Prepare path remains correct (we continue to write signatures to BlockSignature and keep Extra empty).No changes needed here; keeps our authored blocks consistent while still allowing ingestion of reth-authored blocks after the VerifyHeader fixes.
354-359: Seal path remains correct; signature is written to BlockSignature.Matches our authoring convention; no need to place the signature into Extra. Good.
381-391: SealHash excludes Extra; compatible with both storage locations for the signature.No issues; the signed payload is unaffected by where the signature is stored.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
consensus/system_contract/consensus.go (3)
407-407: Reduce log verbosity and standardize keys.Three Info logs per header (“pre”, “fail”, “post”) will spam node logs at scale. Prefer
Debugfor routine tracing andWarn(orError) on failures. Also, use consistent, lowercase keys without punctuation.Apply this diff:
- log.Info("Morty: ecrecover", "hash", header.Hash(), "signature", hex.EncodeToString(signature)) + log.Debug("ecrecover attempt", "hash", header.Hash(), "sig", hex.EncodeToString(signature)) … - log.Info("Morty: failed to ecrecover", "hash", header.Hash(), "signature", hex.EncodeToString(signature), "error", err) + log.Warn("ecrecover failed", "hash", header.Hash(), "sig", hex.EncodeToString(signature), "err", err) … - log.Info("Morty: ecrecover", "hash", header.Hash(), "signer", signer.Hex()) + log.Debug("ecrecover ok", "hash", header.Hash(), "signer", signer.Hex())Also applies to: 412-412, 417-417
216-218: Remove duplicate unauthorized-signer logs and standardize key names.We currently log both Info and Error for the same condition, which is noisy. Keep a single
log.Errorwith consistent keys.Apply this diff:
- log.Info("Morty: ecrecover Unauthorized signer", "Got", signer, "Expected:", s.signerAddressL1) - log.Error("Unauthorized signer", "Got", signer, "Expected:", s.signerAddressL1) + log.Error("Unauthorized signer", "got", signer, "expected", s.signerAddressL1)
138-143: Optional: also validate “all-zero” 65-byte signatures.Length check alone allows a zeroed 65-byte
BlockSignatureto pass the early guard. If feasible, add a quick non-zero R|S check before proceeding toecrecoverto fail fast on empty seals.Example (pseudo within current guard):
if bytes.Equal(header.BlockSignature, make([]byte, extraSeal)) { return errMissingSignature }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
consensus/system_contract/consensus.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
consensus/system_contract/consensus.go (2)
consensus/clique/clique.go (1)
SealHash(743-748)crypto/crypto.go (1)
Keccak256(78-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-and-push
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
consensus/system_contract/consensus.go (2)
5-5: Import looks good and is used.
encoding/hexis appropriately added for hex-encoding in the new logs.
396-419: Confirm V-normalization behavior against go-ethereum expectations.Normalizing V from 27/28 to 0/1 for
crypto.Ecrecoveris the right direction; upstream low-level crypto expects 0/1 while some signers emit 27/28. Please confirm with your signer mix (reth alloy vs. geth) and add a small test that covers both inputs. (github.com, goethereumbook.org)I can add table-driven tests that run
ecrecoveron identical R/S with V∈{0,1,27,28} and assert the same recovered address.
yiweichi
changed the title
extra_data field
|
@yiweichi is this PR still needed? |
no, it was covered on RN side. |
2 participants
1. Purpose or design rationale of this PR
This PR makes l2geth compatible with Scroll’s reth node. Currently, the sequencer signs blocks using alloy_signer::Signer, which has been modified to align with Ethereum by mapping the recovery ID from 0|1 to 27|28. However, l2geth relies on a vanilla secp256k1 signer that expects the raw 0|1 values. As a result, l2geth fails to verify signatures produced by the sequencer.
So here we override the recovery ID to make them compatible.
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