Refactor/separate validation from other executions (#1837)

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](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] 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](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else? --------- Co-authored-by: Hannes Karppila <hannes.karppila@gmail.com>
FuelLabs · Apr 22, 2024 · 88a2ce7 · 88a2ce7
1 parent 614d0db
commit 88a2ce7
Show file tree

Hide file tree

Showing 15 changed files with 623 additions and 344 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,13 +8,21 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 Description of the upcoming release here.
 
+### Changed
+
+- [#1837](https://github.com/FuelLabs/fuel-core/pull/1837): Refactor the executor and separate validation from the other use cases
+
 ## [Version 0.25.2]
 
 ### Fixed
 
 - [#1844](https://github.com/FuelLabs/fuel-core/pull/1844): Fixed the publishing of the `fuel-core 0.25.1` release.
 - [1842](https://github.com/FuelLabs/fuel-core/pull/1842): Ignore RUSTSEC-2024-0336: `rustls::ConnectionCommon::complete_io` could fall into an infinite loop based on network
 
+### Changed
+
+- [#1837](https://github.com/FuelLabs/fuel-core/pull/1837): Refactor the executor and separate validation from the other use cases
+
 ## [Version 0.25.1]
 
 ### Fixed
@@ -38,6 +46,7 @@ Description of the upcoming release here.
 
 ### Changed
 
+- [#1837](https://github.com/FuelLabs/fuel-core/pull/1837): Refactor the executor and separate validation from the other use cases
 - [#1833](https://github.com/FuelLabs/fuel-core/pull/1833): Regenesis of `SpentMessages` and `ProcessedTransactions`.
 - [#1830](https://github.com/FuelLabs/fuel-core/pull/1830): Use versioning enum for WASM executor input and output.
 - [#1816](https://github.com/FuelLabs/fuel-core/pull/1816): Updated the upgradable executor to fetch the state transition bytecode from the database when the version doesn't match a native one. This change enables the WASM executor in the "production" build and requires a `wasm32-unknown-unknown` target.

diff --git a/crates/fuel-core/src/executor.rs b/crates/fuel-core/src/executor.rs
@@ -329,7 +329,7 @@ mod tests {
     #[test]
     fn executor_validates_correctly_produced_block() {
         let mut producer = create_executor(Default::default(), Default::default());
-        let mut verifier = create_executor(Default::default(), Default::default());
+        let verifier = create_executor(Default::default(), Default::default());
         let block = test_block(1u32.into(), 0u64.into(), 10);
 
         let ExecutionResult {
@@ -340,8 +340,7 @@ mod tests {
             .execute_and_commit(ExecutionTypes::Production(block.into()))
             .unwrap();
 
-        let validation_result =
-            verifier.execute_and_commit(ExecutionTypes::Validation(block));
+        let validation_result = verifier.validate(block);
         assert!(validation_result.is_ok());
         assert!(skipped_transactions.is_empty());
     }
@@ -717,9 +716,7 @@ mod tests {
             let ExecutionResult {
                 block: validated_block,
                 ..
-            } = validator
-                .execute_and_commit(ExecutionBlock::Validation(produced_block))
-                .unwrap();
+            } = validator.validate_and_commit(produced_block).unwrap();
             assert_eq!(validated_block.transactions(), produced_txs);
             let ContractBalance {
                 asset_id, amount, ..
@@ -841,7 +838,7 @@ mod tests {
                 },
             );
             let validation_err = validator
-                .execute_and_commit(ExecutionBlock::Validation(block))
+                .validate_and_commit(block)
                 .expect_err("Expected error because coinbase if invalid");
             assert!(matches!(
                 validation_err,
@@ -867,7 +864,7 @@ mod tests {
 
             let mut validator = create_executor(Default::default(), Default::default());
             let validation_err = validator
-                .execute_and_commit(ExecutionBlock::Validation(block))
+                .validate_and_commit(block)
                 .expect_err("Expected error because coinbase if invalid");
             assert!(matches!(
                 validation_err,
@@ -881,7 +878,7 @@ mod tests {
 
             let mut validator = create_executor(Default::default(), Default::default());
             let validation_err = validator
-                .execute_and_commit(ExecutionBlock::Validation(block))
+                .validate_and_commit(block)
                 .expect_err("Expected error because coinbase is missing");
             assert!(matches!(validation_err, ExecutorError::MintMissing));
         }
@@ -903,7 +900,7 @@ mod tests {
 
             let mut validator = create_executor(Default::default(), Default::default());
             let validation_err = validator
-                .execute_and_commit(ExecutionBlock::Validation(block))
+                .validate_and_commit(block)
                 .expect_err("Expected error because coinbase if invalid");
 
             assert!(matches!(
@@ -938,7 +935,7 @@ mod tests {
             };
             let mut validator = create_executor(Default::default(), config);
             let validation_err = validator
-                .execute_and_commit(ExecutionBlock::Validation(block))
+                .validate_and_commit(block)
                 .expect_err("Expected error because coinbase if invalid");
 
             assert!(matches!(
@@ -966,7 +963,7 @@ mod tests {
 
             let mut validator = create_executor(Default::default(), Default::default());
             let validation_err = validator
-                .execute_and_commit(ExecutionBlock::Validation(block))
+                .validate_and_commit(block)
                 .expect_err("Expected error because coinbase if invalid");
             assert!(matches!(
                 validation_err,
@@ -1028,15 +1025,14 @@ mod tests {
 
         // Produced block is valid
         let ExecutionResult { mut block, .. } = verifier
-            .execute_without_commit(ExecutionTypes::Validation(block))
+            .validate_without_commit(block)
             .unwrap()
             .into_result();
 
         // Invalidate the block with Insufficient tx
         let len = block.transactions().len();
         block.transactions_mut().insert(len - 1, tx);
-        let verify_result =
-            verifier.execute_without_commit(ExecutionTypes::Validation(block));
+        let verify_result = verifier.validate_without_commit(block);
         assert!(matches!(
             verify_result,
             Err(ExecutorError::InvalidTransaction(
@@ -1077,7 +1073,7 @@ mod tests {
 
         // Produced block is valid
         let ExecutionResult { mut block, .. } = verifier
-            .execute_without_commit(ExecutionTypes::Validation(block))
+            .validate_without_commit(block)
             .unwrap()
             .into_result();
 
@@ -1086,8 +1082,7 @@ mod tests {
         block
             .transactions_mut()
             .insert(len - 1, Transaction::default_test_tx());
-        let verify_result =
-            verifier.execute_without_commit(ExecutionTypes::Validation(block));
+        let verify_result = verifier.validate_without_commit(block);
         assert!(matches!(
             verify_result,
             Err(ExecutorError::TransactionIdCollision(_))
@@ -1150,15 +1145,14 @@ mod tests {
 
         // Produced block is valid
         let ExecutionResult { mut block, .. } = verifier
-            .execute_without_commit(ExecutionTypes::Validation(block))
+            .validate_without_commit(block)
             .unwrap()
             .into_result();
 
         // Invalidate block by adding transaction with not existing coin
         let len = block.transactions().len();
         block.transactions_mut().insert(len - 1, tx);
-        let verify_result =
-            verifier.execute_without_commit(ExecutionTypes::Validation(block));
+        let verify_result = verifier.validate_without_commit(block);
         assert!(matches!(
             verify_result,
             Err(ExecutorError::TransactionValidity(
@@ -1203,8 +1197,7 @@ mod tests {
             }
         }
 
-        let verify_result =
-            verifier.execute_and_commit(ExecutionBlock::Validation(block));
+        let verify_result = verifier.validate_and_commit(block);
         assert!(matches!(
             verify_result,
             Err(ExecutorError::InvalidTransactionOutcome { transaction_id }) if transaction_id == tx_id
@@ -1240,8 +1233,7 @@ mod tests {
         block.header_mut().set_transaction_root(rng.gen());
         block.header_mut().recalculate_metadata();
 
-        let verify_result =
-            verifier.execute_and_commit(ExecutionBlock::Validation(block));
+        let verify_result = verifier.validate_and_commit(block);
 
         assert!(matches!(verify_result, Err(ExecutorError::InvalidBlockId)))
     }
@@ -1914,9 +1906,7 @@ mod tests {
         assert_eq!(tx.inputs()[0].state_root(), state_root);
 
         let _ = executor
-            .execute_without_commit_with_source::<OnceTransactionsSource>(
-                ExecutionTypes::Validation(block),
-            )
+            .validate(block)
             .expect("Validation of block should be successful");
     }
 
@@ -2110,8 +2100,7 @@ mod tests {
         assert!(skipped_transactions.is_empty());
 
         let verifier = create_executor(db, Default::default());
-        let verify_result =
-            verifier.execute_without_commit(ExecutionBlock::Validation(second_block));
+        let verify_result = verifier.validate_without_commit(second_block);
         assert!(verify_result.is_ok());
     }
 
@@ -2188,8 +2177,7 @@ mod tests {
         }
 
         let verifier = create_executor(db, Default::default());
-        let verify_result =
-            verifier.execute_without_commit(ExecutionBlock::Validation(second_block));
+        let verify_result = verifier.validate_without_commit(second_block);
 
         assert!(matches!(
             verify_result,
@@ -2343,7 +2331,7 @@ mod tests {
             .expect("block execution failed unexpectedly");
 
         make_executor(&[&message])
-            .execute_and_commit(ExecutionBlock::Validation(block))
+            .validate_and_commit(block)
             .expect("block validation failed unexpectedly");
     }
 
@@ -2468,14 +2456,14 @@ mod tests {
 
         // Produced block is valid
         make_executor(&[]) // No messages in the db
-            .execute_and_commit(ExecutionBlock::Validation(block.clone()))
+            .validate_and_commit(block.clone())
             .unwrap();
 
         // Invalidate block by returning back `tx` with not existing message
         let index = block.transactions().len() - 1;
         block.transactions_mut().insert(index, tx);
         let res = make_executor(&[]) // No messages in the db
-            .execute_and_commit(ExecutionBlock::Validation(block));
+            .validate_and_commit(block);
         assert!(matches!(
             res,
             Err(ExecutorError::TransactionValidity(
@@ -2510,14 +2498,13 @@ mod tests {
 
         // Produced block is valid
         make_executor(&[&message])
-            .execute_and_commit(ExecutionBlock::Validation(block.clone()))
+            .validate_and_commit(block.clone())
             .unwrap();
 
         // Invalidate block by return back `tx` with not ready message.
         let index = block.transactions().len() - 1;
         block.transactions_mut().insert(index, tx);
-        let res = make_executor(&[&message])
-            .execute_and_commit(ExecutionBlock::Validation(block));
+        let res = make_executor(&[&message]).validate_and_commit(block);
         assert!(matches!(
             res,
             Err(ExecutorError::TransactionValidity(
@@ -2590,16 +2577,14 @@ mod tests {
 
         // Produced block is valid
         let exec = make_executor(&[&message]);
-        let ExecutionResult { mut block, .. } = exec
-            .execute_without_commit(ExecutionTypes::Validation(block))
-            .unwrap()
-            .into_result();
+        let ExecutionResult { mut block, .. } =
+            exec.validate_without_commit(block).unwrap().into_result();
 
         // Invalidate block by return back `tx2` transaction skipped during production.
         let len = block.transactions().len();
         block.transactions_mut().insert(len - 1, tx2);
         let exec = make_executor(&[&message]);
-        let res = exec.execute_without_commit(ExecutionTypes::Validation(block));
+        let res = exec.validate_without_commit(block);
         assert!(matches!(
             res,
             Err(ExecutorError::TransactionValidity(
@@ -2811,10 +2796,7 @@ mod tests {
         assert!(skipped_transactions.is_empty());
 
         let validator = create_executor(db.clone(), config);
-        let result = validator
-            .execute_without_commit_with_source::<OnceTransactionsSource>(
-                ExecutionTypes::Validation(block),
-            );
+        let result = validator.validate(block);
         assert!(result.is_ok(), "{result:?}")
     }
 
@@ -3370,7 +3352,7 @@ mod tests {
             add_events_to_relayer(&mut verifier_relayer_db, da_height.into(), &events);
             let verifier = create_relayer_executor(verifyer_db, verifier_relayer_db);
             let (result, _) = verifier
-                .execute_without_commit(ExecutionTypes::Validation(produced_block))
+                .validate_without_commit(produced_block)
                 .unwrap()
                 .into();
 

diff --git a/crates/fuel-core/src/service/adapters/block_importer.rs b/crates/fuel-core/src/service/adapters/block_importer.rs
@@ -1,4 +1,3 @@
-use super::TransactionsSource;
 use crate::{
     database::Database,
     service::adapters::{
@@ -10,8 +9,8 @@ use crate::{
 use fuel_core_importer::{
     ports::{
         BlockVerifier,
-        Executor,
         ImporterDatabase,
+        Validator,
     },
     Config,
     Importer,
@@ -44,7 +43,6 @@ use fuel_core_types::{
         ChainId,
     },
     services::executor::{
-        ExecutionTypes,
         Result as ExecutorResult,
         UncommittedResult as UncommittedExecutionResult,
     },
@@ -102,13 +100,11 @@ impl ImporterDatabase for Database {
     }
 }
 
-impl Executor for ExecutorAdapter {
-    fn execute_without_commit(
+impl Validator for ExecutorAdapter {
+    fn validate(
         &self,
         block: Block,
     ) -> ExecutorResult<UncommittedExecutionResult<Changes>> {
-        self._execute_without_commit::<TransactionsSource>(ExecutionTypes::Validation(
-            block,
-        ))
+        self.executor.validate(block)
     }
 }