fix(node): Fix validator set hash mismatch at height 18409547 #828
Conversation
WalkthroughExecutor and sequencer flows were made height-aware: initialization and DeliverBlock now pass L2 block height into sequencer updates. BLS key decoding is gated by a configured fork height (BlsKeyCheckForkHeight): decode failures are logged and handled differently depending on the fork boundary. Changes
Sequence Diagram(s)sequenceDiagram
participant Executor
participant Sequencer as SequencerSetManager
participant Cache
participant Logger
Executor->>Sequencer: updateSequencerSet(height)
Sequencer->>Cache: check cached sequencer set(height)
alt at fork boundary (height == fork or fork+1)
Sequencer->>Cache: bypass cache
end
Sequencer->>Sequencer: decode BLS keys for validators
alt decode failure
Sequencer->>Logger: log decode error
alt height <= fork
Sequencer-->>Sequencer: include historical TmKey despite decode fail
else
Sequencer-->>Sequencer: skip invalid entry
end
end
Sequencer->>Cache: update cache with new validator set
Sequencer-->>Executor: return updated sequencer params
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![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)
node/core/executor.go (1)
377-377: Consider type consistency for height parameters.
getParamsAndValsAtHeightusesint64forheight, whilesequencerSetUpdatesusesuint64. The comparison on line 405 works correctly, but consistent typing would reduce cognitive overhead. This is a pre-existing pattern, so acceptable to defer.Also applies to: 405-405
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
node/core/executor.go(3 hunks)node/core/sequencers.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
node/core/sequencers.go (1)
node/core/executor.go (1)
Executor(31-60)
⏰ 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). (8)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
node/core/sequencers.go (3)
21-24: LGTM - Fork height constant is well documented.The constant clearly explains its purpose for maintaining historical block compatibility. The value aligns with the documented block height where the issue occurred.
63-63: Height-aware BLS validation logic is correct for historical compatibility.The implementation correctly skips invalid BLS keys only after the fork height, preserving historical block compatibility.
One consideration: when
decodeBlsPubKeyfails for heights ≤ fork height, a zero-valueblsPubKeyis stored invalsByTmKey(line 118). IfVerifySignatureis later called for such a validator, it would attempt verification with a zero BLS public key. Verify this doesn't cause issues for historical signature verification paths.Also applies to: 103-120
159-160: LGTM - Height parameter correctly propagated.node/core/executor.go (3)
138-143: LGTM - Initialization now uses current height for sequencer set update.This ensures height-aware BLS key validation is applied during executor startup.
335-335: LGTM - Block number correctly passed for height-aware sequencer updates.
398-410: BLS validation now consistent withsequencerSetUpdates- this fixes the root cause.The logic correctly mirrors the validation in
sequencerSetUpdates, ensuring both code paths produce the same validator set. The slice initialization change to capacity-based (make([][]byte, 0, len(addrs))) withappendis correct for conditional inclusion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
node/core/config.go (1)
161-170: Logger format issue and missing flexibility for non-mainnet configurations.
The
logger.Infocall on line 166 uses positional arguments without proper key-value formatting. Tendermint's logger expects key-value pairs.When
--mainnetflag is not set but--upgrade.batchTimeis provided,BlsKeyCheckForkHeightremains at its zero default. This meansisBlsKeyCheckFork()will returntruefor all heights (strict validation). Consider whether testnet/holesky deployments need a separate fork height configuration.case ctx.GlobalIsSet(flags.MainnetFlag.Name): c.UpgradeBatchTime = MainnetUpgradeBatchTime c.BlsKeyCheckForkHeight = MainnetBlsKeyCheckForkHeight - logger.Info("set UpgradeBatchTime: ", c.UpgradeBatchTime, "BlsKeyCheckForkHeight: ", c.BlsKeyCheckForkHeight) + logger.Info("mainnet config applied", "UpgradeBatchTime", c.UpgradeBatchTime, "BlsKeyCheckForkHeight", c.BlsKeyCheckForkHeight) case ctx.GlobalIsSet(flags.UpgradeBatchTime.Name): c.UpgradeBatchTime = ctx.GlobalUint64(flags.UpgradeBatchTime.Name) logger.Info("set UpgradeBatchTime: ", ctx.GlobalUint64(flags.UpgradeBatchTime.Name))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
node/core/config.go(3 hunks)node/core/executor.go(5 hunks)node/core/sequencers.go(4 hunks)node/flags/flags.go(0 hunks)
💤 Files with no reviewable changes (1)
- node/flags/flags.go
🧰 Additional context used
🧬 Code graph analysis (2)
node/core/sequencers.go (1)
node/core/executor.go (1)
Executor(31-61)
node/core/config.go (1)
node/flags/flags.go (2)
MainnetFlag(229-232)UpgradeBatchTime(224-228)
⏰ 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). (7)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
node/core/sequencers.go (4)
21-25: LGTM!The fork detection logic correctly handles:
blsKeyCheckForkHeight == 0: Always enforce validation (for testnets/new deployments)height > blsKeyCheckForkHeight: Enforce validation post-forkheight <= blsKeyCheckForkHeight: Skip validation for historical compatibility
69-73: LGTM!The fork boundary cache bypass logic correctly invalidates the cached sequencer set at both the fork height and the subsequent block. This ensures the transition from lenient to strict BLS key validation produces the correct validator hash.
160-161: LGTM!Clean propagation of the height parameter to
sequencerSetUpdates.
105-121: Add clarification comment about pre-fork invalid BLS key handling.When
decodeBlsPubKeyfails at heights < fork (whereisBlsKeyCheckForkreturnsfalse), a zero-valueblsPubKeyis stored invalsByTmKey. While this is mitigated in practice becauseConvertBlsDataonly processes new blocks being committed (not historical replays), adding a comment explaining this behavior would improve code maintainability. Consider documenting why pre-fork validators with invalid keys are included despite failing decode.node/core/executor.go (4)
54-57: LGTM!The
blsKeyCheckForkHeightfield is correctly added to the Executor struct, aligning with the configuration.
140-147: LGTM!Correctly fetches the current chain height before initializing the sequencer set, ensuring fork-aware BLS key validation from startup.
336-342: LGTM!Correctly passes the delivered block's number to
updateSequencerSet, ensuring fork-aware validation is applied based on the block being processed.
400-410: Core fix looks correct - ensures consistency between the two validation paths.This change addresses the root cause by adding the same fork-aware BLS key validation to
getParamsAndValsAtHeightthat exists insequencerSetUpdates. Both paths now behave identically:
- Heights ≤ 18409547: Include sequencers regardless of BLS key validity
- Heights > 18409547: Skip sequencers with invalid BLS keys
Minor note: The
heightparameter isint64butisBlsKeyCheckForkexpectsuint64. While block heights should never be negative in practice, consider adding a guard or using a consistent type.
Problem
At block height 18409547, a transaction added a new sequencer with an invalid blsKey. This caused all validator nodes to halt because the original code returned an error when blsKey decoding failed.
After deploying a hotfix (using
continueinstead ofreturn nil, err), nodes successfully resumed block production. However, a critical inconsistency emerged:DeliverBlockcalledgetParamsAndValsAtHeight()which returned 5 sequencers (including the invalid one) because it didn't validate blsKey.DeliverBlockcalledupdateSequencerSet()→sequencerSetUpdates()which returned 4 sequencers (skipping the invalid one).This caused different
next_validators_hashvalues:65C2A11E5A28F185EC039D2B9F7A0AAFFC6B577BC596BD46176F8B203F51D9FFD3CF5BD31E9E1776EA7E656E3E10B2DE3CE8AD413B205F286E816404A43D7071New nodes fail to sync past height 18409548 due to validator hash verification failure.
Root Cause
Inconsistent blsKey validation between two code paths:
sequencerSetUpdates()- validates blsKey, skips invalid sequencersgetParamsAndValsAtHeight()- did NOT validate blsKey, included all sequencersSolution
blsKeyCheckForkHeight = 18409547constantsequencerSetUpdates()andupdateSequencerSet()This ensures:
Block Data Reference
Testing
Summary by CodeRabbit
Bug Fixes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.