Enforce Optimistic Sync Conditions & CLI Tests #2986

ethDreamer · 2022-02-01T22:55:08Z

Issue Addressed

Lighthouse will no longer import blocks optimistically that fail the optimistic sync conditions. This PR also addresses #2982.

Proposed Changes

The validation for the optimistic sync conditions is done inside block verification.

Additional Info

I added tests for most of the CLI flags added since v2.0.1:

lighthouse/src/main.rs

terminal-total-difficulty-override
terminal-block-hash-override
terminal-block-hash-epoch-override
safe-slots-to-import-optimistically

These would require significant modifications to test harness code

logfile-debug-level
logfile-max-size
logfile-max-number
logfile-compress

account_manager/src/validator/exit.rs (requires building test harness)

no-confirmation

account_manager/src/validator/slashing_protection.rs (requires building test harness)

pubkeys

beacon_node/src/cli.rs

lcli/src/main.rs (requires building test harness, no lcli flags have tests)

consensus/types/src/chain_spec.rs

ethDreamer · 2022-02-01T23:07:31Z

The defaults appear to match what the spec has as long as u64::MAX == FAR_FUTURE_EPOCH

realbigsean · 2022-02-11T21:05:20Z

Talked to @ethDreamer, he said this one's ready for review, the unchecked tasks are for out of scope CLI tests

beacon_node/beacon_chain/src/block_verification.rs

lighthouse/tests/exec.rs

lighthouse/tests/beacon_node.rs

beacon_node/beacon_chain/src/execution_payload.rs

Co-authored-by: realbigsean <[email protected]>

ethDreamer · 2022-02-22T19:02:52Z

Hey @paulhauner, perhaps you wanna review the optimistic sync conditions?

macladson

I took a quick look at the CLI tests and they seem pretty good to me.
I left a few suggestions but they are pretty minor so feel free to ignore them 🙂

macladson · 2022-02-25T05:23:48Z

lighthouse/src/main.rs

+                    .map_err(|e| format!("Failed to create dumped chain config: {:?}", e))?;
+                serde_yaml::to_writer(&mut file, &chain_config)
+                    .map_err(|e| format!("Error serializing chain config: {:?}", e))?;
+            };


Not a huge deal, but there's starting to be a bit of duplication between the BN and VC with some of the code that runs here.

It might be worth spinning it out into dump_config and dump_chain_config functions which can be reused between both the BN and VC? If it lived in clap_utils (or somewhere else appropriate) they might also be able to be used for the boot_node's dump-chain-config flag. (Although you may run into type issues)

If you wanted to get extra fancy, it would be really cool if it was just 1 function which was generic over different Config's but that's probably more work than it's worth.

A function that's generic over some config: T where T: serde::Serialize would be pretty simple and do the trick, I think.

Done! Let me know what you guys think 👀 @paulhauner @macladson

macladson · 2022-02-25T05:36:07Z

lighthouse/tests/beacon_node.rs

+    for url in urls.into_iter().skip(1) {
+        endpoint_arg.push(',');
+        endpoint_arg.push_str(url);
+    }


I might be misunderstanding, but you can probably just do:

let endpoint_arg = urls.join(",");

@macladson agreed.. this is how I had tried to write it initially. But join isn't a function. What we need is intersperse, which is currently nightly-only :/

When that becomes available, this code nicely simplifies to just:
let endpoint_arg = urls.into_iter().intersperse(',').collect::<String>();

I tried opting into using intersperse, but our CI tests prevent me from committing that for release tests :/

For a Vec of &str's (or String's) you can use join: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.join

Playground link

join is also used elsewhere in the tests.

I could be wrong, but I think that should work here.

lighthouse/tests/exec.rs

paulhauner

Looking good. Only a few changes, I think we're pretty close!

beacon_node/beacon_chain/src/execution_payload.rs

beacon_node/beacon_chain/src/block_verification.rs

paulhauner · 2022-02-26T00:30:42Z

lighthouse/src/main.rs

