External block verifier #916

xgreenx · 2023-01-18T15:30:14Z

Refs: #883

It is the implementation of the block verifier. Right now it provides only one function to verify the block's fields. But in the future, we can add verification of the seals(it is why it holds a relayer) and reuse this verifier in the fuel-core-sync.

Along with the verifier, I changed the fuel_core_poa::Config and ChainConfig to have rules for the block's time of the network and separate rules for block production.

Added NodeRole::Producer and NodeRole::Validator. Based on the role, we will create a fuel_core_poa::Config.
Removed Never from BlockProduction from ChainConfig because it is the block production information of the PoA.

No unit tests right now.

freesig

Looks good, just one question around the time bounds.

freesig · 2023-01-19T03:31:23Z

crates/services/consensus_module/poa/src/verifier.rs

+                } => {
+                    let previous_block_time = Tai64N::from(prev_header.time());
+                    // The block should be in the range
+                    // [min..max + (max - min)/2]


I don't understand why this is true?

Because it is not true=) Created an issue to discuss it #918

I already disabled this check in the production for now.

Dentosal

LGTM; Helpful comments as well

- Added `debug_assert_eq` to verify that the block header id is an identifier of the block. And that it uses the last hash of the application header. No unit tests right now.

Co-authored-by: Tom <tomrgowan@gmail.com>

Updated tests to be more different.

Voxelot · 2023-01-20T23:49:01Z

crates/fuel-core/src/service/config.rs

            #[cfg(feature = "relayer")]
            relayer: Default::default(),
            #[cfg(feature = "p2p")]
            p2p: P2PConfig::<NotInitialized>::default("test_network"),
            consensus_key: Some(Secret::new(default_consensus_dev_key().into())),
        }
    }
+
+    pub fn poa_config(&self) -> anyhow::Result<fuel_core_poa::Config> {


Would using impl TryFrom<&Config> for fuel_core_poa::Config instead be more idiomatic rust?

Voxelot · 2023-01-20T23:59:27Z

crates/services/consensus_module/poa/src/service.rs

@@ -75,6 +75,9 @@ pub struct Task<D, T, B> {
    /// a bit, but doesn't cause any other issues.
    pub(crate) last_block_created: Instant,
    pub(crate) trigger: Trigger,
+    // TODO: Consider that the creation of the block takes some time, and maybe we need to


Voxelot · 2023-01-21T00:05:45Z

crates/services/consensus_module/poa/src/verifier.rs

+    /// This configuration exists because right now consensus module uses the `Tai64::now`
+    /// during the generation of the block. That means if the node(block producer) was
+    /// offline for a while, it would not use the expected block time after
+    /// restarting(it should generate blocks with old timestamps).


(it should generate blocks with old timestamps)

Don't follow why we'd want to forge stale timestamps for blocks. Using older timestamps may make the verification logic easier to implement, but the time should always aim to be as current/correct as possible.

Voxelot · 2023-01-21T00:08:07Z

crates/services/consensus_module/poa/src/verifier.rs

+                    let previous_block_time = Tai64N::from(prev_header.time());
+
+                    // If the `average_block_time` is 15 seconds, the block should be in the range
+                    // [15..15 + 15/2] -> [15..23]


I think we should only have a min bound, there's no reason to be strict about the max time between blocks and enforce the producer makes blocks fast enough. In poa we really only care about the producer making blocks too fast.

Simplified validation rules of verification. Removed `NodeRole`. Instead, the production mode is controlled by CLI argument `--instant`, `--interval 13`, `--hybrid 13,14,15`

Voxelot · 2023-01-21T10:06:00Z

@xgreenx and I were discussing the issue of whether or not we should be validating things like min or max block times in poa.

Ultimately his conclusion was that if something is part of the chainconfig, it should be part of the consensus validation in some way. My conclusion was that we shouldn't add all these verification around timestamps for poa as it could make the system brittle and cause syncing to stall.

So we decided that poa trigger configs should be local parameters that only apply to the authority node, and not included in the chain config. Eventually as our cli args grow in complexity, we could extend clap to support loading from a config file instead which will make it easier to manage.

Green already did the work of stripping the trigger params out of chainconfig, tomorrow I will improve the cli interface a bit using clap argument groups.

ControlCplusControlV · 2023-01-22T21:17:50Z

crates/chain-config/Cargo.toml

 fuel-core-storage = { path = "../storage", version = "0.15.1" }
 fuel-core-types = { path = "../types", version = "0.15.1", features = [
    "serde",
    "random",
 ] }
 hex = { version = "0.4", features = ["serde"] }
+humantime-serde = "1.1.1"


Is this actually used anywhere? I removed the dep and ran ci_checks.sh locally and everything seemed to pass for me

Nope, it is not used, I don't know why we didn't get a warning=D

Oh, I know=D

xgreenx · 2023-01-23T02:26:41Z

Approved!=) Voxelot's changes, thank you <3

crates/services/consensus_module/poa/src/verifier.rs

xgreenx requested a review from a team January 18, 2023 15:30

xgreenx self-assigned this Jan 18, 2023

xgreenx mentioned this pull request Jan 18, 2023

Implementation of the block importer #903

Merged

xgreenx force-pushed the featue/block-verifier branch from 6f48202 to 64facf2 Compare January 19, 2023 00:38

freesig previously approved these changes Jan 19, 2023

View reviewed changes

xgreenx marked this pull request as ready for review January 19, 2023 13:50

xgreenx requested a review from a team January 19, 2023 13:50

ControlCplusControlV added the fuel-block-importer label Jan 19, 2023

Dentosal previously approved these changes Jan 19, 2023

View reviewed changes

xgreenx and others added 7 commits January 19, 2023 23:50

- Added the main logic of the block importer.

393b5a7

- Added `debug_assert_eq` to verify that the block header id is an identifier of the block. And that it uses the last hash of the application header. No unit tests right now.

Tested commit_result

a6d835e

Make clippy happy

0b71b7f

Added tests for verify_and_execute_block part of the importer

d7d7a6c

Added test for lock for verify_and_execute_block

d60971a

Update crates/services/importer/src/importer.rs

aa720f3

Co-authored-by: Tom <tomrgowan@gmail.com>

Make fmt happy

7e3ba96

xgreenx force-pushed the feature/block-importer branch from 3190082 to 7e3ba96 Compare January 19, 2023 23:55

xgreenx and others added 7 commits January 19, 2023 23:57

Added verifeir for the block

3be7239

Fixed merge conflicts

f90e026

Fix test to use Validator role

7d21d6b

add unit tests to verify genesis

d9cade3

add unit tests to verify poa block

d67e729

Linked TODO to the issue

5a16bc0

Added one more test for geensis

838a716

xgreenx force-pushed the featue/block-verifier branch from 1c50045 to 838a716 Compare January 19, 2023 23:58

xgreenx added 2 commits January 20, 2023 12:44

Renamed generics.

27020e1

Updated tests to be more different.

Merge branch 'feature/block-importer' into featue/block-verifier

950fb3c

xgreenx assigned freesig Jan 20, 2023

Base automatically changed from feature/block-importer to master January 20, 2023 22:02

xgreenx dismissed Dentosal’s stale review via 950fb3c January 20, 2023 22:02

xgreenx dismissed freesig’s stale review via 950fb3c January 20, 2023 22:02

Merge branch 'master' into featue/block-verifier

21cc173

Voxelot reviewed Jan 20, 2023

View reviewed changes

Voxelot reviewed Jan 21, 2023

View reviewed changes

xgreenx added 4 commits January 21, 2023 02:08

Removed BlockProduction from ChainConfig.

c2913b9

Simplified validation rules of verification. Removed `NodeRole`. Instead, the production mode is controlled by CLI argument `--instant`, `--interval 13`, `--hybrid 13,14,15`

Merge branch 'master' into featue/block-verifier

21cf872

Replace poa_config with into

a0ef9a3

Push new snapshots

a195c56

ControlCplusControlV reviewed Jan 22, 2023

View reviewed changes

Removed "humantime-serde"

3966ad3

ControlCplusControlV mentioned this pull request Jan 23, 2023

Prepare GraphQL For Future Extraction #909

Merged

Voxelot added 4 commits January 22, 2023 18:14

upgrade clap in fuel-core cli, improve cli interface for consensus args

f692b3d

fix lockfile

eda254e

cargo lint

13bab2c

add readme instructions for disabling block production

457102f

Make clippy happy

11f9f6d

Voxelot reviewed Jan 23, 2023

View reviewed changes

crates/services/consensus_module/poa/src/verifier.rs Outdated Show resolved Hide resolved

Voxelot reviewed Jan 23, 2023

View reviewed changes

crates/services/consensus_module/poa/src/verifier.rs Outdated Show resolved Hide resolved

Update crates/services/consensus_module/poa/src/verifier.rs

0810e47

Voxelot approved these changes Jan 23, 2023

View reviewed changes

Voxelot merged commit 4c65bd2 into master Jan 23, 2023

Voxelot deleted the featue/block-verifier branch January 23, 2023 04:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External block verifier #916

External block verifier #916

xgreenx commented Jan 18, 2023 •

edited

Loading

freesig left a comment

freesig Jan 19, 2023

xgreenx Jan 19, 2023

Dentosal left a comment

Voxelot Jan 20, 2023 •

edited

Loading

Voxelot Jan 20, 2023

Voxelot Jan 21, 2023

Voxelot Jan 21, 2023 •

edited

Loading

Voxelot commented Jan 21, 2023

ControlCplusControlV Jan 22, 2023

xgreenx Jan 23, 2023

xgreenx Jan 23, 2023

xgreenx commented Jan 23, 2023 •

edited

Loading

External block verifier #916

External block verifier #916

Conversation

xgreenx commented Jan 18, 2023 • edited Loading

freesig left a comment

Choose a reason for hiding this comment

freesig Jan 19, 2023

Choose a reason for hiding this comment

xgreenx Jan 19, 2023

Choose a reason for hiding this comment

Dentosal left a comment

Choose a reason for hiding this comment

Voxelot Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Voxelot Jan 20, 2023

Choose a reason for hiding this comment

Voxelot Jan 21, 2023

Choose a reason for hiding this comment

Voxelot Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

Voxelot commented Jan 21, 2023

ControlCplusControlV Jan 22, 2023

Choose a reason for hiding this comment

xgreenx Jan 23, 2023

Choose a reason for hiding this comment

xgreenx Jan 23, 2023

Choose a reason for hiding this comment

xgreenx commented Jan 23, 2023 • edited Loading

xgreenx commented Jan 18, 2023 •

edited

Loading

Voxelot Jan 20, 2023 •

edited

Loading

Voxelot Jan 21, 2023 •

edited

Loading

xgreenx commented Jan 23, 2023 •

edited

Loading