WIP:Update l2 RetryableClient #827
Conversation
WalkthroughThis PR introduces legacy Zk endpoint support and MPT (Merkle Patricia Trie) switching capability. It adds configuration flags for legacy L2 endpoints, implements dual-client routing logic in RetryableClient to switch between legacy and current clients at a configurable timestamp, updates executor and derivation initialization to wire legacy clients, strengthens error handling and logging for MPT compatibility, and provides test infrastructure for validating the MPT switch process. Changes
Sequence DiagramsequenceDiagram
participant RPC as RPC Caller
participant RC as RetryableClient
participant Switch as switchClient
participant Legacy as Legacy Clients<br/>(laClient, leClient)
participant Current as Current Clients<br/>(aClient, eClient)
participant MPT as MPT Geth Node
RPC->>RC: Call RPC Method (e.g., AssembleL2Block)
activate RC
Note over RC: Get current timestamp
RC->>RC: Check if timestamp >= mptTime
alt MPT Switch Needed & Not Yet Switched
RC->>Switch: switchClient()
activate Switch
Switch->>MPT: BlockNumber (poll)
MPT-->>Switch: block height
Note over Switch: Verify MPT node<br/>synchronized
Switch->>Switch: Update atomic mpt state
Switch-->>RC: Switch complete
deactivate Switch
end
alt Using Legacy Clients
RC->>Legacy: Route call to legacy endpoint
Legacy-->>RC: Response
else Using Current Clients
RC->>Current: Route call to current endpoint
Current-->>RC: Response
end
RC-->>RPC: Return result
deactivate RC
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
node/derivation/derivation.go (1)
81-97: Legacy clients use the same endpoints as current clients.Both
laClient/leClientandaClient/eClientare dialed using identical configuration values (cfg.L2.EngineAddr,cfg.L2.EthAddr). This means the timestamp-based routing inRetryableClientwill always connect to the same backend, negating the dual-client purpose.If this is WIP, consider adding separate configuration fields (e.g.,
cfg.L2.LegacyEngineAddr,cfg.L2.LegacyEthAddr) for the legacy endpoints. Otherwise, clarify the TODO on line 81.node/core/executor.go (1)
74-92: Legacy clients use identical endpoints as current clients.Same issue as in
derivation.go:laClientandleClientdial the sameconfig.L2.EngineAddrandconfig.L2.EthAddrasaClientandeClient. The dual-client routing will effectively route to the same backend.Consider adding separate configuration for legacy endpoints, or clarify the TODO comment regarding the intended behavior.
🧹 Nitpick comments (2)
node/derivation/derivation.go (1)
141-141: Use the existing logger instead of creating a new one.A new
TMLoggeris created here, but theloggervariable is already available and configured with the "derivation" module context (line 115). Use the existing logger for consistency.- l2Client: types.NewRetryableClient(laClient, leClient, aClient, eClient, tmlog.NewTMLogger(tmlog.NewSyncWriter(os.Stdout))), + l2Client: types.NewRetryableClient(laClient, leClient, aClient, eClient, logger),node/types/retryable_client.go (1)
3-16: Import ordering: standard library imports should come first.Go convention places standard library imports before third-party imports. Consider reordering:
import ( "context" + "math/big" + "strings" + "time" + "github.com/cenkalti/backoff/v4" "github.com/morph-l2/go-ethereum" "github.com/morph-l2/go-ethereum/common" eth "github.com/morph-l2/go-ethereum/core/types" "github.com/morph-l2/go-ethereum/eth/catalyst" "github.com/morph-l2/go-ethereum/ethclient" "github.com/morph-l2/go-ethereum/ethclient/authclient" tmlog "github.com/tendermint/tendermint/libs/log" - "math/big" - "strings" - "time" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
node/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
node/core/executor.go(3 hunks)node/derivation/derivation.go(2 hunks)node/go.mod(1 hunks)node/types/retryable_client.go(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
node/derivation/derivation.go (1)
node/types/retryable_client.go (1)
NewRetryableClient(41-51)
node/core/executor.go (1)
node/types/retryable_client.go (2)
RetryableClient(29-37)NewRetryableClient(41-51)
node/types/retryable_client.go (1)
tx-submitter/iface/client.go (1)
Client(14-23)
⏰ 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: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
node/types/retryable_client.go (1)
67-70:AssembleL2Blockuses wall-clock time for routing.The timestamp is derived from
time.Now().Unix()rather than from block data. This is appropriate for assembling new blocks, but be aware that:
- Clock drift could cause unexpected routing behavior
- This differs from other methods that extract timestamps from block/transaction data
Ensure this is the intended behavior for block assembly operations.
node/go.mod (1)
14-14: Go-ethereum dependency update is correctly applied.The new pseudo-version
v1.10.14-0.20251211075654-796834acba86is present in node/go.mod line 14 and the codebase already callsAssembleL2Blockwith the timestamp parameter as the second argument after context, confirming the dependency provides the necessary API changes for the RetryableClient functionality.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
node/types/retryable_client.go (3)
117-135: AssembleL2Block routing is fine, but timestamp is captured only once.The overall flow (compute a timestamp, call
switchClient, then go throughaClient) looks consistent with the MPT‑based routing.Note that
timestampis computed once beforebackoff.Retry; if retries happen after the intended MPT switch time, this call will still behave as “pre‑switch” becauseswitchClientalways sees the originaltimestamp. If you want retries to respect the current time, recomputing inside the retry closure would be safer.
193-225: Commit/Append paths rely on existing MPT state only.
CommitBatchandAppendBlsSignaturealways useaClient()without callingswitchClient. That means:
- They will hit the legacy client until some earlier call (e.g., New/Validate/Assemble) has performed the switch and set
mpt=true.- After that, they’ll always go to the new client.
If that ordering is intentional (i.e., commits never lead the cutover), this is fine; otherwise, consider explicitly invoking
switchClienthere as well, using an appropriate timestamp/height.
299-309: Retryable error policy is simple and consistent.Treating everything except
DiscontinuousBlockErroras retryable is straightforward and matches the comment above. Just be aware this interacts with the “infinite” backoff behavior discussed earlier.node/core/executor.go (1)
70-81: Clarify legacy vs new client endpoints; avoid duplicate dials if not needed.
laClient/leClientandaClient/eClientare all dialed against the sameEngineAddr/EthAddr. That’s OK functionally, but it:
- Doubles the TCP/JWT handshakes and keeps four separate connections open.
- Makes it unclear which pair is “legacy” vs “MPT” until configs diverge.
If the intent is to eventually point
laClient/leClientat different legacy endpoints, consider:
- Adding explicit config fields for legacy addresses now, or
- Reusing the same client instances until those endpoints exist, to avoid unnecessary duplication.
Also, you may want to replace the placeholder
// TODOwith a more descriptive comment about the planned split.Also applies to: 91-92
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
node/core/executor.go(2 hunks)node/types/retryable_client.go(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
node/types/retryable_client.go (2)
tx-submitter/iface/client.go (1)
Client(14-23)node/db/store.go (1)
Store(16-18)
node/core/executor.go (1)
node/types/retryable_client.go (1)
NewRetryableClient(44-55)
⏰ 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). (6)
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
node/types/retryable_client.go (3)
57-69: Client selection helpers look correct.
aClient/eClientcleanly encapsulate legacy vs new routing via the atomicmptflag, which keeps the call‑sites simple and centralizes switching behavior.
137-191: Validate/New/NewSafe L2 block flows look consistent with MPT switching.These methods all:
- Call
switchClientwith the block’s ownTimestamp/Number.- Then use
aClientso they follow the MPT/legacy split consistently.Given the constructor wiring, this cleanly moves traffic over once
mptis set; no issues from what’s visible here.
227-297: Read‑only ETH RPC wrappers correctly useeClient.
BlockNumber,HeaderByNumber,CallContract, andCodeAtall delegate througheClient(), so they respect the current MPT/legacy state without introducing fresh switching logic. The retry pattern matches the other methods.node/core/executor.go (1)
91-135: Executor wiring to the newRetryableClientlooks correct.
NewExecutornow constructsl2Clientviatypes.NewRetryableClient(laClient, leClient, aClient, eClient, config.Logger)and threads it through:
getNextL1MsgIndex- contract callers (
SequencerCaller,GovCaller,L2StakingCaller)- all existing executor methods (RequestBlockData, CheckBlockData, DeliverBlock, RequestHeight, etc.)
Struct initialization (
Executor{ l2Client: l2Client, ... }) matches this and there are no obvious miswirings here.
| type RetryableClient struct { | ||
| authClient *authclient.Client | ||
| ethClient *ethclient.Client | ||
| b backoff.BackOff | ||
| logger tmlog.Logger | ||
| legacyAuthClient *authclient.Client | ||
| legacyEthClient *ethclient.Client | ||
| authClient *authclient.Client | ||
| ethClient *ethclient.Client | ||
| mptTime uint64 | ||
| mpt atomic.Bool | ||
| b backoff.BackOff | ||
| logger tmlog.Logger |
There was a problem hiding this comment.
Make mptTime configurable instead of hard‑coding now+1h.
Right now mptTime is derived from time.Now().Add(time.Hour) at construction, which means:
- Each node instance may switch at a different wall‑clock time.
- Restarts will delay switching by another hour, regardless of the canonical MPT cutover time.
Before shipping, this should be driven by config (or passed explicitly into NewRetryableClient) so that routing behavior is deterministic and restart‑safe, not tied to process start time. The existing TODO hints at this, but it’s important for correctness of the migration window.
Also applies to: 43-55
🧩 Analysis chain
🏁 Script executed:
# First, find the retryable_client.go file
find . -name "retryable_client.go" -type fRepository: morph-l2/morph
Length of output: 90
🏁 Script executed:
# Read the entire file to understand the structure and all usages of rc.b
cat -n node/types/retryable_client.goRepository: morph-l2/morph
Length of output: 11289
🏁 Script executed:
# Check the backoff library being used to understand its concurrency characteristics
rg "backoff.Retry|backoff.BackOff|backoff.NewExponential" node/types/retryable_client.go -A 3 -B 1Repository: morph-l2/morph
Length of output: 3325
🌐 Web query:
cenkalti/backoff v4 ExponentialBackOff thread-safe concurrent
💡 Result:
Short answer: No — ExponentialBackOff in cenkalti/backoff v4 is explicitly documented as not thread‑safe for concurrent use. See the package docs and source which state "Note: Implementation is not thread‑safe." [1][2]
If you need concurrent use, either:
- create one ExponentialBackOff per goroutine/operation, or
- guard a single instance with your own synchronization (sync.Mutex), or
- upgrade to a different policy/implementation that documents thread safety. [1][2]
Sources:
[1] pkg.go.dev github.com/cenkalti/backoff/v4 (ExponentialBackOff docs).
[2] ExponentialBackOff implementation (v4) in repository/source.
🏁 Script executed:
# Check if there's any documentation or comments about thread safety in the codebase
rg -i "thread.*safe|concurrent|mutex|lock" node/types/retryable_client.goRepository: morph-l2/morph
Length of output: 2510
🏁 Script executed:
# Look for how this client is used - are methods called concurrently from goroutines?
rg -r "RetryableClient" --type go -A 3 | head -100Repository: morph-l2/morph
Length of output: 454
🏁 Script executed:
# Search for goroutine usage with RetryableClient to verify concurrent calls
rg "go.*rc\.|go.*client\.|RetryableClient" --type go -B 2 -A 2 | head -150Repository: morph-l2/morph
Length of output: 8154
🏁 Script executed:
# Check if RetryableClient methods might be called from multiple goroutines
fd "\.go$" --type f -x rg -l "RetryableClient" {} \; | head -10Repository: morph-l2/morph
Length of output: 174
Shared backoff.BackOff instance is not thread-safe and unsafe for concurrent use.
The single rc.b instance is shared across all retry calls in AssembleL2Block, ValidateL2Block, NewL2Block, NewSafeL2Block, CommitBatch, AppendBlsSignature, BlockNumber, HeaderByNumber, CallContract, and CodeAt methods. Since cenkalti/backoff/v4's ExponentialBackOff is explicitly documented as not thread-safe, and RetryableClient is used concurrently by sequencer, derivation, and executor subsystems, concurrent calls will cause data race conditions on the backoff state.
Create a new ExponentialBackOff per request instead:
type RetryableClient struct {
- b backoff.BackOff
+ // remove shared BackOff; create per-call instances instead
}
func (rc *RetryableClient) AssembleL2Block(...) (..., error) {
- if retryErr := backoff.Retry(func() error { ... }, rc.b); retryErr != nil {
+ b := backoff.NewExponentialBackOff()
+ if retryErr := backoff.Retry(func() error { ... }, b); retryErr != nil {
...
}
}Apply the same fix to all other methods using backoff.Retry.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In node/types/retryable_client.go around lines 31-39, the struct holds a shared
backoff.BackOff (rc.b) which is not thread-safe; replace uses of the shared rc.b
with a fresh ExponentialBackOff instance per request: inside each method that
currently calls backoff.Retry(rc.b, ...) create a new
backoff.NewExponentialBackOff(), configure any required parameters (or copy them
from rc.b if rc.b is only used as a template), and pass that new instance to
backoff.Retry (or backoff.WithContext if you need context cancellation). Apply
this same change to all other methods that call backoff.Retry so no goroutine
reuses a shared backoff state.
| func (rc *RetryableClient) switchClient(ctx context.Context, timeStamp uint64, number uint64) { | ||
| if rc.mpt.Load() || timeStamp <= rc.mptTime { | ||
| return | ||
| } | ||
|
|
||
| rc.logger.Info("MPT switch time reached, MUST wait for MPT node to sync", | ||
| "mpt_time", rc.mptTime, | ||
| "current_time", timeStamp, | ||
| "target_block", number) | ||
|
|
||
| ticker := time.NewTicker(3 * time.Second) | ||
| defer ticker.Stop() | ||
|
|
||
| startTime := time.Now() | ||
| lastLogTime := startTime | ||
|
|
||
| for { | ||
| remote, err := rc.ethClient.BlockNumber(ctx) | ||
| if err != nil { | ||
| rc.logger.Error("Failed to get MPT block number, retrying...", "error", err) | ||
| <-ticker.C | ||
| continue | ||
| } | ||
|
|
||
| if remote+1 >= number { | ||
| rc.mpt.Store(true) | ||
| rc.logger.Info("Successfully switched to MPT client", | ||
| "remote_block", remote, | ||
| "target_block", number, | ||
| "wait_duration", time.Since(startTime)) | ||
| return | ||
| } | ||
|
|
||
| if time.Since(lastLogTime) >= 10*time.Second { | ||
| rc.logger.Info("Waiting for MPT node to sync...", | ||
| "remote_block", remote, | ||
| "target_block", number, | ||
| "blocks_behind", number-remote-1, | ||
| "wait_duration", time.Since(startTime)) | ||
| lastLogTime = time.Now() | ||
| } | ||
|
|
||
| <-ticker.C | ||
| } | ||
| } |
There was a problem hiding this comment.
switchClient ignores context cancellation and can block indefinitely.
The loop keeps calling rc.ethClient.BlockNumber(ctx) and sleeping on the ticker until remote+1 >= number, but:
- It never checks
ctx.Done()and will continue forever if the MPT node is stuck or the context is canceled. - Callers passing a bounded context won’t get timely cancellation.
Consider bailing out on context cancellation and logging, e.g.:
func (rc *RetryableClient) switchClient(ctx context.Context, timeStamp uint64, number uint64) {
@@
- for {
- remote, err := rc.ethClient.BlockNumber(ctx)
+ for {
+ if ctx.Err() != nil {
+ rc.logger.Error("Aborting MPT switch due to context cancellation", "error", ctx.Err())
+ return
+ }
+
+ remote, err := rc.ethClient.BlockNumber(ctx)
if err != nil {
rc.logger.Error("Failed to get MPT block number, retrying...", "error", err)
<-ticker.C
continue
}
@@
}
<-ticker.C
}
}You may also want a maximum wait duration to avoid a permanent stall if the MPT node never catches up.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
node/types/retryable_client.go (2)
87-114:switchClientcan block indefinitely if context is canceled.The loop at lines 87-114 never checks
ctx.Done(), so it will continue polling forever if the context is canceled or the MPT node never catches up. This is a reliability concern that can cause goroutine leaks or hung operations.Apply this diff to respect context cancellation:
for { + select { + case <-ctx.Done(): + rc.logger.Error("Aborting MPT switch due to context cancellation", "error", ctx.Err()) + return + default: + } + remote, err := rc.ethClient.BlockNumber(ctx) if err != nil { rc.logger.Error("Failed to get MPT block number, retrying...", "error", err) - <-ticker.C + select { + case <-ctx.Done(): + rc.logger.Error("Aborting MPT switch due to context cancellation", "error", ctx.Err()) + return + case <-ticker.C: + } continue }
38-38: Sharedbackoff.BackOffinstance is not thread-safe.The
cenkalti/backoff/v4ExponentialBackOffis explicitly documented as not thread-safe. SinceRetryableClientmethods are called concurrently from sequencer, derivation, and executor subsystems, the sharedrc.bwill cause data races.Create a new backoff instance per call instead of sharing one:
type RetryableClient struct { legacyAuthClient *authclient.Client legacyEthClient *ethclient.Client authClient *authclient.Client ethClient *ethclient.Client mptTime uint64 mpt atomic.Bool - b backoff.BackOff logger tmlog.Logger }Then in each method:
func (rc *RetryableClient) AssembleL2Block(...) (..., error) { + b := backoff.NewExponentialBackOff() timestamp := uint64(time.Now().Unix()) - if retryErr := backoff.Retry(func() error { + if retryErr := backoff.Retry(func() error { ... - }, rc.b); retryErr != nil { + }, b); retryErr != nil {
🧹 Nitpick comments (2)
node/core/config.go (1)
31-31: Inconsistent JSON tag:l_2_legacyvsl2_legacy.The JSON tag
"l_2_legacy"uses underscores inconsistently compared to the derivation config which uses"l2_legacy". This could cause issues with configuration file parsing or serialization.- L2Legacy *types.L2Config `json:"l_2_legacy"` + L2Legacy *types.L2Config `json:"l2_legacy"`node/core/executor.go (1)
73-81: Consider adding validation for empty legacy endpoint addresses.If the legacy endpoints are not configured (empty strings), the dial calls will likely fail with confusing errors. Adding explicit validation would provide clearer error messages.
// legacy zk endpoint + if config.L2Legacy.EngineAddr == "" || config.L2Legacy.EthAddr == "" { + return nil, errors.New("L2Legacy endpoints (EngineAddr and EthAddr) must be configured") + } laClient, err := authclient.DialContext(context.Background(), config.L2Legacy.EngineAddr, config.L2Legacy.JwtSecret)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
node/core/config.go(2 hunks)node/core/executor.go(2 hunks)node/derivation/config.go(2 hunks)node/derivation/derivation.go(2 hunks)node/flags/flags.go(2 hunks)node/types/retryable_client.go(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
node/derivation/config.go (2)
node/types/config.go (1)
L2Config(14-18)node/flags/flags.go (3)
MptTime(43-47)L2LegacyEthAddr(31-35)L2LegacyEngineAddr(37-41)
node/core/config.go (2)
node/types/config.go (1)
L2Config(14-18)node/flags/flags.go (3)
MptTime(43-47)L2LegacyEthAddr(31-35)L2LegacyEngineAddr(37-41)
node/core/executor.go (2)
node/types/retryable_client.go (1)
NewRetryableClient(44-55)node/flags/flags.go (1)
MptTime(43-47)
node/types/retryable_client.go (2)
tx-submitter/iface/client.go (1)
Client(14-23)node/db/store.go (1)
Store(16-18)
node/derivation/derivation.go (2)
node/types/retryable_client.go (1)
NewRetryableClient(44-55)node/flags/flags.go (1)
MptTime(43-47)
⏰ 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: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
node/types/retryable_client.go (2)
193-225:CommitBatchandAppendBlsSignaturedon't invokeswitchClient.These methods use
rc.aClient()but never callswitchClient(), unlikeValidateL2Block,NewL2Block, andNewSafeL2Block. If these operations occur during the MPT transition window, they may route to the wrong client.Is this intentional? If these operations should always use the current MPT state without waiting for sync, consider adding a comment explaining why. Otherwise, add
switchClientcalls with appropriate timestamp/block parameters.
227-297:BlockNumber,HeaderByNumber,CallContract, andCodeAtalways use current MPT state.These methods call
rc.eClient()withoutswitchClient(). They will use the legacy client untilmptis set totrueby another operation. This could cause inconsistent behavior if these are called before anyswitchClient-invoking method.Verify this is the intended behavior. If these should reflect the MPT state based on a block number, consider adding routing logic similar to the auth client methods.
node/core/executor.go (1)
73-91: Legacy client initialization depends onL2Legacybeing non-nil.The code at lines 74 and 78 accesses
config.L2Legacy.EngineAddrandconfig.L2Legacy.EthAddr. If the config initialization issue innode/core/config.gois not fixed, this will panic with a nil pointer dereference.Ensure that
config.L2Legacyis properly initialized before this code runs. Consider adding a nil check or ensuring the config struct always initializesL2Legacy:if config.L2Legacy == nil { return nil, errors.New("L2Legacy config is required") }node/derivation/derivation.go (1)
81-89: Legacy client initialization depends onL2Legacybeing non-nil.Similar to
executor.go, lines 82 and 86 accesscfg.L2Legacy.EngineAddrandcfg.L2Legacy.EthAddr. IfL2Legacyis not initialized inderivation/config.go, this will panic.Ensure the config initialization fix in
node/derivation/config.gois applied. Consider adding defensive validation:if cfg.L2Legacy == nil { return nil, errors.New("L2Legacy config is required for derivation") }node/flags/flags.go (1)
325-327: LGTM! Flags properly integrated into the CLI.The new flags are correctly added to the
Flagsslice and positioned logically with other L2-related configuration options.
| MptTime = cli.Uint64Flag{ | ||
| Name: "upgrade.mptTime", | ||
| Usage: "Batch index at which the sequencers start to upgrade the batch format", | ||
| EnvVar: prefixEnvVar("MPT_TIME"), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the flags.go file and examine the MptTime flag and UpgradeBatchTime flag
find . -name "flags.go" -type f | grep -E "node.*flags" | head -5Repository: morph-l2/morph
Length of output: 79
🏁 Script executed:
# Check if node/flags/flags.go exists and read it
if [ -f "node/flags/flags.go" ]; then
echo "=== Lines 43-47 (MptTime) ==="
sed -n '43,47p' node/flags/flags.go
echo ""
echo "=== Lines 240-250 (around UpgradeBatchTime) ==="
sed -n '240,250p' node/flags/flags.go
else
echo "File not found. Searching for flags files:"
find . -name "flags.go" -type f 2>/dev/null | head -10
fiRepository: morph-l2/morph
Length of output: 608
Rename MptTime to clarify it represents a batch index, and distinguish its usage description from UpgradeBatchTime.
The flag name MptTime is misleading since it uses a "Time" suffix despite representing a batch index. Additionally, both MptTime and UpgradeBatchTime have identical usage text ("Batch index at which the sequencers start to upgrade the batch format"), which obscures their different purposes.
Consider:
- Renaming to reflect the batch index nature (e.g.,
MptUpgradeBatchIndexorMptSwitchBatchIndex) - Updating the usage text to clarify the MPT-specific distinction (e.g., "Batch index at which sequencers switch from legacy to MPT-based batch format")
🤖 Prompt for AI Agents
In node/flags/flags.go around lines 43-47, the MptTime identifier and its Usage
text are misleading because the suffix "Time" suggests a timestamp while the
flag actually holds a batch index and duplicates the Usage of UpgradeBatchTime;
rename the flag to something clear (e.g., MptUpgradeBatchIndex or
MptSwitchBatchIndex), update the Usage string to explicitly state it is an
MPT-specific batch index (for example: "Batch index at which sequencers switch
from legacy to MPT-based batch format"), change the EnvVar key passed to
prefixEnvVar to a matching name, and update every reference in the codebase to
the new identifier and env var name to keep behavior consistent.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
node/types/retryable_client.go (2)
38-38: Sharedbackoff.BackOffinstance is not thread-safe.The single
rc.binstance is shared across all retry calls. As noted in a previous review,cenkalti/backoff/v4'sExponentialBackOffis explicitly documented as not thread-safe. WithRetryableClientused concurrently by sequencer, derivation, and executor subsystems, concurrent calls can cause data races on the backoff state.Create a new
ExponentialBackOffper request instead:🔎 Proposed fix pattern
type RetryableClient struct { - b backoff.BackOff + // Removed shared BackOff; create per-call instances instead } func (rc *RetryableClient) AssembleL2Block(...) (..., error) { + b := backoff.NewExponentialBackOff() - if retryErr := backoff.Retry(func() error { ... }, rc.b); retryErr != nil { + if retryErr := backoff.Retry(func() error { ... }, b); retryErr != nil { ... } }Apply this pattern to all methods using
backoff.Retry.Also applies to: 150-150
101-133:switchClientignores context cancellation and can block indefinitely.The loop polls
rc.ethClient.BlockNumber(ctx)until MPT geth catches up, but never checksctx.Done(). If the context is canceled (e.g., node shutdown) or if MPT geth never catches up, this loop runs forever, blocking the calling goroutine.🔎 Proposed fix
for { + select { + case <-ctx.Done(): + rc.logger.Error("Aborting MPT switch due to context cancellation", "error", ctx.Err()) + return + default: + } + remote, err := rc.ethClient.BlockNumber(ctx) if err != nil { rc.logger.Error("Failed to get MPT geth block number", "error", err, "hint", "Please ensure MPT geth is running and accessible") - <-ticker.C + select { + case <-ctx.Done(): + rc.logger.Error("Aborting MPT switch due to context cancellation", "error", ctx.Err()) + return + case <-ticker.C: + } continue } // ... rest of loop ... - <-ticker.C + select { + case <-ctx.Done(): + rc.logger.Error("Aborting MPT switch due to context cancellation", "error", ctx.Err()) + return + case <-ticker.C: + } }Consider also adding a maximum wait duration to prevent indefinite blocking if MPT geth never syncs.
🧹 Nitpick comments (7)
ops/mpt-switch-test/README.md (1)
7-18: Add language specifiers to fenced code blocks.The documentation would benefit from explicit language identifiers for better rendering and syntax highlighting.
🔎 Recommended fixes
For the ASCII diagram (lines 7-18):
-``` +```text 升级前: 升级后 (交换):For the log output example (lines 89-98):
-``` +```log MPT switch time reached, MUST wait for MPT node to syncFor the file structure (lines 102-118):
-``` +```text mpt-switch-test/Also applies to: 89-98, 102-118
ops/mpt-switch-test/send-txs.sh (1)
7-7: Remove unused variable.
GETH_BINis defined but never referenced in the script. The script relies on thecastcommand instead.🔎 Proposed fix
SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" -GETH_BIN="${SCRIPT_DIR}/bin/geth" ZK_GETH_HTTP="http://127.0.0.1:8545"node/core/executor.go (2)
300-305: Consider log verbosity for production.While the enhanced logging with "CRITICAL" prefix and actionable hints is useful during the MPT upgrade window, this verbose style may be noisy in steady-state production. Consider gating this enhanced messaging on a debug flag or removing the dramatic formatting after the upgrade stabilizes.
336-344: Consider consolidating error logging.The multiple separate
logger.Errorcalls with decorative separators could be consolidated into a single structured log entry. This would be cleaner and easier to parse programmatically.🔎 Suggested consolidation
- e.logger.Error("========================================") - e.logger.Error("CRITICAL: Failed to deliver block to geth!") - e.logger.Error("========================================") - e.logger.Error("failed to NewL2Block", - "error", err, - "block_number", l2Block.Number, - "block_timestamp", l2Block.Timestamp) - e.logger.Error("HINT: If this occurs after MPT upgrade, your geth node may not support MPT blocks. " + - "Please ensure you are running an MPT-compatible geth node.") + e.logger.Error("CRITICAL: Failed to deliver block to geth", + "error", err, + "block_number", l2Block.Number, + "block_timestamp", l2Block.Timestamp, + "hint", "If this occurs after MPT upgrade, ensure you are running an MPT-compatible geth node")ops/mpt-switch-test/test-mpt-switch-local.sh (3)
27-29: Remove unused variableMORPH_ROOT.Static analysis (SC2034) correctly identifies that
MORPH_ROOTis declared but never used in this script.🔎 Proposed fix
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" cd "$SCRIPT_DIR" -# 路径配置 -MORPH_ROOT="${SCRIPT_DIR}/../.." BIN_DIR="${SCRIPT_DIR}/bin"
250-278: Test environment uses permissive security settings.The Geth instances are configured with
--http.corsdomain="*",--http.vhosts="*",--ws.origins="*", and--authrpc.vhosts="*". This is acceptable for a local test environment but should never be used in production. Consider adding a comment to make this explicit.
196-196: Declare and assign separately to avoid masking return values (SC2155).Shell best practice is to separate
localdeclaration from command assignment to ensure the exit status of the command is not masked.🔎 Proposed fix
- local seq_node_id=$("$TENDERMINT_BIN" show-node-id --home "${SEQUENCER_NODE_DIR}") + local seq_node_id + seq_node_id=$("$TENDERMINT_BIN" show-node-id --home "${SEQUENCER_NODE_DIR}")This pattern should also be applied to similar occurrences at lines 376-377, 457, 469, 475, and 533.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.gitignorenode/core/config.gonode/core/executor.gonode/types/retryable_client.goops/mpt-switch-test/README.mdops/mpt-switch-test/check-nodes.shops/mpt-switch-test/genesis-mpt.jsonops/mpt-switch-test/genesis-zk.jsonops/mpt-switch-test/local-test/engine-make-block-loop.shops/mpt-switch-test/local-test/engine-make-block.shops/mpt-switch-test/local-test/genesis.jsonops/mpt-switch-test/local-test/jwt-secret.txtops/mpt-switch-test/local-test/run-l2-exec-only.shops/mpt-switch-test/send-txs.shops/mpt-switch-test/test-mpt-switch-local.sh
✅ Files skipped from review due to trivial changes (1)
- ops/mpt-switch-test/local-test/jwt-secret.txt
🧰 Additional context used
🧬 Code graph analysis (3)
node/core/config.go (2)
node/types/config.go (1)
L2Config(14-18)node/flags/flags.go (3)
MptTime(43-47)L2LegacyEthAddr(31-35)L2LegacyEngineAddr(37-41)
node/core/executor.go (2)
node/types/retryable_client.go (1)
NewRetryableClient(44-55)node/flags/flags.go (1)
MptTime(43-47)
node/types/retryable_client.go (2)
tx-submitter/iface/client.go (1)
Client(14-23)node/db/store.go (1)
Store(16-18)
🪛 Gitleaks (8.30.0)
ops/mpt-switch-test/send-txs.sh
[high] 20-20: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
ops/mpt-switch-test/README.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
102-102: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
ops/mpt-switch-test/check-nodes.sh
[warning] 21-21: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 36-36: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 40-40: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 41-41: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 42-42: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 47-47: Declare and assign separately to avoid masking return values.
(SC2155)
ops/mpt-switch-test/local-test/engine-make-block.sh
[warning] 45-45: In POSIX sh, arithmetic base conversion is undefined.
(SC3052)
ops/mpt-switch-test/local-test/run-l2-exec-only.sh
[warning] 10-10: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
ops/mpt-switch-test/local-test/engine-make-block-loop.sh
[warning] 8-8: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
ops/mpt-switch-test/send-txs.sh
[warning] 7-7: GETH_BIN appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 38-38: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 46-46: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 59-59: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 100-100: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 118-118: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 119-119: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 127-127: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 182-182: Quote this to prevent word splitting.
(SC2046)
[warning] 189-189: Quote this to prevent word splitting.
(SC2046)
ops/mpt-switch-test/test-mpt-switch-local.sh
[warning] 28-28: MORPH_ROOT appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 196-196: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 376-376: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 377-377: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 457-457: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 469-469: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 475-475: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 533-533: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (19)
.gitignore (1)
34-39: LGTM!The ignore patterns appropriately exclude test artifacts, logs, and generated data from the MPT switch test infrastructure.
ops/mpt-switch-test/check-nodes.sh (2)
14-54: LGTM!The node health check logic correctly handles error cases (lines 25-29) and provides clear status output. The shellcheck SC2155 warnings about combined declaration and assignment are acceptable here since error handling is present.
73-106: Well-designed stateRoot comparison logic.The script correctly selects the smaller block number for comparison (lines 78-82), avoiding false negatives when nodes are at different heights. The result messaging appropriately notes that differences are expected between ZK and MPT implementations.
ops/mpt-switch-test/local-test/engine-make-block.sh (1)
28-35: LGTM: JWT token generation follows Engine API spec.The HS256 JWT implementation correctly encodes the header and payload, computes the HMAC-SHA256 signature using the hex secret, and applies base64url encoding per the Engine API authentication specification.
ops/mpt-switch-test/local-test/run-l2-exec-only.sh (2)
44-67: Verify that this configuration is restricted to isolated test environments.The geth configuration includes security-sensitive settings appropriate only for local testing:
- Line 54: Exposes
personalAPI enabling account/key management- Line 64:
--allow-insecure-unlockpermits account unlocking over HTTP- Lines 50-53: Binds to
0.0.0.0with unrestricted CORSEnsure this script is never used in production or network-accessible environments.
10-10: Shellcheck SC1007 is a false positive.The
CDPATH= cdpattern (line 10) is a well-known idiom to prevent CDPATH environment variable interference when resolving script directories. The space after=is intentional and correct.ops/mpt-switch-test/send-txs.sh (1)
18-22: LGTM: Standard test account key.Lines 19-22 correctly document that this is the well-known Hardhat/Foundry test account private key. The Gitleaks alert is expected and acceptable for test infrastructure. The key corresponds to the first account in the standard test mnemonic and should never hold real funds.
ops/mpt-switch-test/local-test/engine-make-block-loop.sh (2)
13-22: LGTM: Resilient loop design with proper error handling.The script correctly disables
set -e(line 15) to prevent loop termination on individual failures, captures the exit code (line 17), and provides observability via timestamped logging (line 20). This ensures continuous operation for the test harness.
8-8: Shellcheck SC1007 is a false positive.The
CDPATH= cdpattern is a standard idiom to neutralize the CDPATH environment variable when resolving script directories. The syntax is correct.node/core/config.go (2)
31-33: LGTM: Configuration fields properly initialized.The new
L2LegacyandMptTimefields are correctly declared and initialized. Line 47 ensuresL2Legacyis initialized inDefaultConfig(), addressing the previously reported nil pointer dereference issue. The initialization pattern is consistent with the existingL2field.Also applies to: 47-47
128-133: LGTM: Legacy L2 configuration wiring is correct.The legacy endpoint configuration properly:
- Retrieves values from the new CLI flags (lines 128-129)
- Assigns them to the initialized
L2Legacystruct (lines 130-131)- Reuses the same JWT secret as the primary L2 endpoint (line 132), which is appropriate for local testing
- Populates
MptTimefrom the corresponding flag (line 133)node/core/executor.go (2)
73-91: LGTM: Legacy L2 client initialization and updated RetryableClient construction.The legacy ZK endpoint clients (
laClient,leClient) are properly initialized with context and error handling. The updatedNewRetryableClientcall correctly passes all six parameters includingconfig.MptTimeandconfig.Logger, aligning with the constructor signature innode/types/retryable_client.go.
287-296: LGTM: EnsureSwitched call for P2P-synced blocks.Good addition. The
EnsureSwitchedcall ensures that sentry nodes (which receive blocks via P2P sync rather thanNewL2Block) properly trigger the MPT client switch when the upgrade timestamp is reached. The inline comment clearly documents this edge case.ops/mpt-switch-test/test-mpt-switch-local.sh (2)
1-26: Well-structured test harness for MPT switch testing.The script provides a comprehensive local test environment with clear architecture documentation, proper lifecycle management (start/stop/clean), and helpful monitoring utilities. The inline architecture diagram (lines 4-22) is particularly useful for understanding the test setup.
340-342: Sentry node configuration with--dev-sequencerflag is intentional for this test setup.The
--dev-sequencerflag, defined innode/flags/flags.go, is designed "explicitly specify that running as a sequencer. If it enables, the tendermint validator/batch params won't be changed no matter what happens to staking/gov contacts. Only use in dev/test mode." Both the sequencer and sentry nodes use this flag in this test to avoid relying on L1 staking/governance contract interactions, which is appropriate for an isolated dev/test environment (test-mpt-switch-local.sh). The flag disables contract-based parameter updates rather than forcing sequencer behavior. The sentry node still functions as a follower node that syncs blocks, as documented at line 317.Likely an incorrect or invalid review comment.
node/types/retryable_client.go (4)
31-55: LGTM: Updated struct and constructor with MPT switch support.The
RetryableClientstruct now properly includes legacy and MPT clients, withmptTimepassed as a constructor parameter (addressing the previous review concern). Theatomic.Boolformptstate is appropriate for concurrent access.
57-69: LGTM: Client accessor methods.The
aClient()andeClient()methods provide clean routing logic based on the atomicmptflag. The use ofatomic.Boolensures thread-safe reads.
136-154: Verify timestamp source forAssembleL2Block.
AssembleL2Blockusestime.Now().Unix()(line 137) for the timestamp passed toswitchClientand the RPC call. However, the actual block timestamp will be determined by the sequencer/geth. If there's clock drift or the assembled block gets a different timestamp, the switch decision may be made prematurely or delayed.Consider whether this timestamp should come from a more authoritative source, or if using current wall-clock time is acceptable for the switch check.
212-244:CommitBatchandAppendBlsSignaturedon't trigger MPT switch check.Unlike block-related methods,
CommitBatchandAppendBlsSignaturedon't callswitchClient. This may be intentional since batch operations might not have a timestamp context, but verify this is the expected behavior—especially if batches could span the MPT upgrade boundary.
| @@ -0,0 +1,69 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
Fix shebang or use POSIX-compatible arithmetic.
Line 1 declares #!/bin/sh but line 45 uses bash-specific arithmetic base conversion (16#$CUR_NOX), which is not POSIX-compliant. This will fail on systems where /bin/sh is dash, ash, or another POSIX shell.
🔎 Recommended fix (change shebang to bash)
-#!/bin/sh
+#!/bin/bash
# Assemble and insert a block via Engine API (authrpc :8551, JWT-based)Alternatively, use POSIX-compatible arithmetic:
CUR_NOX=${CUR_HEX#0x}
-NEXT_DEC=$((16#$CUR_NOX + 1))
+NEXT_DEC=$(printf '%d' "0x$CUR_NOX")
+NEXT_DEC=$((NEXT_DEC + 1))
NEXT_HEX=$(printf '0x%X' "$NEXT_DEC")Also applies to: 44-46
🤖 Prompt for AI Agents
In ops/mpt-switch-test/local-test/engine-make-block.sh around lines 1 and 44-46,
the script uses bash-specific arithmetic (16#$CUR_NOX) while declaring
#!/bin/sh; either update the shebang to a bash interpreter (e.g., #!/usr/bin/env
bash) so the existing base-16 arithmetic works, or rewrite the conversion to a
POSIX-compatible form (for example use printf/awk to convert hex to decimal or
use a portable printf lookup) and keep the POSIX #!/bin/sh; ensure the chosen
approach is used consistently and test on a system where /bin/sh is dash.
| ;; | ||
| stop) | ||
| if [ -f "$PID_FILE" ]; then | ||
| kill $(cat "$PID_FILE") 2>/dev/null && rm "$PID_FILE" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Quote command substitutions to prevent word splitting.
Lines 182 and 189 use unquoted $(cat "$PID_FILE") which can cause unexpected behavior if the PID file contains whitespace or special characters.
🔎 Proposed fix
stop)
if [ -f "$PID_FILE" ]; then
- kill $(cat "$PID_FILE") 2>/dev/null && rm "$PID_FILE"
+ kill "$(cat "$PID_FILE")" 2>/dev/null && rm "$PID_FILE"
log_success "Stopped"
else
log_info "Not running" status)
- if [ -f "$PID_FILE" ] && kill -0 $(cat "$PID_FILE") 2>/dev/null; then
- log_success "Running (PID: $(cat $PID_FILE))"
+ if [ -f "$PID_FILE" ] && kill -0 "$(cat "$PID_FILE")" 2>/dev/null; then
+ log_success "Running (PID: $(cat "$PID_FILE"))"
elseAlso applies to: 189-189
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 182-182: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In ops/mpt-switch-test/send-txs.sh around lines 182 and 189, the command
substitutions used to read the PID file are unquoted which can cause
word-splitting if the PID file contains whitespace or special characters; update
those lines to quote the command substitution (or better, read the PID into a
variable first and then quote the variable) so the PID is treated as a single
token when passed to kill and file operations.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ops/mpt-switch-test/send-txs.sh (1)
182-182: Quote command substitutions to prevent word splitting.This was previously flagged. The unquoted
$(cat "$PID_FILE")can cause unexpected behavior.Also applies to: 189-190
🧹 Nitpick comments (7)
ops/mpt-switch-test/README.md (1)
7-18: Consider adding language identifiers to fenced code blocks.Per markdownlint MD040, fenced code blocks should specify a language for proper syntax highlighting. Use
textorplaintextfor ASCII diagrams and log output.🔎 Proposed fix for line 7
-``` +```text Before upgrade: After upgrade (swap):🔎 Proposed fix for line 89
-``` +```text MPT switch time reached, MUST wait for MPT node to sync🔎 Proposed fix for line 102
-``` +```text mpt-switch-test/Also applies to: 89-98, 102-117
ops/mpt-switch-test/send-txs.sh (4)
7-8: Remove unusedGETH_BINvariable and verify port assignment.
GETH_BINis declared but never used. Additionally,ZK_GETH_HTTPpoints to port 9545, but according totest-mpt-switch-local.sh, port 9545 is the MPT Geth port (ZK Geth uses 8545). This naming inconsistency could cause confusion.🔎 Proposed fix
SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" -GETH_BIN="${SCRIPT_DIR}/bin/geth" -ZK_GETH_HTTP="http://127.0.0.1:9545" +# Default to MPT Geth port (9545) - can be overridden +GETH_HTTP="${GETH_HTTP:-http://127.0.0.1:9545}"Then update references from
$ZK_GETH_HTTPto$GETH_HTTPthroughout the script.
77-89: Remove unusedsend_tx_with_castfunction.This function is defined but never called. The
mainloop (lines 124-145) contains its own inlinecast sendimplementation. Either remove this dead code or refactor the main loop to use this function.🔎 Proposed fix - Option 1: Remove dead code
-# Send transaction using cast (if foundry is installed) -send_tx_with_cast() { - if command -v cast &> /dev/null; then - cast send --private-key "$PRIVATE_KEY" \ - --rpc-url "$ZK_GETH_HTTP" \ - "$TO_ADDRESS" \ - --value 1wei \ - --gas-price 0 \ - 2>/dev/null - return $? - fi - return 1 -} -
52-75: Remove unusedsend_txfunction.Similar to
send_tx_with_cast, this function usingeth_sendTransactionis defined but never called in the main loop. The main loop exclusively usescast send.
103-111: Balance check may incorrectly reject valid accounts.The condition
[ "$balance" == "0x0" ]only catches exact zero balance, butcheck_balancereturns the raw hex string which might be empty or malformed on RPC errors. The-z "$balance"check helps, but consider also validating thatbalancestarts with0xto distinguish between zero balance and RPC failure.🔎 Proposed improvement
if [ "$balance" == "0x0" ] || [ -z "$balance" ]; then + if [ -z "$balance" ]; then + echo "Warning: Could not retrieve account balance (RPC error?)" + else + echo "Warning: Account balance is 0!" + fi echo "" - echo "Warning: Account balance is 0 or cannot be retrieved!"ops/mpt-switch-test/test-mpt-switch-local.sh (2)
28-28: Remove unusedMORPH_ROOTvariable.This variable is declared but never referenced in the script.
🔎 Proposed fix
# Path configuration -MORPH_ROOT="${SCRIPT_DIR}/../.." BIN_DIR="${SCRIPT_DIR}/bin"
376-377: Consider separating variable declaration and assignment.Multiple lines combine
localdeclaration with command substitution, which can mask return values. While this is low-risk for a test script, separating them improves debuggability.🔎 Example fix for lines 376-377
for pid_file in "$SENTRY_NODE_PID" "$SEQUENCER_NODE_PID" "$MPT_GETH_PID" "$ZK_GETH_PID"; do if [ -f "$pid_file" ]; then - local pid=$(cat "$pid_file") - local name=$(basename "$pid_file" .pid) + local pid + local name + pid=$(cat "$pid_file") + name=$(basename "$pid_file" .pid)Also applies to: 457-457, 469-469, 475-475, 533-533
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
node/core/sequencers.goops/mpt-switch-test/README.mdops/mpt-switch-test/send-txs.shops/mpt-switch-test/test-mpt-switch-local.sh
✅ Files skipped from review due to trivial changes (1)
- node/core/sequencers.go
🧰 Additional context used
🪛 Gitleaks (8.30.0)
ops/mpt-switch-test/send-txs.sh
[high] 20-20: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
ops/mpt-switch-test/README.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
102-102: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
ops/mpt-switch-test/send-txs.sh
[warning] 7-7: GETH_BIN appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 38-38: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 46-46: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 59-59: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 100-100: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 118-118: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 119-119: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 127-127: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 182-182: Quote this to prevent word splitting.
(SC2046)
[warning] 189-189: Quote this to prevent word splitting.
(SC2046)
ops/mpt-switch-test/test-mpt-switch-local.sh
[warning] 28-28: MORPH_ROOT appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 196-196: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 376-376: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 377-377: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 457-457: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 469-469: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 475-475: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 533-533: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
ops/mpt-switch-test/send-txs.sh (1)
18-22: Test private key is acceptable, but clarify the comment.This is Foundry's default test account #0 private key, which is fine for local testing. The comment already notes this, which is good practice.
ops/mpt-switch-test/test-mpt-switch-local.sh (4)
253-267: Permissive CORS and vhosts settings are acceptable for local testing.The
--http.corsdomain="*",--http.vhosts="*",--ws.origins="*", and--authrpc.vhosts="*"settings are intentionally permissive for a local test environment. This is appropriate given the test-only context.
93-121: Well-structured dependency checking.The
check_dependenciesfunction clearly validates required binaries and provides actionable error messages. Good implementation.
525-606: Main function and command dispatch are well-organized.The command-line interface with start/stop/clean/status/monitor/logs/help commands is cleanly implemented with good user feedback.
340-342: Clarify the node mode flag for the sentry node.The sentry node is explicitly described as a "non-validator (follower node)" but is started with
--dev-sequencer. The flag's documentation states it "explicitly specify[ies] that running as a sequencer," which semantically contradicts the sentry node's role. Either add a comment explaining why sentry nodes use this flag in dev/test mode, or consider if a different flag or approach is needed.
| ```bash | ||
| cd /Users/corey.zhang/workspace/morph/ops/mpt-switch-test | ||
|
|
There was a problem hiding this comment.
Replace hardcoded absolute path with a relative path or placeholder.
The path /Users/corey.zhang/workspace/morph/ops/mpt-switch-test is developer-specific and won't work for other contributors. Use a relative path or environment variable placeholder.
🔎 Proposed fix
-cd /Users/corey.zhang/workspace/morph/ops/mpt-switch-test
+cd ops/mpt-switch-test📝 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.
| ```bash | |
| cd /Users/corey.zhang/workspace/morph/ops/mpt-switch-test |
🤖 Prompt for AI Agents
In ops/mpt-switch-test/README.md around lines 43-45, replace the
developer-specific absolute path
`/Users/corey.zhang/workspace/morph/ops/mpt-switch-test` with a portable
alternative: either a relative path (e.g., ./ops/mpt-switch-test or
../ops/mpt-switch-test depending on README location) or an environment-variable
placeholder (e.g., $PROJECT_ROOT/ops/mpt-switch-test or
${WORKSPACE}/ops/mpt-switch-test) and update the example command to use that
placeholder so other contributors can run it without changing the file.
| export MORPH_NODE_L2_ENGINE_RPC="http://127.0.0.1:${MPT_GETH_AUTH_PORT}" | ||
| export MORPH_NODE_L2_ENGINE_AUTH="$jwt_file" | ||
| export MORPH_NODE_MPT_TIME="$mpt_time" | ||
| export MORPH_NODE_L1_ETH_RPC="${L1_ETH_RPC:-http://127.0.0.1:9545}" |
There was a problem hiding this comment.
L1_ETH_RPC defaults to port 9545, which conflicts with MPT Geth.
Both sequencer and sentry nodes default L1_ETH_RPC to http://127.0.0.1:9545, but port 9545 is already assigned to MPT Geth's HTTP endpoint. This will likely cause issues unless L1_ETH_RPC is explicitly set by the caller.
🔎 Proposed fix
Consider using a different default port or requiring explicit configuration:
- export MORPH_NODE_L1_ETH_RPC="${L1_ETH_RPC:-http://127.0.0.1:9545}"
+ export MORPH_NODE_L1_ETH_RPC="${L1_ETH_RPC:-http://127.0.0.1:8545}"Or document that L1_ETH_RPC must be set before running the test.
Also applies to: 335-335
🤖 Prompt for AI Agents
In ops/mpt-switch-test/test-mpt-switch-local.sh around lines 300 and 335, the
script defaults L1_ETH_RPC to http://127.0.0.1:9545 which conflicts with MPT
Geth’s HTTP endpoint; update both occurrences to avoid the port collision by
either choosing a non-conflicting default port (e.g., 8545 or another free port)
or require the caller to set L1_ETH_RPC and fail fast with a clear error if it
is unset, and add a short comment or doc note in the script explaining that
L1_ETH_RPC must be explicitly configured to avoid conflicts with MPT Geth.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.