BLS12-381 and BLS12-377 both need to be updated to be constant. Side benefit is compilation to ARM and AMD

Expected Behaviour

Calling GetContributionChallenge for all revealed proof messages should not throw an error.

Actual Behaviour

The function throws the following error:
panic: runtime error: makeslice: cap out of range

Steps to Reproduce the Problem

In the test file, replace proofMsgs with the following

	proofMsgs := []common.ProofMessage{
		&common.RevealedMessage{
			Message: msgs[0],
		},
		&common.RevealedMessage{
			Message: msgs[1],
		},
		&common.RevealedMessage{
			Message: msgs[2],
		},
		&common.RevealedMessage{
			Message: msgs[3],
		},
	}

and replace the revealedMsgs with

	revealedMsgs := map[int]curves.Scalar{
		0: msgs[0],
		1: msgs[1],
		2: msgs[2],
		3: msgs[3],
	}

Issue

The following expression calculates an expected array capacity that is smaller than the length of proof2Points and proof2Scalars.

Suggested Fix

Replace pts := 2 + generators.length - len(revealedMessages) with pts := 3 + generators.length - len(revealedMessages)

Replace 2 with 3 since the affected arrays have a minimum length of 3.

Is it possible to use FROST tss with secp256k1 curve? I saw in the code that it requires just a generic curve implementation, but it also requires Challenge Deriver when creating a signer and is labeled under ted25519.

Given f func() hash.Hash, calling f repeatedly to get new hashers creates a possible subtle risk: It doesn't have to return the same hasher every time it is invoked. ExpandMessageXmd however assumes that the hasher always produces the same kind of hash. In the direct call paths to this in the library, it'll usually be something like sha256.New, but the function is exported, so someone could call it with anything. This is probably low-risk, but then, that's what we thought about handling JNDI lookups in message text in a logging package.

One simple solution would be to take a hash.Hash, and use its reset method when you need another hash. Of course, someone could provide a maliciously crafted hash object, too, but at least they'd have to work for it a bit.

I recently looked into using the camshoup package to be used in a VRFworkflow, but have a few questions:

1. Is it possible to rename DecryptionKey and EncryptionKey to VerificationKey and SecretKey respectively?

2. Image 1 below shows that the EncryptionKey can be retrieved from the DecryptionKey. I looked through some of the reference papers (image 3) and additional documentation (image 2), but still don't fully understand how this should be used.

   If a verifier, who has access to the DecryptionKey, can retrieve the EncryptionKey, then anyone can recreate the proof. My expectation is that the DecryptionKey (i.e. the Public Key) can be used for verification, the EncryptionKey (i.e. the Secret Key) can be used for encryption, and DecryptionKey can be derived from the EncryptionKey.

   If my understanding here is incorrect, then the first question is a moot point.

3. What is the purpose of the numMsgs parameter when calling NewKeys? It seems that it controls the number of messages that the ciphertext could contain but is this a design decision or a technical limitation? In other words, why not simply use the same key and treat each ciphertext as a separate message (i.e. numMessages is always exactly 1).

Image 1: README in this github repo:

Image 2: README in this github repo:

Image 3: The referenced paper Practical Verifiable Encryption and Decryption of Discrete Logarithms:

I am suspecting there is a bug in the kryptology/pkg/core/curves/native/bls12-381/arithmetic_fallback.go, since there are a bunch of test failures in the kryptology libarary itself when running with generic build tag.

We at Obol implemented bls threshold signature splitting and verification using pkg/sharing/felman.go and pkg/signatures/bls/bls_sig and our tests fail when running with -tags=generic (it passes when running with GOARCH=amd64).

➜  kryptology git:(master) ✗ git rev-parse HEAD
71ffd4cbf01951cd0ee056fc7b45b13ffb178330

➜  kryptology git:(master) ✗ go version
go version go1.17.6 darwin/arm64

➜  kryptology git:(master) ✗ GOARCH=amd64 go test github.com/coinbase/kryptology/pkg/signatures/bbs github.com/coinbase/kryptology/pkg/accumulator
ok  	github.com/coinbase/kryptology/pkg/signatures/bbs	(cached)
ok  	github.com/coinbase/kryptology/pkg/accumulator	(cached)

