Fixed some [nit]s after review the PR for fuel-core-sync integration
#928
Conversation
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| impl fuel_core_poa::ports::BlockImporter for BlockImporterAdapter { |
| @@ -37,7 +37,7 @@ mod tests; | |||
|
|
|||
| /// Creates an instance of runnable sync service. | |||
| pub fn new_service<P, E, C>( | |||
| current_fuel_block_height: Option<BlockHeight>, | |||
| current_fuel_block_height: BlockHeight, | |||
There was a problem hiding this comment.
We always have a last height before start of the service, so we don't need Option here
| @@ -199,7 +201,7 @@ impl BlockHeader { | |||
| } | |||
|
|
|||
| /// Validate the transactions match the header. | |||
| pub fn validate_transactions(&self, transactions: &[Vec<u8>]) -> bool { | |||
| pub fn validate_transactions(&self, transactions: &[Transaction]) -> bool { | |||
There was a problem hiding this comment.
We will use this method in the block verifier, so I simplified the API.
| @@ -265,10 +267,13 @@ impl PartialBlockHeader { | |||
| } | |||
| } | |||
|
|
|||
| fn generate_txns_root(transactions: &[Vec<u8>]) -> Bytes32 { | |||
| fn generate_txns_root(transactions: &[Transaction]) -> Bytes32 { | |||
| // TODO: The `to_bytes` requires mutability(but it is problem of the API). | |||
There was a problem hiding this comment.
The serialization should not require &mut self. It is a problem of our current approach with the io::Read trait for serialization. Later we will rework it, so for now we can clone it here.
FuelLabs/fuel-tx#190 fixes the problem, so maybe we need to consider finishing this PR.
| @@ -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>, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| self.non_reserved_connected_peers | ||
| .iter() | ||
| .chain(self.reserved_connected_peers.iter()) | ||
| .filter(|(_, peer_info)| { | ||
| peer_info.heartbeat_data.block_height == Some(*height) | ||
| peer_info.heartbeat_data.block_height >= Some(*height) |
| @@ -31,28 +31,21 @@ 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( | |||
There was a problem hiding this comment.
should we rename this to verify_poa_consensus?
There was a problem hiding this comment.
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
Because I didn't have a chance to review the PR in time, I did a review after=)