+                    .map_err(|e| format!("Failed to create dumped chain config: {:?}", e))?;
+                serde_yaml::to_writer(&mut file, &chain_config)
+                    .map_err(|e| format!("Error serializing chain config: {:?}", e))?;
+            };


A function that's generic over some config: T where T: serde::Serialize would be pretty simple and do the trick, I think.

Co-authored-by: Paul Hauner <[email protected]>

Suggestions for `optimistic_conditions` branch

realbigsean

New changes look good to me!

paulhauner

LGTM! 🚀

bors r+

bors · 2022-03-01T00:29:57Z

Merge conflict.

paulhauner · 2022-03-01T03:59:53Z

I think this will fix CI :)

ethDreamer#6

Fix merge conflict

paulhauner · 2022-03-01T09:13:11Z

I think this will fix CI :)

ethDreamer#6

Looks like I missed something! I'll let you sort it out :)

paulhauner · 2022-03-01T22:54:47Z

Looks like I missed something! I'll let you sort it out :)

Sorry, I forgot you're AFK this week 🙏 I'm resolving the little conflict over in #3050.

Closed in favor of #3050. All of @ethDreamer's hard work will be merged there.

@ethDreamer

## Description This PR adds a single, trivial commit (f5d2b27) atop #2986 to resolve a tests compile error. The original author (@ethDreamer) is AFK so I'm getting this one merged ☺️ Please see #2986 for more information about the other, significant changes in this PR. Co-authored-by: Mark Mackey <[email protected]> Co-authored-by: ethDreamer <[email protected]>

@ethDreamer

