Closed jtraglia closed 1 month ago
This looks great!
Some thoughts:
constants.h
and types.h
over just stuffing them into the (already quite small) api.h
.eip7594/api.c
entry point. I would go for eip7594/eip7594.c
but I understand that might not be best either. Both points above are just personal preference, and I hold no strong opinions.
Some thoughts after reviewing this PR.
As a personal preference, I find oversplitting can sometimes be counterproductive. Having files that hold a single function or type can introduce a mental overhead when trying to locate specific functions, and it can also create a practical overhead in terms of handling numerous files.
IMO for c-kzg, file splitting should aim to highlight the responsibilities of each module, and to specify the scope of objects.
On this regard, I think this PR takes good directions by moving generic algebra functionality to common/
and keeping the Ethereum protocol functonality into eip4844/
and eip7594/
. We have also agreed that it's OK for eip7594/
to import eip4844/
because that's how the spec does it, but the opposite direction is not OK.
Some notes on the PR:
common/
:
g1.c
and g2.c
into ec.c
that contains elliptic curve operations.ret.c
since it's empty (as well as settings.c
and any other empty file).helpers.c
to utils.c
. (Nit)KZGSettings
should be in common/
since it contains protocol-specific information. I'm not sure if it should be in eip4844/
either though, since it contains 7594 information. Keeping it as is might be the best option now, but this abstraction leak is something to note.KZGCommitment
and KZGProof
to eip4844/
since they are protocol-specific structures and not generic.eip4844/
:
api.c
to eip4844.c
. IMO it's fine to have a repeated eip4844/eip4844.c
structure as it indicates that the file contains the main implementation of the 4844 module. I would consider doing the same for 7594.eip7594/
:
eip7594.c
which contains all the public methods as well as central EIP7594 types (like Cell)fft.c
which contains field FFT, group FFT, and coset FFTfk20.c
which contains the FK20 codebaserecovery.c
which contains the recovery implementation
This PR continues to split the core C code into more manageable files.
Four new directories have been created:
common
-- Contains general code, some of which is shared between eip4844 and eip7594.eip4844
-- As the name implies, contains the implementation for EIP-4844.eip4844.c
) has been mostly left as-as. Don't poke the audited dragon.eip7594
-- As the name implies, contains the implementation for EIP-7594.fk20.c
which handles FK20 proof generation,recovery.c
which implements cell recovery, andfft.c
which provides methods for FFT.setup
-- Contains code pertaining to loading the trusted setup.KZGSettings
, which didn't quite make sense incommon
. This is a bit of a weird situation. We don't want to include it ineip4844
because it's been updated foreip7594
.We needed to include the C source directory in the Rust bindings generator so the updated includes would work.
There are various other small changes, but no changes to the implementation have been made here.
Added
InsertNewlineAtEOF: true
to the clang formatter configuration file. Several of my new files did not contain a newline at the bottom, which showed an ugly crossed out circle on GitHub.