Bos coster msm #311

mykolas-alt · 2025-10-22T12:25:13Z

No description provided.

ArtiomTr

Please rebase from main branch, and when you finish, squash all your commits into one

ArtiomTr · 2025-12-09T15:44:00Z

kzg/src/msm/bos_coster.rs

+
+impl Scalar256 {
+    #[inline(always)]
+    fn bitlen_256(&self) -> u32 {


I don't think this is a good idea to extend type for your custom usecase - move them to near scalar struct definition, or replace with simple functions

also, don't make up your own functions - it is better to mirror standard library, e.g. instead of bitlen_256, you can easily make leading_zeros function, and then do something like Scalar256::BITS - scalar.leading_zeros() - so it looks the same, like for other types, e.g.: u64::BITS - something.leading_zeros().

ArtiomTr · 2025-12-10T10:37:59Z

Cargo.toml

-    "arkworks3-sppark",
-    "arkworks3-sppark-wlc",
    "blst",
-    "blst-sppark",


please keep CUDA backends, don't delete them

ArtiomTr · 2025-12-10T10:40:02Z

fuzz/fuzz_targets/blst_fixed_msm.rs

@@ -0,0 +1,65 @@
+#![no_main]


all fuzzing is already merged into main branch, so need to be rebased

ArtiomTr · 2025-12-10T10:48:16Z

kzg/src/msm/bos_coster.rs

+    }
+
+    #[inline(always)]
+    fn shl_256(&self, n: u32) -> Scalar256 {


I haven't read code thoroughly, but it looks like << implementation for scalar type. For standard operations, please use traits defined in standard library, e.g. here it would be Shl. So you can then use them nicely:

let scalar: Scalar256; scalar << 8;

ArtiomTr · 2025-12-10T10:52:31Z

kzg/src/msm/bos_coster.rs

+        if heap.is_empty() {
+            return TG1::zero();
+        }
+
+        let pair = heap.pop().unwrap();
+        return pair.point.mul(&TFr::from_u64_arr(&pair.scalar.data));


this can be written a bit more idiomatically:

if let Some(pair) = heap.pop() { pair.point.mul(&TFr::from_u64_arr(&pair.scalar.data)); } else { TG1::zero() }

allows to remove panicking .unwrap() call

ArtiomTr · 2025-12-10T11:34:52Z

kzg/src/msm/bos_coster.rs

+                for _ in 0..k {
+                    new_point.dbl_assign();
+                }
+                pair2.point.add_assign(&new_point);


unless you're absolutely sure that pair2 and new_point are not equal, you should use add_or_dbl_assign

ArtiomTr · 2025-12-10T11:37:16Z

kzg/src/msm/bos_coster.rs

+    }
+    let mut d = bitlen_x - bitlen_y;
+    let mut shifted_y = y.shl_256(d);
+    if shifted_y.cmp(&x) == std::cmp::Ordering::Greater {


why not shifted_y > x?

ArtiomTr · 2025-12-10T11:39:01Z

kzg/src/msm/bos_coster.rs

+    }
+}
+
+// x >= y


you can make this comment as debug_assert, to detect errors early without slowing down runtime:

debug_assert!(x >= y);

ArtiomTr · 2025-12-10T11:45:08Z

kzg/src/msm/bos_coster.rs

+
+// x >= y
+#[inline(always)]
+fn find_k(x: &Scalar256, y: &Scalar256) -> (u32, Scalar256) {


really confusing function name - rename it

ArtiomTr · 2025-12-10T11:46:24Z

msm-benches/tests/failing_fuzzer.rs

@@ -0,0 +1,101 @@
+#[cfg(test)]
+mod tests {


msm-benches crate is not for custom tests - keep them in backend crate(s)

mykolas-alt · 2025-12-14T19:48:08Z

rebased, aligned api with the way arkmsm does it

ArtiomTr · 2025-12-15T10:40:18Z

kzg/src/msm/bos_coster.rs

+    }
+}
+
+impl Ord for Scalar256 {


still not fixed - please keep all trait implementations near struct definition, not in separate file

ArtiomTr · 2025-12-15T10:41:33Z

kzg/src/msm/bos_coster.rs

+}
+
+trait Shr1Assign {
+    fn shr_once_assign(&mut self);


I don't think this is good idea to create custom method just for >> 1 -- just implement general Shr, and with inline(always) compiler will optimize that usecase for you.

ArtiomTr · 2025-12-15T11:54:55Z

kzg/src/msm/bos_coster.rs

+
+use crate::{Fr, G1Affine, G1Fp, G1Mul, G1ProjAddAffine, Scalar256, G1};
+
+pub struct VariableBaseMSM;


should be named "BosCosterMSM"

ArtiomTr · 2025-12-15T12:11:50Z

kzg/src/msm/bos_coster.rs

+                break;
+            };
+            let pair2_scalar = {
+                let Some(pair2) = heap.peek_mut() else {


this no longer needs to be peek_mut

ArtiomTr · 2025-12-15T12:12:32Z

kzg/src/msm/bos_coster.rs

+            let Some(pair1) = heap.pop() else {
+                break;
+            };
+            let pair2_scalar = {


I believe you can remove this block entirely, and just put all its content into loop body

ArtiomTr · 2025-12-15T12:14:36Z

kzg/src/msm/bos_coster.rs

+    }
+}
+
+trait LeadingZeros {


no need to define extension trait, just implement needed method directly on struct

ArtiomTr · 2025-12-15T12:15:30Z

kzg/src/msm/bos_coster.rs

+impl Shl<u32> for Scalar256 {
+    type Output = Scalar256;
+
+    // This function was generated with ChatGPT


would be nice to add unit tests for this function

ArtiomTr · 2025-12-15T12:18:07Z

kzg/src/msm/msm_impls.rs

    precomputation: Option<&PrecomputationTable<TFr, TG1, TG1Fp, TG1Affine, TProjAddAffine>>,
 ) -> TG1 {
-    #[cfg(not(feature = "arkmsm"))]
+    #[cfg(not(any(feature = "arkmsm", feature = "bos_coster")))]


I believe bos_coster shouldn't conflict with precomputation methods? you can probably just put it instead of pippenger call, if feature is enabled.

what do you mean by conflict? how does arkmsm conflict?

you mean change it like this?

what do you mean by conflict?

For example, you can't use bgmw and wbits at the same time, pippenger and arkmsm, etc. - that is just does not make sense.

how does arkmsm conflict?

arkmsm doesn't, someone who implemented arkmsm just didn't think about this.

you mean change it like this?

exactly! maybe there are more places, where pippenger is being called, but that's fine for now.

ArtiomTr · 2025-12-15T12:18:33Z

msm-benches/Cargo.toml

+    "rust-kzg-blst/bos_coster",
+    "rust-kzg-arkworks4/bos_coster",
+    "rust-kzg-arkworks5/bos_coster",
+    "rust-kzg-constantine/bos_coster",


mcl missing here

ArtiomTr · 2025-12-15T12:25:00Z

fuzz/Cargo.toml

    "rust-kzg-mcl/parallel",
 ]
+bos_coster = [
+    "rust-kzg-blst/bos_coster",


other backends are missing here

after #322 merged, zkcrypto backend also should be working. MCL is also missing here

pairing heap

ArtiomTr

looks good, but still few changes needed. Also, don't forget to fix CI - kzg crate needs formatting to be fixed, and zkcrypto crate now has some issues with clippy. Those clippy issues would probably resolve after you remove feature checks between bos_coster and bgmw/arkmsm/wbits ones.

ArtiomTr · 2025-12-17T08:03:20Z

fuzz/Cargo.toml

    "rust-kzg-mcl/parallel",
 ]
+bos_coster = [
+    "rust-kzg-blst/bos_coster",


after #322 merged, zkcrypto backend also should be working. MCL is also missing here

ArtiomTr · 2025-12-17T08:07:18Z

kzg/src/msm/precompute.rs

+    all(feature = "bgmw", feature = "bos_coster"),
+    all(feature = "sppark", feature = "wbits"),
+    all(feature = "sppark", feature = "bos_coster"),
+    all(feature = "wbits", feature = "bos_coster"),


no need to check this, bos_coster is compatible with all other features (except arkmsm, but you can ignore this for now)

ArtiomTr · 2025-12-17T08:14:03Z

kzg/src/lib.rs

+
+    // This function was generated with ChatGPT
+    #[inline(always)]
+    fn shl(self, n: u32) -> Scalar256 {


still want to see unit tests for this function. you can put them in the same file, at the end, like so:

#[cfg(test)] mod test { #[test] fn shl_must_shift_2_bits() { let scalar = /* some scalar */; let received = scalar << 2; let expected = /* scalar shifted 2 bits by hand */; assert_eq!(expected, received); } }

mykolas-alt force-pushed the bos-coster-msm branch from 820c108 to d381d85 Compare October 27, 2025 19:23

ArtiomTr suggested changes Dec 10, 2025

View reviewed changes

mykolas-alt force-pushed the bos-coster-msm branch from bd1fc73 to d506500 Compare December 14, 2025 19:44

mykolas-alt force-pushed the bos-coster-msm branch 2 times, most recently from 4ef9c02 to fc4a2db Compare December 14, 2025 21:09

ArtiomTr suggested changes Dec 15, 2025

View reviewed changes

ArtiomTr reviewed Dec 15, 2025

View reviewed changes

mykolas-alt force-pushed the bos-coster-msm branch from fc4a2db to d013787 Compare December 16, 2025 22:09

Added Bos Coster MSM implementation

b7d180b

pairing heap

mykolas-alt force-pushed the bos-coster-msm branch from d013787 to b7d180b Compare December 17, 2025 06:51

mykolas-alt marked this pull request as ready for review December 17, 2025 06:54

ArtiomTr suggested changes Dec 17, 2025

View reviewed changes


		use crate::{Fr, G1Affine, G1Fp, G1Mul, G1ProjAddAffine, Scalar256, G1};

		pub struct VariableBaseMSM;

Bos coster msm #311

Are you sure you want to change the base?

Bos coster msm #311

Conversation

mykolas-alt commented Oct 22, 2025

Uh oh!

ArtiomTr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykolas-alt commented Dec 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArtiomTr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development