Skip to content

[Merged by Bors] - Fix broken Nethermind integration tests #4836

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.

Sign up for GitHub

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

Closed
macladson wants to merge 14 commits into from
Closed

[Merged by Bors] - Fix broken Nethermind integration tests #4836

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
48a57c4
Initial fix
macladson Oct 12, 2023
a05228f
Clippy
macladson Oct 12, 2023
57900aa
Enable Shapella from genesis
michaelsproul Oct 13, 2023
d743448
Remove outdated comment
michaelsproul Oct 13, 2023
aee58b0
Tweak some things blindly
michaelsproul Oct 13, 2023
b83ed10
Run exec engine tests on self-hosted runner
michaelsproul Oct 17, 2023
9e22c61
Merge remote-tracking branch 'origin/unstable' into fix-nethermind-tests
michaelsproul Oct 17, 2023
488f923
Don't run rust update on runner
michaelsproul Oct 17, 2023
bc75adb
cleanup some disk space
antondlr Oct 17, 2023
284d831
`cargo fmt`
antondlr Oct 17, 2023
f44b9a5
run on gh-hosted
antondlr Oct 17, 2023
e4b53a9
run on self-hosted with `go` added to path
antondlr Oct 17, 2023
6fc4266
fix nethermind v1.21 path
antondlr Oct 17, 2023
cce7ccf
Fix compilation error in release test
jimmygchen Oct 18, 2023
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
33 changes: 21 additions & 12 deletions testing/execution_engine_integration/src/genesis_json.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,10 @@ pub fn geth_genesis_json() -> Value {
"muirGlacierBlock":0,
"berlinBlock":0,
"londonBlock":0,
"clique": {
"period": 5,
"epoch": 30000
},
"terminalTotalDifficulty":0
"mergeNetsplitBlock": 0,
"shanghaiTime": 0,
"terminalTotalDifficulty": 0,
"terminalTotalDifficultyPassed": true
},
"nonce":"0x42",
"timestamp":"0x0",
Expand Down Expand Up @@ -72,8 +71,10 @@ pub fn nethermind_genesis_json() -> Value {
"accountStartNonce": "0x0",
"maximumExtraDataSize": "0x20",
"minGasLimit": "0x1388",
"networkID": "0x1469ca",
"MergeForkIdTransition": "0x3e8",
"networkID": "0x00146A2E",
"MergeForkIdTransition": "0x0",
"maxCodeSize": "0x6000",
"maxCodeSizeTransition": "0x0",
"eip150Transition": "0x0",
"eip158Transition": "0x0",
"eip160Transition": "0x0",
Expand Down Expand Up @@ -101,7 +102,15 @@ pub fn nethermind_genesis_json() -> Value {
"eip1559Transition": "0x0",
"eip3198Transition": "0x0",
"eip3529Transition": "0x0",
"eip3541Transition": "0x0"
"eip3541Transition": "0x0",
"eip3540TransitionTimestamp": "0x0",
"eip3651TransitionTimestamp": "0x0",
"eip3670TransitionTimestamp": "0x0",
"eip3675TransitionTimestamp": "0x0",
"eip3855TransitionTimestamp": "0x0",
"eip3860TransitionTimestamp": "0x0",
"eip4895TransitionTimestamp": "0x0",
"terminalTotalDifficulty": "0x0"
},
"genesis": {
"seal": {
Expand All @@ -112,20 +121,20 @@ pub fn nethermind_genesis_json() -> Value {
},
"difficulty": "0x01",
"author": "0x0000000000000000000000000000000000000000",
"timestamp": "0x0",
"timestamp": "0x63585F88",
"parentHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
"extraData": "",
"gasLimit": "0x1C9C380"
"gasLimit": "0x400000"
},
"accounts": {
"0x7b8C3a386C0eea54693fFB0DA17373ffC9228139": {
"balance": "10000000000000000000000000"
},
"0xdA2DD7560DB7e212B945fC72cEB54B7D8C886D77": {
"balance": "10000000000000000000000000"
},
}
},
"nodes": []
}
}
)
}
2 changes: 2 additions & 0 deletions testing/execution_engine_integration/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![recursion_limit = "256"] // for inline json

/// This binary runs integration tests between Lighthouse and execution engines.
///
/// It will first attempt to build any supported integration clients, then it will run tests.
Expand Down
5 changes: 3 additions & 2 deletions testing/execution_engine_integration/src/nethermind.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use unused_port::unused_tcp4_port;
/// We've pinned the Nethermind version since our method of using the `master` branch to
/// find the latest tag isn't working. It appears Nethermind don't always tag on `master`.
/// We should fix this so we always pull the latest version of Nethermind.
const NETHERMIND_BRANCH: &str = "release/1.18.2";
const NETHERMIND_BRANCH: &str = "release/1.21.0";
const NETHERMIND_REPO_URL: &str = "https://github.com/NethermindEth/nethermind";

