[Merged by Bors] - Move the `BeaconProcessor` into a new crate by paulhauner · Pull Request #4435 · sigp/lighthouse

paulhauner · 2023-06-27T06:40:32Z

Replaces #4434. It is identical, but this PR has a smaller diff due to a curated commit history.

Issue Addressed

NA

Proposed Changes

This PR moves the scheduling logic for the BeaconProcessor into a new crate in beacon_node/beacon_processor. Previously it existed in the beacon_node/network crate.

This addresses a circular-dependency problem where it's not possible to use the BeaconProcessor from the beacon_chain crate. The network crate depends on the beacon_chain crate (network -> beacon_chain), but importing the BeaconProcessor into the beacon_chain crate would create a circular dependancy of beacon_chain -> network.

The BeaconProcessor was designed to provide queuing and prioritized scheduling for messages from the network. It has proven to be quite valuable and I believe we'd make Lighthouse more stable and effective by using it elsewhere. In particular, I think we should use the BeaconProcessor for:

HTTP API requests.
Scheduled tasks in the BeaconChain (e.g., state advance).

Using the BeaconProcessor for these tasks would help prevent the BN from becoming overwhelmed and would also help it to prioritize operations (e.g., choosing to process blocks from gossip before responding to low-priority HTTP API requests).

Additional Info

