-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_gateway: use proof manager #11141
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: einat/proof_manager/delete_proof_validator
Are you sure you want to change the base?
apollo_gateway: use proof manager #11141
Conversation
b22bbce to
6d3080f
Compare
be181f4 to
3ee7e01
Compare
6d3080f to
d726a58
Compare
a730132 to
9a8c6ce
Compare
d726a58 to
2f896e9
Compare
noaov1
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.
@noaov1 reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @einat-starkware).
crates/apollo_gateway/src/gateway.rs line 149 at r1 (raw file):
.map_err(convert_proof_manager_error)?; } }
Consider moving it to convert_rpc_tx_to_internal_rpc_tx.
Suggestion:
// If a transaction has a proof then we add it to the proof manager.
if let RpcTransaction::Invoke(RpcInvokeTransaction::V3(ref tx)) = tx {
if !tx.proof_facts.is_empty() {
let proof_facts_hash = tx.proof_facts.hash();
self.proof_manager_client
.set_proof(proof_facts_hash, tx.proof.clone())
.await
.map_err(convert_proof_manager_error)?;
}
}9a8c6ce to
df265bc
Compare
90a06bc to
4fb6110
Compare
noaov1
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.
@noaov1 reviewed 3 files and all commit messages, made 3 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @einat-starkware).
crates/apollo_gateway/src/gateway.rs line 148 at r4 (raw file):
.set_proof(proof_facts_hash, tx.proof.clone()) .await .map_err(convert_proof_manager_error)?;
Suggestion:
.map_err(|e| StarknetError::internal_with_logging(
"Proof manager error",
&e)?;crates/apollo_gateway/src/errors.rs line 478 at r4 (raw file):
} } }
Why is this method needed?
See my comment below.
Code quote:
pub fn convert_proof_manager_error(err: ProofManagerClientError) -> StarknetError {
match err {
ProofManagerClientError::ClientError(client_error) => {
StarknetError::internal_with_logging("Proof manager client error", &client_error)
}
ProofManagerClientError::ProofManagerError(proof_manager_error) => {
match proof_manager_error {
ProofManagerError::Client(_) => StarknetError::internal_with_logging(
"Proof manager error",
&proof_manager_error,
),
ProofManagerError::ProofStorage(_) | ProofManagerError::Io(_) => {
StarknetError::internal_with_logging(
"Proof manager storage error",
&proof_manager_error,
)
}
}
}
}
}crates/apollo_gateway/src/gateway_test.rs line 282 at r4 (raw file):
let input_tx = tx_args.get_rpc_tx(); let expected_internal_tx = tx_args.get_internal_tx(); if let RpcTransaction::Invoke(RpcInvokeTransaction::V3(ref v3_tx)) = input_tx {
Where can I see that we do the same for declare (and sierra/casm)?
Same question below
Code quote:
if let RpcTransaction::Invoke(RpcInvokeTransaction::V3(ref v3_tx)) = input_tx {
noaov1
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.
@noaov1 made 1 comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @einat-starkware).
crates/apollo_gateway/src/gateway.rs line 142 at r4 (raw file):
// TODO(Einat): Consider moving to `convert_rpc_tx_to_internal_and_executable_txs` function. // If a transaction has a proof then we add it to the proof manager. if let RpcTransaction::Invoke(RpcInvokeTransaction::V3(ref tx)) = tx {
Please note that we already validated the proof.
It will be better when you will move this to convert_rpc_tx_to_internal_and_executable_txs, this way it will be clearer why we store the proofs (right before we "dump" them)
Code quote:
if let RpcTransaction::Invoke(RpcInvokeTransaction::V3(ref tx)) = tx {
noaov1
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.
@noaov1 made 1 comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @einat-starkware).
crates/apollo_gateway/src/gateway.rs line 142 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Please note that we already validated the proof.
It will be better when you will move this toconvert_rpc_tx_to_internal_and_executable_txs, this way it will be clearer why we store the proofs (right before we "dump" them)
Note- the gateway code is different from the validators. In the gateway- we validate the proof using the StatelessTransactionValidator while in the batcher (validators) we use ProofValidator. Why?
4fb6110 to
c70fbc1
Compare
77fc5f5 to
2eb1fac
Compare
2eb1fac to
e78f7a2
Compare
c70fbc1 to
4b0a8bf
Compare
4b0a8bf to
40925fa
Compare
einat-starkware
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.
@einat-starkware made 4 comments.
Reviewable status: 0 of 8 files reviewed, 4 unresolved discussions (waiting on @noaov1).
crates/apollo_gateway/src/errors.rs line 478 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why is this method needed?
See my comment below.
Not needed, I now added the error to the transaction converter error enum (as is done by the class manager client error as well).
crates/apollo_gateway/src/gateway.rs line 142 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Note- the gateway code is different from the validators. In the gateway- we validate the proof using the
StatelessTransactionValidatorwhile in the batcher (validators) we useProofValidator. Why?
Moved directly into the transaction converter's convert_rpc_tx_to_internal_rpc_tx function. In the next PR, I delete the proof validator and add a verify_proof function into the transaction converter.
crates/apollo_gateway/src/gateway_test.rs line 282 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Where can I see that we do the same for declare (and sierra/casm)?
Same question below
all changes reverted here now that the proof manager is in the transaction converter (test is there instead as well).
crates/apollo_gateway/src/gateway.rs line 148 at r4 (raw file):
.set_proof(proof_facts_hash, tx.proof.clone()) .await .map_err(convert_proof_manager_error)?;
Deleted the convert_proof_manager_error function entirely :)
e78f7a2 to
54a1f1b
Compare
40925fa to
2a31748
Compare
noaov1
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.
@noaov1 reviewed 8 files and all commit messages, made 2 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @einat-starkware).
crates/apollo_class_manager_types/src/transaction_converter_test.rs line 64 at r5 (raw file):
#[rstest] #[tokio::test] async fn test_proof_manager_called_when_proof_facts_present() {
Can you please add a test case to verify that set_proof is not called in case of an empty proof?
Suggestion:
async fn test_set_proof_called_for_invoke_v3_with_proof_facts() {crates/apollo_class_manager_types/src/transaction_converter.rs line 210 at r5 (raw file):
.set_proof(tx.proof_facts.clone(), tx.proof.clone()) .await?; }
Please add acomment that at this point we assume the proof has been lready verified.
Code quote:
if !tx.proof_facts.is_empty() {
self.proof_manager_client
.set_proof(tx.proof_facts.clone(), tx.proof.clone())
.await?;
}54a1f1b to
fdd9d55
Compare
2a31748 to
2a28293
Compare
einat-starkware
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.
@einat-starkware made 2 comments.
Reviewable status: 6 of 9 files reviewed, 2 unresolved discussions (waiting on @noaov1).
crates/apollo_class_manager_types/src/transaction_converter.rs line 210 at r5 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Please add acomment that at this point we assume the proof has been lready verified.
We no longer assume this (as it gets verified within the new function)
crates/apollo_class_manager_types/src/transaction_converter_test.rs line 64 at r5 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Can you please add a test case to verify that
set_proofis not called in case of an empty proof?
Done. Also added to test that set_proof isn't called if the proof already exists.
fdd9d55 to
42486e5
Compare
2a28293 to
0846765
Compare

No description provided.