chore: refactor the two execution clients (pure and op) to main branch #40
Conversation
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as EngineClient
participant Engine as Ethereum Engine API
participant EthRPC as Ethereum JSON-RPC
Client->>Engine: Authenticate (JWT)
Client->>Engine: Set forkchoice (InitChain)
Client->>EthRPC: Get block info
Client->>EthRPC: Get pending/queued txs (GetTxs)
Client->>Engine: Update forkchoice (ExecuteTxs)
Client->>Engine: Request new payload ID
Client->>Engine: Get execution payload
Client->>Engine: Submit payload
Engine-->>Client: Payload status
Client->>Engine: Update forkchoice (finalization)
Client->>EthRPC: Get updated block info
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
RollkitBot
requested review from
a team,
tuxcanfly,
tzdybal and
yarikbratashchuk
and removed request for
a team
gupadhyaya
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
execution/pure/execution_test.go (1)
69-87: 🛠️ Refactor suggestionUse the sub-test handle (
tt) for assertions inside the sub-testWithin the “Build chain” sub-test most assertions still use the outer
thandle.
If any of those assertions fail you will not get the nice sub-test name in the
output and – when the parent test is executed in parallel – you can even get
data races.- executionClient, err := NewPureEngineExecutionClient( + executionClient, err := NewPureEngineExecutionClient( ... - require.NoError(t, err) + require.NoError(tt, err) ... - require.NoError(t, err) - require.Equal(t, rollkitGenesisStateRoot, stateRoot) - require.NotZero(t, gasLimit) + require.NoError(tt, err) + require.Equal(tt, rollkitGenesisStateRoot, stateRoot) + require.NotZero(tt, gasLimit)Do the same search-&-replace for the remaining assertions that run under the
sub-test (including the “Sync chain” sub-test).
This keeps the test output and failure attribution consistent.
🧹 Nitpick comments (8)
execution/pure/execution.go (2)
31-34: Fix stale type name in interface–conformance commentThe comment still refers to
EngineAPIExecutionClient, but the actual type is nowPureEngineClient. This can confuse future readers and automated lint tools.-// Ensure EngineAPIExecutionClient implements the execution.Execute interface -var _ execution.Executor = (*PureEngineClient)(nil) +// Ensure PureEngineClient implements the execution.Executor interface +var _ execution.Executor = (*PureEngineClient)(nil)
45-53: Doc-comment mismatch with constructor nameThe constructor is called
NewPureEngineExecutionClient, but its doc-comment says it creates an EngineAPIExecutionClient. Align them to avoidrevivewarnings and improve IDE hints.-// NewPureEngineExecutionClient creates a new instance of EngineAPIExecutionClient +// NewPureEngineExecutionClient creates a new PureEngineClientexecution/op/go.mod (2)
7-10: Double-check thereplacedirective forgo-ethereumReplacing
github.com/ethereum/go-ethereumwith the Optimism fork (op-geth) is fine if all packages in this repo expect the altered behaviour/extra RPCs the fork provides.
However, it also pins you to a fork‐specific release cadence and may break downstream users that import your module and expect vanilla geth APIs.Action items
- Confirm that every internal consumer truly needs OP-specific code.
- If the fork is needed only in the new
opclient, consider adding it viago.workor a sub-module instead of a root-level replace, so the “pure” client can still compile against stock geth.
11-224: Rungo mod tidyto trim 200+ stale indirect depsThe file lists ~210 indirect requirements; many (e.g.
docker,k8s.io,aws-sdk-go-v2, etc.) do not appear in the code underexecution/op.
Large, unused dep graphs increasego.sum, download time, and vulnerability surface.go mod tidy -vwill keep only transitive deps actually needed after the next build/test and makes future scans faster.
execution/op/execution.go (2)
31-33: Update interface-conformance commentThe guard variable is correct, but the preceding comment mentions the old type name.
-// Ensure EngineAPIExecutionClient implements the execution.Execute interface +// Ensure EngineClient implements the execution.Executor interface
45-53: Constructor doc-comment & function name divergeThe function is
NewEngineExecutionClient, but the comment says NewPureEngineExecutionClient. Fix to silencereviveand avoid confusion.-// NewPureEngineExecutionClient creates a new instance of EngineAPIExecutionClient +// NewEngineExecutionClient creates a new EngineClient🧰 Tools
🪛 golangci-lint (1.64.8)
[warning] 45-45: exported: comment on exported function NewEngineExecutionClient should be of the form "NewEngineExecutionClient ..."
(revive)
execution/op/execution_test.go (2)
401-402: GloballastNonceis not concurrency-safe
lastNonceis a package-level mutable variable updated from multiple tests
(TestEngineExecution,TestSubmitTransaction). If the package is ever run
withgo test -parallelor individual tests are executed concurrently (e.g.
viat.Parallel()), non-atomic increments will cause nonce reuse and
transaction rejection.If you need shared state, protect it with a mutex or, simpler, make
lastNonce
a local variable passed around.
38-68: Large amount of duplicated helper code betweenpureandoptestsBoth test suites define identical helpers (
createEthClient,checkLatestBlock,
generateJWTSecret, docker orchestration, tx utilities, …). Moving these into
a shared test-only package (e.g.execution/testsupport) would:• Remove ~400 lines of duplication
• Ensure behaviour stays identical between clients
• Make future fixes (like the nonce or off-by-one issues) in one place.No blocking issue, but worth addressing before the helpers diverge further.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
execution/op/go.sumis excluded by!**/*.sumexecution/pure/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
execution/op/execution.go(1 hunks)execution/op/execution_test.go(1 hunks)execution/op/go.mod(1 hunks)execution/pure/execution.go(1 hunks)execution/pure/execution_test.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
execution/op/execution.go
[high] 193-193: G115: integer overflow conversion int64 -> uint64
(gosec)
[warning] 45-45: exported: comment on exported function NewEngineExecutionClient should be of the form "NewEngineExecutionClient ..."
(revive)
290-290: func (*EngineClient).derivePrevRandao is unused
(unused)
🔇 Additional comments (2)
execution/op/execution.go (1)
290-292:derivePrevRandaonow used – or removeIf you accept the earlier suggestion to wire
derivePrevRandao, this method will no longer be “unused”.
Otherwise, delete it to keep the package warning-free.🧰 Tools
🪛 golangci-lint (1.64.8)
290-290: func
(*EngineClient).derivePrevRandaois unused(unused)
execution/pure/execution_test.go (1)
34-36: Verify newDOCKER_PATHrelative location
DOCKER_PATHmoved two levels up ("../../docker").
If someone runs the test from another working directory (e.g.go test ./...
from repo root)filepath.Abswill resolve against that directory, not the
file’s directory, and the path may break.Consider anchoring the path to the module root by using
testing’s helper
filepath.Absont.TempDir()or by detecting the repo root viago list -m -f '{{.Dir}}'in init code.
| blockHash := payloadResult.ExecutionPayload.BlockHash | ||
| err = c.setFinal(ctx, blockHash, false) | ||
| if err != nil { | ||
| return nil, 0, err | ||
| } | ||
|
|
||
| return payloadResult.ExecutionPayload.StateRoot.Bytes(), payloadResult.ExecutionPayload.GasUsed, nil | ||
| } |
There was a problem hiding this comment.
Return GasLimit, not GasUsed, for maxBytes
The sibling “pure” client returns ExecutionPayload.GasLimit.
Returning GasUsed here changes the meaning of maxBytes and may break anything consuming the execution.Executor contract.
-return payloadResult.ExecutionPayload.StateRoot.Bytes(), payloadResult.ExecutionPayload.GasUsed, nil
+return payloadResult.ExecutionPayload.StateRoot.Bytes(), payloadResult.ExecutionPayload.GasLimit, nil| &engine.PayloadAttributes{ | ||
| Timestamp: ts, | ||
| Random: prevBlockHash, //c.derivePrevRandao(height), | ||
| SuggestedFeeRecipient: c.feeRecipient, | ||
| Withdrawals: []*types.Withdrawal{}, | ||
| BeaconRoot: &c.genesisHash, | ||
| Transactions: txsPayload, // force to use txsPayload | ||
| NoTxPool: true, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use derivePrevRandao and avoid dead code
derivePrevRandao is currently unused, triggering golangci-lint.
You already compute prevBlockHash but reuse it as Random. Either delete the helper or use it here:
- Random: prevBlockHash, //c.derivePrevRandao(height),
+ Random: c.derivePrevRandao(blockHeight),This keeps the helper relevant and documents how the RANDAO value is derived.
📝 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.
| &engine.PayloadAttributes{ | |
| Timestamp: ts, | |
| Random: prevBlockHash, //c.derivePrevRandao(height), | |
| SuggestedFeeRecipient: c.feeRecipient, | |
| Withdrawals: []*types.Withdrawal{}, | |
| BeaconRoot: &c.genesisHash, | |
| Transactions: txsPayload, // force to use txsPayload | |
| NoTxPool: true, | |
| }, | |
| &engine.PayloadAttributes{ | |
| Timestamp: ts, | |
| - Random: prevBlockHash, //c.derivePrevRandao(height), | |
| + Random: c.derivePrevRandao(blockHeight), | |
| SuggestedFeeRecipient: c.feeRecipient, | |
| Withdrawals: []*types.Withdrawal{}, | |
| BeaconRoot: &c.genesisHash, | |
| Transactions: txsPayload, // force to use txsPayload | |
| NoTxPool: true, | |
| }, |
| func (c *EngineClient) ExecuteTxs(ctx context.Context, txs [][]byte, blockHeight uint64, timestamp time.Time, prevStateRoot []byte) (updatedStateRoot []byte, maxBytes uint64, err error) { | ||
| // convert rollkit tx to eth tx | ||
| ethTxs := make([]*types.Transaction, len(txs)) | ||
| for i, tx := range txs { | ||
| ethTxs[i] = new(types.Transaction) | ||
| err := ethTxs[i].UnmarshalBinary(tx) | ||
| if err != nil { | ||
| return nil, 0, fmt.Errorf("failed to unmarshal transaction: %w", err) | ||
| } | ||
| } | ||
|
|
||
| // encode | ||
| txsPayload := make([][]byte, len(txs)) | ||
| for i, tx := range ethTxs { | ||
| buf := bytes.Buffer{} | ||
| err := tx.EncodeRLP(&buf) | ||
| if err != nil { | ||
| return nil, 0, fmt.Errorf("failed to RLP encode tx: %w", err) | ||
| } | ||
| txsPayload[i] = buf.Bytes() | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential integer-overflow & clarity for timestamp conversion
timestamp.Unix() returns int64; casting directly to uint64 triggers gosec G115 because a negative value would wrap.
While real block timestamps are non-negative, adding an explicit guard silences the warning and documents intent.
-ts := uint64(timestamp.Unix())
+rawTs := timestamp.Unix()
+if rawTs < 0 {
+ return nil, 0, fmt.Errorf("negative timestamp is invalid: %d", rawTs)
+}
+ts := uint64(rawTs)Committable suggestion skipped: line range outside the PR's diff.
| newStateRoot, maxBytes, err := executionClient.ExecuteTxs(ctx, payload, blockHeight, time.Now(), prevStateRoot) | ||
| require.NoError(t, err) | ||
| if len(payload) > 0 { | ||
| require.NotZero(tt, maxBytes) | ||
| } | ||
| require.NotEqual(tt, prevStateRoot, newStateRoot) | ||
|
|
||
| err = executionClient.SetFinal(ctx, blockHeight) |
There was a problem hiding this comment.
Empty-block handling inconsistent with build phase
During sync you always assert that the state root changes:
require.NotEqual(tt, prevStateRoot, newStateRoot)But in the build phase you correctly treat an empty block (block 4) as
state-root preserving.
The same block height will be replayed here, so this assertion will fail (or,
worse, silently hide a divergence by forcing a change).
Either replicate the earlier check or make the assertion conditional on
len(payload) > 0.
| executionClient, err := NewEngineExecutionClient( | ||
| TEST_ETH_URL, | ||
| TEST_ENGINE_URL, | ||
| jwtSecret, | ||
| genesisHash, | ||
| common.Address{}, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 300*time.Second) | ||
| defer cancel() | ||
| stateRoot, gasLimit, err := executionClient.InitChain(ctx, genesisTime, initialHeight, CHAIN_ID) | ||
| require.NoError(t, err) | ||
| require.Equal(t, rollkitGenesisStateRoot, stateRoot) | ||
| require.NotZero(t, gasLimit) | ||
|
|
||
| prevStateRoot := rollkitGenesisStateRoot |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the correct testing.T handle inside sub-tests
Analogous to the pure client tests, several assertions still call
require.*(t, …) instead of require.*(tt, …) inside the sub-test, e.g.:
require.NoError(t, err)
require.Equal(t, rollkitGenesisStateRoot, stateRoot)Replace t with the sub-test handle (tt) throughout “Build chain” and “Sync
chain” to ensure proper failure attribution and avoid data races when tests
are run in parallel.
| for blockHeight := initialHeight; blockHeight <= 10; blockHeight++ { | ||
| nTxs := int(blockHeight) + 10 | ||
| // randomly use no transactions | ||
| if blockHeight == 4 { | ||
| nTxs = 0 | ||
| } | ||
| txs := make([]*ethTypes.Transaction, nTxs) | ||
| for i := range txs { | ||
| txs[i] = getRandomTransaction(t, 22000) | ||
| } |
There was a problem hiding this comment.
Off-by-one: executing the genesis height again
The build-phase loop starts at blockHeight := initialHeight (which is 1),
but InitChain already initialises the chain at that height.
Running ExecuteTxs immediately for height 1 re-executes the genesis slot,
which most engines reject or silently ignore.
- for blockHeight := initialHeight; blockHeight <= 10; blockHeight++ {
+ for blockHeight := initialHeight + 1; blockHeight <= 10; blockHeight++ {Remember to adjust the allPayloads index and the sync-phase loop
accordingly:
- payload := allPayloads[blockHeight-1]
+ payload := allPayloads[blockHeight-initialHeight-1]Committable suggestion skipped: line range outside the PR's diff.
|
we have decided to do: #41, hence closing |
Overview
Summary by CodeRabbit