Straus algorithm implementation#312
Conversation
|
Also please squash your commits & fix pipeline |
|
Just a note - obviously we'll squash, the commit history is a mess |
ArtiomTr
left a comment
There was a problem hiding this comment.
Tests still fail:
- Parallel tests:
cargo test -p rust-kzg-blst --no-default-features --features strauss,std,rand,parallel
Fail with:
error[E0599]: no method named `multiply_parallel` found for reference `&StraussTable<TFr, TG1, TG1Fp, TG1Affine, TProjAddAffine>` in the current scope
--> kzg/src/msm/msm_impls.rs:26:24
|
26 | precomputation.multiply_parallel(scalars)
| ^^^^^^^^^^^^^^^^^ method not found in `&StraussTable<TFr, TG1, TG1Fp, TG1Affine, TProjAddAffine>`
NOTE: No need to implement parallel MSM version. You can just call sequential version now.
- Mcl backend MSM:
cargo test -p rust-kzg-mcl --no-default-features --features std,rand,strauss
Fail with:
test tests::g1_make_linear_combination_ ... FAILED
test tests::g1_small_linear_combination_ ... FAILED
test tests::g1_random_linear_combination_ ... FAILED
failures:
---- tests::g1_make_linear_combination_ stdout ----
thread 'tests::g1_make_linear_combination_' panicked at mcl/src/types/g1.rs:494:9:
not yet implemented
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- tests::g1_small_linear_combination_ stdout ----
thread 'tests::g1_small_linear_combination_' panicked at mcl/src/types/g1.rs:494:9:
not yet implemented
---- tests::g1_random_linear_combination_ stdout ----
thread 'tests::g1_random_linear_combination_' panicked at mcl/src/types/g1.rs:494:9:
not yet implemented
failures:
tests::g1_make_linear_combination_
tests::g1_random_linear_combination_
tests::g1_small_linear_combination_
kzg/src/msm/strauss.rs
Outdated
| scalars | ||
| .iter() | ||
| .map(|s| self.multiply_sequential(s)) | ||
| .collect() |
There was a problem hiding this comment.
For the third time, this code is wrong. It doesn't matter how many times you resolve this comment, your code won't magically fix itself.
You're using wrong table here. If ever this code path gets hit, you'll get incorrect result. And you can easily check other implementations - bgmw and wbits - and you'll see, that those implementations don't do this.
I still have no idea why you keep this code branch here. Why if check is needed here at all? You can replace your code with panic!, and not even single test will fail - this code is unreachable, only else { branch gets triggered.
constantine/tests/mod.rs
Outdated
| @@ -1 +1,4 @@ | |||
| pub mod local_tests; | |||
|
|
|||
| #[cfg(test)] | |||
There was a problem hiding this comment.
no need to put #[cfg(test)] twice
constantine/tests/serialization.rs
Outdated
|
|
||
| // Test generator | ||
| let ct_gen = CtG1::generator(); | ||
| let blst_gen = rust_kzg_blst::types::g1::FsG1::generator(); |
There was a problem hiding this comment.
all good, just don't overcomplicate this test so much: no need to bring rust-kzg-blst as dependency - you can just hardcode bytes here. For convenience you can use hex crate, then it will look like so:
let point = CtG1::generator().mul(&CtFr::from_u64(5));
let bytes = point.to_bytes_uncompressed();
assert_eq!(bytes, hex::decode("<hex value>"));
assert!(point.equals(&CtG1::from_bytes_uncompressed(bytes).unwrap()));And that's all, no need for any other checks. Then you can copy-paste this few times, to test with few more points (e.g., g * 5, g * 7, g * 1235, ...).
ArtiomTr
left a comment
There was a problem hiding this comment.
looks good 👍 commits need to be squashed tho
none of us can merge, we don't have write/push access, can you squash merge? |
BenasKc
left a comment
There was a problem hiding this comment.
Since I am added as a reviewer, I guess I still have to approve?
you don't need push access to rebase your own branch, and squash commits into one. |
|
The build fails because the dependency URLs in github return 500, will investigate. No additional changes have been made |
|
Please attribute all authors properly: https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors |
Co-authored-by: Eimantoshis <eimantas.labzentis@mif.stud.vu.lt> Co-authored-by: Cartelis <deividas.sapalas@mif.stud.vu.lt>
|
Added all the authors |

No description provided.