Refactor/separate validation from other executions #1837

MitchTurner · 2024-04-17T22:00:24Z

Part of: #1751

Executor is use both for producing a block, executing dry-run txs, and validating blocks. This is done using the same functions and we have internal conditional logic for each use case. This makes the code more complicated than it needs to be and makes it hard to read.

This PR removes the Validation variant from ExecutionTypes and creates new methods for just validation. Since there is only a set number of places in our code that uses this variant, it cleans the code up a lot to just have it be separate method, thus removing the need for all the internal conditional logic for each variant.

Remaining work to do in subsequent PRs:

Remove block production code from validation branch (Just use Block everywhere instead of Partial Block and Components, etc)
Separate DryRun from Production and remove ExecutionType
Hopefully remove ExecutionKind as well. It goes deep so I still need to figure out how to untangle it.

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

…with unimplemented code

…ecutions

xgreenx · 2024-04-18T19:49:25Z

crates/services/upgradable-executor/src/executor.rs

+        _block: Block,
+    ) -> fuel_core_types::services::executor::Result<ExecutionResult> {
+        let (result, changes) = self.validate_without_commit(_block)?.into();


Suggested change

_block: Block,

) -> fuel_core_types::services::executor::Result<ExecutionResult> {

let (result, changes) = self.validate_without_commit(_block)?.into();

block: Block,

) -> fuel_core_types::services::executor::Result<ExecutionResult> {

let (result, changes) = self.validate_without_commit(block)?.into();

xgreenx · 2024-04-22T09:26:34Z

CHANGELOG.md

+### Changed
+
+- [#1837](https://github.com/FuelLabs/fuel-core/pull/1837): Refactor the executor and separate validation from the other use cases


We need to move it into "Unreleased" section

Still not removed=)

xgreenx · 2024-04-22T09:27:17Z

crates/fuel-core/src/service/adapters/executor.rs

@@ -55,6 +58,13 @@ impl ExecutorAdapter {
    ) -> ExecutorResult<Vec<TransactionExecutionStatus>> {
        self.executor.dry_run(block, utxo_validation)
    }
+
+    pub(crate) fn _validate_block(


You can name the method validate_block_inner=)

Can we just get rid of these "private" methods? They are all just used in one/two places and think I like the idea of just calling executor.validate, etc in those places.

xgreenx · 2024-04-22T09:31:54Z

crates/services/upgradable-executor/wasm-executor/src/main.rs

+    let (
+        ExecutionResult {
+            block,
+            skipped_transactions,
+            tx_status,
+            events,
+        },
+        changes,
+    ) = instance.execute_without_commit(block)?.into();
+
+    Ok(Uncommitted::new(
+        ExecutionResult {
+            block,
+            skipped_transactions,
+            tx_status,
+            events,
+        },
+        changes,
+    ))


I don't know why I used into=) But it seems now we can simplify it to be:

Could you update all 3 places, please?=)

xgreenx · 2024-04-22T09:47:35Z

crates/services/executor/src/executor.rs

@@ -294,10 +318,6 @@ where
        // Compute the block id before execution if there is one.
        let pre_exec_block_id = block.id();


It seems you can remove this code and related logic=)

I've mostly done that in the next PR:
#1847

I left a note over there to remove the rest.

xgreenx · 2024-04-22T09:50:58Z

crates/services/executor/src/executor.rs

+    fn execute_transaction_and_commit(
+        &self,
+        block: &mut PartialFuelBlock,
+        thread_block_transaction: &mut StorageTransaction<


You can use a generic here, as we do in other places=) Or create an alias if you want to have control over the storage type

xgreenx · 2024-04-22T09:53:26Z

crates/services/executor/src/executor.rs

                    Ok(_) => {}
                    Err(err) => match execution_kind {
                        ExecutionKind::Production => {
-                            execution_data.skipped_transactions.push((tx_id, err));
+                            data.skipped_transactions.push((tx_id, err));
                        }
                        ExecutionKind::DryRun | ExecutionKind::Validation => {


It seems you can remove handling of the ExecutionKind::Validation here=) Maybe you even can remove this variant from the enum

The goal is to remove this enum entirely. Unfortunately there are some spots that are still using this variant. I have a follow-up PR already in the works and I can probably remove it as part of that:
#1847

Trying to keep these PRs small, so I just stopped this one after I got rid of ExecutionType.

xgreenx · 2024-04-22T09:55:40Z

crates/services/executor/src/executor.rs

+        gas_price: Word,
+        coinbase_contract_id: ContractId,
+        execution_kind: ExecutionKind,


In later PRs we can make these fields as part of the Instance=) And maybe have ExecutionInstance, ValidationInstance, etc.

Nice. That seems like it might be the right approach to get rid of ExecutionKind.

xgreenx · 2024-04-22T09:59:18Z

crates/services/executor/src/executor.rs

+
+        // L2 originated transactions should be in the `TxSource`. This will be triggered after
+        // all relayed transactions are processed.
+        let mut regular_tx_iter = source.next(block_gas_limit).into_iter().peekable();


Hmm, maybe we can also simplify that logic, because you already have pre defined list of transactions.

Done in the next PR:
#1847

xgreenx · 2024-04-22T10:01:33Z

crates/services/executor/src/executor.rs

+                )?;
+            }
+
+            let new_remaining_gas_limit = block_gas_limit.saturating_sub(data.used_gas);


Hmm, instead of saturating_sub it should be checked_add. Because maybe bloc producer included more transactions than was allowed=)

Also, maybe we need to update execute_inner as well

In the follow-up PR, I don't event have this variable:
#1847

I just expect the execution to fail if it exceeds the limit. There is no source in that case anyway, so there is no use for this.

…ecutions

Dentosal · 2024-04-22T10:55:24Z

crates/types/src/services/executor.rs

@@ -336,7 +311,7 @@ impl ExecutionKind {
        match self {
            ExecutionKind::DryRun => ExecutionTypes::DryRun(t),
            ExecutionKind::Production => ExecutionTypes::Production(t),
-            ExecutionKind::Validation => ExecutionTypes::Validation(t),
+            ExecutionKind::Validation => todo!("remove this variant"),


If this todo! still supposed to be here? Or is this part of the follow-up PR to remove ExecutionKind?

Hmm. My IDE didn't find any uses for this method anywhere so I just removed it entirely.

xgreenx · 2024-04-22T22:09:20Z

CHANGELOG.md

+### Changed
+
+- [#1837](https://github.com/FuelLabs/fuel-core/pull/1837): Refactor the executor and separate validation from the other use cases


Still not removed=)

xgreenx · 2024-04-22T22:16:21Z

crates/services/executor/src/executor.rs

+    // TODO: Refactor this function to reduce the number of arguments.
+    #[allow(clippy::too_many_arguments)]
+    fn execute_transaction_and_commit<
+        W: WriteTransaction + KeyValueInspect<Column = Column> + Modifiable,


Suggested change

W: WriteTransaction + KeyValueInspect<Column = Column> + Modifiable,

W: KeyValueInspect<Column = Column> + Modifiable,

xgreenx · 2024-04-22T22:18:12Z

crates/services/executor/src/executor.rs

+        let block_height = *block.header.height();
+        let execution_kind = ExecutionKind::Validation;
+
+        let forced_transactions = if self.relayer.enabled() {


Maybe it make sense to move relayer related logic into a separate functions and reuse it in validate_block and execute_block=) It can be a part of the follow up

xgreenx

You can fix comments in the follow-up PR=)

@MitchTurner

## Version v0.26.0 ### Fixed #### Breaking - [#1868](#1868): Include the `event_inbox_root` in the header hash. Changed types of the `transactions_count` to `u16` and `message_receipt_count` to `u32` instead of `u64`. Updated the application hash root calculation to not pad numbers. - [#1866](#1866): Fixed a runtime panic that occurred when restarting a node. The panic happens when the relayer database is already populated, and the relayer attempts an empty commit during start up. This invalid commit is removed in this PR. - [#1871](#1871): Fixed `block` endpoint to return fetch the blocks from both databases after regenesis. - [#1856](#1856): Replaced instances of `Union` with `Enum` for GraphQL definitions of `ConsensusParametersVersion` and related types. This is needed because `Union` does not support multiple `Version`s inside discriminants or empty variants. - [#1870](#1870): Fixed benchmarks for the `0.25.3`. - [#1870](#1870): Improves the performance of getting the size of the contract from the `InMemoryTransaction`. - [#1851](#1851): Provided migration capabilities (enabled addition of new column families) to RocksDB instance. ### Added - [#1853](#1853): Added a test case to verify the database's behavior when new columns are added to the RocksDB database. - [#1860](#1860): Regenesis now preserves `FuelBlockIdsToHeights` off-chain table. ### Changed - [#1847](#1847): Simplify the validation interface to use `Block`. Remove `Validation` variant of `ExecutionKind`. - [#1832](#1832): Snapshot generation can be cancelled. Progress is also reported. - [#1837](#1837): Refactor the executor and separate validation from the other use cases ## What's Changed * Weekly `cargo update` by @github-actions in #1850 * Refactor/separate validation from other executions by @MitchTurner in #1837 * fix: Use `Enum` for `ConsensusParametersVersion` and related types by @bvrooman in #1856 * feat: snapshot generation graceful shutdown by @segfault-magnet in #1832 * regenesis: migrate FuelBlockIdsToHeights by @Dentosal in #1860 * Weekly `cargo update` by @github-actions in #1869 * Refactor/Simplify validation logic by @MitchTurner in #1847 * Fixed `block` endpoint to return fetch the blocks from both databases after regenesis by @xgreenx in #1871 * Add Eq and Partial Eq to tx response and status by @MujkicA in #1872 * test: restart with relayer data by @bvrooman in #1866 * Fix `BlockHeader` hash by @MitchTurner in #1868 * Added a test for the case of adding new columns into the existing RocksDB database by @xgreenx in #1853 * Fixed benchmarks for the `0.25.3` by @xgreenx in #1870 **Full Changelog**: v0.25.3...v0.26.0

MitchTurner added 14 commits April 17, 2024 12:50

Naively remove Validation exectution variant

c1e0f54

Remove all uses of Validation execution type in tests, break tests …

5dfb4a3

…with unimplemented code

WIP getting tests to pass

777212d

Implement validate route and get most of tests passing again

4a311e5

Implement validate and fix other tests

94bff04

Merge branch 'master' into refactor/separate-validation-from-other-ex…

8ba0235

…ecutions

Update CHANGELOG

74f5e1f

Fix checks

cc78682

Merge branch 'master' into refactor/separate-validation-from-other-ex…

cebe68b

…ecutions

Fix most of the Wasm tests

70d672b

Merge branch 'master' into refactor/separate-validation-from-other-ex…

7d37d04

…ecutions

Refactor and DRY up executor code

1cdc886

Appease Clippy-sama

28a6118

Replace relayed tx stuff

fbfa9de

MitchTurner marked this pull request as ready for review April 19, 2024 02:33

MitchTurner requested a review from a team April 19, 2024 02:34

MitchTurner added 4 commits April 19, 2024 10:19

Merge branch 'master' into refactor/separate-validation-from-other-ex…

41a1a37

…ecutions

Merge branch 'master' into refactor/separate-validation-from-other-ex…

363c2c4

…ecutions

Merge branch 'master' into refactor/separate-validation-from-other-ex…

54b6c05

…ecutions

Update CHANGELOG.md

aad5b63

xgreenx reviewed Apr 22, 2024

View reviewed changes

Merge branch 'master' into refactor/separate-validation-from-other-ex…

947d5d6

…ecutions

Dentosal reviewed Apr 22, 2024

View reviewed changes

Cleanup, make suggested changes from PR review

2639ba6

MitchTurner self-assigned this Apr 22, 2024

xgreenx reviewed Apr 22, 2024

View reviewed changes

xgreenx approved these changes Apr 22, 2024

View reviewed changes

MitchTurner merged commit 88a2ce7 into master Apr 22, 2024
33 checks passed

MitchTurner deleted the refactor/separate-validation-from-other-executions branch April 22, 2024 22:21

xgreenx mentioned this pull request Apr 23, 2024

Refactor/Simplify validation logic #1847

Merged

9 tasks

xgreenx mentioned this pull request Apr 30, 2024

Release v0.26.0 #1874

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/separate validation from other executions #1837

Refactor/separate validation from other executions #1837

MitchTurner commented Apr 17, 2024 •

edited

Loading

xgreenx Apr 18, 2024

xgreenx Apr 22, 2024

xgreenx Apr 22, 2024

xgreenx Apr 22, 2024

MitchTurner Apr 22, 2024

xgreenx Apr 22, 2024

xgreenx Apr 22, 2024

MitchTurner Apr 22, 2024

xgreenx Apr 22, 2024

xgreenx Apr 22, 2024

MitchTurner Apr 22, 2024

xgreenx Apr 22, 2024

MitchTurner Apr 22, 2024

xgreenx Apr 22, 2024

MitchTurner Apr 22, 2024

xgreenx Apr 22, 2024

MitchTurner Apr 22, 2024 •

edited

Loading

Dentosal Apr 22, 2024

MitchTurner Apr 22, 2024

xgreenx Apr 22, 2024

xgreenx Apr 22, 2024

xgreenx Apr 22, 2024

xgreenx left a comment

		### Changed

		- [#1837](https://github.com/FuelLabs/fuel-core/pull/1837): Refactor the executor and separate validation from the other use cases

		@@ -294,10 +318,6 @@ where
		// Compute the block id before execution if there is one.
		let pre_exec_block_id = block.id();

	W: WriteTransaction + KeyValueInspect<Column = Column> + Modifiable,
	W: KeyValueInspect<Column = Column> + Modifiable,

Refactor/separate validation from other executions #1837

Refactor/separate validation from other executions #1837

Conversation

MitchTurner commented Apr 17, 2024 • edited Loading

Checklist

Before requesting review

After merging, notify other teams

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx left a comment

Choose a reason for hiding this comment

MitchTurner commented Apr 17, 2024 •

edited

Loading

MitchTurner Apr 22, 2024 •

edited

Loading