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

Use ref instead of owned Block in validation #1886

Merged
merged 10 commits into from
May 8, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
## [Unreleased]
### Changed

- [#1886](https://github.com/FuelLabs/fuel-core/pull/1886): Use ref to `Block` in validation code
- [#1876](https://github.com/FuelLabs/fuel-core/pull/1876): Updated benchmark to include the worst scenario for `CROO` opcode. Also include consensus parameters in bench output.

### Changed
Expand Down
90 changes: 37 additions & 53 deletions crates/fuel-core/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ mod tests {
..
} = producer.produce_and_commit(block.into()).unwrap();

let validation_result = verifier.validate(block);
let validation_result = verifier.validate(&block);
assert!(validation_result.is_ok());
assert!(skipped_transactions.is_empty());
}
Expand Down Expand Up @@ -677,7 +677,7 @@ mod tests {
let producer = create_executor(database.clone(), config.clone());

let ExecutionResult {
block: produced_block,
block,
skipped_transactions,
..
} = producer
Expand All @@ -690,18 +690,15 @@ mod tests {
.unwrap()
.into_result();
assert!(skipped_transactions.is_empty());
let produced_txs = produced_block.transactions().to_vec();
let produced_txs = block.transactions().to_vec();

let mut validator = create_executor(
Default::default(),
// Use the same config as block producer
config,
);
let ExecutionResult {
block: validated_block,
..
} = validator.validate_and_commit(produced_block).unwrap();
assert_eq!(validated_block.transactions(), produced_txs);
let _ = validator.validate_and_commit(&block).unwrap();
assert_eq!(block.transactions(), produced_txs);
let ContractBalance {
asset_id, amount, ..
} = validator
Expand Down Expand Up @@ -822,7 +819,7 @@ mod tests {
},
);
let validation_err = validator
.validate_and_commit(block)
.validate_and_commit(&block)
.expect_err("Expected error because coinbase if invalid");
assert!(matches!(
validation_err,
Expand All @@ -848,7 +845,7 @@ mod tests {

let mut validator = create_executor(Default::default(), Default::default());
let validation_err = validator
.validate_and_commit(block)
.validate_and_commit(&block)
.expect_err("Expected error because coinbase if invalid");
assert!(matches!(
validation_err,
Expand All @@ -862,7 +859,7 @@ mod tests {

let mut validator = create_executor(Default::default(), Default::default());
let validation_err = validator
.validate_and_commit(block)
.validate_and_commit(&block)
.expect_err("Expected error because coinbase is missing");
assert!(matches!(validation_err, ExecutorError::MintMissing));
}
Expand All @@ -884,7 +881,7 @@ mod tests {

let mut validator = create_executor(Default::default(), Default::default());
let validation_err = validator
.validate_and_commit(block)
.validate_and_commit(&block)
.expect_err("Expected error because coinbase if invalid");

assert!(matches!(
Expand Down Expand Up @@ -919,7 +916,7 @@ mod tests {
};
let mut validator = create_executor(Default::default(), config);
let validation_err = validator
.validate_and_commit(block)
.validate_and_commit(&block)
.expect_err("Expected error because coinbase if invalid");

assert!(matches!(
Expand Down Expand Up @@ -947,7 +944,7 @@ mod tests {

let mut validator = create_executor(Default::default(), Default::default());
let validation_err = validator
.validate_and_commit(block)
.validate_and_commit(&block)
.expect_err("Expected error because coinbase if invalid");
assert!(matches!(
validation_err,
Expand Down Expand Up @@ -991,7 +988,7 @@ mod tests {

let ExecutionResult {
skipped_transactions,
block,
mut block,
..
} = producer
.produce_without_commit(block)
Expand All @@ -1008,15 +1005,12 @@ mod tests {
));

// Produced block is valid
let ExecutionResult { mut block, .. } = verifier
.validate_without_commit(block)
.unwrap()
.into_result();
let _ = verifier.validate(&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.validate_without_commit(block);
let verify_result = verifier.validate(&block);
assert!(matches!(
verify_result,
Err(ExecutorError::InvalidTransaction(
Expand All @@ -1043,7 +1037,7 @@ mod tests {

let ExecutionResult {
skipped_transactions,
block,
mut block,
..
} = producer
.produce_without_commit(block)
Expand All @@ -1056,17 +1050,14 @@ mod tests {
));

// Produced block is valid
let ExecutionResult { mut block, .. } = verifier
.validate_without_commit(block)
.unwrap()
.into_result();
let _ = verifier.validate(&block).unwrap().into_result();

// Make the block invalid by adding of the duplicating transaction
let len = block.transactions().len();
block
.transactions_mut()
.insert(len - 1, Transaction::default_test_tx());
let verify_result = verifier.validate_without_commit(block);
let verify_result = verifier.validate(&block);
assert!(matches!(
verify_result,
Err(ExecutorError::TransactionIdCollision(_))
Expand Down Expand Up @@ -1113,7 +1104,7 @@ mod tests {

let ExecutionResult {
skipped_transactions,
block,
mut block,
..
} = producer
.produce_without_commit(block)
Expand All @@ -1128,15 +1119,12 @@ mod tests {
));

// Produced block is valid
let ExecutionResult { mut block, .. } = verifier
.validate_without_commit(block)
.unwrap()
.into_result();
let _ = verifier.validate(&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.validate_without_commit(block);
let verify_result = verifier.validate(&block);
assert!(matches!(
verify_result,
Err(ExecutorError::TransactionValidity(
Expand Down Expand Up @@ -1181,7 +1169,7 @@ mod tests {
}

// then
let err = verifier.validate_and_commit(block).unwrap_err();
let err = verifier.validate_and_commit(&block).unwrap_err();
assert_eq!(
err,
ExecutorError::InvalidTransactionOutcome { transaction_id }
Expand Down Expand Up @@ -1216,7 +1204,7 @@ mod tests {
block.header_mut().set_transaction_root(rng.gen());
block.header_mut().recalculate_metadata();

let err = verifier.validate_and_commit(block).unwrap_err();
let err = verifier.validate_and_commit(&block).unwrap_err();

assert_eq!(err, ExecutorError::BlockMismatch)
}
Expand Down Expand Up @@ -1869,7 +1857,7 @@ mod tests {
assert_eq!(tx.inputs()[0].state_root(), state_root);

let _ = executor
.validate(block)
.validate(&block)
.expect("Validation of block should be successful");
}

Expand Down Expand Up @@ -2058,7 +2046,7 @@ mod tests {
assert!(skipped_transactions.is_empty());

let verifier = create_executor(db, Default::default());
let verify_result = verifier.validate_without_commit(second_block);
let verify_result = verifier.validate(&second_block);
assert!(verify_result.is_ok());
}

Expand Down Expand Up @@ -2133,7 +2121,7 @@ mod tests {
}

let verifier = create_executor(db, Default::default());
let err = verifier.validate_without_commit(second_block).unwrap_err();
let err = verifier.validate(&second_block).unwrap_err();

assert_eq!(
err,
Expand Down Expand Up @@ -2283,7 +2271,7 @@ mod tests {
.expect("block execution failed unexpectedly");

make_executor(&[&message])
.validate_and_commit(block)
.validate_and_commit(&block)
.expect("block validation failed unexpectedly");
}

Expand Down Expand Up @@ -2402,14 +2390,14 @@ mod tests {

// Produced block is valid
make_executor(&[]) // No messages in the db
.validate_and_commit(block.clone())
.validate_and_commit(&block)
.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
.validate_and_commit(block);
.validate_and_commit(&block);
assert!(matches!(
res,
Err(ExecutorError::TransactionValidity(
Expand Down Expand Up @@ -2444,13 +2432,13 @@ mod tests {

// Produced block is valid
make_executor(&[&message])
.validate_and_commit(block.clone())
.validate_and_commit(&block)
.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]).validate_and_commit(block);
let res = make_executor(&[&message]).validate_and_commit(&block);
assert!(matches!(
res,
Err(ExecutorError::TransactionValidity(
Expand Down Expand Up @@ -2504,7 +2492,7 @@ mod tests {
let exec = make_executor(&[&message]);
let ExecutionResult {
skipped_transactions,
block,
mut block,
..
} = exec.produce_without_commit(block).unwrap().into_result();
// One of two transactions is skipped.
Expand All @@ -2520,14 +2508,13 @@ mod tests {

// Produced block is valid
let exec = make_executor(&[&message]);
let ExecutionResult { mut block, .. } =
exec.validate_without_commit(block).unwrap().into_result();
let _ = exec.validate(&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.validate_without_commit(block);
let res = exec.validate(&block);
assert!(matches!(
res,
Err(ExecutorError::TransactionValidity(
Expand Down Expand Up @@ -2739,7 +2726,7 @@ mod tests {
assert!(skipped_transactions.is_empty());

let validator = create_executor(db.clone(), config);
let result = validator.validate(block);
let result = validator.validate(&block);
assert!(result.is_ok(), "{result:?}")
}

Expand Down Expand Up @@ -3292,13 +3279,10 @@ mod tests {
let events = vec![event];
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
.validate_without_commit(produced_block)
.unwrap()
.into();
let (result, _) = verifier.validate(&produced_block).unwrap().into();

// then
let txs = result.block.transactions();
let txs = produced_block.transactions();
assert_eq!(txs.len(), 1);

// and
Expand Down Expand Up @@ -3450,7 +3434,7 @@ mod tests {

let validator = create_relayer_executor(on_chain_db, relayer_db);
// When
let result = validator.validate_without_commit(result.block).map(|_| ());
let result = validator.validate(&result.block).map(|_| ());

// Then
assert_eq!(Ok(()), result);
Expand Down
6 changes: 3 additions & 3 deletions crates/fuel-core/src/service/adapters/block_importer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ use fuel_core_types::{
},
services::executor::{
Result as ExecutorResult,
UncommittedResult as UncommittedExecutionResult,
UncommittedValidationResult,
},
};
use std::sync::Arc;
Expand Down Expand Up @@ -103,8 +103,8 @@ impl ImporterDatabase for Database {
impl Validator for ExecutorAdapter {
fn validate(
&self,
block: Block,
) -> ExecutorResult<UncommittedExecutionResult<Changes>> {
block: &Block,
) -> ExecutorResult<UncommittedValidationResult<Changes>> {
self.executor.validate(block)
}
}
Loading
Loading