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.

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

Standalone Multi-TX Block Producer #409

Merged
merged 41 commits into from
Sep 24, 2022
Merged

Conversation

Voxelot
Copy link
Member

@Voxelot Voxelot commented Jun 14, 2022

Initial base for the block producer service WIP

Leveraging ports & adapters architecture to unblock development from depending on other components & also to improve composability & testability.

The main principle behind the P&A design is to allow the component to specify all external dependencies via its' own traits which don't need to be depended on by others. Fuel Core can provide adapter impls at a later time when feasible, while allowing the block producer to be logically implemented and functionally tested in the meantime as a self-contained unit.

I'm also experimenting with the service design a bit, it seems like treating the service as a dummy wrapper for spawning the stateful block producer task makes the borrow checker happier and avoids the need for mutexes and other concurrency primitives.

@rakita
Copy link
Contributor

rakita commented Jun 14, 2022

What is intention behind this change?

@Voxelot
Copy link
Member Author

Voxelot commented Jun 14, 2022

Most of this PR is just additional logic for the block producer. I brought in the P&A concepts as I went along because I ran into blockers directly integrating with the relayer and txpool since they aren't done yet.

Then I realized the ports pattern could be used to start developing the other modules in parallel so that we don't have to serialize the development of these modules based on their order of dependencies. I'd consider it complementary to the event-driven arch. It's purely an additional abstraction which makes the core domain logic of the modules more pure, testable, and easier to follow. For example, the impl for the core domain logic of block production is already there and fits into a 30 LOC function.

If we go with this P&A approach in the other modules, we'll basically need to do two passes. The first pass is building the core domain logic and testing all the edge cases, and then the second pass would be simply implementing the adapters in the main fuel-core crate and injecting them into the appropriate modules.

I'll get more of this wired up and then make an official design proposal using this as an example.

@rakita
Copy link
Contributor

rakita commented Jun 15, 2022

Why i asked that directly for intention is that I was not sure if you looked at how it is now, and for example how fuel-relayer did this, as it does rely on database and fuel-importer and can be standalone tested:

db: Box<dyn RelayerDb>,
new_block_event: broadcast::Receiver<NewBlockEvent>,

For relayer channels, I need to move them from new function but the idea is there.

same for txpool:

pub async fn start(&self, new_block: broadcast::Receiver<ImportBlockBroadcast>) -> bool {

Abstraction is done over channel message enums.

Maybe there is a need for having new, init and start functions. new to create, init to bind channels, and start/stop to stop and start service.

rebase with master

add async-trait to producer
@Voxelot Voxelot force-pushed the Voxelot/block-producer-init branch from 0c5621c to ce06c0e Compare July 11, 2022 15:22
@Dentosal Dentosal self-assigned this Sep 5, 2022
* Run a full integration test between
 * Replace DummyDb with Database::in_memory
* Remove txs from txpool when new block is added
 * Still WIP
@Dentosal Dentosal force-pushed the Voxelot/block-producer-init branch from f633f68 to 7dc1f3f Compare September 8, 2022 19:02
@Dentosal Dentosal marked this pull request as ready for review September 8, 2022 21:07
@Dentosal
Copy link
Member

Dentosal commented Sep 8, 2022

As discussed with @Voxelot earlier today, this PR now contains the standalone block producer module with minimal implementation. It will not be used elsewhere in the code yet, but to keep PRs small enough to review, that integration will be done later.

Follow-up PRs will include:

@Dentosal Dentosal requested a review from a team September 8, 2022 21:13
@Dentosal Dentosal changed the title Block Producer WIP Standalone Multi-TX Block Producer Sep 8, 2022
@Dentosal Dentosal mentioned this pull request Sep 9, 2022
Mitch Martin and others added 4 commits September 19, 2022 07:13
fix clippy issue by removing unnecessary return

Co-authored-by: ControlCplusControlV <44706811+ControlCplusControlV@users.noreply.github.com>
Copy link
Member

@Dentosal Dentosal left a comment

Choose a reason for hiding this comment

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

LGTM, but as I've written a large chunk of this, someone else should review it as well.

@Dentosal Dentosal requested review from xgreenx and freesig September 20, 2022 20:28
fuel-block-producer/src/adapters.rs Show resolved Hide resolved
fuel-block-producer/src/adapters.rs Show resolved Hide resolved
fuel-block-producer/src/block_producer.rs Show resolved Hide resolved
fuel-block-producer/src/block_producer.rs Outdated Show resolved Hide resolved
fuel-block-producer/src/block_producer.rs Show resolved Hide resolved
fuel-block-producer/src/block_producer.rs Show resolved Hide resolved
fuel-block-producer/src/block_producer/tests.rs Outdated Show resolved Hide resolved
fuel-block-producer/src/config.rs Show resolved Hide resolved
fuel-block-producer/src/ports.rs Outdated Show resolved Hide resolved
@Voxelot
Copy link
Member Author

Voxelot commented Sep 23, 2022

blocked by: FuelLabs/fuel-tx#189

@Dentosal
Copy link
Member

Blocker cleared. Now this has merge conflicts.

Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

Look good!=) It is hard to read the integration test, but maybe in the future, we will find how to make it more readable=D

@Voxelot
Copy link
Member Author

Voxelot commented Sep 24, 2022

Look good!=) It is hard to read the integration test, but maybe in the future, we will find how to make it more readable=D

I think once the followup tasks are implemented this test should look a lot simpler and not have to simulate as much of the node behavior.

@Voxelot Voxelot merged commit b869d0c into master Sep 24, 2022
@Voxelot Voxelot deleted the Voxelot/block-producer-init branch September 24, 2022 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants