Skip to content
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

Jump to bottom

Move the BeaconProcessor into a new crate #4434

Closed
wants to merge 33 commits into from
Closed

Move the BeaconProcessor into a new crate #4434

wants to merge 33 commits into from

Conversation

paulhauner
Copy link
Member

@paulhauner paulhauner commented Jun 27, 2023 •
edited
Loading

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.
  2. Scheduled tasks in the BeaconChain (e.g., state advance).

Using the BeaconProcessor for these tasks would help prevent the BN from becoming overwhelmed (by using bounded queues) 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 can build upon these works to actually use the BeaconProcessor for more operations.

I did a little refactoring in the ClientBuilder since passing channels between the BeaconProcessor and network components is a little more complex now. Ultimately, I think these changes bring the ClientBuilder closer to a true builder pattern, when all the actual work happens in fn build.

I had to lift some types from the network crate to the lighthouse_network crate so they could be shared between the network and new beacon_processor crate without a circular dep.

TODO

  • Address TODO(paul)s
  • Wire a shutdown-sender into the BeaconProcessor

paulhauner added 30 commits June 20, 2023 09:32
@paulhauner
Create new beacon_processor crate
445dbf6
@paulhauner
Add attestation batching
67f1923
@paulhauner
Move work reprocessing queue
8349325
@paulhauner
Work on reprocessing queue
441fe93
@paulhauner
Misc progress
4a7fd49
@paulhauner
Add TODOs
6043e3c
@paulhauner
New beacon processor crate compiles (but no clippy)
89475ae
@paulhauner
Add async functions
768d2c5
@paulhauner
Box large objects
7162b78
@paulhauner
Fix all clippy lints
399cdd4
@paulhauner
Unify types in lighthouse_network
328ee50
@paulhauner
Start refactoring network crate
4b9f2f4
@paulhauner
Misc network refactoring
54628f1
@paulhauner
Start adding builder pattern
cab145d
@paulhauner
Remove builder pattern
39415c2
@paulhauner
Start removing process_fn generators
805a1d4
@paulhauner
Add misc progress
45f4ef5
@paulhauner
Add WorkEvent constructor functions
cb7fe28
@paulhauner
Add misc progress
f6a35ef
@paulhauner
Add WorkEventBuilder
13d137c
@paulhauner
Remove builder pattern (again, lel)
1a3121f
@paulhauner
Change Error, update chain segment
03b538f
@paulhauner
Remove constuctor impls for WorkEvent
e43b25d
@paulhauner
Fix misc compile errors
9c4faf6
@paulhauner
Add misc progress
4b7875f
@paulhauner
Fix aggregation fns
e9c6249
@paulhauner
Fix misc warnings
6e81aaa
@paulhauner
Fix small compile errors
cc85768
@paulhauner
Integrate into client builder
dc4dc10
@paulhauner
Address misc compile issues
352b543
@paulhauner paulhauner added the work-in-progress PR is a work-in-progress label Jun 27, 2023
@paulhauner paulhauner mentioned this pull request Jun 27, 2023
Closed
@paulhauner
Copy link
Member Author

Closed in favor of #4435. It is the same, but has a smaller diff due to some history curation.

@paulhauner paulhauner closed this Jun 27, 2023
bors bot pushed a commit that referenced this pull request Jul 10, 2023
@paulhauner
Move the BeaconProcessor into a new crate (#4435)
a75c4fe 
*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 bot pushed a commit that referenced this pull request Jul 10, 2023
@paulhauner
Move the BeaconProcessor into a new crate (#4435)
c25825a 
*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.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
@paulhauner @Woodpile37
Move the BeaconProcessor into a new crate (sigp#4435)
ebaac13 
*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.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
@paulhauner @Woodpile37
Move the BeaconProcessor into a new crate (sigp#4435)
1c4ee78 
*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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jxs jxs Awaiting requested review from jxs jxs will be requested when the pull request is marked ready for review jxs is a code owner

Assignees
No one assigned
Labels
work-in-progress PR is a work-in-progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@paulhauner