[Merged by Bors] - Use async code when interacting with EL #3244

paulhauner · 2022-06-06T06:18:21Z

Overview

This rather extensive PR achieves two primary goals:

Uses the finalized/justified checkpoints of fork choice (FC), rather than that of the head state.
Refactors fork choice, block production and block processing to async functions.

Additionally, it achieves:

Concurrent forkchoice updates to the EL and cache pruning after a new head is selected.
Concurrent "block packing" (attestations, etc) and execution payload retrieval during block production.
Concurrent per-block-processing and execution payload verification during block processing.
The Arc-ification of SignedBeaconBlock during block processing (it's never mutated, so why not?):
- I had to do this to deal with sending blocks into spawned tasks.
- Previously we were cloning the beacon block at least 2 times during each block processing, these clones are either removed or turned into cheaper Arc clones.
- We were also Box-ing and un-Box-ing beacon blocks as they moved throughout the networking crate. This is not a big deal, but it's nice to avoid shifting things between the stack and heap.
- Avoids cloning all the blocks in every chain segment during sync.
- It also has the potential to clean up our code where we need to pass an owned block around so we can send it back in the case of an error (I didn't do much of this, my PR is already big enough 😅)
The BeaconChain::HeadSafetyStatus struct was removed. It was an old relic from prior merge specs.

For motivation for this change, see #3244 (comment)

Changes to `canonical_head` and `fork_choice`

Previously, the BeaconChain had two separate fields:

canonical_head: RwLock<Snapshot>,
fork_choice: RwLock<BeaconForkChoice>

Now, we have grouped these values under a single struct:

canonical_head: CanonicalHead {
  cached_head: RwLock<Arc<Snapshot>>,
  fork_choice: RwLock<BeaconForkChoice>
}

Apart from ergonomics, the only actual change here is wrapping the canonical head snapshot in an Arc. This means that we no longer need to hold the cached_head (canonical_head, in old terms) lock when we want to pull some values from it. This was done to avoid deadlock risks by preventing functions from acquiring (and holding) the cached_head and fork_choice locks simultaneously.

Breaking Changes

The `state` (root) field in the `finalized_checkpoint` SSE event

Consider the scenario where epoch n is just finalized, but start_slot(n) is skipped. There are two state roots we might in the finalized_checkpoint SSE event:

The state root of the finalized block, which is get_block(finalized_checkpoint.root).state_root.
The state root at slot of start_slot(n), which would be the state from (1), but "skipped forward" through any skip slots.

Previously, Lighthouse would choose (2). However, we can see that when Teku generates that event it uses getStateRootFromBlockRoot which uses (1).

I have switched Lighthouse from (2) to (1). I think it's a somewhat arbitrary choice between the two, where (1) is easier to compute and is consistent with Teku.

Notes for Reviewers

I've renamed BeaconChain::fork_choice to BeaconChain::recompute_head. Doing this helped ensure I broke all previous uses of fork choice and I also find it more descriptive. It describes an action and can't be confused with trying to get a reference to the ForkChoice struct.

I've changed the ordering of SSE events when a block is received. It used to be [block, finalized, head] and now it's [block, head, finalized]. It was easier this way and I don't think we were making any promises about SSE event ordering so it's not "breaking".

I've made it so fork choice will run when it's first constructed. I did this because I wanted to have a cached version of the last call to get_head. Ensuring get_head has been run at least once means that the cached values doesn't need to wrapped in an Option. This was fairly simple, it just involved passing a slot to the constructor so it knows when it's being run. When loading a fork choice from the store and a slot clock isn't handy I've just used the slot that was saved in the fork_choice_store. That seems like it would be a faithful representation of the slot when we saved it.

I added the genesis_time: u64 to the BeaconChain. It's small, constant and nice to have around.

Since we're using FC for the fin/just checkpoints, we no longer get the 0x00..00 roots at genesis. You can see I had to remove a work-around in ef-tests here: b56be3b. I can't find any reason why this would be an issue, if anything I think it'll be better since the genesis-alias has caught us out a few times (0x00..00 isn't actually a real root). Edit: I did find a case where the network expected the 0x00..00 alias and patched it here: 3f26ac3.

You'll notice a lot of changes in tests. Generally, tests should be functionally equivalent. Here are the things creating the most diff-noise in tests:

Changing tests to be tokio::async tests.
Adding .await to fork choice, block processing and block production functions.
Refactor of the canonical_head "API" provided by the BeaconChain. E.g., chain.canonical_head.cached_head() instead of chain.canonical_head.read().
Wrapping SignedBeaconBlock in an Arc.
In the beacon_chain/tests/block_verification, we can't use the lazy_static CHAIN_SEGMENT variable anymore since it's generated with an async function. We just generate it in each test, not so efficient but hopefully insignificant.

I had to disable rayon concurrent tests in the fork_choice tests. This is because the use of rayon and block_on was causing a panic.

paulhauner · 2022-06-07T07:13:20Z

beacon_node/beacon_chain/src/builder.rs

-                return Err(format!(
-                    "Database corrupt: fork choice is finalized at {:?} whilst head is finalized at \
+        let head_finalized = head_snapshot.beacon_state.finalized_checkpoint();
+        if fc_finalized.epoch < head_finalized.epoch {


Reviewers note: the uncoupling of just/fin from the canonical head means that we can't be as strict here.

winksaville · 2022-06-09T20:19:34Z

Such a large change at this time makes me very nervous. It currently modifies 95 files in 26 commits, in my opinion those numbers are way to high for a single PR. At a minimum there should be two PR's one for each the two "primary goals", but I'd contend there should probably more. I would say it somewhere between 2 and 26 and obviously all tests should succeed on each PR.

Another suggestion, before doing this massive of change I believe it would be prudent to determine what the current code coverage metric is and make it a goal to not decrease the code coverage with each PR.

Finally, have there been discussions on what alternative solutions are available and are both primary goals absolutely necessary now?

paulhauner · 2022-06-10T00:55:17Z

Such a large change at this time makes me very nervous.

I can see why, I also don't like big changes before the merge. However, the code without this PR also makes me nervous. I won't go into detail here, but this PR addresses the two largest concerns I have with Lighthouse and the merge.

Hopefully I can address your concerns and make you feel less nervous. We're talking about the subjective topic of how best to manage code changes, so even if you don't end up agreeing with me I hope that I can at least show that we chose this path with much deliberation and walk it with caution.

It currently modifies 95 files in 26 commits, in my opinion those numbers are way to high for a single PR.

I generally agree. I don't like the "mega-PRs" and would much prefer smaller, segmented PRs. This is usually how we work, but like any good rule it has exceptions.

There sometimes arises a "choose one" problem between:

Having an unstable branch with all tests passing.
Breaking a PR up into small chunks.

As an example, consider turning the BeaconChain::fork_choice function into an async function by simply adding async in front of fn fork_choice. Firstly, that's going to affect all tests which call fork choice, which is a lot of tests. Just from that we're going to have to touch ~80-90 files to change #[test] to #[tokio::test] and fn my_test to async fn my_test.

Given that those 80-90 changes are trivial to review, I would say it's perfectly fine (and necessary) to create a PR that touches 80-90 files. In fact, if you look through this PR you'll find a significant number of changes are just that, making sync tests async.

Now, even if we do that, the tests are going to start failing because the block processing and block production functions use block_on and will be called from an async context and therefore panic. Now, at this point we must choose between:

Allowing the tests to fail.
Hacking the tests so that all calls to BeaconChain::fork_choice are wrapped in a way that avoids panics.
Modifying block production and processing to be async as well.

(1) means that we can't have tests passing on unstable. This means that all other PRs after that PR aren't getting the full test-suite and might be quietly introducing bugs. (2) is doing "throw away" work, it's work that needs to be reviewed thoroughly to ensure it's not introducing bugs, but then needs to be thrown away once we make block production and processing async (as we intend to do). Something else to note about (2) is that it significantly increases the total amount of lines of code to be reviewed. So, given that (1) and (2) have distinct downsides, I've gone ahead and attempted to do (3) in a minimal way that produces a diff that is as small-as-possible.

I think it's important to understand that the vast majority of lines changes and files touched are just non-functional changes to tests (e.g., adding .await to functions or getting info about the head from a different API). The changes to fork choice are substantial, but the changes to block processing and production should be non-substantial and I've tried to make them in a way that produces the cleanest and easiest-to-reason-about diff.

At a minimum there should be two PR's one for each the two "primary goals"

The problem here is that in order to refactor BeaconChain::fork_choice to make it ideal (in my view) I believe it needs to be async. So, I could refactor it in a less-than-ideal way so that it's not async, then come back for another refactor to make it async (which, as I've discussed earlier, is either going to be very large or break tests in unstable). Once again I've broken the change into smaller segments but I've increased the total amount of code that needs to be reviewed.

I believe it would be prudent to determine what the current code coverage metric is and make it a goal to not decrease the code coverage with each PR.

Good idea, I'll run some coverage metrics. I don't take coverage as gospel though. Let's consider this PR, it consists of three components:

Refactoring fork choice to use the justified/finalized checkpoints of fork choice rather than the head state.
Wrapping block processing and block production in async blocks, without changing their core functionality.
Doing tedious one-line changes to all the tests to make them async and to read the justified/finalized checkpoints from a different source.

The vast majority of these changes per "files touched" and "lines changed" is from (3). So, a huge amount of these changes are trivial changes to tests that have nothing to do with code coverage (tests don't cover tests).

For (2), we're adding non-functional wrapping code. The underlying code (under the "wrapping") is already extensively tested. I can't write tests for the wrapping code because it's supposed to be transparent (i.e., has no observable effect on the output of the function) and this can only be confirmed by manual review and the existing test suite. These non-functional wrappers create lines of code that is not able to be tested, so code coverage must decrease.

Finally, have there been discussions on what alternative solutions are available and are both primary goals absolutely necessary now?

Yep, absolutely. I've had a one-on-one meeting with another senior developer regarding this. I've also raised it the rest of the team several times at different internal meetings and I believe the entire core team is across this and my reasoning for it.

As always, everyone is cautious about large changes but I'm not aware of anyone on the core team that opposes this.

## Description Add a new lint to CI that attempts to detect calls to functions like `block_on` from async execution contexts. This lint was written from scratch exactly for this purpose, on my fork of Clippy: https://github.com/michaelsproul/rust-clippy/tree/disallow-from-async ## Additional Info - I've successfully detected the previous two issues we had with `block_on` by running the linter on the commits prior to each of these PRs: #3165, #3199. - The lint runs on CI with `continue-on-error: true` so that if it fails spuriously it won't block CI. - I think it would be good to merge this PR before #3244 so that we can lint the extensive executor-related changes in that PR. - I aim to upstream the lint to Clippy, at which point building a custom version of Clippy from my fork will no longer be necessary. I imagine this will take several weeks or months though, because the code is currently a bit hacky and will need some renovations to pass review.

paulhauner · 2022-06-20T23:50:27Z

I'm moving some text from the description to this comment. I feel it was crowding the description, but I wanted to keep it for reference. Text is below.

Why have you done this to us?

One might wonder if we need a huge refactor just before the merge. That's for you to decide, but here are my motivations.

Firstly, I think it is well understood that it's necessary to perform the just/fin refactor (1) before the merge. This change required an extensive refactor of the BeaconChain::fork_choice function, both internally and externally. The function had to be structurally changed in order for finalization to advance without the head changing. Furthermore, all downstream functions had to be changed to read the fin/just information from fork choice, rather than from the canonical_head. This posed an immediate problem where the beacon_chain.canonical_head and beacon_chain.fork_choice were behind separate RwLocks, so atomic reads were not possible without interleaving locks and risking deadlocks. Given that the optimistic status of blocks is stored in fork_choice, it became difficult to atomically ask the question "what is the head and is it optimistic?". My solution is to put the "canonical head" and "fork choice" behind the same RwLock, which is implemented here.

So, I have made the point for an extensive refactor of fork choice. Now, if I'm refactoring fork choice and I need to touch everything that calls fork choice, why not also switch fork choice to be async? We've been having difficulties with panics (e.g. #3165) arising from our use of block_on in an async context. The type system does not yet protect us from this (yet) and it clearly prefers that we would make functions that do async things officially async. So, why not make our fork choice function async whilst I'm at it.

Now, I hope I have a case for refactoring fork choice and making it async. This required a horrific refactor of all tests that produce blocks to make them async. Once this was done, I found that any test that processed or produced a block would panic those functions use block_on. So, the solutions are to (a) give up and cede ground, (b) wrap all those functions in spawn_blocking tasks or (c) refactor block production/processing to be async too. I chose (c). It was becoming clear that the use of block_on is an anti-pattern; the compiler does not protect us against it panics arising from it and it is practically impossible to program defensively against. It's also worth noting that #3070 was running into these same issues with mixing of async/sync code (the http_api uses async tests) and this PR will avoid a whole lot of ugly code in that PR.

All that said, I hope the reader can see why I have chosen to undertake such a big refactor at this point in time. The calls to an external EL is what put us in this mess, which is a merge-specific thing. I think we're in the scenario where we can make this change and still see it run on several testnets before it's time to merge. I also think that after this PR we're ultimately safer, since we're back to programming within the protective fortress of the compiler.

realbigsean

Started reviewing today, only really got through canonical_head.rs so far. The new structure is making sense to me, but need to think about it a bit more in the context of all the changes. Looking really good so far!

beacon_node/beacon_chain/src/canonical_head.rs

realbigsean

Made another dent in review here, think I've gotten though the meat of the changes at this point. I'll need another day or so to get through the rest

beacon_node/beacon_chain/src/beacon_chain.rs

beacon_node/beacon_chain/src/beacon_proposer_cache.rs

beacon_node/beacon_chain/src/beacon_chain.rs

beacon_node/beacon_chain/src/block_verification.rs

paulhauner · 2022-06-28T00:24:46Z

I've just rebased onto unstable.

paulhauner · 2022-06-28T06:49:00Z

Latest set of comments addressed, thanks!

beacon_node/beacon_chain/src/beacon_chain.rs

realbigsean

Ok, finished the initial review!

beacon_node/client/src/builder.rs

beacon_node/network/src/beacon_processor/worker/gossip_methods.rs

beacon_node/network/src/beacon_processor/worker/sync_methods.rs

common/task_executor/src/lib.rs

paulhauner · 2022-07-01T06:48:34Z

All comments addressed! Thanks @michaelsproul and @ethDreamer for this round!

I've had a few tests fail after rebasing onto unstable. I'll let the tests run and address any failures.

I think once those tests pass and I have ✔️ from @realbigsean and @ethDreamer then we're in a good place to merge. I'm currently running this on Prater and Ropsten.

realbigsean

🎉

paulhauner · 2022-07-02T05:46:03Z

Thanks again everyone 🙏 Let's get this done!

bors r+

## Overview This rather extensive PR achieves two primary goals: 1. Uses the finalized/justified checkpoints of fork choice (FC), rather than that of the head state. 2. Refactors fork choice, block production and block processing to `async` functions. Additionally, it achieves: - Concurrent forkchoice updates to the EL and cache pruning after a new head is selected. - Concurrent "block packing" (attestations, etc) and execution payload retrieval during block production. - Concurrent per-block-processing and execution payload verification during block processing. - The `Arc`-ification of `SignedBeaconBlock` during block processing (it's never mutated, so why not?): - I had to do this to deal with sending blocks into spawned tasks. - Previously we were cloning the beacon block at least 2 times during each block processing, these clones are either removed or turned into cheaper `Arc` clones. - We were also `Box`-ing and un-`Box`-ing beacon blocks as they moved throughout the networking crate. This is not a big deal, but it's nice to avoid shifting things between the stack and heap. - Avoids cloning *all the blocks* in *every chain segment* during sync. - It also has the potential to clean up our code where we need to pass an *owned* block around so we can send it back in the case of an error (I didn't do much of this, my PR is already big enough 😅) - The `BeaconChain::HeadSafetyStatus` struct was removed. It was an old relic from prior merge specs. For motivation for this change, see #3244 (comment) ## Changes to `canonical_head` and `fork_choice` Previously, the `BeaconChain` had two separate fields: ``` canonical_head: RwLock<Snapshot>, fork_choice: RwLock<BeaconForkChoice> ``` Now, we have grouped these values under a single struct: ``` canonical_head: CanonicalHead { cached_head: RwLock<Arc<Snapshot>>, fork_choice: RwLock<BeaconForkChoice> } ``` Apart from ergonomics, the only *actual* change here is wrapping the canonical head snapshot in an `Arc`. This means that we no longer need to hold the `cached_head` (`canonical_head`, in old terms) lock when we want to pull some values from it. This was done to avoid deadlock risks by preventing functions from acquiring (and holding) the `cached_head` and `fork_choice` locks simultaneously. ## Breaking Changes ### The `state` (root) field in the `finalized_checkpoint` SSE event Consider the scenario where epoch `n` is just finalized, but `start_slot(n)` is skipped. There are two state roots we might in the `finalized_checkpoint` SSE event: 1. The state root of the finalized block, which is `get_block(finalized_checkpoint.root).state_root`. 4. The state root at slot of `start_slot(n)`, which would be the state from (1), but "skipped forward" through any skip slots. Previously, Lighthouse would choose (2). However, we can see that when [Teku generates that event](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/events/EventSubscriptionManager.java#L171-L182) it uses [`getStateRootFromBlockRoot`](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/provider/src/main/java/tech/pegasys/teku/api/ChainDataProvider.java#L336-L341) which uses (1). I have switched Lighthouse from (2) to (1). I think it's a somewhat arbitrary choice between the two, where (1) is easier to compute and is consistent with Teku. ## Notes for Reviewers I've renamed `BeaconChain::fork_choice` to `BeaconChain::recompute_head`. Doing this helped ensure I broke all previous uses of fork choice and I also find it more descriptive. It describes an action and can't be confused with trying to get a reference to the `ForkChoice` struct. I've changed the ordering of SSE events when a block is received. It used to be `[block, finalized, head]` and now it's `[block, head, finalized]`. It was easier this way and I don't think we were making any promises about SSE event ordering so it's not "breaking". I've made it so fork choice will run when it's first constructed. I did this because I wanted to have a cached version of the last call to `get_head`. Ensuring `get_head` has been run *at least once* means that the cached values doesn't need to wrapped in an `Option`. This was fairly simple, it just involved passing a `slot` to the constructor so it knows *when* it's being run. When loading a fork choice from the store and a slot clock isn't handy I've just used the `slot` that was saved in the `fork_choice_store`. That seems like it would be a faithful representation of the slot when we saved it. I added the `genesis_time: u64` to the `BeaconChain`. It's small, constant and nice to have around. Since we're using FC for the fin/just checkpoints, we no longer get the `0x00..00` roots at genesis. You can see I had to remove a work-around in `ef-tests` here: b56be3b. I can't find any reason why this would be an issue, if anything I think it'll be better since the genesis-alias has caught us out a few times (0x00..00 isn't actually a real root). Edit: I did find a case where the `network` expected the 0x00..00 alias and patched it here: 3f26ac3. You'll notice a lot of changes in tests. Generally, tests should be functionally equivalent. Here are the things creating the most diff-noise in tests: - Changing tests to be `tokio::async` tests. - Adding `.await` to fork choice, block processing and block production functions. - Refactor of the `canonical_head` "API" provided by the `BeaconChain`. E.g., `chain.canonical_head.cached_head()` instead of `chain.canonical_head.read()`. - Wrapping `SignedBeaconBlock` in an `Arc`. - In the `beacon_chain/tests/block_verification`, we can't use the `lazy_static` `CHAIN_SEGMENT` variable anymore since it's generated with an async function. We just generate it in each test, not so efficient but hopefully insignificant. I had to disable `rayon` concurrent tests in the `fork_choice` tests. This is because the use of `rayon` and `block_on` was causing a panic. Co-authored-by: Mac L <mjladson@pm.me>

bors · 2022-07-02T06:06:25Z

Build failed:

execution-engine-integration-ubuntu

paulhauner · 2022-07-02T09:20:41Z

Hmm, lets try again. It might have been a race condition.

bors r+

## Overview This rather extensive PR achieves two primary goals: 1. Uses the finalized/justified checkpoints of fork choice (FC), rather than that of the head state. 2. Refactors fork choice, block production and block processing to `async` functions. Additionally, it achieves: - Concurrent forkchoice updates to the EL and cache pruning after a new head is selected. - Concurrent "block packing" (attestations, etc) and execution payload retrieval during block production. - Concurrent per-block-processing and execution payload verification during block processing. - The `Arc`-ification of `SignedBeaconBlock` during block processing (it's never mutated, so why not?): - I had to do this to deal with sending blocks into spawned tasks. - Previously we were cloning the beacon block at least 2 times during each block processing, these clones are either removed or turned into cheaper `Arc` clones. - We were also `Box`-ing and un-`Box`-ing beacon blocks as they moved throughout the networking crate. This is not a big deal, but it's nice to avoid shifting things between the stack and heap. - Avoids cloning *all the blocks* in *every chain segment* during sync. - It also has the potential to clean up our code where we need to pass an *owned* block around so we can send it back in the case of an error (I didn't do much of this, my PR is already big enough 😅) - The `BeaconChain::HeadSafetyStatus` struct was removed. It was an old relic from prior merge specs. For motivation for this change, see #3244 (comment) ## Changes to `canonical_head` and `fork_choice` Previously, the `BeaconChain` had two separate fields: ``` canonical_head: RwLock<Snapshot>, fork_choice: RwLock<BeaconForkChoice> ``` Now, we have grouped these values under a single struct: ``` canonical_head: CanonicalHead { cached_head: RwLock<Arc<Snapshot>>, fork_choice: RwLock<BeaconForkChoice> } ``` Apart from ergonomics, the only *actual* change here is wrapping the canonical head snapshot in an `Arc`. This means that we no longer need to hold the `cached_head` (`canonical_head`, in old terms) lock when we want to pull some values from it. This was done to avoid deadlock risks by preventing functions from acquiring (and holding) the `cached_head` and `fork_choice` locks simultaneously. ## Breaking Changes ### The `state` (root) field in the `finalized_checkpoint` SSE event Consider the scenario where epoch `n` is just finalized, but `start_slot(n)` is skipped. There are two state roots we might in the `finalized_checkpoint` SSE event: 1. The state root of the finalized block, which is `get_block(finalized_checkpoint.root).state_root`. 4. The state root at slot of `start_slot(n)`, which would be the state from (1), but "skipped forward" through any skip slots. Previously, Lighthouse would choose (2). However, we can see that when [Teku generates that event](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/events/EventSubscriptionManager.java#L171-L182) it uses [`getStateRootFromBlockRoot`](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/provider/src/main/java/tech/pegasys/teku/api/ChainDataProvider.java#L336-L341) which uses (1). I have switched Lighthouse from (2) to (1). I think it's a somewhat arbitrary choice between the two, where (1) is easier to compute and is consistent with Teku. ## Notes for Reviewers I've renamed `BeaconChain::fork_choice` to `BeaconChain::recompute_head`. Doing this helped ensure I broke all previous uses of fork choice and I also find it more descriptive. It describes an action and can't be confused with trying to get a reference to the `ForkChoice` struct. I've changed the ordering of SSE events when a block is received. It used to be `[block, finalized, head]` and now it's `[block, head, finalized]`. It was easier this way and I don't think we were making any promises about SSE event ordering so it's not "breaking". I've made it so fork choice will run when it's first constructed. I did this because I wanted to have a cached version of the last call to `get_head`. Ensuring `get_head` has been run *at least once* means that the cached values doesn't need to wrapped in an `Option`. This was fairly simple, it just involved passing a `slot` to the constructor so it knows *when* it's being run. When loading a fork choice from the store and a slot clock isn't handy I've just used the `slot` that was saved in the `fork_choice_store`. That seems like it would be a faithful representation of the slot when we saved it. I added the `genesis_time: u64` to the `BeaconChain`. It's small, constant and nice to have around. Since we're using FC for the fin/just checkpoints, we no longer get the `0x00..00` roots at genesis. You can see I had to remove a work-around in `ef-tests` here: b56be3b. I can't find any reason why this would be an issue, if anything I think it'll be better since the genesis-alias has caught us out a few times (0x00..00 isn't actually a real root). Edit: I did find a case where the `network` expected the 0x00..00 alias and patched it here: 3f26ac3. You'll notice a lot of changes in tests. Generally, tests should be functionally equivalent. Here are the things creating the most diff-noise in tests: - Changing tests to be `tokio::async` tests. - Adding `.await` to fork choice, block processing and block production functions. - Refactor of the `canonical_head` "API" provided by the `BeaconChain`. E.g., `chain.canonical_head.cached_head()` instead of `chain.canonical_head.read()`. - Wrapping `SignedBeaconBlock` in an `Arc`. - In the `beacon_chain/tests/block_verification`, we can't use the `lazy_static` `CHAIN_SEGMENT` variable anymore since it's generated with an async function. We just generate it in each test, not so efficient but hopefully insignificant. I had to disable `rayon` concurrent tests in the `fork_choice` tests. This is because the use of `rayon` and `block_on` was causing a panic. Co-authored-by: Mac L <mjladson@pm.me>

bors · 2022-07-02T09:41:02Z

Build failed:

execution-engine-integration-ubuntu

michaelsproul · 2022-07-03T00:00:05Z

The integration test breakage is fixed by #3303, which I'll batch with this PR now

bors r+

## Overview This rather extensive PR achieves two primary goals: 1. Uses the finalized/justified checkpoints of fork choice (FC), rather than that of the head state. 2. Refactors fork choice, block production and block processing to `async` functions. Additionally, it achieves: - Concurrent forkchoice updates to the EL and cache pruning after a new head is selected. - Concurrent "block packing" (attestations, etc) and execution payload retrieval during block production. - Concurrent per-block-processing and execution payload verification during block processing. - The `Arc`-ification of `SignedBeaconBlock` during block processing (it's never mutated, so why not?): - I had to do this to deal with sending blocks into spawned tasks. - Previously we were cloning the beacon block at least 2 times during each block processing, these clones are either removed or turned into cheaper `Arc` clones. - We were also `Box`-ing and un-`Box`-ing beacon blocks as they moved throughout the networking crate. This is not a big deal, but it's nice to avoid shifting things between the stack and heap. - Avoids cloning *all the blocks* in *every chain segment* during sync. - It also has the potential to clean up our code where we need to pass an *owned* block around so we can send it back in the case of an error (I didn't do much of this, my PR is already big enough 😅) - The `BeaconChain::HeadSafetyStatus` struct was removed. It was an old relic from prior merge specs. For motivation for this change, see #3244 (comment) ## Changes to `canonical_head` and `fork_choice` Previously, the `BeaconChain` had two separate fields: ``` canonical_head: RwLock<Snapshot>, fork_choice: RwLock<BeaconForkChoice> ``` Now, we have grouped these values under a single struct: ``` canonical_head: CanonicalHead { cached_head: RwLock<Arc<Snapshot>>, fork_choice: RwLock<BeaconForkChoice> } ``` Apart from ergonomics, the only *actual* change here is wrapping the canonical head snapshot in an `Arc`. This means that we no longer need to hold the `cached_head` (`canonical_head`, in old terms) lock when we want to pull some values from it. This was done to avoid deadlock risks by preventing functions from acquiring (and holding) the `cached_head` and `fork_choice` locks simultaneously. ## Breaking Changes ### The `state` (root) field in the `finalized_checkpoint` SSE event Consider the scenario where epoch `n` is just finalized, but `start_slot(n)` is skipped. There are two state roots we might in the `finalized_checkpoint` SSE event: 1. The state root of the finalized block, which is `get_block(finalized_checkpoint.root).state_root`. 4. The state root at slot of `start_slot(n)`, which would be the state from (1), but "skipped forward" through any skip slots. Previously, Lighthouse would choose (2). However, we can see that when [Teku generates that event](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/events/EventSubscriptionManager.java#L171-L182) it uses [`getStateRootFromBlockRoot`](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/provider/src/main/java/tech/pegasys/teku/api/ChainDataProvider.java#L336-L341) which uses (1). I have switched Lighthouse from (2) to (1). I think it's a somewhat arbitrary choice between the two, where (1) is easier to compute and is consistent with Teku. ## Notes for Reviewers I've renamed `BeaconChain::fork_choice` to `BeaconChain::recompute_head`. Doing this helped ensure I broke all previous uses of fork choice and I also find it more descriptive. It describes an action and can't be confused with trying to get a reference to the `ForkChoice` struct. I've changed the ordering of SSE events when a block is received. It used to be `[block, finalized, head]` and now it's `[block, head, finalized]`. It was easier this way and I don't think we were making any promises about SSE event ordering so it's not "breaking". I've made it so fork choice will run when it's first constructed. I did this because I wanted to have a cached version of the last call to `get_head`. Ensuring `get_head` has been run *at least once* means that the cached values doesn't need to wrapped in an `Option`. This was fairly simple, it just involved passing a `slot` to the constructor so it knows *when* it's being run. When loading a fork choice from the store and a slot clock isn't handy I've just used the `slot` that was saved in the `fork_choice_store`. That seems like it would be a faithful representation of the slot when we saved it. I added the `genesis_time: u64` to the `BeaconChain`. It's small, constant and nice to have around. Since we're using FC for the fin/just checkpoints, we no longer get the `0x00..00` roots at genesis. You can see I had to remove a work-around in `ef-tests` here: b56be3b. I can't find any reason why this would be an issue, if anything I think it'll be better since the genesis-alias has caught us out a few times (0x00..00 isn't actually a real root). Edit: I did find a case where the `network` expected the 0x00..00 alias and patched it here: 3f26ac3. You'll notice a lot of changes in tests. Generally, tests should be functionally equivalent. Here are the things creating the most diff-noise in tests: - Changing tests to be `tokio::async` tests. - Adding `.await` to fork choice, block processing and block production functions. - Refactor of the `canonical_head` "API" provided by the `BeaconChain`. E.g., `chain.canonical_head.cached_head()` instead of `chain.canonical_head.read()`. - Wrapping `SignedBeaconBlock` in an `Arc`. - In the `beacon_chain/tests/block_verification`, we can't use the `lazy_static` `CHAIN_SEGMENT` variable anymore since it's generated with an async function. We just generate it in each test, not so efficient but hopefully insignificant. I had to disable `rayon` concurrent tests in the `fork_choice` tests. This is because the use of `rayon` and `block_on` was causing a panic. Co-authored-by: Mac L <mjladson@pm.me>

bors · 2022-07-03T00:48:06Z

This PR was included in a batch that was canceled, it will be automatically retried

michaelsproul · 2022-07-03T00:59:27Z

bors r-

(for now)

bors · 2022-07-03T00:59:28Z

Canceled.

divagant-martian · 2022-07-03T04:23:32Z

bors r+

## Overview This rather extensive PR achieves two primary goals: 1. Uses the finalized/justified checkpoints of fork choice (FC), rather than that of the head state. 2. Refactors fork choice, block production and block processing to `async` functions. Additionally, it achieves: - Concurrent forkchoice updates to the EL and cache pruning after a new head is selected. - Concurrent "block packing" (attestations, etc) and execution payload retrieval during block production. - Concurrent per-block-processing and execution payload verification during block processing. - The `Arc`-ification of `SignedBeaconBlock` during block processing (it's never mutated, so why not?): - I had to do this to deal with sending blocks into spawned tasks. - Previously we were cloning the beacon block at least 2 times during each block processing, these clones are either removed or turned into cheaper `Arc` clones. - We were also `Box`-ing and un-`Box`-ing beacon blocks as they moved throughout the networking crate. This is not a big deal, but it's nice to avoid shifting things between the stack and heap. - Avoids cloning *all the blocks* in *every chain segment* during sync. - It also has the potential to clean up our code where we need to pass an *owned* block around so we can send it back in the case of an error (I didn't do much of this, my PR is already big enough 😅) - The `BeaconChain::HeadSafetyStatus` struct was removed. It was an old relic from prior merge specs. For motivation for this change, see #3244 (comment) ## Changes to `canonical_head` and `fork_choice` Previously, the `BeaconChain` had two separate fields: ``` canonical_head: RwLock<Snapshot>, fork_choice: RwLock<BeaconForkChoice> ``` Now, we have grouped these values under a single struct: ``` canonical_head: CanonicalHead { cached_head: RwLock<Arc<Snapshot>>, fork_choice: RwLock<BeaconForkChoice> } ``` Apart from ergonomics, the only *actual* change here is wrapping the canonical head snapshot in an `Arc`. This means that we no longer need to hold the `cached_head` (`canonical_head`, in old terms) lock when we want to pull some values from it. This was done to avoid deadlock risks by preventing functions from acquiring (and holding) the `cached_head` and `fork_choice` locks simultaneously. ## Breaking Changes ### The `state` (root) field in the `finalized_checkpoint` SSE event Consider the scenario where epoch `n` is just finalized, but `start_slot(n)` is skipped. There are two state roots we might in the `finalized_checkpoint` SSE event: 1. The state root of the finalized block, which is `get_block(finalized_checkpoint.root).state_root`. 4. The state root at slot of `start_slot(n)`, which would be the state from (1), but "skipped forward" through any skip slots. Previously, Lighthouse would choose (2). However, we can see that when [Teku generates that event](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/events/EventSubscriptionManager.java#L171-L182) it uses [`getStateRootFromBlockRoot`](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/provider/src/main/java/tech/pegasys/teku/api/ChainDataProvider.java#L336-L341) which uses (1). I have switched Lighthouse from (2) to (1). I think it's a somewhat arbitrary choice between the two, where (1) is easier to compute and is consistent with Teku. ## Notes for Reviewers I've renamed `BeaconChain::fork_choice` to `BeaconChain::recompute_head`. Doing this helped ensure I broke all previous uses of fork choice and I also find it more descriptive. It describes an action and can't be confused with trying to get a reference to the `ForkChoice` struct. I've changed the ordering of SSE events when a block is received. It used to be `[block, finalized, head]` and now it's `[block, head, finalized]`. It was easier this way and I don't think we were making any promises about SSE event ordering so it's not "breaking". I've made it so fork choice will run when it's first constructed. I did this because I wanted to have a cached version of the last call to `get_head`. Ensuring `get_head` has been run *at least once* means that the cached values doesn't need to wrapped in an `Option`. This was fairly simple, it just involved passing a `slot` to the constructor so it knows *when* it's being run. When loading a fork choice from the store and a slot clock isn't handy I've just used the `slot` that was saved in the `fork_choice_store`. That seems like it would be a faithful representation of the slot when we saved it. I added the `genesis_time: u64` to the `BeaconChain`. It's small, constant and nice to have around. Since we're using FC for the fin/just checkpoints, we no longer get the `0x00..00` roots at genesis. You can see I had to remove a work-around in `ef-tests` here: b56be3b. I can't find any reason why this would be an issue, if anything I think it'll be better since the genesis-alias has caught us out a few times (0x00..00 isn't actually a real root). Edit: I did find a case where the `network` expected the 0x00..00 alias and patched it here: 3f26ac3. You'll notice a lot of changes in tests. Generally, tests should be functionally equivalent. Here are the things creating the most diff-noise in tests: - Changing tests to be `tokio::async` tests. - Adding `.await` to fork choice, block processing and block production functions. - Refactor of the `canonical_head` "API" provided by the `BeaconChain`. E.g., `chain.canonical_head.cached_head()` instead of `chain.canonical_head.read()`. - Wrapping `SignedBeaconBlock` in an `Arc`. - In the `beacon_chain/tests/block_verification`, we can't use the `lazy_static` `CHAIN_SEGMENT` variable anymore since it's generated with an async function. We just generate it in each test, not so efficient but hopefully insignificant. I had to disable `rayon` concurrent tests in the `fork_choice` tests. This is because the use of `rayon` and `block_on` was causing a panic. Co-authored-by: Mac L <mjladson@pm.me>

michaelsproul · 2022-07-03T05:35:47Z

bors r-

bors · 2022-07-03T05:35:49Z

Canceled.

michaelsproul · 2022-07-03T05:36:35Z

bors r+

## Overview This rather extensive PR achieves two primary goals: 1. Uses the finalized/justified checkpoints of fork choice (FC), rather than that of the head state. 2. Refactors fork choice, block production and block processing to `async` functions. Additionally, it achieves: - Concurrent forkchoice updates to the EL and cache pruning after a new head is selected. - Concurrent "block packing" (attestations, etc) and execution payload retrieval during block production. - Concurrent per-block-processing and execution payload verification during block processing. - The `Arc`-ification of `SignedBeaconBlock` during block processing (it's never mutated, so why not?): - I had to do this to deal with sending blocks into spawned tasks. - Previously we were cloning the beacon block at least 2 times during each block processing, these clones are either removed or turned into cheaper `Arc` clones. - We were also `Box`-ing and un-`Box`-ing beacon blocks as they moved throughout the networking crate. This is not a big deal, but it's nice to avoid shifting things between the stack and heap. - Avoids cloning *all the blocks* in *every chain segment* during sync. - It also has the potential to clean up our code where we need to pass an *owned* block around so we can send it back in the case of an error (I didn't do much of this, my PR is already big enough 😅) - The `BeaconChain::HeadSafetyStatus` struct was removed. It was an old relic from prior merge specs. For motivation for this change, see #3244 (comment) ## Changes to `canonical_head` and `fork_choice` Previously, the `BeaconChain` had two separate fields: ``` canonical_head: RwLock<Snapshot>, fork_choice: RwLock<BeaconForkChoice> ``` Now, we have grouped these values under a single struct: ``` canonical_head: CanonicalHead { cached_head: RwLock<Arc<Snapshot>>, fork_choice: RwLock<BeaconForkChoice> } ``` Apart from ergonomics, the only *actual* change here is wrapping the canonical head snapshot in an `Arc`. This means that we no longer need to hold the `cached_head` (`canonical_head`, in old terms) lock when we want to pull some values from it. This was done to avoid deadlock risks by preventing functions from acquiring (and holding) the `cached_head` and `fork_choice` locks simultaneously. ## Breaking Changes ### The `state` (root) field in the `finalized_checkpoint` SSE event Consider the scenario where epoch `n` is just finalized, but `start_slot(n)` is skipped. There are two state roots we might in the `finalized_checkpoint` SSE event: 1. The state root of the finalized block, which is `get_block(finalized_checkpoint.root).state_root`. 4. The state root at slot of `start_slot(n)`, which would be the state from (1), but "skipped forward" through any skip slots. Previously, Lighthouse would choose (2). However, we can see that when [Teku generates that event](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/events/EventSubscriptionManager.java#L171-L182) it uses [`getStateRootFromBlockRoot`](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/provider/src/main/java/tech/pegasys/teku/api/ChainDataProvider.java#L336-L341) which uses (1). I have switched Lighthouse from (2) to (1). I think it's a somewhat arbitrary choice between the two, where (1) is easier to compute and is consistent with Teku. ## Notes for Reviewers I've renamed `BeaconChain::fork_choice` to `BeaconChain::recompute_head`. Doing this helped ensure I broke all previous uses of fork choice and I also find it more descriptive. It describes an action and can't be confused with trying to get a reference to the `ForkChoice` struct. I've changed the ordering of SSE events when a block is received. It used to be `[block, finalized, head]` and now it's `[block, head, finalized]`. It was easier this way and I don't think we were making any promises about SSE event ordering so it's not "breaking". I've made it so fork choice will run when it's first constructed. I did this because I wanted to have a cached version of the last call to `get_head`. Ensuring `get_head` has been run *at least once* means that the cached values doesn't need to wrapped in an `Option`. This was fairly simple, it just involved passing a `slot` to the constructor so it knows *when* it's being run. When loading a fork choice from the store and a slot clock isn't handy I've just used the `slot` that was saved in the `fork_choice_store`. That seems like it would be a faithful representation of the slot when we saved it. I added the `genesis_time: u64` to the `BeaconChain`. It's small, constant and nice to have around. Since we're using FC for the fin/just checkpoints, we no longer get the `0x00..00` roots at genesis. You can see I had to remove a work-around in `ef-tests` here: b56be3b. I can't find any reason why this would be an issue, if anything I think it'll be better since the genesis-alias has caught us out a few times (0x00..00 isn't actually a real root). Edit: I did find a case where the `network` expected the 0x00..00 alias and patched it here: 3f26ac3. You'll notice a lot of changes in tests. Generally, tests should be functionally equivalent. Here are the things creating the most diff-noise in tests: - Changing tests to be `tokio::async` tests. - Adding `.await` to fork choice, block processing and block production functions. - Refactor of the `canonical_head` "API" provided by the `BeaconChain`. E.g., `chain.canonical_head.cached_head()` instead of `chain.canonical_head.read()`. - Wrapping `SignedBeaconBlock` in an `Arc`. - In the `beacon_chain/tests/block_verification`, we can't use the `lazy_static` `CHAIN_SEGMENT` variable anymore since it's generated with an async function. We just generate it in each test, not so efficient but hopefully insignificant. I had to disable `rayon` concurrent tests in the `fork_choice` tests. This is because the use of `rayon` and `block_on` was causing a panic. Co-authored-by: Mac L <mjladson@pm.me>

bors · 2022-07-03T08:17:43Z

Pull request successfully merged into unstable.

Build succeeded:

michaelsproul · 2022-08-02T07:03:48Z

beacon_node/beacon_chain/src/beacon_chain.rs

-        head_state: &BeaconState<T::EthSpec>,
-        new_finalized_state_root: Hash256,
-    ) -> Result<(), Error> {
-        self.fork_choice.write().prune()?;


@michaelsproul

## Issue Addressed NA ## Proposed Changes There was a regression in #3244 (released in v2.4.0) which stopped pruning fork choice (see [here](#3244 (comment))). This would form a very slow memory leak, using ~100mb per month. The release has been out for ~11 days, so users should not be seeing a dangerous increase in memory, *yet*. Credits to @michaelsproul for noticing this 🎉 ## Additional Info NA

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

paulhauner force-pushed the nfd-2 branch from c1f97da to 4198ac7 Compare June 7, 2022 01:53

paulhauner added the backwards-incompat Backwards-incompatible API change label Jun 7, 2022

paulhauner force-pushed the nfd-2 branch from 3bf2c61 to 8589616 Compare June 7, 2022 06:53

paulhauner commented Jun 7, 2022

View reviewed changes

This was referenced Jun 9, 2022

[Merged by Bors] - Lint against panicky calls in async functions #3250

Closed

HTTP API tests do not work for Bellatrix fork #3251

Closed

paulhauner force-pushed the nfd-2 branch from fcdbc3a to 13e71b3 Compare June 14, 2022 07:43

michaelsproul changed the title ~~Use async code when interfacting with EL~~ Use async code when interacting with EL Jun 14, 2022

realbigsean mentioned this pull request Jun 14, 2022

Remove block_on calls from execution layer #3158

Closed

paulhauner force-pushed the nfd-2 branch from 6e54f29 to d1a65f1 Compare June 16, 2022 08:23

michaelsproul mentioned this pull request Jun 19, 2022

[Merged by Bors] - Add execution_optimistic flag to HTTP responses #3070

Closed

22 tasks

paulhauner force-pushed the nfd-2 branch from ca9be94 to d65d6c9 Compare June 20, 2022 06:54

realbigsean requested changes Jun 22, 2022

View reviewed changes

paulhauner marked this pull request as ready for review June 22, 2022 22:59

paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Jun 23, 2022

paulhauner mentioned this pull request Jun 24, 2022

Remove HeadSafetyStatus #3152

Closed

realbigsean requested changes Jun 27, 2022

View reviewed changes

paulhauner force-pushed the nfd-2 branch from 7f61f36 to 011818f Compare June 28, 2022 00:24

paulhauner requested a review from ethDreamer June 28, 2022 06:49

ethDreamer reviewed Jun 28, 2022

View reviewed changes

beacon_node/beacon_chain/src/beacon_chain.rs Show resolved Hide resolved

realbigsean requested changes Jun 28, 2022

View reviewed changes

paulhauner added 2 commits July 1, 2022 16:31

Spawn fc notifier in blocking thread

cfd9a7f

Fix compile errors in http_api tests

34c7821

realbigsean approved these changes Jul 1, 2022

View reviewed changes

ethDreamer approved these changes Jul 1, 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 Jul 2, 2022

bors bot changed the title ~~Use async code when interacting with EL~~ [Merged by Bors] - Use async code when interacting with EL Jul 3, 2022

bors bot closed this Jul 3, 2022

michaelsproul reviewed Aug 2, 2022

View reviewed changes

paulhauner mentioned this pull request Aug 2, 2022

[Merged by Bors] - Make fork choice prune again #3408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Use async code when interacting with EL #3244

[Merged by Bors] - Use async code when interacting with EL #3244

paulhauner commented Jun 6, 2022 •

edited

Loading

paulhauner Jun 7, 2022

winksaville commented Jun 9, 2022

paulhauner commented Jun 10, 2022

paulhauner commented Jun 20, 2022

realbigsean left a comment

realbigsean left a comment

paulhauner commented Jun 28, 2022

paulhauner commented Jun 28, 2022

realbigsean left a comment

paulhauner commented Jul 1, 2022

realbigsean left a comment

paulhauner commented Jul 2, 2022

bors bot commented Jul 2, 2022

paulhauner commented Jul 2, 2022

bors bot commented Jul 2, 2022

michaelsproul commented Jul 3, 2022

bors bot commented Jul 3, 2022

michaelsproul commented Jul 3, 2022

bors bot commented Jul 3, 2022

divagant-martian commented Jul 3, 2022

michaelsproul commented Jul 3, 2022

bors bot commented Jul 3, 2022

michaelsproul commented Jul 3, 2022

bors bot commented Jul 3, 2022

michaelsproul Aug 2, 2022

[Merged by Bors] - Use async code when interacting with EL #3244

[Merged by Bors] - Use async code when interacting with EL #3244

Conversation

paulhauner commented Jun 6, 2022 • edited Loading

Overview

Changes to canonical_head and fork_choice

Breaking Changes

The state (root) field in the finalized_checkpoint SSE event

Notes for Reviewers

paulhauner Jun 7, 2022

Choose a reason for hiding this comment

winksaville commented Jun 9, 2022

paulhauner commented Jun 10, 2022

paulhauner commented Jun 20, 2022

Why have you done this to us?

realbigsean left a comment

Choose a reason for hiding this comment

realbigsean left a comment

Choose a reason for hiding this comment

paulhauner commented Jun 28, 2022

paulhauner commented Jun 28, 2022

realbigsean left a comment

Choose a reason for hiding this comment

paulhauner commented Jul 1, 2022

realbigsean left a comment

Choose a reason for hiding this comment

paulhauner commented Jul 2, 2022

bors bot commented Jul 2, 2022

paulhauner commented Jul 2, 2022

bors bot commented Jul 2, 2022

michaelsproul commented Jul 3, 2022

bors bot commented Jul 3, 2022

michaelsproul commented Jul 3, 2022

bors bot commented Jul 3, 2022

divagant-martian commented Jul 3, 2022

michaelsproul commented Jul 3, 2022

bors bot commented Jul 3, 2022

michaelsproul commented Jul 3, 2022

bors bot commented Jul 3, 2022

michaelsproul Aug 2, 2022

Choose a reason for hiding this comment

paulhauner commented Jun 6, 2022 •

edited

Loading

Changes to `canonical_head` and `fork_choice`

The `state` (root) field in the `finalized_checkpoint` SSE event