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

Refactor/separate validation from other executions #1837

Merged
merged 20 commits into from
Apr 22, 2024
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
c1e0f54
Naively remove `Validation` exectution variant
MitchTurner Apr 17, 2024
5dfb4a3
Remove all uses of `Validation` execution type in tests, break tests …
MitchTurner Apr 17, 2024
777212d
WIP getting tests to pass
MitchTurner Apr 18, 2024
4a311e5
Implement validate route and get most of tests passing again
MitchTurner Apr 18, 2024
94bff04
Implement `validate` and fix other tests
MitchTurner Apr 18, 2024
8ba0235
Merge branch 'master' into refactor/separate-validation-from-other-ex…
MitchTurner Apr 18, 2024
74f5e1f
Update CHANGELOG
MitchTurner Apr 18, 2024
cc78682
Fix checks
MitchTurner Apr 18, 2024
cebe68b
Merge branch 'master' into refactor/separate-validation-from-other-ex…
MitchTurner Apr 18, 2024
70d672b
Fix most of the Wasm tests
MitchTurner Apr 18, 2024
7d37d04
Merge branch 'master' into refactor/separate-validation-from-other-ex…
MitchTurner Apr 18, 2024
1cdc886
Refactor and DRY up executor code
MitchTurner Apr 19, 2024
28a6118
Appease Clippy-sama
MitchTurner Apr 19, 2024
fbfa9de
Replace relayed tx stuff
MitchTurner Apr 19, 2024
41a1a37
Merge branch 'master' into refactor/separate-validation-from-other-ex…
MitchTurner Apr 19, 2024
363c2c4
Merge branch 'master' into refactor/separate-validation-from-other-ex…
MitchTurner Apr 19, 2024
54b6c05
Merge branch 'master' into refactor/separate-validation-from-other-ex…
MitchTurner Apr 19, 2024
aad5b63
Update CHANGELOG.md
MitchTurner Apr 19, 2024
947d5d6
Merge branch 'master' into refactor/separate-validation-from-other-ex…
Dentosal Apr 22, 2024
2639ba6
Cleanup, make suggested changes from PR review
MitchTurner Apr 22, 2024
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ Description of the upcoming release here.
- [#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
Comment on lines +22 to +24
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to move it into "Unreleased" section

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still not removed=)


## [Version 0.25.1]

### Fixed
Expand Down
78 changes: 30 additions & 48 deletions crates/fuel-core/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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());
}
Expand Down Expand Up @@ -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, ..
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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));
}
Expand All @@ -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!(
Expand Down Expand Up @@ -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!(
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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();

Expand All @@ -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(_))
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)))
}
Expand Down Expand Up @@ -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");
}

Expand Down Expand Up @@ -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());
}

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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");
}

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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:?}")
}

Expand Down Expand Up @@ -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();

Expand Down
12 changes: 4 additions & 8 deletions crates/fuel-core/src/service/adapters/block_importer.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use super::TransactionsSource;
use crate::{
database::Database,
service::adapters::{
Expand All @@ -10,8 +9,8 @@ use crate::{
use fuel_core_importer::{
ports::{
BlockVerifier,
Executor,
ImporterDatabase,
Validator,
},
Config,
Importer,
Expand Down Expand Up @@ -44,7 +43,6 @@ use fuel_core_types::{
ChainId,
},
services::executor::{
ExecutionTypes,
Result as ExecutorResult,
UncommittedResult as UncommittedExecutionResult,
},
Expand Down Expand Up @@ -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._validate_block(block)
}
}
12 changes: 11 additions & 1 deletion crates/fuel-core/src/service/adapters/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ use fuel_core_executor::{
};
use fuel_core_storage::transactional::Changes;
use fuel_core_types::{
blockchain::primitives::DaBlockHeight,
blockchain::{
block::Block,
primitives::DaBlockHeight,
},
fuel_tx,
services::{
block_producer::Components,
Expand Down Expand Up @@ -55,6 +58,13 @@ impl ExecutorAdapter {
) -> ExecutorResult<Vec<TransactionExecutionStatus>> {
self.executor.dry_run(block, utxo_validation)
}

pub(crate) fn _validate_block(
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can name the method validate_block_inner=)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

&self,
block: Block,
) -> ExecutorResult<UncommittedResult<Changes>> {
self.executor.validate(block)
}
}

impl fuel_core_executor::ports::RelayerPort for Database<Relayer> {
Expand Down
Loading
Loading