Feature/reth el integration #185

sapinb · 2024-07-26T11:21:09Z

Description

closes #104
closes #106
closes #133

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactor

Checklist

I have performed a self-review of my code.
I have commented my code where necessary.
I have updated the documentation if needed.
My changes do not introduce new warnings.
I have added tests that prove my changes are effective or that my feature works.
New and existing tests pass with my changes.

Related Issues

codecov · 2024-07-26T12:19:10Z

Codecov Report

Attention: Patch coverage is 39.71377% with 337 lines in your changes missing coverage. Please review.

Project coverage is 51.03%. Comparing base (1edb77f) to head (36c6e9a).

Files	Patch %	Lines
crates/evmexec/src/fork_choice_state.rs	0.00%	54 Missing ⚠️
crates/evmexec/src/http_client.rs	2.32%	42 Missing ⚠️
crates/evmexec/src/engine.rs	70.50%	41 Missing ⚠️
reth/src/main.rs	0.00%	40 Missing ⚠️
sequencer/src/main.rs	0.00%	40 Missing ⚠️
crates/consensus-logic/src/duty_executor.rs	0.00%	34 Missing ⚠️
sequencer/src/config.rs	16.66%	30 Missing ⚠️
crates/consensus-logic/src/genesis.rs	0.00%	16 Missing ⚠️
crates/evmctl/src/messages.rs	43.47%	13 Missing ⚠️
crates/state/src/block.rs	60.86%	9 Missing ⚠️
... and 6 more

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
- Coverage   51.93%   51.03%   -0.91%     
==========================================
  Files          80       85       +5     
  Lines        7051     7413     +362     
==========================================
+ Hits         3662     3783     +121     
- Misses       3389     3630     +241

Files	Coverage Δ
crates/consensus-logic/src/unfinalized_tracker.rs	`95.51% <100.00%> (+0.02%)`	⬆️
crates/db/src/l2/db.rs	`99.37% <100.00%> (ø)`
crates/db/src/l2/schemas.rs	`0.00% <ø> (ø)`
crates/db/src/macros.rs	`82.35% <100.00%> (+12.35%)`	⬆️
crates/db/src/stubs/l2.rs	`65.95% <100.00%> (ø)`
crates/db/src/sync_event/schemas.rs	`100.00% <ø> (ø)`
crates/db/src/traits.rs	`100.00% <ø> (ø)`
crates/primitives/src/buf.rs	`64.76% <ø> (+2.85%)`	⬆️
crates/primitives/src/params.rs	`0.00% <ø> (ø)`
crates/state/src/chain_state.rs	`60.00% <ø> (ø)`
... and 17 more

... and 1 file with indirect coverage changes

delbonis

Looks good so far.

crates/state/src/block.rs

delbonis · 2024-07-26T21:09:57Z

crates/state/src/chain_state.rs

+            151, 170, 71, 78, 222, 173, 105, 242, 232, 9, 47, 21, 45, 160, 207, 234, 161, 29, 114,
+            237, 237, 94, 26, 177, 140, 238, 193, 81, 63, 80, 88, 181,


Don't bother keeping this state test in sync. It's commented out on master now. This test isn't providing us any value if we don't have some alternative means to compute the numbers we're hardcoding in here. We will probably switch to SSZ before mainnet and that has a well-defined structure hashing scheme.

crates/primitives/src/evm_exec.rs

delbonis · 2024-07-26T21:13:01Z

crates/evmexec/src/fcs.rs

Should be named forkchoice_state or something.

crates/evmexec/src/engine.rs

delbonis · 2024-07-26T21:17:30Z

crates/evmexec/src/engine.rs

        let fork_choice_result = self
            .client
            .fork_choice_updated_v2(fork_choice_state, None)
            .await;

