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

feat: Improve OP txpool batch validation #13918

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
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
115 changes: 97 additions & 18 deletions crates/optimism/node/src/txpool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,29 +325,49 @@ where
}
}

/// Validates a single transaction.
///
/// See also [`TransactionValidator::validate_transaction`]
///
/// This behaves the same as [`EthTransactionValidator::validate_one`], but in addition, ensures
/// that the account has enough balance to cover the L1 gas cost.
pub fn validate_one(
fn validate_batch(
&self,
origin: TransactionOrigin,
transaction: Tx,
transactions: Vec<(TransactionOrigin, Tx)>,
) -> Vec<TransactionValidationOutcome<Tx>> {
let outcomes = self.inner.validate_all(transactions);

outcomes
.into_iter()
.map(|outcome| self.validate_gas_fee(self.validate_transaction_type(outcome)))
Copy link
Author

Choose a reason for hiding this comment

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

note: i validate the transaction type after performing the transaction validation (validate_all), which is in a different order than done previously. if you think this is problematic, lmk.

.collect()
}
Copy link
Member

Choose a reason for hiding this comment

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

it makes sense to do the tx type validation first! it is much cheaper in the case of an eip4844 transaction, as then it will bail on first check as opposed to if you do batch validation on inner EthTransactionValidator first, which would allow the eip4844 tx to pass through and then carry out all the other checks on it for no reason

Copy link
Author

@yohkaz yohkaz Jan 24, 2025

Choose a reason for hiding this comment

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

my thoughts aswell! wanted to be sure before getting into this.

required some vector manipulations to keep the ordering, which I supposed is required: fded914

open to better ideas than allocating additional vecs : )


fn validate_transaction_type(
&self,
outcome: TransactionValidationOutcome<Tx>,
) -> TransactionValidationOutcome<Tx> {
if transaction.is_eip4844() {
return TransactionValidationOutcome::Invalid(
match outcome {
TransactionValidationOutcome::Valid {
balance: _,
state_nonce: _,
transaction,
propagate: _,
} if transaction.transaction().is_eip4844() => TransactionValidationOutcome::Invalid(
transaction.into_transaction(),
InvalidTransactionError::TxTypeNotSupported.into(),
)
),
TransactionValidationOutcome::Invalid(tx, _) if tx.is_eip4844() => {
TransactionValidationOutcome::Invalid(
tx,
InvalidTransactionError::TxTypeNotSupported.into(),
)
}
_ => outcome,
}
}

let outcome = self.inner.validate_one(origin, transaction);

fn validate_gas_fee(
&self,
outcome: TransactionValidationOutcome<Tx>,
) -> TransactionValidationOutcome<Tx> {
if !self.requires_l1_data_gas_fee() {
// no need to check L1 gas fee
return outcome
return outcome;
}

// ensure that the account has enough balance to cover the L1 gas cost
Expand Down Expand Up @@ -385,20 +405,42 @@ where
GotExpected { got: balance, expected: cost }.into(),
)
.into(),
)
);
}

return TransactionValidationOutcome::Valid {
balance,
state_nonce,
transaction: valid_tx,
propagate,
}
};
yohkaz marked this conversation as resolved.
Show resolved Hide resolved
}

outcome
}

/// Validates a single transaction.
///
/// See also [`TransactionValidator::validate_transaction`]
///
/// This behaves the same as [`EthTransactionValidator::validate_one`], but in addition, ensures
/// that the account has enough balance to cover the L1 gas cost.
pub fn validate_one(
&self,
origin: TransactionOrigin,
transaction: Tx,
) -> TransactionValidationOutcome<Tx> {
if transaction.is_eip4844() {
return TransactionValidationOutcome::Invalid(
transaction,
InvalidTransactionError::TxTypeNotSupported.into(),
);
}
yohkaz marked this conversation as resolved.
Show resolved Hide resolved

let outcome = self.inner.validate_one(origin, transaction);
self.validate_gas_fee(outcome)
}

/// Validates all given transactions.
///
/// Returns all outcomes for the given transactions in the same order.
Expand All @@ -408,7 +450,7 @@ where
&self,
transactions: Vec<(TransactionOrigin, Tx)>,
) -> Vec<TransactionValidationOutcome<Tx>> {
transactions.into_iter().map(|(origin, tx)| self.validate_one(origin, tx)).collect()
self.validate_batch(transactions)
}
}

Expand Down Expand Up @@ -503,4 +545,41 @@ mod tests {
};
assert_eq!(err.to_string(), "transaction type not supported");
}

#[test]
fn validate_optimism_transaction_all() {
let client = MockEthProvider::default();
let validator = EthTransactionValidatorBuilder::new(MAINNET.clone())
.no_shanghai()
.no_cancun()
.build(client, InMemoryBlobStore::default());
let validator = OpTransactionValidator::new(validator);

let origin = TransactionOrigin::External;
let signer = Default::default();
let deposit_tx = OpTypedTransaction::Deposit(TxDeposit {
source_hash: Default::default(),
from: signer,
to: TxKind::Create,
mint: None,
value: U256::ZERO,
gas_limit: 0,
is_system_transaction: false,
input: Default::default(),
});
let signature = Signature::test_signature();
let signed_tx = OpTransactionSigned::new_unhashed(deposit_tx, signature);
let signed_recovered = Recovered::new_unchecked(signed_tx, signer);
let len = signed_recovered.encode_2718_len();
let pooled_tx = OpPooledTransaction::new(signed_recovered, len);
let outcomes = validator.validate_all(vec![(origin, pooled_tx)]);

for outcome in outcomes {
let err = match outcome {
TransactionValidationOutcome::Invalid(_, err) => err,
_ => panic!("Expected invalid transaction"),
};
assert_eq!(err.to_string(), "transaction type not supported");
}
}
}
Loading