fn build_result(repo_dir: &Path) -> Output {
Expand Down Expand Up @@ -68,7 +68,8 @@ impl NethermindEngine {
.join("bin")
.join("Release")
.join("net7.0")
.join("Nethermind.Runner")
.join("linux-x64")
Copy link
Copy Markdown
Member

@michaelsproul michaelsproul Oct 13, 2023

Choose a reason for hiding this comment

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

this part of the path didn't exist on my machine, lets see if it exists on CI 🤞

Copy link
Copy Markdown
Member

@jimmygchen jimmygchen Oct 13, 2023

Choose a reason for hiding this comment

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

It no longer runs on macos though :(

Copy link
Copy Markdown
Member

@jimmygchen jimmygchen Oct 13, 2023

Choose a reason for hiding this comment

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

path on my machine: testing/execution_engine_integration/execution_clients/nethermind/src/Nethermind/Nethermind.Runner/bin/Release/net7.0/nethermind

Copy link
Copy Markdown
Member

@michaelsproul michaelsproul Oct 13, 2023

Choose a reason for hiding this comment

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

I'm deleting it. I think maybe this was why the job was failing

.join("nethermind")
}
}

Expand Down
53 changes: 33 additions & 20 deletions testing/execution_engine_integration/src/test_rig.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ use types::{
Address, ChainSpec, EthSpec, ExecutionBlockHash, ExecutionPayload, ExecutionPayloadHeader,
ForkName, FullPayload, Hash256, MainnetEthSpec, PublicKeyBytes, Slot, Uint256,
};
const EXECUTION_ENGINE_START_TIMEOUT: Duration = Duration::from_secs(30);
const EXECUTION_ENGINE_START_TIMEOUT: Duration = Duration::from_secs(60);

const TEST_FORK: ForkName = ForkName::Capella;

struct ExecutionPair<E, T: EthSpec> {
/// The Lighthouse `ExecutionLayer` struct, connected to the `execution_engine` via HTTP.
Expand Down Expand Up @@ -110,7 +112,7 @@ impl<E: GenericExecutionEngine> TestRig<E> {
let (runtime_shutdown, exit) = exit_future::signal();
let (shutdown_tx, _) = futures::channel::mpsc::channel(1);
let executor = TaskExecutor::new(Arc::downgrade(&runtime), exit, log.clone(), shutdown_tx);
let mut spec = MainnetEthSpec::default_spec();
let mut spec = TEST_FORK.make_genesis_spec(MainnetEthSpec::default_spec());
spec.terminal_total_difficulty = Uint256::zero();

let fee_recipient = None;
Expand Down Expand Up @@ -269,8 +271,12 @@ impl<E: GenericExecutionEngine> TestRig<E> {
Slot::new(1), // Insert proposer for the next slot
head_root,
proposer_index,
// TODO: think about how to test different forks
PayloadAttributes::new(timestamp, prev_randao, Address::repeat_byte(42), None),
PayloadAttributes::new(
timestamp,
prev_randao,
Address::repeat_byte(42),
Some(vec![]),
),
)
.await;

Expand Down Expand Up @@ -308,8 +314,12 @@ impl<E: GenericExecutionEngine> TestRig<E> {
.execution_layer
.get_suggested_fee_recipient(proposer_index)
.await;
let payload_attributes =
PayloadAttributes::new(timestamp, prev_randao, suggested_fee_recipient, None);
let payload_attributes = PayloadAttributes::new(
timestamp,
prev_randao,
suggested_fee_recipient,
Some(vec![]),
);
let valid_payload = self
.ee_a
.execution_layer
Expand All @@ -318,8 +328,7 @@ impl<E: GenericExecutionEngine> TestRig<E> {
&payload_attributes,
forkchoice_update_params,
builder_params,
// FIXME: think about how to test other forks
ForkName::Merge,
TEST_FORK,
&self.spec,
)
.await
Expand Down Expand Up @@ -448,8 +457,12 @@ impl<E: GenericExecutionEngine> TestRig<E> {
.execution_layer
.get_suggested_fee_recipient(proposer_index)
.await;
let payload_attributes =
PayloadAttributes::new(timestamp, prev_randao, suggested_fee_recipient, None);
let payload_attributes = PayloadAttributes::new(
timestamp,
prev_randao,
suggested_fee_recipient,
Some(vec![]),
);
let second_payload = self
.ee_a
.execution_layer
Expand All @@ -458,8 +471,7 @@ impl<E: GenericExecutionEngine> TestRig<E> {
&payload_attributes,
forkchoice_update_params,
builder_params,
// FIXME: think about how to test other forks
ForkName::Merge,
TEST_FORK,
&self.spec,
)
.await
Expand Down Expand Up @@ -489,11 +501,14 @@ impl<E: GenericExecutionEngine> TestRig<E> {
*/
let head_block_hash = valid_payload.block_hash();
let finalized_block_hash = ExecutionBlockHash::zero();
// TODO: think about how to handle different forks
// To save sending proposer preparation data, just set the fee recipient
// to the fee recipient configured for EE A.
let payload_attributes =
PayloadAttributes::new(timestamp, prev_randao, Address::repeat_byte(42), None);
let payload_attributes = PayloadAttributes::new(
timestamp,
prev_randao,
Address::repeat_byte(42),
Some(vec![]),
);
let slot = Slot::new(42);
let head_block_root = Hash256::repeat_byte(100);
let validator_index = 0;
Expand Down Expand Up @@ -526,11 +541,7 @@ impl<E: GenericExecutionEngine> TestRig<E> {
.notify_new_payload(&second_payload)
.await
.unwrap();
// TODO: we should remove the `Accepted` status here once Geth fixes it
assert!(matches!(
status,
PayloadStatus::Syncing | PayloadStatus::Accepted
));
assert!(matches!(status, PayloadStatus::Syncing));

/*
* Execution Engine B:
Expand Down Expand Up @@ -630,11 +641,13 @@ async fn check_payload_reconstruction<E: GenericExecutionEngine>(
.get_engine_capabilities(None)
.await
.unwrap();

assert!(
// if the engine doesn't have these capabilities, we need to update the client in our tests
capabilities.get_payload_bodies_by_hash_v1 && capabilities.get_payload_bodies_by_range_v1,
"Testing engine does not support payload bodies methods"
);

let mut bodies = ee
.execution_layer
.get_payload_bodies_by_hash(vec![payload.block_hash()])
Expand Down