+        info!(
+            "update_block_state; fork_choice_result: {:?}",


Don't forget to clean these up when getting ready to merge.

crates/evmexec/src/http_client.rs

sequencer/src/config.rs

crates/evmexec/src/el_payload.rs

add evm update input extra payload structure

(currently exec payload)

delbonis

Looks pretty good.

delbonis · 2024-07-29T18:28:17Z

reth/res/alpen-chain.json

Might want to name this to indicate it's just a dev test chain.

delbonis · 2024-07-29T18:29:25Z

reth/src/main.rs

+    command.ext.logs.log_file_directory = command
+        .ext
+        .logs
+        .log_file_directory
+        .join(command.chain.chain.to_string());


This looks weird, is this normal?

I didnt want to duplicate most of the code for NodeCommand and
and LogArgs

So this ended up with putting log args as extension to node commands. 🤷

This piece of code is the equivalent of this reth code

delbonis · 2024-07-29T18:38:09Z

Just make sure that the test framework instantiation works correctly. Like that the engine API keys work.

sapinb · 2024-07-30T11:03:59Z

@delbonis

Just make sure that the test framework instantiation works correctly. Like that the engine API keys work.

Functional tests are working using reth as service during tests, so ok there

delbonis · 2024-07-30T18:44:53Z

functional-tests/entry.py

+            "--reth-authrpc", reth_socket,
+            "--reth-jwtsecret", reth_secret_path,


These should both use the term "engine", I can tweak this myself.

delbonis · 2024-07-30T18:45:18Z

Is this still supposed to be a "draft" or is this closer to being mergeable?

sapinb · 2024-07-30T19:48:21Z

Is this still supposed to be a "draft" or is this closer to being mergeable?

@delbonis If there are no major issues, this can be considered mergable.

delbonis

This kinda steps on my PR I was working on since I was refactoring a few bits related to block assembly but that's fine I should be able to rebase.

delbonis · 2024-07-30T22:50:01Z

I don't know why it says that one toml file is incorrectly formatted, ignoring.

sapinb requested review from bewakes and delbonis July 26, 2024 11:21

delbonis requested changes Jul 26, 2024

View reviewed changes

sapinb added 18 commits July 29, 2024 01:10

add config for reth evmexec

3db6c68

add hardcoded evm el genesis block info in rollup params

d90d648

add block accessory data,

d3361c3

add evm update input extra payload structure

integrate evmexec

6d84a30

fix lints

2a8de8b

fix lints

fa84071

refactor: reduce changes

2d4ca8c

fix broken tests

ba921fb

use secret path in args

03d87c9

add reth binary with hardcoded chain spec

4ad6dc9

wrap L2Block in L2BlockBundle with accessory data

1cbb84a

(currently exec payload)

rename ELHttpClient -> EngineRpc

b4ea2e7

remove default impl for ElPayload

78bdbdb

replace stub chain state db with rocksdb based impl

264800a

get chain tip from updated state location

4da0b8e

add sequencer key in config

82ea98a

add delay to sequencer block generation

c58041e

pass block time as rollup params to duty executor

e741b90

sapinb force-pushed the feature/reth-el-integration branch from f6ee20e to e741b90 Compare July 29, 2024 07:42

sapinb requested a review from delbonis July 29, 2024 09:06

sapinb added 5 commits July 29, 2024 15:46

refactor: rename fcs -> fork_choice_state

ecd8173

use optional args

1bfdb5b

docs: add how to run section in readme

4872849

add reth service to functional tests

0df3e4e

fix example config

1f61013

delbonis requested changes Jul 29, 2024

View reviewed changes

sapinb added 2 commits July 30, 2024 13:08

fix: ensure seekable keys are encoded with correct encoding

2dec3a3

rename to alpen-dev-chain

ed3e3f5

delbonis reviewed Jul 30, 2024

View reviewed changes

sapinb added 2 commits July 31, 2024 01:27

fix: ensure seekable keys are encoded with correct encoding; pt2

34a6e97

remove extra logging

46e0b32

sapinb marked this pull request as ready for review July 30, 2024 19:47

sapinb requested a review from delbonis July 30, 2024 19:49

fix lints

36c6e9a

delbonis approved these changes Jul 30, 2024

View reviewed changes

delbonis merged commit fd5de94 into master Jul 30, 2024
14 of 22 checks passed

storopoli deleted the feature/reth-el-integration branch November 28, 2024 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/reth el integration #185

Feature/reth el integration #185

sapinb commented Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading

delbonis left a comment

delbonis Jul 26, 2024

delbonis Jul 26, 2024

delbonis Jul 26, 2024

delbonis left a comment

delbonis Jul 29, 2024

delbonis Jul 29, 2024

sapinb Jul 30, 2024 •

edited

Loading

delbonis commented Jul 29, 2024 •

edited

Loading

sapinb commented Jul 30, 2024

delbonis Jul 30, 2024

delbonis commented Jul 30, 2024

sapinb commented Jul 30, 2024

delbonis left a comment

delbonis commented Jul 30, 2024

		151, 170, 71, 78, 222, 173, 105, 242, 232, 9, 47, 21, 45, 160, 207, 234, 161, 29, 114,
		237, 237, 94, 26, 177, 140, 238, 193, 81, 63, 80, 88, 181,

		"--reth-authrpc", reth_socket,
		"--reth-jwtsecret", reth_secret_path,

Feature/reth el integration #185

Feature/reth el integration #185

Conversation

sapinb commented Jul 26, 2024 • edited Loading

Description

Type of Change

Checklist

Related Issues

codecov bot commented Jul 26, 2024 • edited Loading

Codecov Report

delbonis left a comment

Choose a reason for hiding this comment

delbonis Jul 26, 2024

Choose a reason for hiding this comment

delbonis Jul 26, 2024

Choose a reason for hiding this comment

delbonis Jul 26, 2024

Choose a reason for hiding this comment

delbonis left a comment

Choose a reason for hiding this comment

delbonis Jul 29, 2024

Choose a reason for hiding this comment

delbonis Jul 29, 2024

Choose a reason for hiding this comment

sapinb Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

delbonis commented Jul 29, 2024 • edited Loading

sapinb commented Jul 30, 2024

delbonis Jul 30, 2024

Choose a reason for hiding this comment

delbonis commented Jul 30, 2024

sapinb commented Jul 30, 2024

delbonis left a comment

Choose a reason for hiding this comment

delbonis commented Jul 30, 2024

sapinb commented Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading

sapinb Jul 30, 2024 •

edited

Loading

delbonis commented Jul 29, 2024 •

edited

Loading