apollo_gateway: extract async code from blocking task

[alonh5]

No description provided.

[reviewable-StarkWare]

This change is 

[ShahakShama]

@ShahakShama reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @noamsp-starkware)

---

crates/apollo_gateway/src/gateway.rs line 271 at r1 (raw file):

        self.stateless_tx_validator.validate(&self.tx)?;

        // let tx_signature = self.tx.signature().clone();

Delete all commented out code

---

crates/apollo_gateway/src/gateway_test.rs line 722 at r1 (raw file):

}

#[rstest]

We still need this test, it should just use the main add_tx function instead

[alonh5]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @noamsp-starkware, and @ShahakShama)

---

crates/apollo_gateway/src/gateway.rs line 271 at r1 (raw file):

Previously, ShahakShama wrote…

Delete all commented out code

oops, thanks. Done.

---

crates/apollo_gateway/src/gateway_test.rs line 722 at r1 (raw file):

Previously, ShahakShama wrote…

We still need this test, it should just use the main add_tx function instead

I replaced it with the test above and added a TODO, we can't mock the transaction converter currently.

[ShahakShama]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5, @ArniStarkware, @ayeletstarkware, and @noamsp-starkware)

---

crates/apollo_gateway/src/gateway.rs line 271 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

oops, thanks. Done.

Forgot to push?

[alonh5]

• apollo_gateway: remove internal tx from blocking task #9499
• apollo_gateway: extract stateless validator from blocking task #9498
• apollo_gateway: dependency inject transaction converter #9476
• apollo_gateway: extract async code from blocking task #9424 👈 (View in Graphite)
• main-v0.14.1

This stack of pull requests is managed by Graphite. Learn more about stacking.

[alonh5]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @ayeletstarkware, @noamsp-starkware, and @ShahakShama)

---

crates/apollo_gateway/src/gateway.rs line 271 at r1 (raw file):

Previously, ShahakShama wrote…

Forgot to push?

Yeah, see now

[ShahakShama]

@ShahakShama reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware, @ayeletstarkware, and @noamsp-starkware)

[ArniStarkware]

crates/apollo_gateway/src/gateway.rs line 269 at r2 (raw file):

    fn process_tx(self) -> GatewayResult<AddTransactionArgs> {
        // Perform stateless validations.
        self.stateless_tx_validator.validate(&self.tx)?;

Move this before the conversions.

No need to process a transaction that does not pass these -simple and lightweight - validations.

Code quote:

        // Perform stateless validations.
        self.stateless_tx_validator.validate(&self.tx)?;

[ArniStarkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5, @ayeletstarkware, and @noamsp-starkware)

---

crates/apollo_gateway/src/gateway.rs line 243 at r2 (raw file):

    executable_tx: AccountTransaction,
    runtime: tokio::runtime::Handle,
}

So, this struct holds the internal_tx just so it could later pass it along to the mempool.
passing the internal_tx to the mempool is indeed a blocking task - but I feel it is very different than other blocking tasks in the gateway.

Passing the internal_tx to the mempool is part of the component train we have in other components.
The other blocking tasks like running the blockifier are indeed blocking...

Code quote:

/// CPU-intensive transaction processing, spawned in a blocking thread to avoid blocking other tasks
/// from running.
struct ProcessTxBlockingTask {
    stateless_tx_validator: Arc<dyn StatelessTransactionValidatorTrait>,
    stateful_tx_validator_factory: Arc<dyn StatefulTransactionValidatorFactoryTrait>,
    state_reader_factory: Arc<dyn StateReaderFactory>,
    mempool_client: SharedMempoolClient,
    tx: RpcTransaction,
    internal_tx: InternalRpcTransaction,
    executable_tx: AccountTransaction,
    runtime: tokio::runtime::Handle,
}

---

crates/apollo_gateway/src/gateway.rs line 266 at r2 (raw file):

    // TODO(Arni): Make into async function and remove all block_on calls once we manage removing
    // the spawn_blocking call.

Of note, you are practically doing a part of this task. So I am generally in favor of this PR.

Code quote:

    // TODO(Arni): Make into async function and remove all block_on calls once we manage removing
    // the spawn_blocking call.

[alonh5]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @noamsp-starkware, and @ShahakShama)

---

crates/apollo_gateway/src/gateway.rs line 243 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

So, this struct holds the internal_tx just so it could later pass it along to the mempool.
passing the internal_tx to the mempool is indeed a blocking task - but I feel it is very different than other blocking tasks in the gateway.

Passing the internal_tx to the mempool is part of the component train we have in other components.
The other blocking tasks like running the blockifier are indeed blocking...

I removed the internal tx from here in hte next (next next) PR, to avoid conflicts.

---

crates/apollo_gateway/src/gateway.rs line 266 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Of note, you are practically doing a part of this task. So I am generally in favor of this PR.

You're right it does do some of it. I don't think we want to make this async because the blockifier isn't async, so this will remain a blocking task. I removed the TODO

---

crates/apollo_gateway/src/gateway.rs line 269 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Move this before the conversions.

No need to process a transaction that does not pass these -simple and lightweight - validations.

Done in next (next) PR to avoid conflicts.

[ArniStarkware]

@ArniStarkware reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @ayeletstarkware and @noamsp-starkware)

[ArniStarkware]

@ArniStarkware reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware and @noamsp-starkware)

[ArniStarkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5, @ayeletstarkware, and @noamsp-starkware)

---

crates/apollo_gateway/src/gateway.rs line 266 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

You're right it does do some of it. I don't think we want to make this async because the blockifier isn't async, so this will remain a blocking task. I removed the TODO

I don't think we should remove this todo.
The stateful transaction validator is secretly async - it gets the tokio::runtime as a parameter. This is partially what this todo is about. See extract_state_nonce_and_run_validations.

[alonh5]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @ayeletstarkware, and @noamsp-starkware)

---

crates/apollo_gateway/src/gateway.rs line 266 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

I don't think we should remove this todo.
The stateful transaction validator is secretly async - it gets the tokio::runtime as a parameter. This is partially what this todo is about. See extract_state_nonce_and_run_validations.

The only part that's async is the mempool query, we will not be removing the spawn_blocking as long as the blockifier is synchronous. I added a task to monday to try and extract the mempool query as well and then not having to pass the runtime.

[ArniStarkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware and @noamsp-starkware)