➜  kryptology git:(master) ✗ go test -tags=generic github.com/coinbase/kryptology/pkg/signatures/bbs github.com/coinbase/kryptology/pkg/accumulator
--- FAIL: TestBlindSignatureContext (0.04s)
    blind_signature_context_test.go:56:
        	Error Trace:	blind_signature_context_test.go:56
        	Error:      	Received unexpected error:
        	            	invalid result
        	Test:       	TestBlindSignatureContext
.... (redacted for brevity)
FAIL
FAIL	github.com/coinbase/kryptology/pkg/signatures/bbs	0.426s
--- FAIL: Test_Membership (0.02s)
    witness_test.go:70:
        	Error Trace:	witness_test.go:70
        	Error:      	Received unexpected error:
        	            	invalid result
        	Test:       	Test_Membership
.... (redacted for brevity)
FAIL
FAIL	github.com/coinbase/kryptology/pkg/accumulator	0.373s
FAIL

The current code assumes that DST is <= 255 bytes long, which in of itself is fine, as the base definition in the RFC and the fact that the extra handling is in a separate section (5.4.3). While this does fall under "Don't do that then", as it is a public API, the correct thing to do would be to either return an error if the DST is oversized, or to implement the (trivial) handling for it as per the draft.

When running the program at test/dkg/bls/main.go, it panics unexpectedly.

Command:
go run main.go -t 3 -n 4

Panic message:

