Verify perf #4

NDobrev · 2025-02-07T12:54:21Z

No description provided.

crates/bls_utils/src/verification.rs

smanilov

Preliminary review; interrupted by other tasks.

src/main.rs

smanilov · 2025-02-10T13:37:57Z

src/main.rs

 }

-fn read_text_file(filename: &str) -> Result<String, Box<dyn Error>>  {
+fn read_text_file(filename: &str) -> Result<String, Box<dyn Error>> {
    let mut file = File::open(filename).map_err(|e| format!("Error opening file: {}", e))?;


The error currently looks like this:

# ./target/release/dvt_prover_host --input-file examples/dvt_bad_share.json --type share --json-schema-file foo Error opening file: No such file or directory (os error 2)

In my opinion, this is a shortcoming of std::io: the name of the file is not printed. I would add it in the error message manually:

let mut file = File::open(filename).map_err(|e| format!("Error opening file {filename}: {e}"))?;

Also, note that you can interpolate variables directly. (But not expressions; format!("{x}"); works, but format!("{call_me()}"); does not.)

I don't see this change in the current code. Did you forget to add it?

smanilov · 2025-02-10T13:42:49Z

src/main.rs

@@ -80,7 +130,7 @@ fn main() {
    }

    if args.json_schema.is_some() {


This section can be rewritten using "if let" statements:

if let Some(json_schema) = args.json_schema { if let Err(err) = validate_json(json_schema.as_str(), args.input_file.as_str()) { eprintln!("{err}"); std::process::exit(1); } }

before, it was

if args.json_schema.is_some() { let ok = validate_json(args.json_schema.unwrap().as_str(), args.input_file.as_str()); if ok.is_err() { eprintln!("{}", ok.unwrap_err()); std::process::exit(1); } }

Generally, using unwrap() is a code smell in Rust.

Additionally, use panic!() for controlled-crashing, like mentioned later:

if let Some(json_schema) = args.json_schema { if let Err(err) = validate_json(json_schema.as_str(), args.input_file.as_str()) { panic!("{err}"); } }

sounds good. Changed

Note also

if let Err(err) = validate_json(json_schema.as_str(), args.input_file.as_str()) {

instead of

let ok = validate_json(path_to_schema.as_str(), args.input_file.as_str()); if ok.is_err() {

Commenting again, in case you didn't just ignore the suggestion, but missed it.

src/main.rs

crates/bls_utils/src/input.rs

smanilov

Finished review of the share_exchange_prove guest program.

Let me know if you would like me to review more of this code (the other guest programs).

smanilov · 2025-02-12T14:58:50Z

crates/share_exchange_prove/src/main.rs

-    let found = data.verification_hashes.iter().find(|h| {
-        h == &&data.initial_commitment.hash
-    });
+    let found = data


You can use any() instead of find() for a micro-optimization in terms of using the best idiom:

let found = data .verification_hashes .iter() .any(|h| h == &data.initial_commitment.hash); if !found {

smanilov · 2025-02-12T15:26:23Z

crates/bls_utils/src/verification.rs

@@ -63,6 +65,14 @@ pub fn get_index_in_commitments(
    )))
 }

+pub fn to_g1_affine(pubkey: &dvt_abi::BLSPubkey) -> G1Affine {
+    G1Affine::from_compressed(&pubkey).into_option().unwrap()


As mentioned before, an unwrap() in the code is a code smell.

In this case, you could replace it with an expect(), which is the same, but gives context why the unwrap failed:

G1Affine::from_compressed(&pubkey) .into_option() .expect("G1 point is not torsion free.")

I inferred this from reading the source of from_compressed(), but perhaps the message could be better.

smanilov · 2025-02-12T15:31:10Z

crates/bls_utils/src/verification.rs

+        Some(g2_sig) => g2_sig,
+        None => {
+            return Err(Box::new(VerificationErrors::UnslashableError(
+                String::from(format!(


You can drop the String::from() here (and later).

smanilov · 2025-02-12T15:35:30Z

crates/bls_utils/src/verification.rs

+    let g1_pubkey = to_g1_affine(&commitment.pubkey);
+
+    let g2_sig = match G2Affine::from_compressed(&commitment.signature).into_option() {
+        Some(g2_sig) => g2_sig,


I was going to suggest the same, as I was looking at the previous version of this code locally. Great minds!

smanilov · 2025-02-12T15:42:47Z

crates/bls_utils/src/bls.rs

    let domain = b"BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_POP_";
-    let pk_projective = G1Projective::from(pubkey);
-    let sig_projective = G2Projective::from(signature);
+    <G2Projective as HashToCurve<ExpandMsgXmd<Sha256>>>::hash_to_curve(msg, domain)


Interesting. I thought maybe you could drop G2Projective as, but it turns out that you cannot. Using dyn also does not help. So much for knowing Rust.

smanilov · 2025-02-12T15:44:41Z

crates/bls_utils/src/verification.rs

+            return Err(Box::new(VerificationErrors::SlashableError(String::from(
+                format!(
+                    "Invalid field seeds_exchange_commitment.shared_secret.secret: {} \n",
+                    e


nit: this e can go in the format string

smanilov · 2025-02-12T15:46:03Z

crates/bls_utils/src/verification.rs


-    let unwraped = dest_id.unwrap() + 1;
-    let test_id = bls_id_from_u32(unwraped);
+    // F(0) is aways reserved for the aggregated key so we need to start from 1


smanilov · 2025-02-12T15:49:14Z

crates/bls_utils/src/verification.rs


    let mut cfst: Vec<G1Affine> = Vec::new();
    for pubkey in &initial_commitment.verification_vector.pubkeys {
-        cfst.push(G1Affine::from_compressed(pubkey).into_option().unwrap());
+        cfst.push(to_g1_affine(pubkey));
    }

    let le_bytes = seed_exchange.shared_secret.dst_id.clone();


This is about the following String::from on line 164, but I cannot leave a comment there, since it's not part of this PR.

For converting &str to String, there are multiple ways, none of which is idiomatic. But you might want to consider using to_owned(), as "it conveys the most intent": https://users.rust-lang.org/t/what-is-the-idiomatic-way-to-convert-str-to-string/12160/7

smanilov · 2025-02-12T15:51:20Z

crates/bls_utils/src/bls.rs

+    if count == 0 {
+        return G1Projective::identity();
+    } else if count == 1 {
+        return G1Projective::from(cfs[0]);
+    } else {
+        let mut y = cfs[count - 1];
+        for i in 2..(count + 1) {
+            y = y * x + cfs[count - i];
+        }
+        return y;
+    }


Two more ways to write this, depending on your taste:

return if count == 0 { G1Affine::identity() } else if count == 1 { cfs[0] } else { let mut y = cfst[count - 1]; for i in 2..(count + 1) { y = y * x + cfs[count - i]; } G1Affine::from(y) };

or even

if count == 0 { G1Affine::identity() } else if count == 1 { cfs[0] } else { let mut y = cfst[count - 1]; for i in 2..(count + 1) { y = y * x + cfs[count - i]; } G1Affine::from(y) }

i am not sure, if it's more readable but changed it anyway

smanilov reviewed Feb 7, 2025

View reviewed changes

crates/bls_utils/src/verification.rs Outdated Show resolved Hide resolved

smanilov reviewed Feb 10, 2025

View reviewed changes

NDobrev added 7 commits February 11, 2025 13:40

WIP

55a54dc

Update bls library and optimize the finalization circuit

2b95610

Update tests

cedb927

Minor refactoring

a138c4a

Add project for encrypted shares

f2eacf3

Add pre-commit hook

a86c8a3

Fix review comments

957e814

NDobrev force-pushed the verify-perf branch from 174e941 to 957e814 Compare February 11, 2025 11:40

Little refactoring

3ae6025

smanilov reviewed Feb 12, 2025

View reviewed changes

WIP: bad partial share prove

148e4bd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify perf #4

Verify perf #4

NDobrev commented Feb 7, 2025

smanilov left a comment

smanilov Feb 10, 2025

NDobrev Feb 10, 2025

smanilov Feb 12, 2025

smanilov Feb 10, 2025

smanilov Feb 10, 2025

NDobrev Feb 10, 2025

smanilov Feb 12, 2025

smanilov left a comment

smanilov Feb 12, 2025

smanilov Feb 12, 2025

smanilov Feb 12, 2025

smanilov Feb 12, 2025

smanilov Feb 12, 2025

smanilov Feb 12, 2025

smanilov Feb 12, 2025

smanilov Feb 12, 2025

smanilov Feb 12, 2025

NDobrev Feb 13, 2025

		@@ -80,7 +130,7 @@ fn main() {
		}

		if args.json_schema.is_some() {

Verify perf #4

Are you sure you want to change the base?

Verify perf #4

Conversation

NDobrev commented Feb 7, 2025

smanilov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smanilov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment