feat(net): test syncing from geth #623

Rjected · 2022-12-26T22:50:22Z

reth integration tests

This adds tests which spawn an instance of geth and configure it to produce blocks with cilque. Geth is sent transactions, which gets used to produce blocks. This is then used to test the following situations:

Reth full sync test, adding geth as reth's only outbound peer
Reth network keepalive test, initializing just reth's networking stack, and making sure we can stay connected to a peered geth instance for 30 seconds.

Depends on:

TODO:

Write sync test that sets up the reth pipeline and peers geth
- Get geth to produce blocks
- Set up a reth Genesis
- Rest of pipeline
- Put transactions in blocks (currently they are empty but we do have a funded account)
Consider moving to different folder since we will be using the entire pipeline
Figure out how to produce blocks
- Configures geth in with clique, sending transactions that should get automatically mined.
Remove patch
Integrate pipeline builder from feat: pipeline builder #1017

gakonst · 2023-01-02T13:47:03Z

@Rjected gm - planned next steps here?

Rjected · 2023-01-02T14:49:22Z

gm @gakonst

still debugging an invalid sender error, looking through geth to see if I activated EIP155 correctly

Then:

Initialize other components
Run the test

gakonst · 2023-01-02T15:32:38Z

Cool - pls ping me if you're still blocked on this by eod (if you're online today?) so that we debug together as it's important we get confidence that it's the network and not us causing the peering issues

codecov-commenter · 2023-01-06T23:15:44Z

Codecov Report

Merging #623 (e6b4844) into main (cc43b72) will increase coverage by 0.89%.
The diff coverage is 73.39%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #623      +/-   ##
==========================================
+ Coverage   74.46%   75.35%   +0.89%     
==========================================
  Files         319      321       +2     
  Lines       34844    34949     +105     
==========================================
+ Hits        25945    26336     +391     
+ Misses       8899     8613     -286

