apollo_transaction_converter: use proof manager to convert between transactions

[einat-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[einat-starkware]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• apollo_integration_tests: add txs with proof #11729
• apollo_transaction_converter: use proof manager to convert between transactions #11707 👈 (View in Graphite)
• 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
• 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.

[avivg-starkware]

@avivg-starkware reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @einat-starkware).

---

crates/apollo_transaction_converter/src/transaction_converter.rs line 122 at r1 (raw file):

            .get_proof(proof_facts.clone())
            .await?
            .ok_or(TransactionConverterError::ProofNotFound { facts_hash: proof_facts.hash() })

Does using the proof_facts hash follow a convention used elsewhere in the code? Is it logged somewhere?

Code quote:

facts_hash: proof_facts.hash() 

---

crates/apollo_transaction_converter/src/transaction_converter.rs line 204 at r1 (raw file):

                    account_deployment_data: tx.account_deployment_data,
                    proof_facts: tx.proof_facts.clone(),
                    proof: self.get_proof(tx.proof_facts).await?,

Document why we expect to already have the proof saved at that stage

Extra - consider documenting for the Declare as well:)

Code quote:

proof: self.get_proof(tx.proof_facts).await?,

[einat-starkware]

@einat-starkware made 2 comments.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @avivg-starkware).

---

crates/apollo_transaction_converter/src/transaction_converter.rs line 122 at r1 (raw file):

Previously, avivg-starkware wrote…

Does using the proof_facts hash follow a convention used elsewhere in the code? Is it logged somewhere?

The hash gets used as the name of the file/folder where we write the proof (i.e. where we would expect to find the proof). The proof facts don't implement display so this is the best alternative I could think of

---

crates/apollo_transaction_converter/src/transaction_converter.rs line 204 at r1 (raw file):

Previously, avivg-starkware wrote…

Document why we expect to already have the proof saved at that stage

Extra - consider documenting for the Declare as well:)

Added an explanation :)

[avivg-starkware]

@avivg-starkware reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @einat-starkware and @meship-starkware).

---

crates/apollo_transaction_converter/src/transaction_converter.rs line 122 at r1 (raw file):

Previously, einat-starkware wrote…

The hash gets used as the name of the file/folder where we write the proof (i.e. where we would expect to find the proof). The proof facts don't implement display so this is the best alternative I could think of

I see. Sounds good. @meship-starkware - Is there a convention you're using for logging errors of proof facts - Just making sure this aligns

---

crates/apollo_transaction_converter/src/transaction_converter.rs line 204 at r1 (raw file):

Previously, einat-starkware wrote…

Added an explanation :)

Is it in the GW in all cases? (mempool and consensus)

[meship-starkware]

@meship-starkware made 1 comment.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @einat-starkware).

---

crates/apollo_transaction_converter/src/transaction_converter.rs line 122 at r1 (raw file):

Previously, avivg-starkware wrote…

I see. Sounds good. @meship-starkware - Is there a convention you're using for logging errors of proof facts - Just making sure this aligns

Not really, I do use the InvalidProofFact error in the StarkNet API

[avivg-starkware]

@avivg-starkware resolved 1 discussion.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @einat-starkware).

[einat-starkware]

@einat-starkware made 1 comment.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @avivg-starkware).

---

crates/apollo_transaction_converter/src/transaction_converter.rs line 204 at r1 (raw file):

Previously, avivg-starkware wrote…

Is it in the GW in all cases? (mempool and consensus)

This function only gets called by proposers so we know that if there are proof facts then there should be proofs also - there is one case where it isn't added (when the proof facts are empty which I now handle :))

[ArniStarkware]

@ArniStarkware made 1 comment.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @avivg-starkware and @einat-starkware).

---

crates/apollo_transaction_converter/src/transaction_converter.rs line 220 at r3 (raw file):

                    },
                })))
            }

Don't clone proof_facts.
(ignore my message if get_proof is consuming - which I assume it is not...).

Suggestion:

            InternalRpcTransactionWithoutTxHash::Invoke(tx) => {
                // We expect the proof to be available here because it has already been verified
                // and stored by the proof manager in the gateway.
                let proof = if tx.proof_facts.is_empty() {
                        Proof::default()
                    } else {
                        self.get_proof(tx.proof_facts).await?
                    };
                Ok(RpcTransaction::Invoke(RpcInvokeTransaction::V3(RpcInvokeTransactionV3 {
                    resource_bounds: tx.resource_bounds,
                    signature: tx.signature,
                    nonce: tx.nonce,
                    tip: tx.tip,
                    paymaster_data: tx.paymaster_data,
                    nonce_data_availability_mode: tx.nonce_data_availability_mode,
                    fee_data_availability_mode: tx.fee_data_availability_mode,
                    sender_address: tx.sender_address,
                    calldata: tx.calldata,
                    account_deployment_data: tx.account_deployment_data,
                    proof_facts: tx.proof_facts,
                    proof,
                })))
            }

[avivg-starkware]

@avivg-starkware made 1 comment and resolved 1 discussion.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @einat-starkware).

[einat-starkware]

@einat-starkware made 1 comment.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @avivg-starkware).

---

crates/apollo_transaction_converter/src/transaction_converter.rs line 220 at r3 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Don't clone proof_facts.
(ignore my message if get_proof is consuming - which I assume it is not...).

Done.