Feat: pbft to rollup sequencer, include P2P & block produce #847
Conversation
📝 WalkthroughWalkthroughThis PR introduces L2 block V2 format support in the executor with new methods to request, apply, and retrieve blocks, along with type conversion utilities. It adds retry-aware client methods for block operations and provides comprehensive Docker infrastructure and test orchestration for sequencer testing, including a test runner script and transaction generator. Changes
Sequence Diagram(s)sequenceDiagram
actor Sequencer
participant Executor
participant RetryableClient
participant L1
participant Blockchain
participant Conversion as Type Converters
Sequencer->>Executor: RequestBlockDataV2(parentHash)
Executor->>RetryableClient: BlockByNumber()<br/>(with retry)
RetryableClient->>L1: Fetch L1 messages
L1-->>RetryableClient: L1 messages
Executor->>RetryableClient: AssembleL2BlockV2()<br/>(with retry)
RetryableClient-->>Executor: ExecutableL2Data
Executor->>Conversion: executableL2DataToBlockV2()
Conversion-->>Executor: BlockV2
Executor-->>Sequencer: (BlockV2, collected flag)
rect rgb(200, 230, 255)
Note over Executor,Blockchain: Apply Phase
Sequencer->>Executor: ApplyBlockV2(block)
Executor->>Conversion: blockV2ToExecutableL2Data()
Conversion-->>Executor: ExecutableL2Data
Executor->>Blockchain: NewL2Block()<br/>(apply block)
Blockchain-->>Executor: Success
Executor->>Executor: Update metrics
end
Executor-->>Sequencer: Block applied
Sequencer->>Executor: GetBlockByNumber(height)
Executor->>Blockchain: BlockByNumber()
Blockchain-->>Executor: eth.Block
Executor->>Conversion: ethBlockToBlockV2()
Conversion-->>Executor: BlockV2
Executor-->>Sequencer: BlockV2
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![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
🧹 Nitpick comments (10)
ops/docker-sequencer-test/Dockerfile.l2-node-test (2)
24-26: Consider removing the-xflag fromgo mod download.The
-xflag enables verbose output during dependency downloads, which can clutter build logs. Unless you specifically need this for debugging, consider removing it for cleaner output.🔎 Proposed change
-RUN go mod download -x +RUN go mod download
38-38: Consider using a smaller base image for the runtime stage.The runtime stage uses the same
go-ubuntu-builderimage as the builder, which includes the full Go toolchain (~500MB+). Since the runtime only needs the compiled binaries, consider using a smaller base image likeubuntu:22.04ordebian:stable-slimto reduce the final image size.🔎 Proposed change
-FROM ghcr.io/morph-l2/go-ubuntu-builder:go-1.24-ubuntu +FROM ubuntu:22.04ops/docker-sequencer-test/Dockerfile.l2-geth-test (1)
13-13: Consider using a smaller base image for the runtime stage.Similar to the node Dockerfile, the runtime stage uses the full
go-ubuntu-builderimage, which includes the Go toolchain. Consider using a smaller base image likeubuntu:22.04since only the compiledgethbinary is needed at runtime.🔎 Proposed change
-FROM ghcr.io/morph-l2/go-ubuntu-builder:go-1.24-ubuntu +FROM ubuntu:22.04ops/docker-sequencer-test/scripts/tx-generator.sh (3)
9-9: Unused variable:PRIVATE_KEYis defined but never used.The
PRIVATE_KEYvariable is set but the script useseth_sendTransactionwhich relies on the node to sign transactions from an unlocked account. Consider removing this unused variable or documenting why it's present.🔎 Proposed change
-PRIVATE_KEY="${PRIVATE_KEY:-0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80}"
25-36: Consider usingjqfor more robust JSON parsing.The functions use
grepandcutto parse JSON responses, which is fragile and may break if the response format changes. Consider usingjqfor more reliable JSON parsing.🔎 Proposed improvement
get_nonce() { curl -s -X POST -H "Content-Type: application/json" \ --data "{\"jsonrpc\":\"2.0\",\"method\":\"eth_getTransactionCount\",\"params\":[\"0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266\",\"latest\"],\"id\":1}" \ - "$L2_RPC" | grep -o '"result":"[^"]*"' | cut -d'"' -f4 + "$L2_RPC" | jq -r '.result' } get_block_number() { curl -s -X POST -H "Content-Type: application/json" \ --data '{"jsonrpc":"2.0","method":"eth_blockNumber","params":[],"id":1}' \ - "$L2_RPC" | grep -o '"result":"[^"]*"' | cut -d'"' -f4 + "$L2_RPC" | jq -r '.result' }
42-44: Add error handling for initial nonce retrieval.If
get_noncefails or returns an empty/invalid value, the script will continue with an incorrect nonce. Consider adding a check to ensure the nonce is valid before entering the main loop.🔎 Proposed improvement
NONCE_HEX=$(get_nonce) +if [ -z "$NONCE_HEX" ] || [ "$NONCE_HEX" = "0x" ]; then + echo "Failed to get initial nonce" + exit 1 +fi NONCE=$((NONCE_HEX))ops/docker-sequencer-test/run-test.sh (4)
277-278: Hardcoded private key should be configured via environment variable.While this is the well-known Anvil/Hardhat default test account private key, hardcoding it triggers security scanners and makes it harder to use different keys for different test scenarios.
🔎 Proposed fix
# Set sequencer private key - export SEQUENCER_PRIVATE_KEY="0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80" + # Default to Anvil/Hardhat test account #0 if not specified + export SEQUENCER_PRIVATE_KEY="${SEQUENCER_PRIVATE_KEY:-0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80}"
320-321: Add a comment explaining the absolute value trick.The
${diff#-}parameter expansion removes a leading minus sign to compute absolute value. This is non-obvious and would benefit from a brief comment for maintainability.+ # Compute absolute difference (${diff#-} removes leading minus) local diff=$((block0 - block1)) if [ ${diff#-} -le 2 ]; then
402-411: TX generator uses duplicate hardcoded private key.This is the same key hardcoded on line 278. If the earlier suggestion to use
$SEQUENCER_PRIVATE_KEYis adopted, this should also reference the environment variable for consistency.🔎 Proposed fix
( while true; do RANDOM_ADDR="0x$(openssl rand -hex 20)" - cast send --private-key 0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80 \ + cast send --private-key "$SEQUENCER_PRIVATE_KEY" \ --rpc-url "$L2_RPC" \ --value 1wei \ "$RANDOM_ADDR" 2>/dev/null || true sleep ${TX_INTERVAL:-5} done ) &
472-476: Inconsistent indentation on line 475.Line 475 has an extra leading space compared to adjacent lines, which affects readability.
echo "Node 1: Block $(get_block_number http://127.0.0.1:8645)" echo "Node 2: Block $(get_block_number http://127.0.0.1:8745)" echo "Node 3: Block $(get_block_number http://127.0.0.1:8845)" - echo "Node 0 (seq-0): Block $(get_block_number http://127.0.0.1:8545)" + echo "Node 0 (seq-0): Block $(get_block_number http://127.0.0.1:8545)" echo "Sentry: Block $(get_block_number http://127.0.0.1:8945 2>/dev/null || echo 'N/A')"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
node/core/executor.gonode/types/retryable_client.goops/docker-sequencer-test/Dockerfile.l2-geth-testops/docker-sequencer-test/Dockerfile.l2-node-testops/docker-sequencer-test/README.mdops/docker-sequencer-test/docker-compose.override.ymlops/docker-sequencer-test/entrypoint-l2.shops/docker-sequencer-test/run-test.shops/docker-sequencer-test/scripts/tx-generator.sh
🧰 Additional context used
🧬 Code graph analysis (4)
ops/docker-sequencer-test/scripts/tx-generator.sh (1)
ops/docker-sequencer-test/run-test.sh (1)
get_block_number(53-60)
ops/docker-sequencer-test/entrypoint-l2.sh (1)
prover/crates/utils/src/metrics/registry.rs (1)
init(27-105)
node/core/executor.go (1)
node/types/errors.go (2)
ErrInvalidL1MessageOrder(10-10)ErrWrongBlockNumber(30-30)
ops/docker-sequencer-test/run-test.sh (1)
ops/docker-sequencer-test/scripts/tx-generator.sh (1)
get_block_number(32-36)
🪛 GitHub Actions: Tx-submitter
node/types/retryable_client.go
[error] 262-262: rc.authClient.AssembleL2BlockV2 undefined (type *authclient.Client has no field or method AssembleL2BlockV2)
🪛 Gitleaks (8.30.0)
ops/docker-sequencer-test/run-test.sh
[high] 278-278: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Shellcheck (0.11.0)
ops/docker-sequencer-test/run-test.sh
[warning] 68-68: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 309-309: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 317-317: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 318-318: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 337-337: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 346-346: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 349-349: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 350-350: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 364-364: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 382-382: 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). (5)
- GitHub Check: check
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
node/types/retryable_client.go (3)
175-187: Good fix: Adds missing retry logic to HeaderByNumber.This change brings
HeaderByNumberin line with the other retryable methods by adding proper logging and retry-on-error handling that was previously missing.
189-205: BlockByNumber method exists and is correctly implemented.The
ethClient.BlockByNumbermethod fromgithub.com/morph-l2/go-ethereum/ethclientis available with the signature(ctx context.Context, number *big.Int) (*types.Block, error). The implementation correctly calls this method with proper retry logic and error handling, consistent with other methods in theRetryableClient.
260-276: TheAssembleL2BlockV2method cannot be verified from this repository alone as it belongs to an external dependency.The method is called on
authClientwhich is of type*authclient.Clientfrom the external packagegithub.com/morph-l2/go-ethereum/ethclient/authclient(versionv1.10.14-0.20251219060125-03910bc750a2). The code in this repository is well-integrated with proper error handling, but verification of the method's existence in the external go-ethereum fork requires inspecting that repository directly. The usage pattern is consistent with the existingAssembleL2BlockV1 method, which functions successfully.node/core/executor.go (6)
429-471: Implementation depends on fixingAssembleL2BlockV2.This method is well-structured and follows the same patterns as
RequestBlockData. However, it depends onAssembleL2BlockV2(line 456), which currently fails to compile due to the missing method in the auth client (see issue innode/types/retryable_client.go).
495-497: Verify thatnilbatch hash is correct for sequencer mode.The comment on line 495 states "no batch hash in sequencer mode for now", and
nilis passed as the batch hash parameter. Please confirm this is the intended behavior and whether batch hashing will be added in the future.
513-520: LGTM!Clean implementation that fetches a block by number and converts it to the V2 format. Error handling is appropriate.
524-531: LGTM!Correctly uses
nilto fetch the latest block, which is the standard Ethereum JSON-RPC convention.
571-619: LGTM!The bidirectional conversion functions are well-implemented with proper nil checks and ensure that the
Transactionsfield is nevernil(important for JSON serialization). The field mappings appear complete and symmetric.
536-568: No action needed. TheethBlockToBlockV2function correctly excludesWithdrawTrieRootbecause this field is not available in theeth.Blockoreth.Headertypes. The reference to line 592 (blockV2ToExecutableL2Data) is a different conversion that operates onBlockV2(which already hasWithdrawTrieRoot), not on raweth.Blockdata. The function properly converts only the fields available from the Ethereum block structure.Likely an incorrect or invalid review comment.
ops/docker-sequencer-test/entrypoint-l2.sh (1)
18-31: Security note: Wildcard CORS and vhosts are acceptable for testing only.The configuration uses wildcard values for CORS origins (lines 21, 27) and vhosts (lines 22, 30). While this is acceptable for a test environment, ensure these settings are never used in production as they allow unrestricted access from any origin or host.
ops/docker-sequencer-test/docker-compose.override.yml (1)
26-27: Hardcoded private key is acceptable for testing only.The default
SEQUENCER_PRIVATE_KEYis the first Hardhat test account, which is publicly known. This is acceptable for a test environment but ensure this configuration is never used in any environment with real value.ops/docker-sequencer-test/run-test.sh (1)
53-60: Robustget_block_numberimplementation - consider reusing intx-generator.sh.This implementation handles the RPC URL parameter and hex-to-decimal conversion gracefully. The version in
ops/docker-sequencer-test/scripts/tx-generator.sh(lines 31-35) lacks the hex conversion and RPC URL parameter. Consider extracting this as a shared utility or updating the other script for consistency.
| wait_for_block() { | ||
| local target_height=$1 | ||
| local rpc_url="${2:-$L2_RPC}" | ||
|
|
||
| log_info "Waiting for block $target_height..." | ||
| while true; do | ||
| local current=$(get_block_number "$rpc_url") | ||
| if [ "$current" -ge "$target_height" ]; then | ||
| log_success "Reached block $current" | ||
| return 0 | ||
| fi | ||
| echo -ne "\r Current block: $current / $target_height" | ||
| sleep 2 | ||
| done | ||
| } |
There was a problem hiding this comment.
wait_for_block can hang indefinitely without a timeout.
Unlike wait_for_rpc which has a max_retries parameter, this function has an unbounded while true loop. If the target block is never reached (e.g., due to network issues or a stalled chain), the script will hang forever.
🔎 Proposed fix to add a timeout
wait_for_block() {
local target_height=$1
local rpc_url="${2:-$L2_RPC}"
+ local max_wait=${3:-600} # Default 10 minute timeout
+ local waited=0
log_info "Waiting for block $target_height..."
- while true; do
+ while [ $waited -lt $max_wait ]; do
local current=$(get_block_number "$rpc_url")
if [ "$current" -ge "$target_height" ]; then
log_success "Reached block $current"
return 0
fi
echo -ne "\r Current block: $current / $target_height"
sleep 2
+ waited=$((waited + 2))
done
+ log_error "Timeout waiting for block $target_height (current: $current)"
+ return 1
}📝 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.
| wait_for_block() { | |
| local target_height=$1 | |
| local rpc_url="${2:-$L2_RPC}" | |
| log_info "Waiting for block $target_height..." | |
| while true; do | |
| local current=$(get_block_number "$rpc_url") | |
| if [ "$current" -ge "$target_height" ]; then | |
| log_success "Reached block $current" | |
| return 0 | |
| fi | |
| echo -ne "\r Current block: $current / $target_height" | |
| sleep 2 | |
| done | |
| } | |
| wait_for_block() { | |
| local target_height=$1 | |
| local rpc_url="${2:-$L2_RPC}" | |
| local max_wait=${3:-600} # Default 10 minute timeout | |
| local waited=0 | |
| log_info "Waiting for block $target_height..." | |
| while [ $waited -lt $max_wait ]; do | |
| local current=$(get_block_number "$rpc_url") | |
| if [ "$current" -ge "$target_height" ]; then | |
| log_success "Reached block $current" | |
| return 0 | |
| fi | |
| echo -ne "\r Current block: $current / $target_height" | |
| sleep 2 | |
| waited=$((waited + 2)) | |
| done | |
| log_error "Timeout waiting for block $target_height (current: $current)" | |
| return 1 | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 68-68: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In ops/docker-sequencer-test/run-test.sh around lines 62 to 76, wait_for_block
uses an unbounded while true loop and can hang forever; modify it to accept an
optional max_retries or timeout parameter (e.g., third arg or default value),
increment a retry counter inside the loop and break when retries exceed the
limit, log an error/timeout message and return non-zero (or exit) on timeout,
and keep the existing success path unchanged so the function returns 0 when the
target block is reached.
| local upgrade_file="$MORPH_ROOT/../tendermint/upgrade/upgrade.go" | ||
| if [ -f "$upgrade_file" ]; then | ||
| sed -i '' "s/UpgradeBlockHeight int64 = [0-9]*/UpgradeBlockHeight int64 = $height/" "$upgrade_file" | ||
| log_success "Updated upgrade height in $upgrade_file" | ||
| else | ||
| log_warn "upgrade.go not found at $upgrade_file" | ||
| fi |
There was a problem hiding this comment.
sed -i '' is macOS-specific and will fail on Linux.
The empty string argument after -i is required on macOS but causes errors on GNU sed (Linux). This will break in Linux-based CI environments.
🔎 Proposed fix for cross-platform compatibility
local upgrade_file="$MORPH_ROOT/../tendermint/upgrade/upgrade.go"
if [ -f "$upgrade_file" ]; then
- sed -i '' "s/UpgradeBlockHeight int64 = [0-9]*/UpgradeBlockHeight int64 = $height/" "$upgrade_file"
+ # Cross-platform sed in-place edit
+ if [[ "$OSTYPE" == "darwin"* ]]; then
+ sed -i '' "s/UpgradeBlockHeight int64 = [0-9]*/UpgradeBlockHeight int64 = $height/" "$upgrade_file"
+ else
+ sed -i "s/UpgradeBlockHeight int64 = [0-9]*/UpgradeBlockHeight int64 = $height/" "$upgrade_file"
+ fi
log_success "Updated upgrade height in $upgrade_file"
else
log_warn "upgrade.go not found at $upgrade_file"
fi📝 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.
| local upgrade_file="$MORPH_ROOT/../tendermint/upgrade/upgrade.go" | |
| if [ -f "$upgrade_file" ]; then | |
| sed -i '' "s/UpgradeBlockHeight int64 = [0-9]*/UpgradeBlockHeight int64 = $height/" "$upgrade_file" | |
| log_success "Updated upgrade height in $upgrade_file" | |
| else | |
| log_warn "upgrade.go not found at $upgrade_file" | |
| fi | |
| local upgrade_file="$MORPH_ROOT/../tendermint/upgrade/upgrade.go" | |
| if [ -f "$upgrade_file" ]; then | |
| # Cross-platform sed in-place edit | |
| if [[ "$OSTYPE" == "darwin"* ]]; then | |
| sed -i '' "s/UpgradeBlockHeight int64 = [0-9]*/UpgradeBlockHeight int64 = $height/" "$upgrade_file" | |
| else | |
| sed -i "s/UpgradeBlockHeight int64 = [0-9]*/UpgradeBlockHeight int64 = $height/" "$upgrade_file" | |
| fi | |
| log_success "Updated upgrade height in $upgrade_file" | |
| else | |
| log_warn "upgrade.go not found at $upgrade_file" | |
| fi |
🤖 Prompt for AI Agents
In ops/docker-sequencer-test/run-test.sh around lines 85–91, the sed call uses
the macOS-only form `sed -i ''` which fails on Linux; change it to a
cross-platform in-place edit approach: detect the platform (e.g., via uname -s)
and set a variable for the sed inplace flag (use `-i ''` on Darwin/macOS and
`-i` or `-i.bak` on Linux), then run sed with that variable and remove any
created backup file if used (or use a portable fallback like perl -pi -e).
Update the script to use that variable in the sed invocation so the upgrade.go
replacement works on both macOS and Linux.
| # Generate random recipient address | ||
| RANDOM_SUFFIX=$(od -An -N4 -tx1 /dev/urandom | tr -d ' ') | ||
| TO_ADDR="0x000000000000000000000000000000${RANDOM_SUFFIX}" |
There was a problem hiding this comment.
Fix: Random address generation produces invalid length.
The generated address is 40 characters total (38 hex digits after "0x") instead of the required 42 characters (40 hex digits). This will cause transaction failures.
- Line 51 generates 4 bytes = 8 hex characters
- Line 52 adds a 30-character prefix
- Total: "0x" + 30 + 8 = 40 characters (need 42)
🔎 Proposed fix
- RANDOM_SUFFIX=$(od -An -N4 -tx1 /dev/urandom | tr -d ' ')
- TO_ADDR="0x000000000000000000000000000000${RANDOM_SUFFIX}"
+ RANDOM_SUFFIX=$(od -An -N8 -tx1 /dev/urandom | tr -d ' ')
+ TO_ADDR="0x00000000000000000000000000${RANDOM_SUFFIX}"This generates 8 bytes (16 hex chars) and uses a 24-char prefix for a total of 42 chars.
📝 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.
| # Generate random recipient address | |
| RANDOM_SUFFIX=$(od -An -N4 -tx1 /dev/urandom | tr -d ' ') | |
| TO_ADDR="0x000000000000000000000000000000${RANDOM_SUFFIX}" | |
| # Generate random recipient address | |
| RANDOM_SUFFIX=$(od -An -N8 -tx1 /dev/urandom | tr -d ' ') | |
| TO_ADDR="0x00000000000000000000000000${RANDOM_SUFFIX}" |
🤖 Prompt for AI Agents
In ops/docker-sequencer-test/scripts/tx-generator.sh around lines 50 to 52, the
random recipient address is built with a 30-hex-digit prefix plus 4 bytes (8 hex
chars), producing a 40-char address instead of the required 42-char (40 hex
digits). Change the generation to use 8 random bytes (od -An -N8 -tx1 ...) so
you get 16 hex chars and reduce the hardcoded zero prefix to 24 hex digits,
resulting in "0x" + 24 zeros + 16 random hex chars to produce a 42-character
address.
tomatoishealthy
changed the title
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.