commit 6370edd Author: Paul Hauner <[email protected]> Date: Wed Mar 2 16:31:18 2022 +1100 Update testing/execution_engine_integration/src/build_geth.rs Co-authored-by: Michael Sproul <[email protected]> commit f5a1dee Author: Paul Hauner <[email protected]> Date: Wed Mar 2 16:24:03 2022 +1100 Fix proto-array bug commit 4f7610f Author: Paul Hauner <[email protected]> Date: Wed Mar 2 16:23:52 2022 +1100 Fix payload tests commit cd7a0f5 Author: Paul Hauner <[email protected]> Date: Wed Mar 2 15:22:54 2022 +1100 Implement consensus-specs/2844 commit 5313364 Merge: d5e84a5 668115a Author: Paul Hauner <[email protected]> Date: Wed Mar 2 14:23:19 2022 +1100 Merge branch 'unstable' into prev-randao-rename commit d5e84a5 Author: Paul Hauner <[email protected]> Date: Wed Mar 2 13:31:45 2022 +1100 Fix windows test errors commit 668115a Author: Akihito Nakano <[email protected]> Date: Wed Mar 2 01:05:08 2022 +0000 Rename Eth1/Eth2 in documents (sigp#3021) ## Issue Addressed Resolves sigp#3019 ## Proposed Changes - Eth2 Eth2.0 Ethereum 2.0 -> Ethereum consensus - Eth2 network -> consensus layer - Ethereum 2.0 specification -> Ethereum proof-of-stake consensus specification - Eth2 deposit contract -> Staking deposit contract - Eth1 -> execution client ## Additional Info The description needs to be updated by someone who has permission to do. 📝 <img width="487" alt="image" src="https://user-images.githubusercontent.com/1885716/153995211-816d9561-751e-4810-abb9-83d979379783.png"> commit e34524b Author: Age Manning <[email protected]> Date: Wed Mar 2 01:05:07 2022 +0000 Increase default target-peer count to 80 (sigp#3005) Increase the default peer count from 50 to 80 commit db2e3ae Author: Paul Hauner <[email protected]> Date: Wed Mar 2 11:08:40 2022 +1100 Fix yaml indenting commit a2f99e5 Author: Paul Hauner <[email protected]> Date: Wed Mar 2 11:05:57 2022 +1100 Specify go version in github actions commit 0c5e9c6 Author: Paul Hauner <[email protected]> Date: Wed Mar 2 11:04:15 2022 +1100 Switch EE tests to be a binary commit 7f07fa5 Author: Paul Hauner <[email protected]> Date: Wed Mar 2 10:22:32 2022 +1100 Print output of failed make command commit fa20696 Author: Paul Hauner <[email protected]> Date: Wed Mar 2 10:01:00 2022 +1100 Bump spec tests version commit c2d7677 Author: Paul Hauner <[email protected]> Date: Sat Feb 26 10:17:14 2022 +1100 Add auth port commit 540337a Author: Paul Hauner <[email protected]> Date: Sat Feb 26 10:10:06 2022 +1100 Use `merge-kiln-v2` branch for geth commit 1a3d32e Author: Paul Hauner <[email protected]> Date: Fri Feb 25 15:08:38 2022 +1100 Revert "Add serde "random" alias" This reverts commit bb02c2f. commit 63376bd Author: Paul Hauner <[email protected]> Date: Fri Feb 25 14:59:26 2022 +1100 Add serde "random" alias commit 375aa32 Author: Paul Hauner <[email protected]> Date: Fri Feb 25 10:09:38 2022 +1100 Rename random to prev_randao commit b6493d5 Author: Paul Hauner <[email protected]> Date: Tue Mar 1 22:56:47 2022 +0000 Enforce Optimistic Sync Conditions & CLI Tests (v2) (sigp#3050) ## Description This PR adds a single, trivial commit (f5d2b27) atop sigp#2986 to resolve a tests compile error. The original author (@ethDreamer) is AFK so I'm getting this one merged ☺️ Please see sigp#2986 for more information about the other, significant changes in this PR. Co-authored-by: Mark Mackey <[email protected]> Co-authored-by: ethDreamer <[email protected]>

ethDreamer commented Feb 1, 2022

View reviewed changes

consensus/types/src/chain_spec.rs Show resolved Hide resolved

ethDreamer mentioned this pull request Feb 1, 2022

Bellatrix Task Tracker #2929

Closed

ethDreamer force-pushed the optimistic_conditions branch 2 times, most recently from ded85a5 to 0ef3771 Compare February 2, 2022 18:34

paulhauner added work-in-progress PR is a work-in-progress bellatrix Required to support the Bellatrix Upgrade labels Feb 4, 2022

realbigsean added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Feb 11, 2022

realbigsean requested changes Feb 11, 2022

View reviewed changes

paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Feb 14, 2022

ethDreamer and others added 7 commits February 22, 2022 12:54

Added Spec Restrictions on Optimistic Block Import

3df6da8

Add Tests for Bellatrix CLI flags/Fix Peer Scoring

467eb44

Update beacon_node/beacon_chain/src/block_verification.rs

59a527c

Co-authored-by: realbigsean <[email protected]>

Update lighthouse/tests/exec.rs

b3cce88

Co-authored-by: realbigsean <[email protected]>

Update beacon_node/beacon_chain/src/execution_payload.rs

a6fce33

Co-authored-by: realbigsean <[email protected]>

Update lighthouse/tests/exec.rs

db3fa65

Co-authored-by: realbigsean <[email protected]>

Addressed Sean's Comments

e6863c1

ethDreamer force-pushed the optimistic_conditions branch from 9e329cb to e6863c1 Compare February 22, 2022 18:56

ethDreamer added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Feb 22, 2022

ethDreamer requested review from realbigsean and paulhauner February 22, 2022 18:59

Fixed Comment

3e652b4

macladson reviewed Feb 25, 2022

View reviewed changes

paulhauner added 2 commits February 25, 2022 17:06

Add just/fin block getters to fork choice

58393c2

Add first version of safe check

418f836

Add is_optimistic_candidate_block to fork choice

8c30461

paulhauner mentioned this pull request Feb 26, 2022

Suggestions for optimistic_conditions branch ethDreamer/lighthouse#5

Merged

paulhauner requested changes Feb 26, 2022

View reviewed changes

paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Feb 26, 2022

ethDreamer and others added 3 commits February 28, 2022 11:45

Update beacon_node/beacon_chain/src/execution_payload.rs

ba171ab

Co-authored-by: Paul Hauner <[email protected]>

Merge pull request #5 from paulhauner/optimistic_conditions_paul

f01e56d

Suggestions for `optimistic_conditions` branch

Created Function to Reduce Duplicated Code

dbedc93

ethDreamer force-pushed the optimistic_conditions branch from a9f6008 to dbedc93 Compare February 28, 2022 20:18

ethDreamer added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Feb 28, 2022

realbigsean approved these changes Feb 28, 2022

View reviewed changes

paulhauner approved these changes Feb 28, 2022

View reviewed changes

paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Feb 28, 2022

ethDreamer and others added 2 commits February 28, 2022 19:31

Merge branch 'unstable' into optimistic_conditions

8a6c32b

Fix compile issue from merge

2440614

Merge pull request #6 from paulhauner/opt-cond-merge

760e69a

Fix merge conflict

paulhauner mentioned this pull request Mar 1, 2022

[Merged by Bors] - Enforce Optimistic Sync Conditions & CLI Tests (v2) #3050

Closed

paulhauner closed this Mar 1, 2022

paulhauner mentioned this pull request Mar 8, 2022

Check new CLI flags introduced by Bellatrix #2982

Closed

ethDreamer deleted the optimistic_conditions branch December 19, 2022 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce Optimistic Sync Conditions & CLI Tests #2986

Enforce Optimistic Sync Conditions & CLI Tests #2986

ethDreamer commented Feb 1, 2022 •

edited

Loading

ethDreamer commented Feb 1, 2022

realbigsean commented Feb 11, 2022

ethDreamer commented Feb 22, 2022

macladson left a comment

macladson Feb 25, 2022

paulhauner Feb 26, 2022

ethDreamer Feb 28, 2022 •

edited

Loading

macladson Feb 25, 2022

ethDreamer Feb 28, 2022

macladson Mar 1, 2022

paulhauner left a comment

paulhauner Feb 26, 2022

realbigsean left a comment

paulhauner left a comment

bors bot commented Mar 1, 2022

paulhauner commented Mar 1, 2022

paulhauner commented Mar 1, 2022

paulhauner commented Mar 1, 2022 •

edited

Loading

Enforce Optimistic Sync Conditions & CLI Tests #2986

Enforce Optimistic Sync Conditions & CLI Tests #2986

Conversation

ethDreamer commented Feb 1, 2022 • edited Loading

Issue Addressed

Proposed Changes

Additional Info

lighthouse/src/main.rs

account_manager/src/validator/exit.rs (requires building test harness)

account_manager/src/validator/slashing_protection.rs (requires building test harness)

beacon_node/src/cli.rs

lcli/src/main.rs (requires building test harness, no lcli flags have tests)

ethDreamer commented Feb 1, 2022

realbigsean commented Feb 11, 2022

ethDreamer commented Feb 22, 2022

macladson left a comment

Choose a reason for hiding this comment

macladson Feb 25, 2022

Choose a reason for hiding this comment

paulhauner Feb 26, 2022

Choose a reason for hiding this comment

ethDreamer Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

macladson Feb 25, 2022

Choose a reason for hiding this comment

ethDreamer Feb 28, 2022

Choose a reason for hiding this comment

macladson Mar 1, 2022

Choose a reason for hiding this comment

paulhauner left a comment

Choose a reason for hiding this comment

paulhauner Feb 26, 2022

Choose a reason for hiding this comment

realbigsean left a comment

Choose a reason for hiding this comment

paulhauner left a comment

Choose a reason for hiding this comment

bors bot commented Mar 1, 2022

paulhauner commented Mar 1, 2022

paulhauner commented Mar 1, 2022

paulhauner commented Mar 1, 2022 • edited Loading

ethDreamer commented Feb 1, 2022 •

edited

Loading

ethDreamer Feb 28, 2022 •

edited

Loading

paulhauner commented Mar 1, 2022 •

edited

Loading