apollo_gateway: only set proofs if they have passed tx validation in gateway

[einat-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[einat-starkware]

• apollo_integration_tests: add txs with proof #11729
• apollo_transaction_converter: use proof manager to convert between transactions #11707
• apollo_transaction_converter: move transaction converter to its own crate #11705
• apollo_proof_manager: log verify and store times #11471
• apollo_gateway: only set proofs if they have passed tx validation in gateway #11703 👈 (View in Graphite)
• apollo_gateway: set the proof when adding a transaction #11638
• apollo_gateway: create GCS client and write proof to GCS #11497
• apollo_gateway: add large storage writer #11362
• apollo_gateway_config: create proof archive writer config #11478
• apollo_gateway: create large storage writer trait #11361
• apollo_gateway: use proof manager #11141 : 1 other dependent PR (#11147 )
• apollo_batcher: delete proof validator and move proof verification to transaction converter #11421 : 1 other dependent PR (#11157 )
• apollo_proof_manager: add proof manager client to the transaction converter #11044
• apollo_node: integrate proof manager server #11043
• apollo_node: integrate proof manager client into node #11042
• apollo_proof_manager: add metrics file #11041
• apollo_http_server: turn off unsupported tx version test #11625
• main

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

[einat-starkware]

@einat-starkware made 1 comment.
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @noaov1).

---

crates/apollo_class_manager_types/src/transaction_converter_test.rs line 125 at r2 (raw file):

#[rstest]
#[tokio::test]
async fn test_proof_verification_skipped_when_proof_already_exists() {

I removed this test because set_proof wouldn't be called anyways after moving it to the gateway so it doesn't check anything

[ArniStarkware]

crates/apollo_gateway/src/gateway_test.rs line 319 at r3 (raw file):

        if !invoke_tx.proof_facts.is_empty() {
            mock_dependencies
                .expect_set_proof(invoke_tx.proof_facts.clone(), invoke_tx.proof.clone());

Suggestion:

.expect_set_proof(invoke_tx.proof_facts.clone(), invoke_tx.proof.clone())
.once()
.with(eq(...)); // If it is easy (i.e., this part of the comment is non-blocking), 
                // please add to the expectation the expected input to this call of `set_proof`). 

[ArniStarkware]

crates/apollo_gateway/src/gateway_test.rs line 171 at r3 (raw file):

        self.mock_transaction_converter
            .expect_get_proof_manager_client()
            .returning(move || pmc.clone());

Why here and not in the constructor of MockDependencies?

Code quote:

        let proof_manager_client: Arc<dyn ProofManagerClient> =
            Arc::new(self.mock_proof_manager_client);
        let pmc = proof_manager_client.clone();
        self.mock_transaction_converter
            .expect_get_proof_manager_client()
            .returning(move || pmc.clone());

[ArniStarkware]

crates/apollo_gateway/src/gateway.rs line 273 at r3 (raw file):

    let transaction_converter = Arc::new(TransactionConverter::new(
        class_manager_client,
        proof_manager_client.clone(),

why clone?

Code quote:

        proof_manager_client.clone(),

[ArniStarkware]

crates/apollo_class_manager_types/src/transaction_converter.rs line 161 at r3 (raw file):

                };

                let internal_tx = self.convert_rpc_tx_to_internal_rpc_tx(tx).await?;

Please move this logic into convert_rpc_tx_to_internal_rpc_tx. It has a match arm for invoke anyway...

Code quote:

convert_rpc_tx_to_internal_rpc_tx

[einat-starkware]

crates/apollo_class_manager_types/src/transaction_converter.rs line 161 at r3 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Please move this logic into convert_rpc_tx_to_internal_rpc_tx. It has a match arm for invoke anyway...

I don't want this logic in there as that function also gets called in the gateway (where I don't want to write until after the validations in the gateway have passed).

[einat-starkware]

@einat-starkware made 3 comments.
Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @meship-starkware, and @noaov1).

---

crates/apollo_gateway/src/gateway.rs line 273 at r3 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

why clone?

An accidental leftover from an earlier version of the PR, removed now :)