Flag	Coverage Δ
unit-tests	`75.35% <73.39%> (+0.89%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
crates/staged-sync/src/lib.rs	`100.00% <ø> (ø)`
crates/staged-sync/src/test_utils/clique.rs	`63.93% <63.93%> (ø)`
...es/staged-sync/src/test_utils/clique_middleware.rs	`85.41% <85.41%> (ø)`
crates/metrics/metrics-derive/src/expand.rs	`84.36% <0.00%> (-14.19%)`	⬇️
crates/metrics/metrics-derive/src/metric.rs	`75.00% <0.00%> (-3.13%)`	⬇️
bin/reth/src/p2p/mod.rs	`0.00% <0.00%> (ø)`
bin/reth/src/node/mod.rs	`0.00% <0.00%> (ø)`
bin/reth/src/stage/mod.rs	`0.00% <0.00%> (ø)`
crates/net/network/src/peers/manager.rs	`83.84% <0.00%> (+0.18%)`	⬆️
crates/stages/src/stages/bodies.rs	`93.34% <0.00%> (+0.22%)`	⬆️
... and 32 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

mattsse

this looks great!
incredibly useful.

should we go over this after the call Monday to assess what's missing/blockers etc?

Rjected · 2023-01-09T00:40:41Z

should we go over this after the call Monday to assess what's missing/blockers etc?

yeah that would be very useful, lets do it @mattsse

Rjected · 2023-01-10T06:12:45Z

big refactor:

network test utils are now in a new crate reth-net-test-utils. I am thinking of bringing this out into a module reth_network::test_utils (like reth_provider::test_utils), as long as it doesn't add unnecessary dependencies to the network crate. additional dev-deps are fine
the geth sync test now lives in reth-tests (in crates/tests), which will contain other structs and utils for writing integration tests with geth. we can also import other parts of reth from here

Rjected · 2023-01-12T03:36:55Z

almost done, just noting down things left TODO:

Create conversions from ethers_core::utils::ChainConfig => reth_consensus::Config, then use that to initialize BeaconConsensus and pass it into the RethBuilder
Create conversion from ethers_core::utils::Genesis => reth_cli_utils::Genesis, also pass it into RethBuilder
Fill blocks with transactions and change period to 0 so blocks seal instantly
Assert that we get blocks, headers, etc

We should have enough support code once this is done s.t. we can write all sorts of tests with geth, for example testing that reth passes the geth snap checkpoint. We would need to configure geth's checkpoint for this

gakonst

I like where this landed. A big nitpick but I think we need some refactors and splitting in smaller PRs because it's a lot. I also feel we don't need dedicated crates for these and should be able to roll most in the existing ones?

crates/primitives/src/proofs.rs

crates/primitives/src/account.rs

crates/net/network/tests/it/connect.rs

crates/net/network/Cargo.toml

crates/net/test-utils/src/testnet.rs

crates/tests/src/clique.rs

crates/tests/src/sync.rs

Rjected · 2023-01-18T22:24:18Z

squashed the commits and rebased on main - this now only contains code related to crates/tests

The only thing blocking this PR is a CliqueConsensus implementation, because BeaconConsensus enforces that extraData cannot be longer than 32 bytes, and clique always produces extraData longer than 32 bytes, which causes the geth peer to be kicked after failing header validation.
As a result, this integration test would not be covering BeaconConsenus validation.

* the tests properly sync

* introduce an extension trait to simplify the use of providers in sync tests * add test-utils feature in staged-sync

mattsse

After tons of ether PRs and debugging, this will be very useful.
great work!

only smol nit.
and we need to discuss where to put this exactly.

mattsse · 2023-01-30T20:22:22Z

crates/staged-sync/tests/sync.rs

+        fs::remove_dir_all(dir_path).await.unwrap();
+    })
+    .await
+    .unwrap();


I believe this should be unnecessary because tempdir already does this on drop

ah I had to do this because I was using into_path, which requires manual deletion, but switching to path works and does not require doing this

mattsse · 2023-01-30T20:22:38Z

crates/staged-sync/tests/sync.rs

+        fs::remove_dir_all(dir_path).await.unwrap();
+    })
+    .await
+    .unwrap();


mattsse · 2023-01-30T20:25:33Z

crates/staged-sync/src/test_utils/clique.rs

+        tokio::spawn(async move {
+            loop {
+                if let (Ok(line), _line_str) = {
+                    let mut buf = String::new();
+                    (err_reader.read_line(&mut buf), buf.clone())
+                } {
+                    if line == 0 {
+                        break
+                    }
+                }
+            }


this is just for debugging, right?

print_logs is for debugging, but prevent_blocking is necessary to prevent the stderr pipe buffer from filling up and causing the test to hang. Using Drop doesn't seem to work here because it causes geth to crash (probably due to EPIPE, but I'm not entirely sure). I'm not sure if there's a better way to prevent the stderr pipe buffer from filling though

can't we send to /dev/null instead and not bother with this? either way let's do in a followup

we can't do it by default because Geth captures the stderr to make sure rpc, p2p, etc are up when spawn is called. So have to first pipe the Command's output with Stdio::piped(). Maybe we could spawn cat or something, with geth's stderr as cat's stdin, and pipe cat to /dev/null

mattsse · 2023-01-30T20:28:18Z

crates/staged-sync/src/lib.rs

+
+#[cfg(any(test, feature = "test-utils"))]
+/// Common helpers for integration testing.
+pub mod test_utils;


I wonder if staged-sync is the right crate here, I guess so we can use the pipeline and test certain parts of it against a GethInstance?

perhaps it would be appropriate to move to its own crate?

wdyt @onbjerg ?

prefer it here, or in any node-named packages (ref #1079 )

gakonst · 2023-01-31T04:41:32Z

crates/staged-sync/src/test_utils/clique.rs

+        tokio::spawn(async move {
+            loop {
+                if let (Ok(line), _line_str) = {
+                    let mut buf = String::new();
+                    (err_reader.read_line(&mut buf), buf.clone())
+                } {
+                    if line == 0 {
+                        break
+                    }
+                }
+            }


can't we send to /dev/null instead and not bother with this? either way let's do in a followup

gakonst · 2023-01-31T04:42:35Z

crates/staged-sync/src/lib.rs

+
+#[cfg(any(test, feature = "test-utils"))]
+/// Common helpers for integration testing.
+pub mod test_utils;


prefer it here, or in any node-named packages (ref #1079 )

gakonst · 2023-01-31T04:55:11Z

crates/staged-sync/tests/sync.rs

+        let remote_genesis = SealedHeader::from(provider.remote_genesis_block().await.unwrap());
+
+        let mut local_genesis_header: Header = chainspec.genesis().clone().into();


Doesn't clique.0.genesis() load Geth's genesis file? how is remote_genesis_block() relevant here?

gakonst · 2023-01-31T04:55:47Z

crates/staged-sync/tests/sync.rs

+        let local_genesis = local_genesis_header.seal();
+        assert_eq!(local_genesis, remote_genesis, "genesis blocks should match, we computed {local_genesis:#?} but geth computed {remote_genesis:#?}");


this is a redundant sanity check right? aren't these always going to be the same since they're both from geth?

gakonst · 2023-01-31T04:59:38Z

crates/staged-sync/tests/sync.rs

+    reth_tracing::init_test_tracing();
+    tokio::time::timeout(GETH_TIMEOUT, async move {
+        // first create a signer that we will fund so we can make transactions
+        let chain_id = 13337u64;
+        let data_dir = tempfile::tempdir().expect("should be able to create temp geth datadir");
+        let dir_path = data_dir.path();
+        tracing::info!(
+            data_dir=?dir_path,
+            "initializing geth instance"
+        );
+
+        // this creates a funded geth
+        let clique_geth = Geth::new()
+            .chain_id(chain_id)
+            .p2p_port(unused_port())
+            .data_dir(dir_path.to_str().unwrap());
+
+        // build the funded geth
+        let (mut clique, provider) = CliqueGethInstance::new(clique_geth, None).await;
+        let geth_p2p_port = clique.0.p2p_port().expect("geth should be configured with a p2p port");
+        tracing::info!(
+            p2p_port=%geth_p2p_port,
+            rpc_port=%clique.0.port(),
+            "configured clique geth instance in keepalive test"
+        );
+
+        // don't print logs, but drain the stderr
+        clique.prevent_blocking().await;
+
+        // get geth to start producing blocks - use a blank password
+        let clique_private_key = clique.0.clique_private_key().clone().expect("clique should be configured with a private key");
+        provider.enable_mining(clique_private_key, "".into()).await.unwrap();
+
+        // === check that we have the same genesis hash ===
+
+        // get the chainspec from the genesis we configured for geth
+        let mut chainspec: ChainSpec = clique.0.genesis().clone().expect("clique should be configured with a genesis").into();
+        let remote_genesis = SealedHeader::from(provider.remote_genesis_block().await.unwrap());
+
+        let mut local_genesis_header = Header::from(chainspec.genesis().clone());
+
+        let hardforks = chainspec.hardforks();
+
+        // set initial base fee depending on eip-1559
+        if let Some(0) = hardforks.get(&Hardfork::London) {
+            local_genesis_header.base_fee_per_gas = Some(EIP1559_INITIAL_BASE_FEE);
+        }
+
+        let local_genesis = local_genesis_header.seal();
+        assert_eq!(local_genesis, remote_genesis, "genesis blocks should match, we computed {local_genesis:#?} but geth computed {remote_genesis:#?}");
+
+        // set the chainspec genesis hash
+        chainspec.genesis_hash = local_genesis.hash();
+
+        // === create many blocks ===
+
+        let nonces = 0..1000u64;
+        let txs = nonces
+            .map(|nonce| {
+                // create a tx that just sends to the zero addr
+                TypedTransaction::Eip1559(Eip1559TransactionRequest::new()
+                    .to(H160::zero())
+                    .value(1u64)
+                    .nonce(nonce))
+            });
+        tracing::info!("generated transactions for blocks");
+
+        // finally send the txs to geth
+        provider.send_requests(txs).await.unwrap();
+
+        let block = provider.get_block_number().await.unwrap();
+        assert!(block > U64::zero());


this boilerplate should go to a helper function, since it's duped with the code above

gakonst · 2023-01-31T05:00:18Z

crates/staged-sync/tests/sync.rs

+        // wait for the session to be established
+        let _peer_id = events.peer_added_and_established().await.unwrap();


shouldn't we add a assert_eq!(geth_peer_id, peer_id) here? what is the purpose of the keepalive stuff below?

gakonst

finally :) this exposed a lot of things to improve, so im glad we did this.

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

Rjected force-pushed the dan/geth-sync-test branch from 7d253d2 to 053b091 Compare December 28, 2022 21:29

Rjected added C-test A change that impacts how or what we test A-networking Related to networking in general labels Dec 29, 2022

Rjected force-pushed the dan/geth-sync-test branch from 053b091 to d56aff0 Compare December 29, 2022 18:44

Rjected force-pushed the dan/geth-sync-test branch 5 times, most recently from 920587b to 76cb0f7 Compare January 6, 2023 23:03

mattsse reviewed Jan 8, 2023

View reviewed changes

Rjected force-pushed the dan/geth-sync-test branch from c97b65a to 2db2cc8 Compare January 10, 2023 05:36

Rjected force-pushed the dan/geth-sync-test branch 2 times, most recently from 2b0e05d to 659c95a Compare January 13, 2023 04:59

gakonst requested changes Jan 15, 2023

View reviewed changes

Rjected force-pushed the dan/geth-sync-test branch from 5892531 to 368ba7d Compare January 17, 2023 05:44

This was referenced Jan 17, 2023

feat(network): move test helpers to test-utils mod #901

Merged

feat(primitives): impl genesis state root helper #904

Merged

feat(primitives): impl From<Genesis> for ChainSpec #921

Merged

Rjected force-pushed the dan/geth-sync-test branch from 887475f to 82601c8 Compare January 18, 2023 22:13

Rjected force-pushed the dan/geth-sync-test branch 3 times, most recently from 77e2a39 to afe1e75 Compare January 20, 2023 15:26

Rjected added 15 commits January 30, 2023 15:06

correct local genesis hash calculation

8e464f7

set keepalive test timeout to 30secs

588b030

tell the pipeline to stop when it's done

9299a4e

* the tests properly sync

refactor with geth utilities from ethers

67b1b38

accomodate staged-sync refactor

0bb5a81

remove unnecessary stage config fields

89d07d8

correct reth builder comment

54522bd

use new PipelineBuilder instead of RethTestBuilder

caa053f

integrate sync tests in staged-sync crate

c7c5017

* introduce an extension trait to simplify the use of providers in sync tests * add test-utils feature in staged-sync

remove tests crate

b0c9765

use a result in enable_mining

d50660b

add CliqueGethInstance example

ca770fd

remove final assert in CliqueMiddleware

75e4ff3

properly configure geth with p2p port

359c9b4

update ethers

ebada9a

Rjected force-pushed the dan/geth-sync-test branch from 15b2810 to ebada9a Compare January 30, 2023 20:07

revert forkkind change

d4881bc

mattsse requested changes Jan 30, 2023

View reviewed changes

Rjected and others added 6 commits January 30, 2023 16:48

init genesis and switch to tempdir path

50dc5d2

fix doctests

3e56757

add no_run to clique doctest

eea9265

fix: remove lib.rs, caused tests to double execute

eec3449

Merge branch 'main' into dan/geth-sync-test

e6b4844

cleanup: only run 1 test and check we got the right peerid

96bcea0

gakonst approved these changes Jan 31, 2023

View reviewed changes

chore: fix doctest

2c3f45e

gakonst approved these changes Jan 31, 2023

View reviewed changes

gakonst merged commit f771e23 into main Jan 31, 2023

gakonst deleted the dan/geth-sync-test branch January 31, 2023 05:35

literallymarvellous pushed a commit to literallymarvellous/reth that referenced this pull request Feb 6, 2023

feat(net): test syncing from geth (paradigmxyz#623)

20842d2

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(net): test syncing from geth #623

feat(net): test syncing from geth #623

Rjected commented Dec 26, 2022 •

edited

gakonst commented Jan 2, 2023

Rjected commented Jan 2, 2023 •

edited

gakonst commented Jan 2, 2023

codecov-commenter commented Jan 6, 2023 •

edited

mattsse left a comment

Rjected commented Jan 9, 2023

Rjected commented Jan 10, 2023

Rjected commented Jan 12, 2023 •

edited

gakonst left a comment

Rjected commented Jan 18, 2023

mattsse left a comment

mattsse Jan 30, 2023

Rjected Jan 30, 2023

mattsse Jan 30, 2023

mattsse Jan 30, 2023

Rjected Jan 30, 2023

gakonst Jan 31, 2023

Rjected Jan 31, 2023 •

edited

mattsse Jan 30, 2023

gakonst Jan 31, 2023

gakonst Jan 31, 2023

gakonst Jan 31, 2023

gakonst Jan 31, 2023

gakonst Jan 31, 2023

gakonst Jan 31, 2023

gakonst Jan 31, 2023

gakonst left a comment

		let remote_genesis = SealedHeader::from(provider.remote_genesis_block().await.unwrap());

		let mut local_genesis_header: Header = chainspec.genesis().clone().into();

		let local_genesis = local_genesis_header.seal();
		assert_eq!(local_genesis, remote_genesis, "genesis blocks should match, we computed {local_genesis:#?} but geth computed {remote_genesis:#?}");

		// wait for the session to be established
		let _peer_id = events.peer_added_and_established().await.unwrap();

feat(net): test syncing from geth #623

feat(net): test syncing from geth #623

Conversation

Rjected commented Dec 26, 2022 • edited

reth integration tests

gakonst commented Jan 2, 2023

Rjected commented Jan 2, 2023 • edited

gakonst commented Jan 2, 2023

codecov-commenter commented Jan 6, 2023 • edited

Codecov Report

mattsse left a comment

Choose a reason for hiding this comment

Rjected commented Jan 9, 2023

Rjected commented Jan 10, 2023

Rjected commented Jan 12, 2023 • edited

gakonst left a comment

Choose a reason for hiding this comment

Rjected commented Jan 18, 2023

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rjected Jan 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gakonst left a comment

Choose a reason for hiding this comment

Rjected commented Dec 26, 2022 •

edited

Rjected commented Jan 2, 2023 •

edited

codecov-commenter commented Jan 6, 2023 •

edited

Rjected commented Jan 12, 2023 •

edited

Rjected Jan 31, 2023 •

edited