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

Connect the block importer with outside world #915

Merged
merged 43 commits into from
Jan 23, 2023

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Jan 18, 2023

What is done:

  • Removed CHAIN_HEIGHT_KEY, because now we have a genesis block to track this information.
  • Unified all database API that works with BlockHeight to work with references. Previously in some places, it was the reference, in some not.
  • Added manual_produce_block into PoA and used it in the GraphQL to have the one source of the block production. There are no unit tests(help is appreciated).
  • Connected PoA and BlockImporter via ports. Added BlockImporterAdapter.
  • Added the FuelBlockRoots column with the corresponding table. It is used to track BMT MMR roots for fuel blocks. The implementation adds new DatabaseColumn and IntoDatabaseKey traits to automate the storage trait implementation for some tables.
  • Moved implementation of database ports into the corresponding file in the fuel_core::service::adapter.
  • Removed the Database generic from PoA at all. Now we must pass the last_height while creating the Task. Updated tests to work with it.
  • Block producer now accepts the block_time along with block_height.

@xgreenx xgreenx force-pushed the featue/connect-block-importer branch from 64df92f to 646f3c5 Compare January 18, 2023 15:38
@xgreenx xgreenx changed the base branch from feature/block-importer to featue/block-verifier January 18, 2023 15:38
@xgreenx xgreenx self-assigned this Jan 18, 2023
@xgreenx xgreenx requested a review from a team January 18, 2023 17:13
@xgreenx xgreenx force-pushed the featue/block-verifier branch from 6f48202 to 64facf2 Compare January 19, 2023 00:38
@xgreenx xgreenx force-pushed the featue/connect-block-importer branch from 88f203b to 1400dcd Compare January 19, 2023 00:52
@xgreenx xgreenx force-pushed the featue/block-verifier branch from 1c50045 to 838a716 Compare January 19, 2023 23:58
…track this information.

- Added `manual_produce_block` into `PoA` and used it in the `GraphQL` to have the one source of the block production. There are no unit tests(help is appreciated).
- Connected `PoA` and `BlockImporter` via ports. Added `BlockImporterAdapter`.
- Added the `FuelBlockRoots` column with the corresponding table. It is used to track BMT MMR roots for fuel blocks. The implementation adds new `DatabaseColumn` and `IntoDatabaseKey` traits to automate the storage trait implementation for some tables.
- Moved implementation of database ports into the corresponding file in the `fuel_core::service::adapter`.
- Removed the `Database` generic from `PoA` at all. Now we must pass the `last_height` while creating the `Task`. Updated tests to work with it.
- Block producer now accepts the `block_time` along with `block_height`.
@xgreenx xgreenx force-pushed the featue/connect-block-importer branch from d4b9fbb to ab8c5fd Compare January 20, 2023 00:44
@@ -95,6 +95,8 @@ jobs:
include:
- command: clippy
args: --all-targets --all-features
- command: check
args: --all-targets
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything caught by this that isn't already caught?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep it catches running cargo check --all-targets at the workspace root which nothing else does. It has a different feature set the all-features or running the command at each crate level like the make command does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, I guess we can remove it, because cargo test --all-features --workspace do the same)

Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer to remove it then since CI times are already pretty long

crates/fuel-core/src/database.rs Outdated Show resolved Hide resolved
crates/fuel-core/src/database/storage.rs Show resolved Hide resolved
// TODO: When the port of BlockProducer will exist we need to replace it with
// `Box<dyn BlockProducerPort>
pub type BlockProducer = Arc<fuel_core_producer::Producer<crate::database::Database>>;
pub type BlockProducer = crate::service::adapters::BlockProducerAdapter;
Copy link
Contributor

Choose a reason for hiding this comment

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

The surface area is really small to change this to Box<dyn BlockProducerPort> and have it in line with the rest of the crate, there is already reference work in #909 you could cannabilize

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To use Box here requires the definition of the port. I don't want to add a new code and mix the work that is already handled by a mentioned PR=)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, will refactor that PR off of this one when this gets merged then

};
use itertools::Itertools;

/// Loads state from the chain config into database
pub(crate) fn initialize_state(
pub(crate) fn maybe_initialize_state(
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the primary reason for the rename? The function returns a result so is the maybe needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Returning the result means nothing=) The Err only shows that we got an error during initialization, while Ok maybe be "We changed something" or "We changed nothing"

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, so it only maybe will have an impact

crates/fuel-core/src/service/genesis.rs Show resolved Hide resolved
crates/services/producer/src/ports.rs Outdated Show resolved Hide resolved
crates/fuel-core/src/database/block.rs Outdated Show resolved Hide resolved
crates/fuel-core/src/database/block.rs Show resolved Hide resolved
Copy link
Contributor

@freesig freesig left a comment

Choose a reason for hiding this comment

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

Just a few issues around errors instead of options

crates/chain-config/src/config/state.rs Show resolved Hide resolved
}

/// The table has a corresponding column in the database.
trait DatabaseColumn {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Voxelot Is this the pattern you were talking about in last weeks sync?

crates/fuel-core/src/database/storage.rs Outdated Show resolved Hide resolved
@xgreenx xgreenx requested a review from freesig January 23, 2023 01:36
# Conflicts:
#	crates/fuel-core/src/service/config.rs
#	crates/fuel-core/src/service/sub_services.rs
freesig
freesig previously approved these changes Jan 23, 2023
Copy link
Contributor

@freesig freesig left a comment

Choose a reason for hiding this comment

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

Ok looks good

@ControlCplusControlV ControlCplusControlV requested a review from a team January 23, 2023 02:09
@ControlCplusControlV
Copy link
Contributor

Bumping the CI check issue, would prefer to remove that before merging so CI times don't get too long if --workspace is already covering that case

Base automatically changed from featue/block-verifier to master January 23, 2023 04:20
# Conflicts:
#	crates/fuel-core/src/service/config.rs
#	crates/fuel-core/src/service/sub_services.rs
#	crates/services/consensus_module/poa/src/service.rs
#	crates/services/consensus_module/poa/src/verifier.rs
@freesig freesig merged commit 9037d3a into master Jan 23, 2023
@freesig freesig deleted the featue/connect-block-importer branch January 23, 2023 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Importer/CM integration [CM] defer commitment of executed blocks to the importer
4 participants