-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_gateway: extract async mempool query from blocking task #9941
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
base: 11-06-apollo_gateway_use_get_nonce_outside_extract_state_nonce_and_run_validations
Are you sure you want to change the base?
Conversation
693a7c8 to
474b0e0
Compare
8349379 to
7faf351
Compare
|
Can we simplify this test util? Suggestion: fn process_tx_task(
stateful_transaction_validator_factory: MockStatefulTransactionValidatorFactoryTrait,
) -> ProcessTxBlockingTask {
let is_account_tx_in_mempool = false;
ProcessTxBlockingTask {
stateful_tx_validator_factory: Arc::new(stateful_transaction_validator_factory),
state_reader_factory: Arc::new(MockStateReaderFactory::new()),
executable_tx: executable_invoke_tx(invoke_args()),
is_account_tx_in_mempool,
} |
ArniStarkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @itamar-starkware and @TzahiTaub)
crates/apollo_gateway/src/gateway.rs line 162 at r2 (raw file):
.mempool_client .account_tx_in_pool_or_recent_block(executable_tx.contract_address()) .await;
We don't want to call the mempool for every transaction. That is very cost-ineffective:
Suggestion:
// Please clean up this code - no need for this value to be mut.
let mut is_account_tx_in_mempool = None;
if tx.nonce() == Nonce(Felt::ONE) && account_nonce == Nonce(Felt::ZERO) {
// Value is Some only if the question `is_account_tx_in_mempool` is in any way interesting.
// Otherwise we don't care - it is irrelevant for skip validate.
let is_account_tx_in_mempool: Optional<bool> = Some(self
.mempool_client
.account_tx_in_pool_or_recent_block(executable_tx.contract_address())
.await.map_err(..).inspect(..)?);
}crates/apollo_gateway/src/stateful_transaction_validator.rs line 309 at r2 (raw file):
} Ok(false)
Philosophically, I would extract this error handling outside of the blocking task and next to the query to the mempool.
Suggestion:
fn skip_stateful_validations(
tx: &ExecutableTransaction,
account_nonce: Nonce,
is_account_tx_in_mempool: bool,
) -> StatefulTransactionValidatorResult<bool> {
if let ExecutableTransaction::Invoke(ExecutableInvokeTransaction { tx, .. }) = tx {
// check if the transaction nonce is 1, meaning it is post deploy_account, and the
// account nonce is zero, meaning the account was not deployed yet.
if tx.nonce() == Nonce(Felt::ONE) && account_nonce == Nonce(Felt::ZERO) {
let account_address = tx.sender_address();
debug!("Checking if deploy_account transaction exists for account {account_address}.");
// We verify that a deploy_account transaction exists for this account. It is sufficient
// to check if the account exists in the mempool since it means that either it has a
// deploy_account transaction or transactions with future nonces that passed
// validations.
return Ok(is_account_tx_in_mempool);
}
}
Ok(false)
ArniStarkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @itamar-starkware and @TzahiTaub)
crates/apollo_gateway/src/gateway.rs line 162 at r2 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
We don't want to call the mempool for every transaction. That is very cost-ineffective:
another thing to make sure before we even start the call to the mempool - that the transaction in question is an invoke transaction.
I.E. move all the logic related to which transaction we query on.
7faf351 to
f8cf519
Compare
8b83c3b to
c58285c
Compare
f8cf519 to
c51f57e
Compare
c58285c to
fb39f9f
Compare
c51f57e to
d2d5f77
Compare
d2d5f77 to
e827b03
Compare
fb3c348 to
05a86d7
Compare
e827b03 to
14142cc
Compare
05a86d7 to
677edc2
Compare
677edc2 to
29e2f3f
Compare
14142cc to
90e1beb
Compare
90e1beb to
ba27508
Compare
29e2f3f to
a14ecc8
Compare
ba27508 to
a9b8430
Compare
a14ecc8 to
8c1e6ea
Compare
a1ef90a to
fc642eb
Compare
8c1e6ea to
6af545f
Compare
fc642eb to
b4115d2
Compare
6af545f to
081adc6
Compare

TL;DR
Optimize transaction validation by prefetching mempool data asynchronously instead of blocking on async operations.
What changed?
ProcessTxBlockingTaskto receive the mempool check result instead of performing it during executionextract_state_nonce_and_run_validationsto accept the prefetched mempool result instead of performing the check itselfHow to test?
Why make this change?
This change eliminates a performance bottleneck where a blocking thread was calling
block_onfor an async operation. By prefetching the mempool data in an async context before creating the blocking task, we avoid blocking-on-async in a blocking thread, which improves overall performance and resource utilization. This is particularly important for the gateway component which handles transaction processing and needs to maintain high throughput.