Skip to content

Perturbing/add msm bls #514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 72 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
72 commits
Select commit Hold shift + click to select a range
8d004b6
add initial bls MSM binding
perturbing Nov 22, 2024
f24a9bf
cleanup affine/scalar list ptr operations
perturbing Nov 22, 2024
13743fd
remove unused imports
perturbing Nov 22, 2024
c4c2520
fix nbits arg of pippenger call
perturbing Nov 22, 2024
4338c37
add a small remark and fix bit size arg
perturbing Nov 26, 2024
32e9fc0
add testing executable for msm debugging with valgrind
perturbing Nov 27, 2024
8944877
process feedback
perturbing Nov 27, 2024
73c1eca
cleanup
perturbing Nov 27, 2024
6df15a0
fix memory layout issue for MSM
perturbing Jan 21, 2025
955a262
remove tentative test and add tasty tests, also rewrite MSM to accept…
perturbing Jan 21, 2025
9bad7c5
Restore cardano-crypto-class cabal file
perturbing Jan 21, 2025
ebc9bae
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing Jan 23, 2025
94dd555
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing Jan 23, 2025
e40dd2d
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing Jan 23, 2025
01e78c7
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing Jan 23, 2025
3433096
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing Jan 23, 2025
42fc5b3
Update cardano-crypto-tests/src/Test/Crypto/EllipticCurve.hs
perturbing Jan 23, 2025
957f048
Update cardano-crypto-tests/src/Test/Crypto/EllipticCurve.hs
perturbing Jan 23, 2025
850ea87
Update cardano-crypto-tests/src/Test/Crypto/EllipticCurve.hs
perturbing Jan 23, 2025
3034e93
add NonEmptyList to imports of EC tests
perturbing Jan 23, 2025
9965974
fix memory issue with recursive foreignPtr call
perturbing Feb 3, 2025
f46d1b1
Merge branch 'master' into perturbing/add-msm-bls
perturbing Feb 4, 2025
53daf8e
cleanup
perturbing Feb 4, 2025
f973143
add base dependent unzip
perturbing Feb 4, 2025
3b18699
set correct bounds on base version and suppressed warning
perturbing Feb 4, 2025
3f5860d
fix fourmule error
perturbing Feb 5, 2025
6fc8a40
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing Feb 11, 2025
a49f041
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing Feb 11, 2025
94c8af6
add comment about scalar period
perturbing Feb 11, 2025
12b78a1
change blsMSM interface to use a list and filter for inf point
perturbing Feb 11, 2025
a0e833c
remove redundant pragmas
perturbing Feb 11, 2025
8cd6ab6
change test instance Curve Points to include more zero points
perturbing Feb 11, 2025
45d07b9
Merge branch 'master' into perturbing/add-msm-bls
perturbing Apr 28, 2025
368e488
fix windows build by adding null pointer to affine/scalar arrays
perturbing Apr 28, 2025
8b340b1
test execution order on windows build
perturbing Apr 28, 2025
5cd6d88
test windows build with strict eval
perturbing Apr 29, 2025
677279c
test failure in windows build for no terminator ptr in arrays
perturbing Apr 29, 2025
c063ee4
revert terminator ptr for windows build, and make test work again
perturbing Apr 29, 2025
42e3e9e
windows test on msm with only one input with zero scalar
perturbing Apr 29, 2025
223f6d1
windows test on msm with only one input with generator and zero
perturbing Apr 29, 2025
be32e0c
windows test on msm with only one input with generator and zero on c_…
perturbing Apr 29, 2025
e16cfd3
try fixing windows build: calling withNewPoint' before the other mem …
perturbing Apr 29, 2025
d4c3e47
try fixing windows build: calling withNewPoint' before the other mem …
perturbing Apr 29, 2025
c241ffb
try fixing windows build: add logging of result ptr before/after c call
perturbing Apr 30, 2025
5dd67ab
try fixing windows build: test if return ptr is even on curve after C…
perturbing Apr 30, 2025
a96da12
try fixing windows build: increase scatch size
perturbing Apr 30, 2025
a181476
filter for inputs under which msm is invariant and crash the c code
perturbing May 1, 2025
28c2020
test windows build
perturbing May 1, 2025
aab5a16
test windows build
perturbing May 1, 2025
894f6c3
test windows build
perturbing May 1, 2025
e1e64b0
clean up
perturbing May 1, 2025
a5fd37c
add a more efficien blsMSM' approach using blst_pXs_to_affine
perturbing May 2, 2025
877d9b9
expose fast blsMSM' function
perturbing May 2, 2025
866bdcc
cleanup code and add propper types for different unsafe pointers used
perturbing May 2, 2025
06dfaec
change usage of undefined ptr for size to use nullptr
perturbing May 2, 2025
de8c255
remove inefficient blsMSM function, change interface and test windows…
perturbing May 14, 2025
1783b8f
fix windows build
perturbing May 14, 2025
03c0c4e
make interface to switch to naive dot product approach for a certain …
perturbing May 14, 2025
96cd8fb
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing May 15, 2025
122760d
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing May 15, 2025
af7e600
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing May 15, 2025
c151449
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing May 15, 2025
41a2a9f
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing May 15, 2025
c129455
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing May 15, 2025
5850a9f
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing May 15, 2025
d7b19b9
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing May 15, 2025
9f87f12
Update cardano-crypto-class/src/Cardano/Crypto/EllipticCurve/BLS12_38…
perturbing May 15, 2025
62ec7e2
test blsMSM on windows without filtering zero scalar, but not passing…
perturbing May 15, 2025
2b5735d
make comment about filtering zeroscalar more precise
perturbing May 15, 2025
3e710b0
improve safety of withPointArray and withScalarArray by adding explic…
perturbing May 15, 2025
80ef26d
test windows build for withAffineBlockArrayPtr without null-termination
perturbing May 15, 2025
8ae3644
Merge branch 'IntersectMBO:master' into perturbing/add-msm-bls
perturbing May 16, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ module Cardano.Crypto.EllipticCurve.BLS12_381 (
blsMult,
blsCneg,
blsNeg,
blsMSM,
blsCompress,
blsSerialize,
blsUncompress,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,12 @@

module Cardano.Crypto.EllipticCurve.BLS12_381.Internal (
-- * Unsafe Types
ScalarPtr,
ScalarPtr (..),
PointPtr (..),
AffinePtr,
PointArrayPtr (..),
AffinePtr (..),
AffineArrayPtr (..),
AffineBlockPtr (..),
Point1Ptr,
Point2Ptr,
Affine1Ptr,
Expand Down Expand Up @@ -54,6 +57,9 @@ module Cardano.Crypto.EllipticCurve.BLS12_381.Internal (
c_blst_add_or_double,
c_blst_mult,
c_blst_cneg,
c_blst_scratch_sizeof,
c_blst_to_affines,
c_blst_mult_pippenger,
c_blst_hash,
c_blst_compress,
c_blst_serialize,
Expand Down Expand Up @@ -129,6 +135,7 @@ module Cardano.Crypto.EllipticCurve.BLS12_381.Internal (
blsMult,
blsCneg,
blsNeg,
blsMSM,
blsCompress,
blsSerialize,
blsUncompress,
Expand Down Expand Up @@ -165,15 +172,18 @@ import Data.ByteString (ByteString)
import qualified Data.ByteString as BS
import qualified Data.ByteString.Internal as BSI
import qualified Data.ByteString.Unsafe as BSU

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant newline

Suggested change

import Control.Monad (foldM, forM_)
import Data.Proxy (Proxy (..))
import Data.Void
import Foreign (Storable (..), poke, sizeOf)
import Foreign.C.String
import Foreign.C.Types
import Foreign.ForeignPtr
import Foreign.Marshal (advancePtr)
import Foreign.Marshal.Alloc (allocaBytes)
import Foreign.Marshal.Utils (copyBytes)
import Foreign.Ptr (Ptr, castPtr, nullPtr, plusPtr)
import Foreign.Storable (peek)
import System.IO.Unsafe (unsafePerformIO)

---- Phantom Types
Expand All @@ -183,16 +193,36 @@ data Curve2

---- Unsafe PointPtr types

-- | A pointer to a (projective) point one of the two elliptical curves
newtype PointPtr curve = PointPtr (Ptr Void)

-- | A pointer to a null-terminated array of pointers to points
newtype PointArrayPtr curve = PointArrayPtr (Ptr Void)

type Point1Ptr = PointPtr Curve1
type Point2Ptr = PointPtr Curve2

type Point1ArrayPtr = PointArrayPtr Curve1
type Point2ArrayPtr = PointArrayPtr Curve2

-- | A pointer to an affine point on one of the two elliptical curves
newtype AffinePtr curve = AffinePtr (Ptr Void)

-- | A pointer to a contiguous array of affine points
newtype AffineBlockPtr curve = AffineBlockPtr (Ptr Void)

-- | A pointer to a null-terminated array of pointers to affine points
newtype AffineArrayPtr curve = AffineArrayPtr (Ptr Void)

type Affine1Ptr = AffinePtr Curve1
type Affine2Ptr = AffinePtr Curve2

type Affine1BlockPtr = AffineBlockPtr Curve1
type Affine2BlockPtr = AffineBlockPtr Curve2

type Affine1ArrayPtr = AffineArrayPtr Curve1
type Affine2ArrayPtr = AffineArrayPtr Curve2

newtype PTPtr = PTPtr (Ptr Void)

unsafePointFromPointPtr :: PointPtr curve -> Point curve
Expand Down Expand Up @@ -288,6 +318,38 @@ withNewAffine_ = fmap fst . withNewAffine
withNewAffine' :: BLS curve => (AffinePtr curve -> IO a) -> IO (Affine curve)
withNewAffine' = fmap snd . withNewAffine

withPointArray :: [Point curve] -> (Int -> PointArrayPtr curve -> IO a) -> IO a
withPointArray points go = do
let numPoints = length points
sizeReference = sizeOf (nullPtr :: Ptr ())
-- Allocate space for the points and a null terminator
allocaBytes ((numPoints + 1) * sizeReference) $ \ptr ->
-- The accumulate function ensures that each `withPoint` call is properly nested.
-- This guarantees that the foreign pointers remain valid while we populate `ptr`.
-- If we instead used `zipWithM_` for example, the pointers could be finalized too early.
-- By nesting `withPoint` calls in `accumulate`, we ensure they stay in scope until `go` is executed.
let accumulate [] = do
poke (ptr `advancePtr` numPoints) nullPtr
go numPoints (PointArrayPtr (castPtr ptr))
accumulate ((ix, point) : rest) =
withPoint point $ \(PointPtr pPtr) -> do
poke (ptr `advancePtr` ix) pPtr
accumulate rest
in accumulate (zip [0 ..] points)
Comment on lines +331 to +338
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight simplification that avoids redundant iteration over the list and plays better with list fusion than zip does.

Suggested change
let accumulate [] = do
poke (ptr `advancePtr` numPoints) nullPtr
go numPoints (PointArrayPtr (castPtr ptr))
accumulate ((ix, point) : rest) =
withPoint point $ \(PointPtr pPtr) -> do
poke (ptr `advancePtr` ix) pPtr
accumulate rest
in accumulate (zip [0 ..] points)
let accumulate curPtr [] = do
poke curPtr nullPtr
go numPoints (PointArrayPtr (castPtr ptr))
accumulate curPtr (point : rest) =
withPoint point $ \(PointPtr pPtr) -> do
poke curPtr pPtr
accumulate (curPtr `plusPtr` sizeReference) rest
in accumulate ptr points

Nice comment about zipWithM_ not being a valid abstraction here! 👍


-- | Given a block of affine points and a count, produce a pointer array
withAffineBlockArrayPtr ::
forall curve a.
BLS curve =>
Ptr Void -> Int -> (AffineArrayPtr curve -> IO a) -> IO a
withAffineBlockArrayPtr affinesBlockPtr numPoints go = do
allocaBytes (numPoints * sizeOf (nullPtr :: Ptr ())) $ \affineVectorPtr -> do
let ptrArray = castPtr affineVectorPtr :: Ptr (Ptr ())
forM_ [0 .. numPoints - 1] $ \i -> do
let ptr = affinesBlockPtr `plusPtr` (i * sizeAffine (Proxy @curve))
pokeElemOff ptrArray i ptr
go (AffineArrayPtr affineVectorPtr)

withPT :: PT -> (PTPtr -> IO a) -> IO a
withPT (PT pt) go = withForeignPtr pt (go . PTPtr)

Expand Down Expand Up @@ -317,6 +379,11 @@ class BLS curve where
c_blst_mult :: PointPtr curve -> PointPtr curve -> ScalarPtr -> CSize -> IO ()
c_blst_cneg :: PointPtr curve -> Bool -> IO ()

c_blst_scratch_sizeof :: Proxy curve -> CSize -> CSize
c_blst_to_affines :: AffineBlockPtr curve -> PointArrayPtr curve -> CSize -> IO ()
c_blst_mult_pippenger ::
PointPtr curve -> AffineArrayPtr curve -> CSize -> ScalarArrayPtr -> CSize -> ScratchPtr -> IO ()

c_blst_hash ::
PointPtr curve -> Ptr CChar -> CSize -> Ptr CChar -> CSize -> Ptr CChar -> CSize -> IO ()
c_blst_compress :: Ptr CChar -> PointPtr curve -> IO ()
Expand Down Expand Up @@ -345,6 +412,10 @@ instance BLS Curve1 where
c_blst_mult = c_blst_p1_mult
c_blst_cneg = c_blst_p1_cneg

c_blst_scratch_sizeof _ = c_blst_p1s_mult_pippenger_scratch_sizeof
c_blst_to_affines = c_blst_p1s_to_affine
c_blst_mult_pippenger = c_blst_p1s_mult_pippenger

c_blst_hash = c_blst_hash_to_g1
c_blst_compress = c_blst_p1_compress
c_blst_serialize = c_blst_p1_serialize
Expand Down Expand Up @@ -373,6 +444,10 @@ instance BLS Curve2 where
c_blst_mult = c_blst_p2_mult
c_blst_cneg = c_blst_p2_cneg

c_blst_scratch_sizeof _ = c_blst_p2s_mult_pippenger_scratch_sizeof
c_blst_to_affines = c_blst_p2s_to_affine
c_blst_mult_pippenger = c_blst_p2s_mult_pippenger

c_blst_hash = c_blst_hash_to_g2
c_blst_compress = c_blst_p2_compress
c_blst_serialize = c_blst_p2_serialize
Expand Down Expand Up @@ -428,6 +503,26 @@ withNewScalar_ = fmap fst . withNewScalar
withNewScalar' :: (ScalarPtr -> IO a) -> IO Scalar
withNewScalar' = fmap snd . withNewScalar

withScalarArray :: [Scalar] -> (Int -> ScalarArrayPtr -> IO a) -> IO a
withScalarArray scalars go = do
let numScalars = length scalars
sizeReference = sizeOf (undefined :: Ptr ())
-- Allocate space for the scalars and a null terminator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the null terminator? AFAICT, all code that consumes this API uses an explicit length to determine the end of the array; the null terminator is never used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, I had it without, but the C code does use it (I discovered this by looking at the Rust implementation of the blst bindings here).

And to test it without, I ran this CI test, which failed on all windows machines.

Copy link
Contributor Author

@perturbing perturbing May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And to add to that, there are more things that are implicit in for the blst lib that I discovered. For example, for windows builds, given one pair for an input (so one point + scalar), the msm call will also fail. Which Is why I added this

    [(scalar, pt)] -> do
      i <- scalarToInteger scalar
      return (blsMult pt i)

And to test it, see this and this failure on all windows tests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is pretty surprising to see null termination. However, unlike usage of null termination for binary data or strings using, for an array of pointers I think it is fairly safe. What BLST designers should have done, is to produce some sort of error when number of pointers does not match where null termination is placed, since both carry the same information and there is a chance for a discrepancy.

It would be good to learn the expected semantics here. For example this would sound sensible, but I can't find any information on it:

  • when there is a null pointer in a position that resulted in less points than was expected that it is safe, since one of those points could contain a null pointer
  • when number of points (numScalars) supplied is reached and the next element in the array is not a null pointer than it is an error.

On a separate subject same comment as I made on withPointArray applies here too

allocaBytes ((numScalars + 1) * sizeReference) $ \ptr ->
-- The accumulate function ensures that each `withScalar` call is properly nested.
-- This guarantees that the foreign pointers remain valid while we populate `ptr`.
-- If we instead used `zipWithM_` for example, the pointers could be finalized too early.
-- By nesting `withScalar` calls in `accumulate`, we ensure they stay in scope until `go` is executed.
let accumulate [] = do
-- Add a null terminator to the end of the array
poke (ptr `advancePtr` numScalars) nullPtr
go numScalars (ScalarArrayPtr (castPtr ptr))
accumulate ((ix, scalar) : rest) =
withScalar scalar $ \(ScalarPtr sPtr) -> do
poke (ptr `advancePtr` ix) sPtr
accumulate rest
in accumulate (zip [0 ..] scalars)

cloneScalar :: Scalar -> IO Scalar
cloneScalar (Scalar a) = do
b <- mallocForeignPtrBytes sizeScalar
Expand Down Expand Up @@ -512,7 +607,11 @@ scalarFromInteger n = do
---- Unsafe types

newtype ScalarPtr = ScalarPtr (Ptr Void)

-- A pointer to a null-terminated array of pointers to scalars
newtype ScalarArrayPtr = ScalarArrayPtr (Ptr Void)
newtype FrPtr = FrPtr (Ptr Void)
newtype ScratchPtr = ScratchPtr (Ptr Void)

---- Raw Scalar / Fr functions

Expand Down Expand Up @@ -555,6 +654,14 @@ foreign import ccall "blst_p1_generator" c_blst_p1_generator :: Point1Ptr
foreign import ccall "blst_p1_is_equal" c_blst_p1_is_equal :: Point1Ptr -> Point1Ptr -> IO Bool
foreign import ccall "blst_p1_is_inf" c_blst_p1_is_inf :: Point1Ptr -> IO Bool

foreign import ccall "blst_p1s_mult_pippenger_scratch_sizeof"
c_blst_p1s_mult_pippenger_scratch_sizeof :: CSize -> CSize
foreign import ccall "blst_p1s_to_affine"
c_blst_p1s_to_affine :: Affine1BlockPtr -> Point1ArrayPtr -> CSize -> IO ()
foreign import ccall "blst_p1s_mult_pippenger"
c_blst_p1s_mult_pippenger ::
Point1Ptr -> Affine1ArrayPtr -> CSize -> ScalarArrayPtr -> CSize -> ScratchPtr -> IO ()

---- Raw Point2 functions

foreign import ccall "size_blst_p2" c_size_blst_p2 :: CSize
Expand Down Expand Up @@ -582,6 +689,14 @@ foreign import ccall "blst_p2_generator" c_blst_p2_generator :: Point2Ptr
foreign import ccall "blst_p2_is_equal" c_blst_p2_is_equal :: Point2Ptr -> Point2Ptr -> IO Bool
foreign import ccall "blst_p2_is_inf" c_blst_p2_is_inf :: Point2Ptr -> IO Bool

foreign import ccall "blst_p2s_mult_pippenger_scratch_sizeof"
c_blst_p2s_mult_pippenger_scratch_sizeof :: CSize -> CSize
foreign import ccall "blst_p2s_to_affine"
c_blst_p2s_to_affine :: Affine2BlockPtr -> Point2ArrayPtr -> CSize -> IO ()
foreign import ccall "blst_p2s_mult_pippenger"
c_blst_p2s_mult_pippenger ::
Point2Ptr -> Affine2ArrayPtr -> CSize -> ScalarArrayPtr -> CSize -> ScratchPtr -> IO ()

---- Affine operations

foreign import ccall "size_blst_affine1" c_size_blst_affine1 :: CSize
Expand Down Expand Up @@ -824,7 +939,8 @@ blsZero =
error $ "Unexpected failure deserialising point at infinity on BLS12_381.G1: " ++ show err
Right infinity ->
infinity -- The zero point on this curve is chosen to be the point at infinity.
---- Scalar / Fr operations

---- Scalar / Fr operations

scalarFromFr :: Fr -> IO Scalar
scalarFromFr fr =
Expand Down Expand Up @@ -875,6 +991,79 @@ scalarCanonical scalar =
unsafePerformIO $
withScalar scalar c_blst_scalar_fr_check

---- MSM operations

-- | Multi-scalar multiplication using the Pippenger algorithm.
-- The scalars will be brought into the range of modular arithmetic
-- by means of a modulo operation over the 'scalarPeriod'.
-- Negative numbers will also be brought to the range
-- [0, 'scalarPeriod' - 1] via modular reduction.
blsMSM :: forall curve. BLS curve => Int -> [Integer] -> [Point curve] -> Point curve
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest having a list of tuples supplied, instead of two separate lists, because there are no surprises when these two lists have different length. (i.e. burden of correct zipping is put onto the caller of the function)

Suggested change
blsMSM :: forall curve. BLS curve => Int -> [Integer] -> [Point curve] -> Point curve
blsMSM :: forall curve. BLS curve => Int -> [(Integer, Point curve)] -> Point curve

blsMSM threshold ss ps = unsafePerformIO $ do
zeroScalar <- scalarFromInteger 0
filteredPoints <-
foldM
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

foldM is a left foldlM, so this operation will reverse the order when compared to the input.

> foldM (\acc x -> (x:acc) <$ print x) [] [1,2,3,4]
1
2
3
4
[4,3,2,1]

My question is this indeed desired? If it isn't I'd suggest switching to foldrM

Considering that tests successfully check against the same naive implementation, I suspect that it all works out only because MSM operation is commutative. I am not sure if it is dangerous to rely on this property, but at the very least it surprising to see the order of input to be reversed.

( \acc (s, pt) -> do
scalar <- scalarFromInteger s
-- Here we filter out pairs that will not contribute to the result.
-- This is also for safety, as the c_blst_to_affines C call
-- will fail if the input contains the point at infinity.
-- see https://github.com/supranational/blst/blob/165ec77634495175aefd045a48d3469af6950ea4/src/multi_scalar.c#L11C32-L11C37
-- We also filter out the zero scalar, as for any point pt
-- we have:
--
-- pt ^ 0 = id
--
-- Which yields no contribution to summation, and
-- thus we can skip the point and scalar pair. This filter
-- saves us an extra input to the more expensive exponential
-- operation.
if blsIsInf pt || scalar == zeroScalar
then return acc
else return ((scalar, pt) : acc)
Comment on lines +1007 to +1023
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend avoiding construction of scalar if it is top be discarded:

Suggested change
scalar <- scalarFromInteger s
-- Here we filter out pairs that will not contribute to the result.
-- This is also for safety, as the c_blst_to_affines C call
-- will fail if the input contains the point at infinity.
-- see https://github.com/supranational/blst/blob/165ec77634495175aefd045a48d3469af6950ea4/src/multi_scalar.c#L11C32-L11C37
-- We also filter out the zero scalar, as for any point pt
-- we have:
--
-- pt ^ 0 = id
--
-- Which yields no contribution to summation, and
-- thus we can skip the point and scalar pair. This filter
-- saves us an extra input to the more expensive exponential
-- operation.
if blsIsInf pt || scalar == zeroScalar
then return acc
else return ((scalar, pt) : acc)
-- Here we filter out pairs that will not contribute to the result.
-- This is also for safety, as the c_blst_to_affines C call
-- will fail if the input contains the point at infinity.
-- see https://github.com/supranational/blst/blob/165ec77634495175aefd045a48d3469af6950ea4/src/multi_scalar.c#L11C32-L11C37
if blsIsInf pt
then pure acc
else do
scalar <- scalarFromInteger s
-- We also filter out the zero scalar, as for any point pt
-- we have:
--
-- pt ^ 0 = id
--
-- Which yields no contribution to summation, and
-- thus we can skip the point and scalar pair. This filter
-- saves us an extra input to the more expensive exponential
-- operation.
if scalar == zeroScalar
then return acc
else return ((scalar, pt) : acc)

Also, in a one-to-one conversion we could avoid construction of zeroScalar, by checking s == 0 instead, but I suspect because of modular arithmetic there could be other values of Integer that would map to scalarFromInteger 0. If I am wrong and there are no other values that map to 0 except 0, then I'd suggest this instead:

I'd recommend avoiding construction of scalar if it is top be discarded:

Suggested change
scalar <- scalarFromInteger s
-- Here we filter out pairs that will not contribute to the result.
-- This is also for safety, as the c_blst_to_affines C call
-- will fail if the input contains the point at infinity.
-- see https://github.com/supranational/blst/blob/165ec77634495175aefd045a48d3469af6950ea4/src/multi_scalar.c#L11C32-L11C37
-- We also filter out the zero scalar, as for any point pt
-- we have:
--
-- pt ^ 0 = id
--
-- Which yields no contribution to summation, and
-- thus we can skip the point and scalar pair. This filter
-- saves us an extra input to the more expensive exponential
-- operation.
if blsIsInf pt || scalar == zeroScalar
then return acc
else return ((scalar, pt) : acc)
-- Here we filter out pairs that will not contribute to the result.
-- This is also for safety, as the c_blst_to_affines C call
-- will fail if the input contains the point at infinity.
-- see https://github.com/supranational/blst/blob/165ec77634495175aefd045a48d3469af6950ea4/src/multi_scalar.c#L11C32-L11C37
-- We also filter out the zero scalar, as for any point pt
-- we have:
--
-- pt ^ 0 = id
--
-- Which yields no contribution to summation, and
-- thus we can skip the point and scalar pair. This filter
-- saves us an extra input to the more expensive exponential
-- operation.
if blsIsInf pt || s == 0
then pure acc
else do
scalar <- scalarFromInteger s
pure ((scalar, pt) : acc)

)
[]
(zip ss ps)
case filteredPoints of
[] -> return blsZero
-- If there is only one point, we refert to blsMult function
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL there is a word in English refert 😄

-- The blst_mult_pippenger C call will also not work for
-- this case on windows builds.
[(scalar, pt)] -> do
i <- scalarToInteger scalar
return (blsMult pt i)
_ | length filteredPoints <= threshold -> do
return $
foldr
(\(scalar, pt) acc -> blsAddOrDouble acc (blsMult pt (unsafePerformIO $ scalarToInteger scalar)))
blsZero
filteredPoints
_ -> do
let (scalars, points) = unzip filteredPoints

withNewPoint' @curve $ \resultPtr -> do
withPointArray points $ \numPoints pointArrayPtr -> do
withScalarArray scalars $ \_ scalarArrayPtr -> do
let numPoints' :: CSize
numPoints' = fromIntegral numPoints
scratchSize :: Int
scratchSize = fromIntegral @CSize @Int $ c_blst_scratch_sizeof (Proxy @curve) numPoints'
-- Multiply by 8, because blst_mult_pippenger takes number of *bits*, but
-- sizeScalar is in *bytes*
nbits :: CSize
nbits = fromIntegral @Int @CSize $ sizeScalar * 8
allocaBytes (numPoints * sizeAffine (Proxy @curve)) $ \affinesBlockPtr -> do
c_blst_to_affines (AffineBlockPtr affinesBlockPtr) pointArrayPtr numPoints'
withAffineBlockArrayPtr affinesBlockPtr numPoints $ \affineArrayPtr -> do
allocaBytes scratchSize $ \scratchPtr -> do
c_blst_mult_pippenger
resultPtr
affineArrayPtr
numPoints'
scalarArrayPtr
nbits
(ScratchPtr scratchPtr)

---- PT operations

ptMult :: PT -> PT -> PT
Expand Down
34 changes: 25 additions & 9 deletions cardano-crypto-tests/src/Test/Crypto/EllipticCurve.hs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import Test.QuickCheck (
)
import Test.Tasty (TestTree, testGroup)
import Test.Tasty.HUnit (assertBool, assertEqual, testCase)
import Test.Tasty.QuickCheck (testProperty)
import Test.Tasty.QuickCheck (frequency, testProperty)

tests :: TestTree
tests =
Expand Down Expand Up @@ -132,6 +132,11 @@ testBLSCurve name _ =
BLS.blsMult (BLS.blsMult a b) c === BLS.blsMult (BLS.blsMult a c) b
, testProperty "scalar mult distributive left" $ \(a :: BLS.Point curve) (BigInteger b) (BigInteger c) ->
BLS.blsMult a (b + c) === BLS.blsAddOrDouble (BLS.blsMult a b) (BLS.blsMult a c)
, testProperty "MSM matches naive approach" $ \((ps, ss) :: ([BLS.Point curve], [BigInteger])) ->
let pairs = [(p, i) | (BigInteger i, p) <- zip ss ps]
Comment on lines +135 to +136
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't want to generate two lists of different length just to immediately discard elements from the one that is longer.

Suggested change
, testProperty "MSM matches naive approach" $ \((ps, ss) :: ([BLS.Point curve], [BigInteger])) ->
let pairs = [(p, i) | (BigInteger i, p) <- zip ss ps]
, testProperty "MSM matches naive approach" $ \(pairs' :: [(BLS.Point curve, BigInteger)]) ->
let pairs = [(p, i) | (BigInteger i, p) <- pairs']

(ps', ss') = unzip pairs
in BLS.blsMSM 10 ss' ps'
Comment on lines +137 to +138
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will give an idea of how well both cases of blsMSM function are tested.

Suggested change
(ps', ss') = unzip pairs
in BLS.blsMSM 10 ss' ps'
(ps', ss') = unzip pairs
threshold = 10
in classify (length pairs <= threshold) "Below threshold" $
BLS.blsMSM threshold ss' ps'

=== foldr (\(p, s) acc -> BLS.blsAddOrDouble acc (BLS.blsMult p s)) (BLS.blsZero @curve) pairs
, testProperty "scalar mult distributive right" $ \(a :: BLS.Point curve) (b :: BLS.Point curve) (BigInteger c) ->
BLS.blsMult (BLS.blsAddOrDouble a b) c === BLS.blsAddOrDouble (BLS.blsMult a c) (BLS.blsMult b c)
, testProperty "mult by zero is inf" $ \(a :: BLS.Point curve) ->
Expand Down Expand Up @@ -453,21 +458,32 @@ prop_fourPairings a1 a2 a3 b = BLS.ptFinalVerify tt t4
t4 = BLS.millerLoop (BLS.blsAddOrDouble (BLS.blsAddOrDouble a1 a2) a3) b
tt = BLS.ptMult (BLS.ptMult t1 t2) t3

prop_randomFailsFinalVerify :: BLS.Point1 -> BLS.Point1 -> BLS.Point2 -> BLS.Point2 -> Property
prop_randomFailsFinalVerify a b c d =
a /= b && c /= d ==>
BLS.ptFinalVerify (BLS.millerLoop a c) (BLS.millerLoop b d) === False
prop_randomFailsFinalVerify :: BigInteger -> BigInteger -> BigInteger -> BigInteger -> Property
prop_randomFailsFinalVerify (BigInteger a) (BigInteger b) (BigInteger c) (BigInteger d) =
(a * c `mod` BLS.scalarPeriod) /= (b * d `mod` BLS.scalarPeriod) ==>
let a' = BLS.blsMult (BLS.blsGenerator @BLS.Curve1) a
b' = BLS.blsMult (BLS.blsGenerator @BLS.Curve1) b
c' = BLS.blsMult (BLS.blsGenerator @BLS.Curve2) c
d' = BLS.blsMult (BLS.blsGenerator @BLS.Curve2) d
in BLS.ptFinalVerify (BLS.millerLoop a' c') (BLS.millerLoop b' d') === False

newtype BigInteger = BigInteger Integer
deriving (Eq, Show)
instance Arbitrary BigInteger where
arbitrary = BigInteger <$> oneof [arbitrary, chooseAny, choose (-2 ^ (128 :: Int), 2 ^ (128 :: Int))]

instance BLS.BLS curve => Arbitrary (BLS.Point curve) where
arbitrary = do
str <- arbitrary
let bs = BS.pack str
return $ BLS.blsHash bs Nothing Nothing
arbitrary =
frequency
[ (1, pure BLS.blsZero)
,
( 9
, do
str <- arbitrary
let bs = BS.pack str
pure (BLS.blsHash bs Nothing Nothing)
)
]

instance BLS.BLS curve => Arbitrary (BLS.Affine curve) where
arbitrary = BLS.toAffine <$> arbitrary
Expand Down
Loading