apollo_proof_manager: read/write proof from file methods

[einat-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[einat-starkware]

• apollo_batcher: delete proof validator and move proof verification to transaction converter #11421 : 2 dependent PRs (#11141 , #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_proof_manager: component request handler #11040
• apollo_proof_manager: implement component starter #11039
• apollo_proof_manager_types: create proof manager client #10965
• apollo_proof_manager: calculate hash within the proof storage #11382
• apollo_proof_manager: enable proof manager and add to deployments #11274
• apollo_proof_manager: add proof manager to node config and components #10956
• apollo_node_config: add proof manager config to node config #11242
• apollo_proof_manager: create proof manager #10943
• apollo_proof_manager: test fs proof storage #10902
• apollo_proof_manager: impl ProofStorage #10901
• apollo_proof_manager: read/write proof from file methods #10899 👈 (View in Graphite)
• apollo_proof_manager: add fs directory methods #10862
• apollo_proof_manager: add ProofStorage trait #10798
• main

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

[noaov1]

@noaov1 reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @AvivYossef-starkware and @einat-starkware)

---

crates/apollo_proof_manager/src/proof_storage.rs line 91 at r1 (raw file):

    /// Writes a proof to a file in binary format.
    /// The file is named `proof` inside the given directory.

Should the path contain a "proof" suffix?
Should we add let path = dir.join("proof");?

Code quote:

    /// The file is named `proof` inside the given directory.

---

crates/apollo_proof_manager/src/proof_storage.rs line 99 at r1 (raw file):

        // Open a file for writing, deleting any existing content.
        let file = OpenOptions::new().create(true).write(true).truncate(true).open(path)?;

Can this fail?

Code quote:

        let file = OpenOptions::new().create(true).write(true).truncate(true).open(path)?;

---

crates/apollo_proof_manager/src/proof_storage.rs line 104 at r1 (raw file):

        for &value in proof.iter() {
            writer.write_all(&value.to_le_bytes())?;

Same question

Code quote:

writer.write_all(&value.to_le_bytes())?;

---

crates/apollo_proof_manager/src/proof_storage.rs line 107 at r1 (raw file):

        }

        writer.flush()?;

BTW- Why little endian?

Suggestion:

        // Pre-allocate exactly enough space: 4 bytes per u32.
        let mut buf = Vec::with_capacity(proof.len() * std::mem::size_of::<u32>());

        for &value in proof.iter() {
            buf.extend_from_slice(&value.to_le_bytes());
        }
    
        // Single write instead of one per element.
        writer.write_all(&buf)?;
        writer.flush()?;

---

crates/apollo_proof_manager/src/proof_storage.rs line 125 at r1 (raw file):

        let proof_data =
            buffer.chunks_exact(4).map(|c| u32::from_le_bytes(c.try_into().unwrap())).collect();

Why should it always suceeded?
Please use expect

Code quote:

.unwrap())

[einat-starkware]

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

---

crates/apollo_proof_manager/src/proof_storage.rs line 91 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Should the path contain a "proof" suffix?
Should we add let path = dir.join("proof");?

I had it in the outer function, but moved it to here because I agree that it makes more sense

---

crates/apollo_proof_manager/src/proof_storage.rs line 99 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can this fail?

No - changed to use expect :)

---

crates/apollo_proof_manager/src/proof_storage.rs line 107 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

BTW- Why little endian?

No particular reason, changes to big endian to be consistent with the directory methods

---

crates/apollo_proof_manager/src/proof_storage.rs line 125 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why should it always suceeded?
Please use expect

Done.

[noaov1]

@noaov1 reviewed 1 file and all commit messages, made 2 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware and @einat-starkware).

---

crates/apollo_proof_manager/src/proof_storage.rs line 117 at r2 (raw file):

        }

        // Single write instead of one per element.

Suggestion:

        // Single write.

[noaov1]

@noaov1 reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware and @einat-starkware).

[einat-starkware]

@einat-starkware made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @noaov1).

---

crates/apollo_proof_manager/src/proof_storage.rs line 104 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Same question

Done.

[graphite-app]

Merge activity

• Dec 22, 8:54 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.

[noaov1]

@noaov1 resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware).