fix: multi_scalar_multiplication

.vscode/settings.json

@@ -53,6 +53,7 @@
         // "tests/secp256k1-add/Cargo.toml",
         // "tests/secp256k1-decompress/Cargo.toml",
         // "tests/secp256k1-double/Cargo.toml",
+        // "tests/common/Cargo.toml",
         // "tests/sha-compress/Cargo.toml",
         // "tests/sha-extend/Cargo.toml",
         // "tests/sha2/Cargo.toml",

crates/zkvm/entrypoint/src/syscalls/secp256k1.rs

@@ -8,7 +8,8 @@ use core::arch::asm;
 /// ### Safety
 ///
 /// The caller must ensure that `p` and `q` are valid pointers to data that is aligned along a four
-/// byte boundary.
+/// byte boundary. Additionally, the caller must ensure that `p` and `q` are valid points on the
+/// secp256k1 curve, and that `p` and `q` are not equal to each other.
 #[allow(unused_variables)]
 #[no_mangle]
 pub extern "C" fn syscall_secp256k1_add(p: *mut [u32; 16], q: *mut [u32; 16]) {

crates/zkvm/lib/Cargo.toml

@@ -10,14 +10,8 @@ keywords = { workspace = true }
 categories = { workspace = true }
 
 [dependencies]
-anyhow = "1.0.83"
 bincode = "1.3.3"
-cfg-if = "1.0.0"
 serde = { version = "1.0.204", features = ["derive"] }
-amcl = { package = "snowbridge-amcl", version = "1.0.2", default-features = false, features = [
-    "bls381",
-] }
-hex = "0.4.3"
 
 [features]
 default = []

crates/zkvm/lib/src/secp256k1.rs

@@ -1,14 +1,27 @@
-use crate::{syscall_secp256k1_add, syscall_secp256k1_double, utils::AffinePoint};
+use crate::{
+    syscall_secp256k1_add, syscall_secp256k1_double,
+    utils::{AffinePoint, WeierstrassAffinePoint, WeierstrassPoint},
+};
 
-/// The number of limbs in [Secp256k1AffinePoint].
+/// The number of limbs in [Secp256k1Point].
 pub const N: usize = 16;
 
 /// An affine point on the Secp256k1 curve.
 #[derive(Copy, Clone)]
 #[repr(align(4))]
-pub struct Secp256k1AffinePoint(pub [u32; N]);
+pub struct Secp256k1Point(pub WeierstrassPoint<N>);
 
-impl AffinePoint<N> for Secp256k1AffinePoint {
+impl WeierstrassAffinePoint<N> for Secp256k1Point {
+    fn infinity() -> Self {
+        Self(WeierstrassPoint::Infinity)
+    }
+
+    fn is_infinity(&self) -> bool {
+        matches!(self.0, WeierstrassPoint::Infinity)
+    }
+}
+
+impl AffinePoint<N> for Secp256k1Point {
     /// The values are taken from https://en.bitcoin.it/wiki/Secp256k1.
     const GENERATOR: [u32; N] = [
         385357720, 1509065051, 768485593, 43777243, 3464956679, 1436574357, 4191992748, 2042521214,
@@ -17,15 +30,25 @@ impl AffinePoint<N> for Secp256k1AffinePoint {
     ];
 
     fn new(limbs: [u32; N]) -> Self {
-        Self(limbs)
+        Self(WeierstrassPoint::Affine(limbs))
     }
 
     fn limbs_ref(&self) -> &[u32; N] {
-        &self.0
+        match &self.0 {
+            WeierstrassPoint::Infinity => panic!("Infinity point has no limbs"),
+            WeierstrassPoint::Affine(limbs) => limbs,
+        }
     }
 
     fn limbs_mut(&mut self) -> &mut [u32; N] {
-        &mut self.0
+        match &mut self.0 {
+            WeierstrassPoint::Infinity => panic!("Infinity point has no limbs"),
+            WeierstrassPoint::Affine(limbs) => limbs,
+        }
+    }
+
+    fn complete_add_assign(&mut self, other: &Self) {
+        self.weierstrass_add_assign(other);
     }
 
     fn add_assign(&mut self, other: &Self) {
@@ -37,9 +60,11 @@ impl AffinePoint<N> for Secp256k1AffinePoint {
     }
 
     fn double(&mut self) {
-        let a = self.limbs_mut();
-        unsafe {
-            syscall_secp256k1_double(a);
+        match &mut self.0 {
+            WeierstrassPoint::Infinity => (),
+            WeierstrassPoint::Affine(limbs) => unsafe {
+                syscall_secp256k1_double(limbs);
+            },
         }
     }
 }

crates/zkvm/lib/src/utils.rs

@@ -8,7 +8,7 @@ pub trait AffinePoint<const N: usize>: Clone + Sized {
     /// Returns a reference to the limbs.
     fn limbs_ref(&self) -> &[u32; N];
 
-    /// Returns a mutable reference to the limbs.
+    /// Returns a mutable reference to the limbs. If the point is the infinity point, this will panic.
     fn limbs_mut(&mut self) -> &mut [u32; N];
 
     /// Creates a new [`AffinePoint`] from the given x and y coordinates.
@@ -47,6 +47,12 @@ pub trait AffinePoint<const N: usize>: Clone + Sized {
     /// Adds the given [`AffinePoint`] to `self`.
     fn add_assign(&mut self, other: &Self);
 
+    /// Adds the given [`AffinePoint`] to `self`. Can be optionally overriden to use a different
+    /// implementation of addition in multi-scalar multiplication, which is used in secp256k1 recovery.
+    fn complete_add_assign(&mut self, other: &Self) {
+        self.add_assign(other);
+    }
+
     /// Doubles `self`.
     fn double(&mut self);
 
@@ -87,20 +93,23 @@ pub trait AffinePoint<const N: usize>: Clone + Sized {
         b_bits_le: &[bool],
         b: Self,
     ) -> Option<Self> {
+        // The length of the bit vectors must be the same.
+        debug_assert!(a_bits_le.len() == b_bits_le.len());
+
         let mut res: Option<Self> = None;
         let mut temp_a = a.clone();
         let mut temp_b = b.clone();
         for (a_bit, b_bit) in a_bits_le.iter().zip(b_bits_le.iter()) {
             if *a_bit {
                 match res.as_mut() {
-                    Some(res) => res.add_assign(&temp_a),
+                    Some(res) => res.complete_add_assign(&temp_a),
                     None => res = Some(temp_a.clone()),
                 };
             }
 
             if *b_bit {
                 match res.as_mut() {
-                    Some(res) => res.add_assign(&temp_b),
+                    Some(res) => res.complete_add_assign(&temp_b),
                     None => res = Some(temp_b.clone()),
                 };
             }
@@ -130,3 +139,65 @@ pub fn bytes_to_words_le(bytes: &[u8]) -> Vec<u32> {
         .map(|chunk| u32::from_le_bytes(chunk.try_into().unwrap()))
         .collect::<Vec<_>>()
 }
+
+#[derive(Copy, Clone)]
+/// A representation of a point on a Weierstrass curve.
+pub enum WeierstrassPoint<const N: usize> {
+    Infinity,
+    Affine([u32; N]),
+}
+
+/// A trait for affine points on Weierstrass curves.
+pub trait WeierstrassAffinePoint<const N: usize>: AffinePoint<N> {
+    /// The infinity point representation of the Weierstrass curve. Typically an enum variant.
+    fn infinity() -> Self;
+
+    /// Returns true if the point is the infinity point.
+    fn is_infinity(&self) -> bool;
+
+    /// Performs the complete addition of two [`AffinePoint`]'s on a Weierstrass curve.
+    /// For an addition of two points P1 and P2, the cases are:
+    ///     1. P1 is infinity
+    ///     2. P2 is infinity
+    ///     3. P1 equals P2
+    ///     4. P1 is the negation of P2
+    ///     5. Default addition.
+    ///
+    /// Implements the complete addition cases according to the
+    /// [Zcash complete addition spec](https://zcash.github.io/halo2/design/gadgets/ecc/addition.html#complete-addition).
+    fn weierstrass_add_assign(&mut self, other: &Self) {
+        // Case 1: p1 is infinity.
+        if self.is_infinity() {
+            *self = other.clone();
+            return;
+        }
+
+        // Case 2: p2 is infinity.
+        if other.is_infinity() {
+            return;
+        }
+
+        // Once it's known the points are not infinity, their limbs can be safely used.
+        let p1 = self.limbs_mut();
+        let p2 = other.limbs_ref();
+
+        // Case 3: p1 equals p2.
+        if p1 == p2 {
+            self.double();
+            return;
+        }
+
+        // Case 4: p1 is the negation of p2.
+        // Note: If p1 and p2 are valid elliptic curve points, and p1.x == p2.x, that means that
+        // either p1.y == p2.y or p1.y + p2.y == p. Because we are past Case 4, we know that p1.y !=
+        // p2.y, so we can just check if p1.x == p2.x. Therefore, this implictly checks that
+        // p1.x == p2.x AND p1.y + p2.y == p without modular negation.
+        if p1[..N / 2] == p2[..N / 2] {
+            *self = Self::infinity();
+            return;
+        }
+
+        // Case 5: Default addition.
+        self.add_assign(other);
+    }
+}

examples/Cargo.toml

@@ -61,9 +61,13 @@ sp1-zkvm = { path = "../crates/zkvm/entrypoint", default-features = false }
 [patch.crates-io]
 curve25519-dalek = { git = "https://github.com/sp1-patches/curve25519-dalek", branch = "patch-curve25519-v4.1.3" }
 curve25519-dalek-ng = { git = "https://github.com/sp1-patches/curve25519-dalek-ng", branch = "patch-v4.1.1" }
-ecdsa-core = { git = "https://github.com/sp1-patches/signatures", package = "ecdsa", branch = "patch-ecdsa-v0.16.9" }
+# ecdsa-core = { git = "https://github.com/sp1-patches/signatures", package = "ecdsa", branch = "patch-ecdsa-v0.16.8" }
+# Note: This branch of ecdsa-core points to SP1 branch ratan/impl-add-assign-fixes
+ecdsa-core = { git = "https://github.com/sp1-patches/signatures", package = "ecdsa", branch = "ratan/secp256k1-add-fixes-v0.16.8" }
 ed25519-consensus = { git = "https://github.com/sp1-patches/ed25519-consensus", branch = "patch-v2.1.0" }
 secp256k1 = { git = "https://github.com/sp1-patches/rust-secp256k1", branch = "patch-secp256k1-v0.29.0" }
 sha2-v0-10-8 = { git = "https://github.com/sp1-patches/RustCrypto-hashes", package = "sha2", branch = "patch-v0.10.8" }
+sha2-v0-10-6 = { git = "https://github.com/sp1-patches/RustCrypto-hashes", package = "sha2", branch = "patch-sha2-v0.10.6" }
 sha2-v0-9-9 = { git = "https://github.com/sp1-patches/RustCrypto-hashes", package = "sha2", branch = "patch-sha2-v0.9.9" }
+sha2-v0-9-8 = { git = "https://github.com/sp1-patches/RustCrypto-hashes", package = "sha2", branch = "patch-sha2-v0.9.8" }
 tiny-keccak = { git = "https://github.com/sp1-patches/tiny-keccak", branch = "patch-v2.0.2" }