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

Fixed some [nit]s after review the PR for fuel-core-sync integration #928

Merged
merged 2 commits into from
Jan 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions crates/fuel-core/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use crate::{
service::Config,
};
use fuel_core_executor::refs::ContractRef;
use fuel_core_producer::ports::Executor as ExecutorTrait;
use fuel_core_storage::{
tables::{
Coins,
Expand Down Expand Up @@ -138,15 +137,15 @@ impl Executor {
}
}

impl ExecutorTrait<Database> for Executor {
fn execute_without_commit(
impl Executor {
pub fn execute_without_commit(
&self,
block: ExecutionBlock,
) -> ExecutorResult<UncommittedResult<StorageTransaction<Database>>> {
self.execute_inner(block, &self.database)
}

fn dry_run(
pub fn dry_run(
&self,
block: ExecutionBlock,
utxo_validation: Option<bool>,
Expand Down
4 changes: 2 additions & 2 deletions crates/fuel-core/src/query/message_proof/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ async fn can_build_message_proof() {
let header = header.clone();
let message_ids = message_ids.clone();
move |_| {
let header = header.clone().generate(&[vec![]], &message_ids);
let header = header.clone().generate(&[], &message_ids);
let transactions = TXNS.to_vec();
Ok(CompressedBlock::test(header, transactions))
}
Expand All @@ -173,6 +173,6 @@ async fn can_build_message_proof() {
.unwrap();
assert_eq!(p.message_id(), message_id);
assert_eq!(p.signature, Signature::default());
let header = header.generate(&[vec![]], &message_ids);
let header = header.generate(&[], &message_ids);
assert_eq!(p.header.output_messages_root, header.output_messages_root);
}
5 changes: 2 additions & 3 deletions crates/fuel-core/src/service/adapters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ use std::sync::Arc;

pub mod block_importer;
pub mod consensus_module;
pub mod executor;
pub mod graphql_api;
#[cfg(feature = "p2p")]
pub mod p2p;
pub mod producer;
pub mod txpool;

#[cfg(feature = "p2p")]
pub mod sync;
pub mod txpool;

#[derive(Clone)]
pub struct PoAAdapter {
Expand Down Expand Up @@ -60,7 +60,6 @@ pub struct BlockProducerAdapter {
pub struct BlockImporterAdapter {
pub block_importer:
Arc<fuel_core_importer::Importer<Database, ExecutorAdapter, VerifierAdapter>>,
execution_semaphore: Arc<tokio::sync::Semaphore>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the semaphore from here for now because Importer already returns an error if the import process is in progress.

We don't plan to import several blocks simultaneously. I think receiving an error is better than hiding it under the semaphore.

Copy link
Member

Choose a reason for hiding this comment

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

Would this be a problem in local testing if manual-block-production is enabled? I.e. maybe they have interval mode set for block time but manually trigger some blocks and there's a conflict.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If they face an error in the tests, it may be better to update the test=D Because if you produce more blocks during manual production than you asked, it may cause wrong test results.

}

#[cfg(feature = "p2p")]
Expand Down
35 changes: 7 additions & 28 deletions crates/fuel-core/src/service/adapters/block_importer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,10 @@ use fuel_core_types::{
SealedBlock,
},
fuel_tx::Bytes32,
services::{
block_importer::UncommittedResult,
executor::ExecutionBlock,
services::executor::{
ExecutionBlock,
Result as ExecutorResult,
UncommittedResult as UncommittedExecutionResult,
},
};
use std::sync::Arc;
Expand All @@ -52,38 +53,22 @@ impl BlockImporterAdapter {
) -> Self {
Self {
block_importer: Arc::new(Importer::new(config, database, executor, verifier)),
execution_semaphore: Arc::new(tokio::sync::Semaphore::new(1)),
}
}

pub async fn execute_and_commit(
&self,
sealed_block: SealedBlock,
) -> anyhow::Result<()> {
let permit = self.execution_semaphore.acquire().await?;
tokio::task::spawn_blocking({
let importer = self.block_importer.clone();
move || importer.execute_and_commit(sealed_block)
})
.await??;
core::mem::drop(permit);
Ok(())
}
}

impl fuel_core_poa::ports::BlockImporter for BlockImporterAdapter {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to poa.rs

type Database = Database;

fn commit_result(
&self,
result: UncommittedResult<StorageTransaction<Self::Database>>,
) -> anyhow::Result<()> {
self.block_importer
.commit_result(result)
.map_err(Into::into)
}
}

impl BlockVerifier for VerifierAdapter {
fn verify_block_fields(
&self,
Expand Down Expand Up @@ -126,14 +111,8 @@ impl Executor for ExecutorAdapter {
fn execute_without_commit(
&self,
block: ExecutionBlock,
) -> Result<
fuel_core_types::services::executor::UncommittedResult<
StorageTransaction<Self::Database>,
>,
fuel_core_types::services::executor::Error,
> {
fuel_core_producer::ports::Executor::<Database>::execute_without_commit(
self, block,
)
) -> ExecutorResult<UncommittedExecutionResult<StorageTransaction<Self::Database>>>
{
self._execute_without_commit(block)
}
}
20 changes: 19 additions & 1 deletion crates/fuel-core/src/service/adapters/consensus_module/poa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@ use crate::{
database::Database,
fuel_core_graphql_api::ports::ConsensusModulePort,
service::adapters::{
BlockImporterAdapter,
BlockProducerAdapter,
PoAAdapter,
TxPoolAdapter,
},
};
use fuel_core_poa::{
ports::TransactionPool,
ports::{
BlockImporter,
TransactionPool,
},
service::SharedState,
};
use fuel_core_services::stream::BoxStream;
Expand All @@ -22,6 +26,7 @@ use fuel_core_types::{
TxId,
},
services::{
block_importer::UncommittedResult as UncommittedImporterResult,
executor::UncommittedResult,
txpool::{
ArcPoolTx,
Expand Down Expand Up @@ -98,3 +103,16 @@ impl fuel_core_poa::ports::BlockProducer for BlockProducerAdapter {
.await
}
}

impl BlockImporter for BlockImporterAdapter {
type Database = Database;

fn commit_result(
&self,
result: UncommittedImporterResult<StorageTransaction<Self::Database>>,
) -> anyhow::Result<()> {
self.block_importer
.commit_result(result)
.map_err(Into::into)
}
}
39 changes: 39 additions & 0 deletions crates/fuel-core/src/service/adapters/executor.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
use crate::{
database::Database,
executor::Executor,
service::adapters::ExecutorAdapter,
};
use fuel_core_storage::transactional::StorageTransaction;
use fuel_core_types::{
fuel_tx::Receipt,
services::executor::{
ExecutionBlock,
Result as ExecutorResult,
UncommittedResult,
},
};

impl ExecutorAdapter {
pub(crate) fn _execute_without_commit(
&self,
block: ExecutionBlock,
) -> ExecutorResult<UncommittedResult<StorageTransaction<Database>>> {
let executor = Executor {
database: self.database.clone(),
config: self.config.clone(),
};
executor.execute_without_commit(block)
}

pub(crate) fn _dry_run(
&self,
block: ExecutionBlock,
utxo_validation: Option<bool>,
) -> ExecutorResult<Vec<Vec<Receipt>>> {
let executor = Executor {
database: self.database.clone(),
config: self.config.clone(),
};
executor.dry_run(block, utxo_validation)
}
}
13 changes: 2 additions & 11 deletions crates/fuel-core/src/service/adapters/producer.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use crate::{
database::Database,
executor::Executor,
service::adapters::{
BlockProducerAdapter,
ExecutorAdapter,
Expand Down Expand Up @@ -66,23 +65,15 @@ impl fuel_core_producer::ports::Executor<Database> for ExecutorAdapter {
&self,
block: ExecutionBlock,
) -> ExecutorResult<UncommittedResult<StorageTransaction<Database>>> {
let executor = Executor {
database: self.database.clone(),
config: self.config.clone(),
};
executor.execute_without_commit(block)
self._execute_without_commit(block)
}

fn dry_run(
&self,
block: ExecutionBlock,
utxo_validation: Option<bool>,
) -> ExecutorResult<Vec<Vec<Receipt>>> {
let executor = Executor {
database: self.database.clone(),
config: self.config.clone(),
};
executor.dry_run(block, utxo_validation)
self._dry_run(block, utxo_validation)
}
}

Expand Down
3 changes: 1 addition & 2 deletions crates/fuel-core/src/service/sub_services.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,8 @@ pub fn init_sub_services(

#[cfg(feature = "p2p")]
let sync = {
let current_fuel_block_height = Some(database.latest_height()?);
fuel_core_sync::service::new_service(
current_fuel_block_height,
last_height,
p2p_adapter,
importer_adapter.clone(),
verifier,
Expand Down
33 changes: 13 additions & 20 deletions crates/services/consensus_module/poa/src/verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ use fuel_core_storage::Result as StorageResult;
use fuel_core_types::{
blockchain::{
block::Block,
consensus::poa::PoAConsensus,
header::BlockHeader,
primitives::BlockHeight,
SealedBlockHeader,
},
fuel_tx::Input,
fuel_types::Bytes32,
Expand All @@ -31,32 +31,25 @@ pub trait Database {
fn block_header_merkle_root(&self, height: &BlockHeight) -> StorageResult<Bytes32>;
}

// TODO: Make this function `async` and await the synchronization with the relayer.
pub fn verify_consensus(
Copy link
Member

Choose a reason for hiding this comment

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

should we rename this to verify_poa_consensus?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It lives inside of PoA, so we don't need to add this prefix. We need to rename verify_poa_block_fields into verify_block_fields

consensus_config: &ConsensusConfig,
header: &SealedBlockHeader,
header: &BlockHeader,
consensus: &PoAConsensus,
) -> bool {
let SealedBlockHeader {
entity: header,
consensus,
} = header;
match consensus {
fuel_core_types::blockchain::consensus::Consensus::PoA(consensus) => {
match consensus_config {
ConsensusConfig::PoA { signing_key } => {
let id = header.id();
let m = id.as_message();
consensus
.signature
.recover(m)
.map_or(false, |k| Input::owner(&k) == *signing_key)
}
}
match consensus_config {
ConsensusConfig::PoA { signing_key } => {
let id = header.id();
let m = id.as_message();
consensus
.signature
.recover(m)
.map_or(false, |k| Input::owner(&k) == *signing_key)
}
_ => true,
}
}

pub fn verify_poa_block_fields<D: Database>(
pub fn verify_block_fields<D: Database>(
config: &Config,
database: &D,
block: &Block,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,5 +107,5 @@ fn test_verify_genesis_block_fields(input: Input) -> anyhow::Result<()> {
let mut b = Block::default();
b.header_mut().consensus = ch;
b.header_mut().application = ah;
verify_poa_block_fields(&c, &d, &b)
verify_block_fields(&c, &d, &b)
}
27 changes: 18 additions & 9 deletions crates/services/consensus_module/src/block_verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,7 @@ mod tests;

use crate::block_verifier::config::Config;
use anyhow::ensure;
use fuel_core_poa::verifier::{
verify_consensus,
verify_poa_block_fields,
Database as PoAVerifierDatabase,
};
use fuel_core_poa::verifier::Database as PoAVerifierDatabase;
use fuel_core_types::{
blockchain::{
block::Block,
Expand Down Expand Up @@ -66,15 +62,28 @@ where
.unwrap_or_else(|| 0u32.into());
verify_genesis_block_fields(expected_genesis_height, block.header())
}
Consensus::PoA(_) => {
verify_poa_block_fields(&self.config.poa, &self.database, block)
}
Consensus::PoA(_) => fuel_core_poa::verifier::verify_block_fields(
&self.config.poa,
&self.database,
block,
),
}
}

/// Verifies the consensus of the block header.
pub fn verify_consensus(&self, header: &SealedBlockHeader) -> bool {
verify_consensus(&self.config.chain_config.consensus, header)
let SealedBlockHeader {
entity: header,
consensus,
} = header;
match consensus {
Consensus::Genesis(_) => true,
Consensus::PoA(consensus) => fuel_core_poa::verifier::verify_consensus(
&self.config.chain_config.consensus,
header,
consensus,
),
}
}
}

Expand Down
Loading