Nit: Polynomial is not size constrainted, but Blob is. I guess that's fine math-wise, but if that's not useful protocol-wise, I wonder if code could be simplified if the type-system enforces its size.
Since kzg is a generic modular piece of code unrelated to dank-sharding, we allow polynomial to be any size.
Nit: The way you are passing indexInDomain around seems correct (based on code review), but given that it goes through three different functions and also gets sign-casted, I wonder if we should also do a check here that index < len(p) or something.
If the condition is broken, then we want it to fail by panicking which will happen when it tries to index the polynomial. This would be a bug that we cannot recover from.
You are relying on gnark for uncompressing and subgroup checking. That makes sense. In the same manner, c-kzg relies on blst for the same operation. I wonder if these two libraries have identical behavior.
They probably do because they both implement the IETF spec, but I wonder if there are ways an attacker can make one library accept something and the other to reject.
It's interesting that we allow len(p) < len(ck.G1) here. IIUC given the API here, if someone wanted to commit to a low degree polynomial they would also have to provide a custom domain with a small subgroup. Is this something useful for protodank? Or should we expect that users would just pad the polynomial with zeroes or something?
Putting this as won't change, as I'd need to modify tests and since this is called by the api layer which uses fixed blob types, its inconsenquential.
Perhaps this comment belongs in findRootIndex()? Or perhaps it's fine here too.
Has been modified in Gotti's PR.
Nit: Perhaps possible to use batch inv here?
Done :)
I think it would be cleaner if blobsLen is batchSize here and in the line below.
Link to Georges comments are here: https://github.com/asn-d6/go-proto-danksharding-crypto/pull/1
Since kzg is a generic modular piece of code unrelated to dank-sharding, we allow polynomial to be any size.
If the condition is broken, then we want it to fail by panicking which will happen when it tries to index the polynomial. This would be a bug that we cannot recover from.
They probably do because they both implement the IETF spec, but I wonder if there are ways an attacker can make one library accept something and the other to reject.
This was a good point, see: https://github.com/ConsenSys/gnark-crypto/pull/363
Putting this as won't change, as I'd need to modify tests and since this is called by the api layer which uses fixed blob types, its inconsenquential.
Has been modified in Gotti's PR.
Done :)
Done