---

crates/apollo_gateway/src/gateway_test.rs line 171 at r3 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Why here and not in the constructor of MockDependencies?

It needs to be here I believe so that the mock_proof_manager_client is accessible in the tests (so I can check that it gets called).

---

crates/apollo_gateway/src/gateway_test.rs line 319 at r3 (raw file):

        if !invoke_tx.proof_facts.is_empty() {
            mock_dependencies
                .expect_set_proof(invoke_tx.proof_facts.clone(), invoke_tx.proof.clone());

This is already done in the inner function that this function is a wrapper for :)

[ArniStarkware]

crates/apollo_gateway/src/gateway_test.rs line 171 at r3 (raw file):

Previously, einat-starkware wrote…

It needs to be here I believe so that the mock_proof_manager_client is accessible in the tests (so I can check that it gets called).

Please try moving it into

#[fixture]
fn mock_dependencies() -> MockDependencies {

[ArniStarkware]

@ArniStarkware made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @einat-starkware, @meship-starkware, and @noaov1).

---

crates/apollo_gateway/src/gateway_test.rs line 319 at r3 (raw file):

Previously, einat-starkware wrote…

This is already done in the inner function that this function is a wrapper for :)

I don't understand. If this is already done, why do it here?

this = expect_set_proof.

[ArniStarkware]

crates/apollo_class_manager_types/src/transaction_converter.rs line 161 at r3 (raw file):

Previously, einat-starkware wrote…

I don't want this logic in there as that function also gets called in the gateway (where I don't want to write until after the validations in the gateway have passed).

In that case, please define a new function: extract_proof_and_convert_rpc_tx_to_internal_rpc_tx

[einat-starkware]

@einat-starkware made 3 comments.
Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @meship-starkware, and @noaov1).

---

crates/apollo_class_manager_types/src/transaction_converter.rs line 161 at r3 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

In that case, please define a new function: extract_proof_and_convert_rpc_tx_to_internal_rpc_tx

Done.

---

crates/apollo_gateway/src/gateway_test.rs line 171 at r3 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Please try moving it into

#[fixture]
fn mock_dependencies() -> MockDependencies {

I can't figure out how to do this while also keeping the check that I call set_proof for V3 invoke transactions with proof facts (which is what I actually want to test). I believe it's because I extract the proof manager client from the transaction converter using get_proof_manager_client() which gives me a clone of the original and then I can't set expectations for the clone. If you have an idea how to get around this let me know.

---

crates/apollo_gateway/src/gateway_test.rs line 319 at r3 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

I don't understand. If this is already done, why do it here?

this = expect_set_proof.

I defined mock_dependencies.expect_set_prooffollowing the pattern of the mempool client's expect_add_tx

[einat-starkware]

@einat-starkware made 1 comment.
Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @meship-starkware, and @noaov1).

---

crates/apollo_gateway/src/gateway_test.rs line 171 at r3 (raw file):

Previously, einat-starkware wrote…

I can't figure out how to do this while also keeping the check that I call set_proof for V3 invoke transactions with proof facts (which is what I actually want to test). I believe it's because I extract the proof manager client from the transaction converter using get_proof_manager_client() which gives me a clone of the original and then I can't set expectations for the clone. If you have an idea how to get around this let me know.

Added as a TODO for now

[ArniStarkware]

crates/apollo_gateway/src/gateway_test.rs line 319 at r3 (raw file):

Previously, einat-starkware wrote…

I defined mock_dependencies.expect_set_prooffollowing the pattern of the mempool client's expect_add_tx

Unblocking,
But it would be nice if we did not have unnecessary expectations.

[ArniStarkware]

@ArniStarkware made 1 comment and resolved 2 discussions.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @einat-starkware, @meship-starkware, and @noaov1).