drand / kyber Goto Github PK

Currently we prepend a fixed tag to our mesages when we are hashing data:

I think we should consider using keyed Blake instead of prepend the tag. It would be as simply as replacing the nil in the Blake2s instantiation with the H3Tag.

@nikkolasg It would simplify a bit the code and also, keyed hash functions are there exactly for this usecase, no?

See this PR comment: #28 (comment)

Presently blake2s is hardcoded into the IBE module.
We should extract the hashing logic to an interface to enable the caller to provide their own hashing function.
Blake2s should still be the default implementation for callers who don't wish to provide an alternative.

In https://github.com/drand/kyber/blob/master/share/dkg/dkg.go#L1095, the threshold should be n/2 + 1 instead of (n+1)/2. This bug likely comes from the DEDIS original VSS implementation (see dedis#374). If the bug is confirmed, I'm happy to submit a PR.

There have been quite a few changes since the last release, can we get a new one?

While replacing circle CI with github actions, @AnomalRoil suggested running the go test runner with the race flag to check for race conditions. This identified some race conditions in DKG code that require investigation and remediating.

During prior investigation, a data access races were identified in the StatusMatrix struct implementation. Adding some generic RW locking worked fine on my local machine and the tests (including race condition testing) passed successfully.
On the github actions runner however the TestProtoFull test in proto_test.go appeared to suffer deadlock (albeit the go runtime did not detect this). Note: this deadlock does not happen without the -lock flag

Some attempts I tried to remedy this:

• updating go to 1.18
• adding test flag GO111MODULE=on
• adding test flag GODEBUG=cgocheck=2 (see golang/go#48402)
• modifying the resCh in TestProtoFull buffer size to 0 and 2 (see golang/go#48402)
• running tests without make
• implementations of StatusMatrixwith RWLock and vanilla Lock
• a variety of timeouts up to 30mins
• running macos-latest and ubuntu-latest github runners

The current interface for hashing to group elements must consider the use of domain separation strings.

This is aligned with hash to curve IETF specification. All hashes must be domain-separated, so a protocol can specify the separation when the hash is invoked as different random oracles. See more https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-hash-to-curve-16#name-domain-separation-requireme

Current:

Proposed:

type HashablePoint interface { 
    Hash(msg, dst []byte) Point 
} 

Here in the tBLS signing (which is used for partial signatures) the index is a 16bit int:

Throughout the DKG, a uint32 is used:

I haven't actually seen this break anything, but maybe it's a limit for large DKGs!

Current the eviction rules only look generally, but don't look if the node being evicted is himself. If it is, it should already return an error and quit the DKG. Right now, an evicted node would finish the DKG normally if being evicted and return group and share but its share would be invalid.