Updated blst to latest c-kzg hash #237
Conversation
|
So I reviewed a bit more and it seems upstream c-kzg-4844 implementation is quite different. They changed various things, including adding additional checks etc. Tests likely don't cover it. So all the details need to be ported. The best is probably to compare side-by-side our implementation and c-kzg-4844 and port/fix any incompatibilities. |
kzg-bench/src/tests/eip_4844.rs
Outdated
| fn u64_to_bytes(x: u64) -> [u8; 32] { | ||
| let mut bytes = [0u8; 32]; | ||
| bytes[0..8].copy_from_slice(&x.to_le_bytes()); | ||
| bytes[24..32].copy_from_slice(&x.to_be_bytes()); | ||
| bytes | ||
| } |
There was a problem hiding this comment.
This change will affect other backends that expect from_bytes to be LE format. I'm not sure if this is relevant to mcl or zkcrypto, haven't checked, but it does affect arkworks.
Probably same thing is also in other modified tests, have we decided that all from_bytes should expect BE format?
There was a problem hiding this comment.
To clarify - I'm fine with changing arkworks implementation to convert from BE to LE, just think that from_bytes should be consistent across all backends.
EDIT: Alternatively we can add from_bytes_le & from_bytes_be to traits & then this issue will be avoided
There was a problem hiding this comment.
Yeah, I think that's a good idea to rename TFr::from_bytes to TFr::from_be_bytes, although I don't really see a point in implementing TFr::from_le_bytes - all eip4844 functions expect that bytes will be in BE (see "constants" section "KZG_ENDIANNESS").
There was a problem hiding this comment.
Ah right, my mistake, I heard about that switch to BE and thought it applied to all elements though looks like G1/G2 was already BE & only field element was recently changed to BE. I checked arkworks deserialization implementation it seems that G1/G2 already assumed BE format, so all good on that front.
In our case arkworks library Fr serialization assumed input to be LE & that's why that specific test started failing.
Overall I think simplest solution is just to make other teams aware that they should check their Fr from/to bytes/u64 implementations 😄
Or if we decide to change the traits then probably only Fr should have those _le/_be variants.
Anyways, thank you for detailed explanation!
EDIT: saw you changed your comment in the time it took for me to write the original comment xddd. I guess I would agree with from_le_bytes not being really relevant.
There was a problem hiding this comment.
Yep, deleted the original comment, as it is a bit irrelevant - TFr is not a direct API of eip4844, so technically trait could be implemented as you wish.
I believe we should write additional tests for general traits, so all of them work the same.
sauliusgrigaitis
merged commit 4aa475e
into
grandinetech:integration
In this PR updated:
From 5703f6f3536b7692616bc289ac3f3867ab8db9d8 to b2e41491ad1859f1792964a2432a419b64dc6fb2