Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions node/core/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (
)

var (
MainnetBlsKeyCheckForkHeight uint64 = 18409547

// L1 Mainnet Contract Addresses
MainnetRollupContractAddress = common.HexToAddress("0x759894ced0e6af42c26668076ffa84d02e3cef60")
)
Expand All @@ -33,6 +35,7 @@ type Config struct {
SequencerAddress common.Address `json:"sequencer_address"`
L2StakingAddress common.Address `json:"l2staking_address"`
MaxL1MessageNumPerBlock uint64 `json:"max_l1_message_num_per_block"`
BlsKeyCheckForkHeight uint64 `json:"bls_key_check_fork_height"`
DevSequencer bool `json:"dev_sequencer"`
Logger tmlog.Logger `json:"logger"`
}
Expand Down Expand Up @@ -146,5 +149,10 @@ func (c *Config) SetCliContext(ctx *cli.Context) error {
c.DevSequencer = ctx.GlobalBool(flags.DevSequencer.Name)
}

if ctx.GlobalIsSet(flags.MainnetFlag.Name) {
c.BlsKeyCheckForkHeight = MainnetBlsKeyCheckForkHeight
logger.Info("mainnet historical validator compatibility enabled", "BlsKeyCheckForkHeight", c.BlsKeyCheckForkHeight)
}
Comment on lines +152 to +155

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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:


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.

Suggested change
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).


return nil
}
29 changes: 18 additions & 11 deletions node/core/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ type Executor struct {
isSequencer bool
devSequencer bool

blsKeyCheckForkHeight uint64

logger tmlog.Logger
metrics *Metrics
}
Expand Down Expand Up @@ -90,17 +92,18 @@ func NewExecutor(newSyncFunc NewSyncerFunc, config *Config, tmPubKey crypto.PubK
tmPubKeyBytes = tmPubKey.Bytes()
}
executor := &Executor{
l2Client: l2Client,
bc: &Version1Converter{},
sequencerCaller: sequencer,
l2StakingCaller: l2Staking,
tmPubKey: tmPubKeyBytes,
nextL1MsgIndex: index,
maxL1MsgNumPerBlock: config.MaxL1MessageNumPerBlock,
newSyncerFunc: newSyncFunc,
devSequencer: config.DevSequencer,
logger: logger,
metrics: PrometheusMetrics("morphnode"),
l2Client: l2Client,
bc: &Version1Converter{},
sequencerCaller: sequencer,
l2StakingCaller: l2Staking,
tmPubKey: tmPubKeyBytes,
nextL1MsgIndex: index,
maxL1MsgNumPerBlock: config.MaxL1MessageNumPerBlock,
newSyncerFunc: newSyncFunc,
devSequencer: config.DevSequencer,
blsKeyCheckForkHeight: config.BlsKeyCheckForkHeight,
logger: logger,
metrics: PrometheusMetrics("morphnode"),
}

if config.DevSequencer {
Expand Down Expand Up @@ -356,6 +359,10 @@ func (e *Executor) getValidatorsAtHeight(height int64) ([][]byte, error) {
}
newValidators := make([][]byte, 0, len(addrs))
for i := range stakesInfo {
if !e.shouldKeepSequencerAtHeight(uint64(height), stakesInfo[i].BlsKey) {
e.logger.Error("getValidatorsAtHeight: skip sequencer with invalid bls key", "height", height, "addr", stakesInfo[i].Addr)
continue
}
newValidators = append(newValidators, stakesInfo[i].TmKey[:])
}
return newValidators, nil
Expand Down
32 changes: 31 additions & 1 deletion node/core/sequencers.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"slices"

"github.com/morph-l2/go-ethereum/common"
"github.com/morph-l2/go-ethereum/crypto/bls12381"
"github.com/tendermint/tendermint/crypto/ed25519"
)

Expand All @@ -15,7 +16,7 @@ func (e *Executor) sequencerSetUpdates(height uint64) ([][]byte, error) {
if err != nil {
return nil, err
}
if e.currentSeqHash != nil && bytes.Equal(e.currentSeqHash[:], seqHash[:]) {
if e.shouldReuseSequencerCache(height, seqHash) {
return e.nextValidators, nil
}

Expand Down Expand Up @@ -49,6 +50,10 @@ func (e *Executor) sequencerSetUpdates(height uint64) ([][]byte, error) {
seqTmKeySet := make(map[[tmKeySize]byte]struct{}, len(stakesInfo))
nextValidators := make([][]byte, 0, len(sequencerSet2))
for i := range stakesInfo {
if !e.shouldKeepSequencerAtHeight(height, stakesInfo[i].BlsKey) {
e.logger.Error("sequencerSetUpdates: skip sequencer with invalid bls key", "height", height, "addr", stakesInfo[i].Addr)
continue
}
// sequencerSet2 is the latest updated sequencer set which is considered as the next validator set for tendermint
if slices.Contains(sequencerSet2, stakesInfo[i].Addr) {
nextValidators = append(nextValidators, stakesInfo[i].TmKey[:])
Expand All @@ -63,6 +68,26 @@ func (e *Executor) sequencerSetUpdates(height uint64) ([][]byte, error) {
return nextValidators, nil
}

func (e *Executor) shouldReuseSequencerCache(height uint64, seqHash [32]byte) bool {
if e.currentSeqHash == nil || !bytes.Equal(e.currentSeqHash[:], seqHash[:]) {
return false
}

if e.blsKeyCheckForkHeight > 0 &&
(height == e.blsKeyCheckForkHeight || height == e.blsKeyCheckForkHeight+1) {
return false
}
return true
}

func (e *Executor) shouldKeepSequencerAtHeight(height uint64, blsKey []byte) bool {
if isValidBlsKey(blsKey) {
return true
}

return e.blsKeyCheckForkHeight > 0 && height <= e.blsKeyCheckForkHeight
}

func (e *Executor) updateSequencerSet(height uint64) ([][]byte, error) {
validatorUpdates, err := e.sequencerSetUpdates(height)
if err != nil {
Expand Down Expand Up @@ -94,3 +119,8 @@ func (e *Executor) updateSequencerSet(height uint64) ([][]byte, error) {
e.isSequencer = isSequencer
return validatorUpdates, nil
}

func isValidBlsKey(in []byte) bool {
_, err := bls12381.NewG2().DecodePoint(in)
return err == nil
}
Loading