panic: runtime error: invalid memory address or nil pointer dereference`

Machine:
MacOS Monterey v12.3.1, Apple M1 Pro

Go version:
go version go1.18.1 darwin/arm64

The proof is wrong. It says:

wlog p < q so
(p+q)/pq <= 2q/qq

This is not correct:
E.g. take p = 3, q = 7 we get 0.476 < 0.226

To overestimate the size, the numerator should be as large as possible (2q) and the denominator should be as small as possible (p^2).

The theorem is still obviously true though.

I'm not sure a formal proof is necessary as the result is quite intuitive, but here is my attempt:

Proof:
(p+q)/pq = p/pq + q/pq = 1/q + 1/p.
Both terms tend to 0 as p and q tend to infinity.

Happy to do a pull request if you give me the .tex code.

I did quite like the proof by security reduction though :)

Right now errors are in the internal directory. That means a repository importing this one cannot import those error variables and thus cannot check the error with errors.Is(err, ErrXYZ).

One could move errors to their own errors package in pkg/errors or have them locally defined for each exported package.

The IETF spec describing the I2OSP function describes it as returning an error for invalid (too small) n. This would probably make sense; otherwise, it produces truncated results. e.g., I2OSP(65536, 2) produces 0x01, 0x00, which would be converted back to 256, not 65536.

It might be possible to do this more cleanly with math/bits.Len64() and encoding/binary/BigEndian.PutUint64.

The direct uses of this function in the library don't seem to run into this (they're mostly calling it with small values like 0, 1, or the length in bytes of a hash), but since the symbol is exported, and the library's stated goal is to be hard-to-misuse, it should probably be returning an error explicitly when invoked with invalid parameters.

Unreviewed commit(s) COMPARE was merged to master by Mike Lodder.

Comment with appropriate number of 👍 to approve.

Reviewed requirements satisfied? No
Ancestors reviewed? Yes

I’m getting the following error when I call g2.MulScalar concurrently:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x4ed760]

goroutine 19 [running]:
github.com/coinbase/kryptology/pkg/core/curves/native/bls12-381.(*G2).toBytesJacobi(0xc0001500a0, 0x0)
	go/pkg/mod/github.com/coinbase/[email protected]/pkg/core/curves/native/bls12-381/g2.go:274 +0x40
github.com/coinbase/kryptology/pkg/core/curves/native/bls12-381.(*G2).MulScalar(0xc00008c360, 0x5312e0, 0x537740, 0x753700)
	go/pkg/mod/github.com/coinbase/[email protected]/pkg/core/curves/native/bls12-381/g2.go:502 +0x148
github.com/coinbase/kryptology/pkg/core/curves.(*PointBls12381G2).Mul(0xc000010038, {0x6968f0, 0xc00000e060})
	go/pkg/mod/github.com/coinbase/[email protected]/pkg/core/curves/bls12381_curve.go:704 +0x7b
github.com/coinbase/kryptology/pkg/signatures/bbs.(*SecretKey).PublicKey(0xc000012020)
	go/pkg/mod/github.com/coinbase/[email protected]/pkg/signatures/bbs/secret_key.go:132 +0x7d
github.com/coinbase/kryptology/pkg/signatures/bbs.NewKeys(0x6961b0)
	go/pkg/mod/github.com/coinbase/[email protected]/pkg/signatures/bbs/secret_key.go:78 +0x3c

It looks like the ‘q’ variable somehow gets dereferenced.

Since Golang is quite new to me, I don’t have a solution at the moment but I think pointing this out could be useful.

Here is some code to reproduce the error

	package main

	import (
		"time"

		"github.com/coinbase/kryptology/pkg/core/curves"
		"github.com/coinbase/kryptology/pkg/signatures/bbs"
	)

	func main() {
		go newKeys()
		go newKeys()

		time.Sleep(time.Second)
	}

	func newKeys() {
		curve := curves.BLS12381(&curves.PointBls12381G2{})

		bbs.NewKeys(curve)
	}

Unreviewed commit(s) COMPARE was merged to master by cbdnyu.

Comment with appropriate number of 👍 to approve.

Reviewed requirements satisfied? No
Ancestors reviewed? No

Seems like there is an issue with the bls12-381 package in the 1.5.4 release.

Using go get github.com/coinbase/[email protected] in my working directory works, while go get github.com/coinbase/[email protected] does not and results in the same errors as when I run the tests (see the image below).

I believe it is related to the fact that bls12-381/arithmetic_decl.go uses the package package bls12381.

According to https://go.dev/blog/package-names, package names should either be camelCase or snake_case and not be hyphenated. If it is hyphenated, the package name should replace - with _.

I spent about 40 minutes trying to simply rename everything, but the issue is a little more complex because of conflicting name dependencies and autogenerated vs source code, so I've decided to open this issue instead.

Unreviewed commit(s) COMPARE was merged to master by Mike Lodder.

Comment with appropriate number of 👍 to approve.

Reviewed requirements satisfied? No
Ancestors reviewed? Yes

Unreviewed commit(s) COMPARE was merged to master by dB2510.

Comment with appropriate number of 👍 to approve.

Reviewed requirements satisfied? No
Ancestors reviewed? No

I'm sure this is a honest mistake, but why does this library contain files derived from the Go standard library with a different copyright and license?

https://github.com/coinbase/kryptology/blob/3d28fe04e750bc4f1f321174a4a79f9e8f378d40/pkg/ted25519/ted25519/ed25519.go
https://github.com/golang/go/blob/54e75e8f9db09868db61f97edb98ace3aab7234f/src/crypto/ed25519/ed25519.go

I get that the backend curve/scalar implementation was replaced (by the same code that the standard library later switched to), but it still is a derived work.

Modulus N was likely intended for calculating y_i = x_i^M
at ProvePSF, Figure 15

Hello, dear developers. I hope you are well-doing. I'm using Kryptology functions in my research and it works well. I'm really enjoy your product.

I ran into a problem while using DKG results. The PublicShares returned after round 4 does not match the PublicShare generated from the ShamirShare.

	field := curves.NewField(curve.Params().N)
	share1 := v1.NewShamirShare(1, dkgR4Out[1].SigningKeyShare.Bytes(), field)

	publicShare1, err := curves.NewScalarBaseMult(curve, share1.Value.BigInt())
	require.NoError(t, err)

	ok := reflect.DeepEqual(publicShare1, dkgR4Out[1].PublicShares[0]) // ok = false

I assume they should be equal, otherwise round3(GG20) will return an error "e != eHat".

How should I use the PublicShares returned from the DKG to create a Signer object?

To reproduce, run a test that passes the result of DKG to gg20.

https://github.com/youricorocks/kryptology/blob/5f8afd0811a0e40dac543e9959a2998b20a9861c/pkg/tecdsa/gg20/participant/dkg_rounds_test.go#L900

Hello, dear developers. I hope you are well-doing. I'm using Kryptology functions in my research and it works well. I'm really enjoy your product.

Could you help with one issue? I want to sign ethereum transaction with signature after the sixth round and the SignOut call. I send the same hash to the fifth round in several runs, but each time I get a different signature. Is this really correct logic?

Thank you in advance for the help!

Kind Regards,
Ahmed

Unreviewed commit(s) COMPARE was merged to master by Mike Lodder.

Comment with appropriate number of 👍 to approve.

Reviewed requirements satisfied? No
Ancestors reviewed? Yes

To keep the documentation complete
(avoiding the need to get Paillier encrypt/decrypt specification),
one would include L(x) function definition:

if
N \not | (x-1) return wrong_ciphertext
else
return \frac {x-1} {N}

Also, for any a \in Z_N: (N+1)^a = 1 + aN \pmod{N^2}

Change it to SK .

Missing index of h_1: u_i = h_1^{\alpha_i} at Figure 16.

Unreviewed commit(s) COMPARE was merged to master by 0x2b3bfa0.

Comment with appropriate number of 👍 to approve.

Reviewed requirements satisfied? No
Ancestors reviewed? No

The spec linked for the hashing function using the "DST" value talks about creating a value, but in fact this is just a static string which doesn't contain some of the suggested information, or vary in the expected ways. See:

https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-hash-to-curve-10#section-3.1

3. Tags SHOULD begin with a fixed identification string that is
   unique to the application.

4. Tags SHOULD include a version number.

5. For applications that define multiple ciphersuites, each
   ciphersuite's tag MUST be different. For this purpose, it is
   RECOMMENDED to include a ciphersuite identifier in each tag.

6. For applications that use multiple encodings, either to the same
   curve or to different curves, each encoding MUST use a different
   tag. For this purpose, it is RECOMMENDED to include the
   encoding's Suite ID (Section 8) in the domain separation tag.
   For independent encodings based on the same suite, each tag
   should also include a distinct identifier, e.g., "ENC1" and
   "ENC2".

Contrast with:

	DST := []byte("Coinbase_tECDSA")

	[...]
	uniformBytes, err := ExpandMessageXmd(f, msg, DST, lenInBytes)

which uses a fixed string of length 15, one less than the RECOMMENDED 16 minimum.

I was testing bulletproof with different curves such as bls12381 and it doesn't work.
In the tests range_verifier_test.go and range_batch_verifier_test.go in pkg/bulletproof,
when replacing curve := curves.ED25519() with other curves, such as curve := curves.BLS12381G1(),
curve := curves.P256(), curve := curves.BLS12377G1(), the verifier.Verify(proof, capV, proofGenerators, n, transcriptVerifier) function fails.

Have been trying to integrate the gg20 dkg implementation into a distributed service and wanted to raise a few things.

• DkgParticipant has no external CTOR a calling lib can use
  • Ended up adding one in our fork
• DkgRound2P2PSend's xij is private and cannot be serialized
  • Ended up exposing this so it can be serialized but feels like its unsafe to share in unencrypted form
• Didn't see any usage of the paillier encryption key in the keygen spec. Does xij needed to be encrypted?
  • It looks like the swingby's forked tss-lib implementation encrypts the share:
    https://github.com/SwingbyProtocol/tss-lib/blob/668d0061fadf08bf2ba9f7e9287516fc173b6b9c/ecdsa/keygen/round_3.go#L127-L133

I am suspecting there is a bug in

We at Obol when updated to latest v1.8.0 found this issue while updating our codebase.

Based on the following comment from [0] and the "obsolescence" of the GG20 paper I think it would make sense to "deprecate" the tecdsa/gg20 package so that any new implementations or those which already rely on this code are aware of the risks of this protocol.

This should be possible via the following godoc declaration: https://rakyll.org/deprecated/

On the other hand, the authors of that paper appear to have declared it "obsolete". In light of this declaration, we cannot attest,
given the information we currently have, that the protocol implemented here is secure. We advise caution regarding its use.

[0] https://github.com/coinbase/kryptology/blob/master/pkg/tecdsa/gg20/SECURITY.md

Can we run those ones in a webassembly vm? or it has binding to external non-pure dependencies?

If so, could we add documentation about it;