Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: initial EVM Execution Client Implementation #10

Merged
merged 44 commits into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
a28c3ca
feat: evm client implementation
jim380 Oct 29, 2024
d192f94
chore: add docker setup for reth
jim380 Oct 30, 2024
adc0ad0
chore: gitignore jwt token
jim380 Oct 30, 2024
8d69c2e
feat: support json-rpc proxy client
jim380 Oct 30, 2024
594f3f9
refactor: separate proxy client from engine api client
jim380 Oct 30, 2024
e845c33
chore: naming changes
jim380 Oct 30, 2024
c7cfbef
chore: address initial PR review comments
jim380 Nov 5, 2024
d74ca6e
chore: address initial PR review comments batch II
jim380 Nov 5, 2024
207cbc3
refactor: remove redundant abstraction in the proxy layer
jim380 Nov 6, 2024
2c06ad7
chore: rm rollkit as a dependency
jim380 Nov 6, 2024
50b32f0
chore: refine client implementation and tests
jim380 Nov 7, 2024
8020a03
chore: update execution api interface name
jim380 Nov 8, 2024
037d5ae
chore: renaming go-execution types import path
jim380 Nov 8, 2024
e739975
refactor: move mocks to its own pkg
jim380 Nov 8, 2024
d222190
test: SetFinal unit test passing
jim380 Nov 8, 2024
ee613fc
chore: add ctx in executor methods
jim380 Nov 12, 2024
40257d9
feat: upgrade to engine api cancun
jim380 Nov 12, 2024
e0957ca
spin up local network for tests
ProgramCpp Nov 7, 2024
764240e
init reth db for the local test network
ProgramCpp Nov 7, 2024
6fc3ea5
add integration tests
ProgramCpp Nov 7, 2024
60623e8
merge changes - upgrade to cancun api
ProgramCpp Nov 8, 2024
463b9c5
merge base: upgrade to cancun api
ProgramCpp Nov 8, 2024
cab390b
programatically setup docker dependencies for integration tests
ProgramCpp Nov 10, 2024
aae31e7
fix integration test cleanup
ProgramCpp Nov 11, 2024
3f848ec
add integration test for GetTxs
ProgramCpp Nov 12, 2024
d9ca6fd
fix getTxs integration test to assert the tx in mempool
ProgramCpp Nov 13, 2024
9d6a016
Add integration tests for evm api's
ProgramCpp Nov 13, 2024
4577d1f
fix mandatory field validation for payload creation
ProgramCpp Nov 14, 2024
a456a09
send signed transaction to execution layer client
ProgramCpp Nov 14, 2024
a64658d
feat: add proper jwt auth in engine api calls
jim380 Nov 14, 2024
fff67cf
refactor: use more concrete types for building engine api payloads
jim380 Nov 15, 2024
00a5fa7
fix blockhash for block proposal
ProgramCpp Nov 16, 2024
fe309e1
upgrade reth version for integration tests
ProgramCpp Nov 18, 2024
ca25040
Merge branch 'jay/execution-api' into jni/1802
ProgramCpp Nov 18, 2024
c2ab7be
merge jay/execution-api
ProgramCpp Nov 19, 2024
9dbd197
fix initChain unit tests
ProgramCpp Nov 19, 2024
2aef2ca
fix executeTxs api unit tests
ProgramCpp Nov 19, 2024
2f5442e
fix initChain api integration tests
ProgramCpp Nov 19, 2024
f0431cc
fix reth setup for integration tests
ProgramCpp Nov 19, 2024
a66d9c7
fix genproto module dependency
ProgramCpp Nov 19, 2024
73cc0eb
downgrade go-ethereum
ProgramCpp Nov 19, 2024
4f6afe6
fix: block hash mismatch when executing txs
jim380 Nov 20, 2024
ef272c4
test: fix ExecuteTxs
jim380 Nov 20, 2024
bd037f4
chore: remove redundant debug prints
jim380 Nov 20, 2024
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
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
docker/jwttoken/*
!docker/jwttoken/.gitkeep
70 changes: 70 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
## Architecture

```mermaid
jim380 marked this conversation as resolved.
Show resolved Hide resolved
graph LR
subgraph Test Environment
TestClient[Test Client]
MockExecutor[Mock Executor]
end

subgraph Execution Client
EngineAPIExecutionClient
subgraph Client Components
EthClient[Eth Client]
JsonRpcClient[JSON-RPC Client]
end
end

subgraph Execution Layer
Reth[Reth Node]
subgraph Reth APIs
EngineAPI[Engine API]
JsonRPC[JSON-RPC API]
end
end

%% Test Environment Connections
TestClient -->|uses| EngineAPIExecutionClient
JsonRpcClient -->|test mode| MockExecutor

%% Execution Client Connections
EngineAPIExecutionClient -->|eth calls| EthClient
EngineAPIExecutionClient -->|engine calls| JsonRpcClient
EthClient -->|eth/net/web3| JsonRPC
JsonRpcClient -->|engine api| EngineAPI

%% Reth Internal Connections
JsonRPC -->|internal| Reth
EngineAPI -->|internal| Reth

%% Styling
classDef primary fill:#f9f,stroke:#333,stroke-width:2px
classDef secondary fill:#bbf,stroke:#333,stroke-width:1px
class EngineAPIExecutionClient primary
class EthClient,JsonRpcClient,MockExecutor,EngineAPI,JsonRPC secondary
```

The architecture consists of several key components:

1. **Execution Client**

- `EngineAPIExecutionClient`: Main client interface that implements the Execute interface
- `EthClient`: Handles standard Ethereum JSON-RPC calls
- `JsonRpcClient`: Handles Engine API calls

2. **Execution Layer**

- `Reth Node`: Ethereum execution client
- Exposes Engine API and standard JSON-RPC endpoints

3. **Test Environment**
- `Test Client`: Integration tests
- `Mock Executor`: Simulates execution behavior for unit tests

## Development

```bash
$ cd docker
$ docker compose up -d
$ docker compose down
```
Comment on lines +64 to +70
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance development setup documentation.

The development instructions need more context and details:

  1. Add prerequisites (Docker, Docker Compose)
  2. Explain what each command does
  3. Include information about JWT token setup
  4. Remove $ prefix from commands as per markdown best practices
 ## Development

+### Prerequisites
+
+- Docker
+- Docker Compose
+
+### Setup
+
+The following commands will set up the development environment:
+
 ```bash
-$ cd docker
-$ docker compose up -d
-$ docker compose down
+# Navigate to the Docker configuration directory
+cd docker
+
+# Start the services (Reth node and JWT initialization)
+docker compose up -d
+
+# Stop the services when done
+docker compose down

+The setup process includes:
+- Initializing a JWT token for secure communication
+- Starting a Reth node as the execution client


<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 Markdownlint</summary>

67-67: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

---

68-68: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

---

69-69: null
Dollar signs used before commands without showing output

(MD014, commands-show-output)

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit -->

59 changes: 59 additions & 0 deletions docker/docker-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
name: "reth"

services:
jwt-init:
container_name: jwt-init
image: alpine:3.19
volumes:
- ./jwttoken:/jwt
healthcheck:
test: ["CMD", "test", "-f", "/jwt/jwt.hex"]
interval: 5s
timeout: 5s
retries: 3
command: >
/bin/sh -c "mkdir -p /jwt &&
if [ ! -f /jwt/jwt.hex ]; then
apk add --no-cache openssl &&
openssl rand -hex 32 | tr -d '\n' > /jwt/jwt.hex;
fi"
Comment on lines +14 to +19
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Set secure file permissions for JWT token.

The JWT token file needs restricted permissions since it's used for authentication. Add chmod 600 to ensure only the owner can read/write the file.

Apply this fix:

    command: >
      /bin/sh -c "mkdir -p /jwt && 
      if [ ! -f /jwt/jwt.hex ]; then 
        apk add --no-cache openssl &&
-        openssl rand -hex 32 | tr -d '\n' > /jwt/jwt.hex; 
+        openssl rand -hex 32 | tr -d '\n' > /jwt/jwt.hex &&
+        chmod 600 /jwt/jwt.hex;
      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.

Suggested change
command: >
/bin/sh -c "mkdir -p /jwt &&
if [ ! -f /jwt/jwt.hex ]; then
apk add --no-cache openssl &&
openssl rand -hex 32 | tr -d '\n' > /jwt/jwt.hex;
fi"
command: >
/bin/sh -c "mkdir -p /jwt &&
if [ ! -f /jwt/jwt.hex ]; then
apk add --no-cache openssl &&
openssl rand -hex 32 | tr -d '\n' > /jwt/jwt.hex &&
chmod 600 /jwt/jwt.hex;
fi"
🧰 Tools
🪛 yamllint

[error] 15-15: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 18-18: trailing spaces

(trailing-spaces)

Comment on lines +4 to +19
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance JWT token security and maintenance

While the basic JWT setup is good, there are additional security considerations:

  1. Set directory permissions for /jwt
  2. Add cleanup for old tokens
  3. Fix trailing spaces in the command

Apply these improvements:

    command: >
-      /bin/sh -c "mkdir -p /jwt && 
-      if [ ! -f /jwt/jwt.hex ]; then 
+      /bin/sh -c "mkdir -p /jwt &&
+      chmod 700 /jwt &&
+      if [ ! -f /jwt/jwt.hex ]; then
+        rm -f /jwt/*.hex &&
         apk add --no-cache openssl &&
-        openssl rand -hex 32 | tr -d '\n' > /jwt/jwt.hex; 
+        openssl rand -hex 32 | tr -d '\n' > /jwt/jwt.hex &&
+        chmod 600 /jwt/jwt.hex;
       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.

Suggested change
jwt-init:
container_name: jwt-init
image: alpine:3.19
volumes:
- ./jwttoken:/jwt
healthcheck:
test: ["CMD", "test", "-f", "/jwt/jwt.hex"]
interval: 5s
timeout: 5s
retries: 3
command: >
/bin/sh -c "mkdir -p /jwt &&
if [ ! -f /jwt/jwt.hex ]; then
apk add --no-cache openssl &&
openssl rand -hex 32 | tr -d '\n' > /jwt/jwt.hex;
fi"
jwt-init:
container_name: jwt-init
image: alpine:3.19
volumes:
- ./jwttoken:/jwt
healthcheck:
test: ["CMD", "test", "-f", "/jwt/jwt.hex"]
interval: 5s
timeout: 5s
retries: 3
command: >
/bin/sh -c "mkdir -p /jwt &&
chmod 700 /jwt &&
if [ ! -f /jwt/jwt.hex ]; then
rm -f /jwt/*.hex &&
apk add --no-cache openssl &&
openssl rand -hex 32 | tr -d '\n' > /jwt/jwt.hex &&
chmod 600 /jwt/jwt.hex;
fi"
🧰 Tools
🪛 yamllint

[error] 15-15: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 18-18: trailing spaces

(trailing-spaces)


reth:
container_name: reth
restart: unless-stopped
image: ghcr.io/paradigmxyz/reth
depends_on:
jwt-init:
condition: service_completed_successfully
ports:
- "9001:9001" # metrics
- "30303:30303" # eth/66 peering
- "8545:8545" # rpc
- "8551:8551" # engine
Comment on lines +28 to +32
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance network security configuration.

The current port exposure configuration could be more secure:

  1. Consider restricting metrics port (9001) to internal networks only
  2. Document the purpose and security implications of each exposed port
  3. Consider using network segmentation for different types of traffic

Apply these improvements:

    ports:
-      - "9001:9001" # metrics
-      - "30303:30303" # eth/66 peering
-      - "8545:8545" # rpc
-      - "8551:8551" # engine
+      # Internal metrics - restrict to internal network
+      - "127.0.0.1:9001:9001"
+      # P2P networking port
+      - "30303:30303/tcp"
+      - "30303:30303/udp"
+      # JSON-RPC API
+      - "127.0.0.1:8545:8545"
+      # Engine API - only needed for validator connections
+      - "127.0.0.1:8551:8551"
📝 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
ports:
- "9001:9001" # metrics
- "30303:30303" # eth/66 peering
- "8545:8545" # rpc
- "8551:8551" # engine
ports:
# Internal metrics - restrict to internal network
- "127.0.0.1:9001:9001"
# P2P networking port
- "30303:30303/tcp"
- "30303:30303/udp"
# JSON-RPC API
- "127.0.0.1:8545:8545"
# Engine API - only needed for validator connections
- "127.0.0.1:8551:8551"

volumes:
- mainnet_data:/root/.local/share/reth/mainnet
- sepolia_data:/root/.local/share/reth/sepolia
- holesky_data:/root/.local/share/reth/holesky
- logs:/root/logs
- ./jwttoken:/root/jwt:ro
pid: host
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove unnecessary host PID namespace access

The pid: host configuration grants excessive privileges to the container and is not required for running a Reth node.

Remove this line:

-    pid: host
📝 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
pid: host

command: >
node
--chain sepolia
--metrics 0.0.0.0:9001
--log.file.directory /root/logs
--authrpc.addr 0.0.0.0
--authrpc.port 8551
--authrpc.jwtsecret /root/jwt/jwt.hex
--http --http.addr 0.0.0.0 --http.port 8545
--http.api "eth,net,web3"
jim380 marked this conversation as resolved.
Show resolved Hide resolved

volumes:
mainnet_data:
driver: local
sepolia_data:
driver: local
holesky_data:
driver: local
logs:
driver: local
258 changes: 258 additions & 0 deletions execution.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,258 @@
package execution

import (
"context"
"errors"
"fmt"
"math/big"
"time"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/ethclient"
"github.com/ethereum/go-ethereum/rpc"
execution "github.com/rollkit/go-execution"
proxy_json_rpc "github.com/rollkit/go-execution/proxy/jsonrpc"
rollkit_types "github.com/rollkit/go-execution/types"
)

type PayloadStatus string

const (
PayloadStatusValid PayloadStatus = "VALID"
PayloadStatusInvalid PayloadStatus = "INVALID"
PayloadStatusSyncing PayloadStatus = "SYNCING"
)

var (
ErrNilPayloadStatus = errors.New("nil payload status")
ErrInvalidPayloadStatus = errors.New("invalid payload status")
)

// Ensure EngineAPIExecutionClient implements the execution.Execute interface
var _ execution.Execute = (*EngineAPIExecutionClient)(nil)

// EngineAPIExecutionClient implements the execution.Execute interface
type EngineAPIExecutionClient struct {
client *proxy_json_rpc.Client
engineClient *rpc.Client // engine api
ethClient *ethclient.Client
genesisHash common.Hash
feeRecipient common.Address
}

// NewEngineAPIExecutionClient creates a new instance of EngineAPIExecutionClient
func NewEngineAPIExecutionClient(config *proxy_json_rpc.Config, ethURL, engineURL string, genesisHash common.Hash, feeRecipient common.Address) (*EngineAPIExecutionClient, error) {
client := proxy_json_rpc.NewClient()
client.SetConfig(config)

ethClient, err := ethclient.Dial(ethURL)
if err != nil {
return nil, err
}

engineClient, err := rpc.Dial(engineURL)
if err != nil {
return nil, err
}

return &EngineAPIExecutionClient{
client: client,
engineClient: engineClient,
ethClient: ethClient,
genesisHash: genesisHash,
feeRecipient: feeRecipient,
}, nil
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate input parameters in the constructor

The NewEngineAPIExecutionClient constructor does not validate its input parameters. Passing nil values or empty strings could lead to nil pointer dereferences or unexpected behavior.

Consider adding validation checks for config, ethURL, engineURL, and other parameters:

func NewEngineAPIExecutionClient(config *proxy_json_rpc.Config, ethURL, engineURL string, genesisHash common.Hash, feeRecipient common.Address) (*EngineAPIExecutionClient, error) {
+	if config == nil {
+		return nil, fmt.Errorf("config cannot be nil")
+	}
+	if ethURL == "" {
+		return nil, fmt.Errorf("ethURL cannot be empty")
+	}
+	if engineURL == "" {
+		return nil, fmt.Errorf("engineURL cannot be empty")
+	}
	client := proxy_json_rpc.NewClient()
	client.SetConfig(config)
	...
}

Committable suggestion skipped: line range outside the PR's diff.

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling and resource cleanup in constructor

The constructor should clean up resources when an error occurs to prevent potential resource leaks.

 func NewEngineAPIExecutionClient(config *proxy_json_rpc.Config, ethURL, engineURL string, genesisHash common.Hash, feeRecipient common.Address) (*EngineAPIExecutionClient, error) {
 	client := proxy_json_rpc.NewClient()
 	client.SetConfig(config)

 	ethClient, err := ethclient.Dial(ethURL)
 	if err != nil {
+		client.Stop()
-		return nil, err
+		return nil, fmt.Errorf("failed to connect to eth client: %w", err)
 	}

 	engineClient, err := rpc.Dial(engineURL)
 	if err != nil {
+		ethClient.Close()
+		client.Stop()
-		return nil, err
+		return nil, fmt.Errorf("failed to connect to engine client: %w", err)
 	}

 	return &EngineAPIExecutionClient{
 		client:       client,
 		engineClient: engineClient,
 		ethClient:    ethClient,
 		genesisHash:  genesisHash,
 		feeRecipient: feeRecipient,
 	}, nil
 }
📝 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
// NewEngineAPIExecutionClient creates a new instance of EngineAPIExecutionClient
func NewEngineAPIExecutionClient(config *proxy_json_rpc.Config, ethURL, engineURL string, genesisHash common.Hash, feeRecipient common.Address) (*EngineAPIExecutionClient, error) {
client := proxy_json_rpc.NewClient()
client.SetConfig(config)
ethClient, err := ethclient.Dial(ethURL)
if err != nil {
return nil, err
}
engineClient, err := rpc.Dial(engineURL)
if err != nil {
return nil, err
}
return &EngineAPIExecutionClient{
client: client,
engineClient: engineClient,
ethClient: ethClient,
genesisHash: genesisHash,
feeRecipient: feeRecipient,
}, nil
}
// NewEngineAPIExecutionClient creates a new instance of EngineAPIExecutionClient
func NewEngineAPIExecutionClient(config *proxy_json_rpc.Config, ethURL, engineURL string, genesisHash common.Hash, feeRecipient common.Address) (*EngineAPIExecutionClient, error) {
client := proxy_json_rpc.NewClient()
client.SetConfig(config)
ethClient, err := ethclient.Dial(ethURL)
if err != nil {
client.Stop()
return nil, fmt.Errorf("failed to connect to eth client: %w", err)
}
engineClient, err := rpc.Dial(engineURL)
if err != nil {
ethClient.Close()
client.Stop()
return nil, fmt.Errorf("failed to connect to engine client: %w", err)
}
return &EngineAPIExecutionClient{
client: client,
engineClient: engineClient,
ethClient: ethClient,
genesisHash: genesisHash,
feeRecipient: feeRecipient,
}, nil
}


// Start starts the execution client
func (c *EngineAPIExecutionClient) Start(url string) error {
return c.client.Start(url)
}

// Stop stops the execution client and closes all connections
func (c *EngineAPIExecutionClient) Stop() {
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle the error returned by c.client.Stop()

The Stop() method may return an error that should be checked to ensure proper shutdown procedures.

Apply this diff to fix the issue:

func (c *EngineAPIExecutionClient) Stop() {
-	c.client.Stop()
+	if err := c.client.Stop(); err != nil {
+		// Handle the error appropriately, e.g., log it
+		fmt.Printf("Error stopping client: %v\n", err)
+	}
	if c.engineClient != nil {
		c.engineClient.Close()
	}
	if c.ethClient != nil {
		c.ethClient.Close()
	}
}

Committable suggestion skipped: line range outside the PR's diff.

c.client.Stop()
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle the error returned by c.client.Stop()

The error returned by c.client.Stop() is not checked. Ignoring errors can lead to unexpected behavior or resource leaks. Please handle the error appropriately.

Apply this diff to handle the error:

func (c *EngineAPIExecutionClient) Stop() {
-	c.client.Stop()
+	if err := c.client.Stop(); err != nil {
+		// Handle the error appropriately, e.g., log it
+		fmt.Printf("Error stopping client: %v\n", err)
+	}
	if c.engineClient != nil {
		c.engineClient.Close()
	}
	if c.ethClient != nil {
		c.ethClient.Close()
	}
}

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 golangci-lint

75-75: Error return value of c.client.Stop is not checked

(errcheck)


if c.engineClient != nil {
c.engineClient.Close()
}

if c.ethClient != nil {
c.ethClient.Close()
}
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle error from client.Stop()

The error returned by c.client.Stop() should be handled appropriately.

-func (c *EngineAPIExecutionClient) Stop() {
+func (c *EngineAPIExecutionClient) Stop() error {
-	c.client.Stop()
+	var errs []error
+	if err := c.client.Stop(); err != nil {
+		errs = append(errs, fmt.Errorf("failed to stop client: %w", err))
+	}

 	if c.engineClient != nil {
 		c.engineClient.Close()
 	}

 	if c.ethClient != nil {
 		c.ethClient.Close()
 	}
+	
+	if len(errs) > 0 {
+		return fmt.Errorf("errors during shutdown: %v", errs)
+	}
+	return nil
 }
📝 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
func (c *EngineAPIExecutionClient) Stop() {
c.client.Stop()
if c.engineClient != nil {
c.engineClient.Close()
}
if c.ethClient != nil {
c.ethClient.Close()
}
}
func (c *EngineAPIExecutionClient) Stop() error {
var errs []error
if err := c.client.Stop(); err != nil {
errs = append(errs, fmt.Errorf("failed to stop client: %w", err))
}
if c.engineClient != nil {
c.engineClient.Close()
}
if c.ethClient != nil {
c.ethClient.Close()
}
if len(errs) > 0 {
return fmt.Errorf("errors during shutdown: %v", errs)
}
return nil
}
🧰 Tools
🪛 golangci-lint

75-75: Error return value of c.client.Stop is not checked

(errcheck)


// InitChain initializes the blockchain with genesis information
func (c *EngineAPIExecutionClient) InitChain(genesisTime time.Time, initialHeight uint64, chainID string) (rollkit_types.Hash, uint64, error) {
ctx := context.Background()
var forkchoiceResult map[string]interface{}
err := c.engineClient.CallContext(ctx, &forkchoiceResult, "engine_forkchoiceUpdatedV1",
map[string]interface{}{
"headBlockHash": c.genesisHash,
"safeBlockHash": c.genesisHash,
"finalizedBlockHash": c.genesisHash,
},
map[string]interface{}{
"timestamp": genesisTime.Unix(),
"prevRandao": common.Hash{},
"suggestedFeeRecipient": c.feeRecipient,
},
)
if err != nil {
return rollkit_types.Hash{}, 0, fmt.Errorf("engine_forkchoiceUpdatedV1 failed: %w", err)
}

payloadID, ok := forkchoiceResult["payloadId"].(string)
if !ok {
return rollkit_types.Hash{}, 0, ErrNilPayloadStatus
}

var payload map[string]interface{}
err = c.engineClient.CallContext(ctx, &payload, "engine_getPayloadV1", payloadID)
if err != nil {
return rollkit_types.Hash{}, 0, fmt.Errorf("engine_getPayloadV1 failed: %w", err)
}

stateRoot := common.HexToHash(payload["stateRoot"].(string))
gasLimit := uint64(payload["gasLimit"].(float64))
var rollkitStateRoot rollkit_types.Hash
copy(rollkitStateRoot[:], stateRoot[:])
return rollkitStateRoot, gasLimit, nil
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Safely handle type assertions to prevent panics

When accessing payload["stateRoot"] and payload["gasLimit"], the code assumes these keys exist and are of the expected types. If the keys are missing or of a different type, this can cause a panic.

Add checks to ensure the keys exist and handle type assertions safely:

stateRootStr, ok := payload["stateRoot"].(string)
if !ok {
    return rollkit_types.Hash{}, 0, fmt.Errorf("payload missing or invalid 'stateRoot'")
}
stateRoot := common.HexToHash(stateRootStr)

gasLimitFloat, ok := payload["gasLimit"].(float64)
if !ok {
    return rollkit_types.Hash{}, 0, fmt.Errorf("payload missing or invalid 'gasLimit'")
}
gasLimit := uint64(gasLimitFloat)
📝 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
stateRoot := common.HexToHash(payload["stateRoot"].(string))
gasLimit := uint64(payload["gasLimit"].(float64))
var rollkitStateRoot rollkit_types.Hash
copy(rollkitStateRoot[:], stateRoot[:])
return rollkitStateRoot, gasLimit, nil
stateRootStr, ok := payload["stateRoot"].(string)
if !ok {
return rollkit_types.Hash{}, 0, fmt.Errorf("payload missing or invalid 'stateRoot'")
}
stateRoot := common.HexToHash(stateRootStr)
gasLimitFloat, ok := payload["gasLimit"].(float64)
if !ok {
return rollkit_types.Hash{}, 0, fmt.Errorf("payload missing or invalid 'gasLimit'")
}
gasLimit := uint64(gasLimitFloat)
var rollkitStateRoot rollkit_types.Hash
copy(rollkitStateRoot[:], stateRoot[:])
return rollkitStateRoot, gasLimit, nil

}

// GetTxs retrieves transactions from the transaction pool
func (c *EngineAPIExecutionClient) GetTxs() ([]rollkit_types.Tx, error) {
ctx := context.Background()
var result struct {
Pending map[string]map[string]*types.Transaction `json:"pending"`
Queued map[string]map[string]*types.Transaction `json:"queued"`
}
err := c.ethClient.Client().CallContext(ctx, &result, "txpool_content")
if err != nil {
return nil, fmt.Errorf("failed to get tx pool content: %w", err)
}

var txs []rollkit_types.Tx
for _, accountTxs := range result.Pending {
for _, tx := range accountTxs {
txBytes, err := tx.MarshalBinary()
if err != nil {
return nil, fmt.Errorf("failed to marshal transaction: %w", err)
}
txs = append(txs, rollkit_types.Tx(txBytes))
}
}
for _, accountTxs := range result.Queued {
for _, tx := range accountTxs {
txBytes, err := tx.MarshalBinary()
if err != nil {
return nil, fmt.Errorf("failed to marshal transaction: %w", err)
}
txs = append(txs, rollkit_types.Tx(txBytes))
}
}
return txs, nil
}

// ExecuteTxs executes the given transactions and returns the new state root and gas used
func (c *EngineAPIExecutionClient) ExecuteTxs(txs []rollkit_types.Tx, height uint64, timestamp time.Time, prevStateRoot rollkit_types.Hash) (rollkit_types.Hash, uint64, error) {
ctx := context.Background()
ethTxs := make([][]byte, len(txs))
for i, tx := range txs {
ethTxs[i] = tx
}

// 1. First call engine_newPayloadV1 with the transactions
var newPayloadResult map[string]interface{}
err := c.engineClient.CallContext(ctx, &newPayloadResult, "engine_newPayloadV1", map[string]interface{}{
"parentHash": common.BytesToHash(prevStateRoot[:]),
"timestamp": timestamp.Unix(),
"prevRandao": c.derivePrevRandao(height),
"feeRecipient": c.feeRecipient,
"transactions": ethTxs,
})
if err != nil {
return rollkit_types.Hash{}, 0, fmt.Errorf("engine_newPayloadV1 failed: %w", err)
}

status, ok := newPayloadResult["status"].(string)
if !ok || PayloadStatus(status) != PayloadStatusValid {
return rollkit_types.Hash{}, 0, ErrInvalidPayloadStatus
}

// 2. Then update fork choice
var forkchoiceResult map[string]interface{}
err = c.engineClient.CallContext(ctx, &forkchoiceResult, "engine_forkchoiceUpdatedV1",
map[string]interface{}{
"headBlockHash": common.BytesToHash(prevStateRoot[:]),
"safeBlockHash": common.BytesToHash(prevStateRoot[:]),
"finalizedBlockHash": common.BytesToHash(prevStateRoot[:]),
},
map[string]interface{}{
"timestamp": timestamp.Unix(),
"prevRandao": c.derivePrevRandao(height),
"suggestedFeeRecipient": c.feeRecipient,
},
)
if err != nil {
return rollkit_types.Hash{}, 0, fmt.Errorf("engine_forkchoiceUpdatedV1 failed: %w", err)
}

// 3. Get the execution results
var payload map[string]interface{}
payloadID, ok := forkchoiceResult["payloadId"].(string)
if !ok {
return rollkit_types.Hash{}, 0, ErrNilPayloadStatus
}

err = c.engineClient.CallContext(ctx, &payload, "engine_getPayloadV1", payloadID)
if err != nil {
return rollkit_types.Hash{}, 0, fmt.Errorf("engine_getPayloadV1 failed: %w", err)
}

newStateRoot := common.HexToHash(payload["stateRoot"].(string))
gasUsed := uint64(payload["gasUsed"].(float64))
var rollkitNewStateRoot rollkit_types.Hash
copy(rollkitNewStateRoot[:], newStateRoot[:])
return rollkitNewStateRoot, gasUsed, nil
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Safely handle type assertions to prevent panics

Similar to a previous issue, when accessing payload["stateRoot"] and payload["gasUsed"], the code assumes these keys exist and are of the expected types. This could lead to panics if the data is missing or malformed.

Ensure that the keys exist and handle type assertions properly:

stateRootStr, ok := payload["stateRoot"].(string)
if !ok {
    return rollkit_types.Hash{}, 0, fmt.Errorf("payload missing or invalid 'stateRoot'")
}
newStateRoot := common.HexToHash(stateRootStr)

gasUsedFloat, ok := payload["gasUsed"].(float64)
if !ok {
    return rollkit_types.Hash{}, 0, fmt.Errorf("payload missing or invalid 'gasUsed'")
}
gasUsed := uint64(gasUsedFloat)
📝 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
newStateRoot := common.HexToHash(payload["stateRoot"].(string))
gasUsed := uint64(payload["gasUsed"].(float64))
var rollkitNewStateRoot rollkit_types.Hash
copy(rollkitNewStateRoot[:], newStateRoot[:])
return rollkitNewStateRoot, gasUsed, nil
stateRootStr, ok := payload["stateRoot"].(string)
if !ok {
return rollkit_types.Hash{}, 0, fmt.Errorf("payload missing or invalid 'stateRoot'")
}
newStateRoot := common.HexToHash(stateRootStr)
gasUsedFloat, ok := payload["gasUsed"].(float64)
if !ok {
return rollkit_types.Hash{}, 0, fmt.Errorf("payload missing or invalid 'gasUsed'")
}
gasUsed := uint64(gasUsedFloat)
var rollkitNewStateRoot rollkit_types.Hash
copy(rollkitNewStateRoot[:], newStateRoot[:])
return rollkitNewStateRoot, gasUsed, nil

}

// SetFinal marks a block at the given height as final
func (c *EngineAPIExecutionClient) SetFinal(height uint64) error {
ctx := context.Background()
block, err := c.ethClient.BlockByNumber(ctx, big.NewInt(int64(height)))
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix potential integer overflow in block height conversion.

Converting uint64 to int64 can cause overflow for large block heights.

Apply this diff:

-    block, err := c.ethClient.BlockByNumber(ctx, big.NewInt(int64(height)))
+    block, err := c.ethClient.BlockByNumber(ctx, new(big.Int).SetUint64(height))
📝 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
block, err := c.ethClient.BlockByNumber(ctx, big.NewInt(int64(height)))
block, err := c.ethClient.BlockByNumber(ctx, new(big.Int).SetUint64(height))
🧰 Tools
🪛 golangci-lint

[high] 267-267: G115: integer overflow conversion uint64 -> int64

(gosec)

if err != nil {
return fmt.Errorf("failed to get block at height %d: %w", height, err)
}

var result map[string]interface{}
err = c.engineClient.CallContext(ctx, &result, "engine_forkchoiceUpdatedV1",
map[string]interface{}{
"headBlockHash": block.Hash(),
"safeBlockHash": block.Hash(),
"finalizedBlockHash": block.Hash(),
},
nil, // No payload attributes for finalization
)
if err != nil {
return fmt.Errorf("engine_forkchoiceUpdatedV1 failed for finalization: %w", err)
}

payloadStatus, ok := result["payloadStatus"].(map[string]interface{})
if !ok {
return ErrNilPayloadStatus
}

status, ok := payloadStatus["status"].(string)
if !ok || PayloadStatus(status) != PayloadStatusValid {
return ErrInvalidPayloadStatus
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Safely handle response data to prevent panics

When accessing result["payloadStatus"] and its status field, the code assumes they exist and are of the expected types. If the response format changes or is incorrect, this can cause a panic.

Add checks to ensure the response data exists and is valid:

payloadStatus, ok := result["payloadStatus"].(map[string]interface{})
if !ok {
    return fmt.Errorf("result missing or invalid 'payloadStatus'")
}

statusStr, ok := payloadStatus["status"].(string)
if !ok {
    return fmt.Errorf("payloadStatus missing or invalid 'status'")
}

if PayloadStatus(statusStr) != PayloadStatusValid {
    return ErrInvalidPayloadStatus
}
📝 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
payloadStatus, ok := result["payloadStatus"].(map[string]interface{})
if !ok {
return ErrNilPayloadStatus
}
status, ok := payloadStatus["status"].(string)
if !ok || PayloadStatus(status) != PayloadStatusValid {
return ErrInvalidPayloadStatus
}
payloadStatus, ok := result["payloadStatus"].(map[string]interface{})
if !ok {
return fmt.Errorf("result missing or invalid 'payloadStatus'")
}
statusStr, ok := payloadStatus["status"].(string)
if !ok {
return fmt.Errorf("payloadStatus missing or invalid 'status'")
}
if PayloadStatus(statusStr) != PayloadStatusValid {
return ErrInvalidPayloadStatus
}


return nil
}

// derivePrevRandao generates a deterministic prevRandao value based on block height
func (c *EngineAPIExecutionClient) derivePrevRandao(blockHeight uint64) common.Hash {
return common.BigToHash(big.NewInt(int64(blockHeight)))
}
Comment on lines +295 to +297
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Prevent integer overflow when converting blockHeight

In derivePrevRandao, converting blockHeight from uint64 to int64 can cause an integer overflow if blockHeight exceeds MaxInt64, leading to incorrect behavior.

Use new(big.Int).SetUint64(blockHeight) to safely handle the conversion:

func (c *EngineAPIExecutionClient) derivePrevRandao(blockHeight uint64) common.Hash {
-	return common.BigToHash(big.NewInt(int64(blockHeight)))
+	return common.BigToHash(new(big.Int).SetUint64(blockHeight))
}
📝 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
func (c *EngineAPIExecutionClient) derivePrevRandao(blockHeight uint64) common.Hash {
return common.BigToHash(big.NewInt(int64(blockHeight)))
}
func (c *EngineAPIExecutionClient) derivePrevRandao(blockHeight uint64) common.Hash {
return common.BigToHash(new(big.Int).SetUint64(blockHeight))
}

Comment on lines +294 to +297
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Review prevRandao implementation for production use.

  1. The current implementation has potential integer overflow issues
  2. Using block height directly as prevRandao might be too predictable for production use

For the immediate fix:

 func (c *EngineAPIExecutionClient) derivePrevRandao(blockHeight uint64) common.Hash {
-	return common.BigToHash(big.NewInt(int64(blockHeight)))
+	return common.BigToHash(new(big.Int).SetUint64(blockHeight))
 }

Consider implementing a more secure randomness source for production use. Would you like assistance in implementing a more robust solution?

📝 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
// derivePrevRandao generates a deterministic prevRandao value based on block height
func (c *EngineAPIExecutionClient) derivePrevRandao(blockHeight uint64) common.Hash {
return common.BigToHash(big.NewInt(int64(blockHeight)))
}
// derivePrevRandao generates a deterministic prevRandao value based on block height
func (c *EngineAPIExecutionClient) derivePrevRandao(blockHeight uint64) common.Hash {
return common.BigToHash(new(big.Int).SetUint64(blockHeight))
}
🧰 Tools
🪛 golangci-lint

[high] 288-288: G115: integer overflow conversion uint64 -> int64

(gosec)

Loading