This PR is intended to have zero impact on runtime behaviour. It aims to simply separate the scheduling code (i.e., the BeaconProcessor) from the business logic in the network crate (i.e., the Worker impls). Future PRs (see #4462) can build upon these works to actually use the BeaconProcessor for more operations.

I've gone to some effort to use git mv to make the diff look more like "file was moved and modified" rather than "file was deleted and a new one added". This should reduce review burden and help maintain commit attribution.

This reverts commit 68fe7df.

paulhauner · 2023-07-05T02:26:46Z

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

All this code is just not needed anymore. It's not really replicated anywhere else.

paulhauner · 2023-07-05T03:04:08Z

beacon_node/network/src/router.rs

+        // generate the message channel
+        let (sync_send, sync_recv) = mpsc::unbounded_channel::<SyncMessage<T::EthSpec>>();
+
+        let network_beacon_processor = NetworkBeaconProcessor {


Since the BeaconProcessor is started elsewhere, we just wrap the beacon_processor_send in a new struct, the NetworkBeaconProcessor which provides convenience functions for sending things to the BeaconProcessor.

paulhauner · 2023-07-05T03:12:01Z

This is now ready for review! 🎉 I've gone through and added a bunch of comments with the intention of helping review.

divagant-martian · 2023-07-05T05:05:39Z

beacon_node/network/src/sync/network_context.rs

        network_globals: Arc<NetworkGlobals<T::EthSpec>>,
-        beacon_processor_send: BeaconProcessorSend<T>,
+        network_beacon_processor: Arc<NetworkBeaconProcessor<T>>,


Now SyncNetworkContext has two Arcs to the NetworkGlobals, so the unwrapped one could be removed.. later maybe?

Thanks @divagant-martian 🙏 Fixed in d2e5f9d

divagant-martian · 2023-07-05T05:07:26Z

beacon_node/network/src/sync/manager.rs

    beacon_chain: Arc<BeaconChain<T>>,
    network_globals: Arc<NetworkGlobals<T::EthSpec>>,
    network_send: mpsc::UnboundedSender<NetworkMessage<T::EthSpec>>,
-    beacon_processor_send: BeaconProcessorSend<T>,
+    beacon_processor: Arc<NetworkBeaconProcessor<T>>,


same with these

Also fixed in d2e5f9d

michaelsproul

Looks great, really appreciated the guiding review comments, thanks!

beacon_node/network/src/metrics.rs

michaelsproul

Niiice, shall we merge this in, then get to reviewing #4462?

paulhauner · 2023-07-10T07:03:01Z

Niiice, shall we merge this in, then get to reviewing #4462?

SGTM! Thanks for the review!

bors r+

*Replaces #4434. It is identical, but this PR has a smaller diff due to a curated commit history.* ## Issue Addressed NA ## Proposed Changes This PR moves the scheduling logic for the `BeaconProcessor` into a new crate in `beacon_node/beacon_processor`. Previously it existed in the `beacon_node/network` crate. This addresses a circular-dependency problem where it's not possible to use the `BeaconProcessor` from the `beacon_chain` crate. The `network` crate depends on the `beacon_chain` crate (`network -> beacon_chain`), but importing the `BeaconProcessor` into the `beacon_chain` crate would create a circular dependancy of `beacon_chain -> network`. The `BeaconProcessor` was designed to provide queuing and prioritized scheduling for messages from the network. It has proven to be quite valuable and I believe we'd make Lighthouse more stable and effective by using it elsewhere. In particular, I think we should use the `BeaconProcessor` for: 1. HTTP API requests. 1. Scheduled tasks in the `BeaconChain` (e.g., state advance). Using the `BeaconProcessor` for these tasks would help prevent the BN from becoming overwhelmed and would also help it to prioritize operations (e.g., choosing to process blocks from gossip before responding to low-priority HTTP API requests). ## Additional Info This PR is intended to have zero impact on runtime behaviour. It aims to simply separate the *scheduling* code (i.e., the `BeaconProcessor`) from the *business logic* in the `network` crate (i.e., the `Worker` impls). Future PRs (see #4462) can build upon these works to actually use the `BeaconProcessor` for more operations. I've gone to some effort to use `git mv` to make the diff look more like "file was moved and modified" rather than "file was deleted and a new one added". This should reduce review burden and help maintain commit attribution.

bors · 2023-07-10T07:27:20Z

Build failed:

release-tests-windows

paulhauner · 2023-07-10T07:45:36Z

bors retry

*Replaces #4434. It is identical, but this PR has a smaller diff due to a curated commit history.* ## Issue Addressed NA ## Proposed Changes This PR moves the scheduling logic for the `BeaconProcessor` into a new crate in `beacon_node/beacon_processor`. Previously it existed in the `beacon_node/network` crate. This addresses a circular-dependency problem where it's not possible to use the `BeaconProcessor` from the `beacon_chain` crate. The `network` crate depends on the `beacon_chain` crate (`network -> beacon_chain`), but importing the `BeaconProcessor` into the `beacon_chain` crate would create a circular dependancy of `beacon_chain -> network`. The `BeaconProcessor` was designed to provide queuing and prioritized scheduling for messages from the network. It has proven to be quite valuable and I believe we'd make Lighthouse more stable and effective by using it elsewhere. In particular, I think we should use the `BeaconProcessor` for: 1. HTTP API requests. 1. Scheduled tasks in the `BeaconChain` (e.g., state advance). Using the `BeaconProcessor` for these tasks would help prevent the BN from becoming overwhelmed and would also help it to prioritize operations (e.g., choosing to process blocks from gossip before responding to low-priority HTTP API requests). ## Additional Info This PR is intended to have zero impact on runtime behaviour. It aims to simply separate the *scheduling* code (i.e., the `BeaconProcessor`) from the *business logic* in the `network` crate (i.e., the `Worker` impls). Future PRs (see #4462) can build upon these works to actually use the `BeaconProcessor` for more operations. I've gone to some effort to use `git mv` to make the diff look more like "file was moved and modified" rather than "file was deleted and a new one added". This should reduce review burden and help maintain commit attribution.

bors · 2023-07-10T10:38:34Z

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

*Replaces sigp#4434. It is identical, but this PR has a smaller diff due to a curated commit history.* NA This PR moves the scheduling logic for the `BeaconProcessor` into a new crate in `beacon_node/beacon_processor`. Previously it existed in the `beacon_node/network` crate. This addresses a circular-dependency problem where it's not possible to use the `BeaconProcessor` from the `beacon_chain` crate. The `network` crate depends on the `beacon_chain` crate (`network -> beacon_chain`), but importing the `BeaconProcessor` into the `beacon_chain` crate would create a circular dependancy of `beacon_chain -> network`. The `BeaconProcessor` was designed to provide queuing and prioritized scheduling for messages from the network. It has proven to be quite valuable and I believe we'd make Lighthouse more stable and effective by using it elsewhere. In particular, I think we should use the `BeaconProcessor` for: 1. HTTP API requests. 1. Scheduled tasks in the `BeaconChain` (e.g., state advance). Using the `BeaconProcessor` for these tasks would help prevent the BN from becoming overwhelmed and would also help it to prioritize operations (e.g., choosing to process blocks from gossip before responding to low-priority HTTP API requests). This PR is intended to have zero impact on runtime behaviour. It aims to simply separate the *scheduling* code (i.e., the `BeaconProcessor`) from the *business logic* in the `network` crate (i.e., the `Worker` impls). Future PRs (see sigp#4462) can build upon these works to actually use the `BeaconProcessor` for more operations. I've gone to some effort to use `git mv` to make the diff look more like "file was moved and modified" rather than "file was deleted and a new one added". This should reduce review burden and help maintain commit attribution.

paulhauner added 15 commits June 27, 2023 15:58

Move BP to NBP dir

f851e05

Add new beacon_processor dir

ac4eb78

Move NBP files to BP crate

01f482d

Move worker files

f484f9f

Remove placeholder file

386ecd7

Add changes to network/sync

45786b9

Add changes to beacon_node/client

308f62a

Add changes to Cargo.lock, Cargo.toml

4f5114a

Add changes to lighthouse_network

341b075

Add changes to beacon_node src.rs

587e130

Add non-BP network files

680033c

Add two Cargo.tomls

b05b96d

Add changes to beacon_processor

f321504

Add changes to network_beacon_processor

1109353

Fix compile errors

2cea3e9

paulhauner added the work-in-progress PR is a work-in-progress label Jun 27, 2023

paulhauner mentioned this pull request Jun 27, 2023

Move the BeaconProcessor into a new crate #4434

Closed

2 tasks

paulhauner added 3 commits June 27, 2023 17:50

Remove unused functions

79b6348

Simplify changes to ClientBuilder

fb42607

Start processing should_process

68fe7df

paulhauner mentioned this pull request Jun 29, 2023

[Merged by Bors] - added debounce to log #4269

Closed

paulhauner added 9 commits June 30, 2023 14:55

Revert "Start processing should_process"

7b6507c

This reverts commit 68fe7df.

Allow for ignoring requeued RPC blocks

3a5aa55

Merge branch 'unstable' into move-bp-3

c59ed9d

Move tests file

341e7fe

Enable tests

c91e1bb

Reduce queue lengths

a7b279e

Create backfill specific events

224b998

Remove process_id from BP

9182538

Avoid modifying ChainSegmentProcessId

26b903c

paulhauner commented Jul 5, 2023

View reviewed changes

Deduplicate metrics

c42bf0d

paulhauner commented Jul 5, 2023

View reviewed changes

paulhauner added ready-for-review The code is ready for review v4.4.1 ETA August 2023 and removed work-in-progress PR is a work-in-progress labels Jul 5, 2023

paulhauner marked this pull request as ready for review July 5, 2023 03:11

divagant-martian reviewed Jul 5, 2023

View reviewed changes

michaelsproul reviewed Jul 10, 2023

View reviewed changes

beacon_node/network/src/metrics.rs Show resolved Hide resolved

michaelsproul 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 Jul 10, 2023

paulhauner added 2 commits July 10, 2023 14:09

Remove duplicate metric

fa4770e

Remove duplicate NetworkGlobals

d2e5f9d

paulhauner 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 Jul 10, 2023

michaelsproul approved these changes Jul 10, 2023

View reviewed changes

michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jul 10, 2023

bors bot changed the title ~~Move the BeaconProcessor into a new crate~~ [Merged by Bors] - Move the BeaconProcessor into a new crate Jul 10, 2023

bors bot closed this Jul 10, 2023

Conversation

paulhauner commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

paulhauner Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

paulhauner Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

paulhauner commented Jul 5, 2023

Uh oh!

divagant-martian Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

paulhauner Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

divagant-martian Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

paulhauner Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

paulhauner commented Jul 10, 2023

Uh oh!

bors bot commented Jul 10, 2023

Uh oh!

paulhauner commented Jul 10, 2023

Uh oh!

bors bot commented Jul 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

paulhauner commented Jun 27, 2023 •

edited

Loading