Conversation
Greptile SummaryThis PR adds a new Key observations from the review:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Constantine Library] --> B[High-level Protocols\nconstantine/]
A --> C[C API\ninclude/]
A --> D[Go API\nconstantine-go/]
A --> E[Rust API\nconstantine-rust/]
B --> CL[Consensus Layer]
B --> EL[Execution Layer]
CL --> KZG[KZG Polynomial Commitments\nEIP-4844]
CL --> BLS[BLS Signatures\nBLS12-381]
CL --> EIP2333[EIP-2333 Key Derivation]
EL --> PRE[EVM Precompiles]
EL --> VKL[Verkle IPA\nin scope?]
PRE --> SHA[SHA-256]
PRE --> MODEXP[Modular Exponentiation]
PRE --> BN254[BN254 add/mul/pairing]
PRE --> BLS381[BLS12-381 ops\nnext upgrade]
A --> MATH[Math Layer]
MATH --> ARITH[Bigint & Field Arithmetic]
MATH --> EXT[Extension Fields]
MATH --> EC[Elliptic Curves]
MATH --> PAIR[Pairings]
MATH --> H2C[Hash to Curve]
MATH --> POLY[Polynomial Arithmetic]
MATH --> ENDO[Endomorphisms]
A --> MISC[Misc]
MISC --> HASH[SHA-256 / HMAC / HKDF]
MISC --> RNG[CSPRNG]
MISC --> TP[Threadpool]
Last reviewed commit: b88ba53 |
|
|
||
| ## Dependencies | ||
|
|
||
| To remove the thread of supply chain attacks, Constantine has no external dependencies except: |
There was a problem hiding this comment.
"thread" should be "threat"
The word "thread" is used where "threat" is clearly intended. In the context of a security-focused audit document, this creates a confusing sentence.
| To remove the thread of supply chain attacks, Constantine has no external dependencies except: | |
| To remove the threat of supply chain attacks, Constantine has no external dependencies except: |
| ## Overview | ||
|
|
||
| The Ethereum community is pursuing credible neutrality and implementation diversification wants to avoid single points of failure. | ||
| This is examplified by investing in 5 different clients for the Consensus Layer (CL). |
There was a problem hiding this comment.
Typo: "examplified" → "exemplified"
| This is examplified by investing in 5 different clients for the Consensus Layer (CL). | |
| This is exemplified by investing in 5 different clients for the Consensus Layer (CL). |
| C, Go and Rust API are exposed respectively in | ||
| - https://github.com/mratsim/constantine/tree/master/include | ||
| - https://github.com/mratsim/constantine/tree/master/constantine-rust | ||
| - https://github.com/mratsim/constantine/tree/master/constantine-go |
There was a problem hiding this comment.
"respectively" order mismatch
The text states "C, Go and Rust API are exposed respectively in" but the list order below is C → Rust → Go (not C → Go → Rust). Either the prose or the list order should be corrected so "respectively" is accurate.
| C, Go and Rust API are exposed respectively in | |
| - https://github.com/mratsim/constantine/tree/master/include | |
| - https://github.com/mratsim/constantine/tree/master/constantine-rust | |
| - https://github.com/mratsim/constantine/tree/master/constantine-go | |
| C, Go and Rust API are exposed respectively in | |
| - https://github.com/mratsim/constantine/tree/master/include | |
| - https://github.com/mratsim/constantine/tree/master/constantine-go | |
| - https://github.com/mratsim/constantine/tree/master/constantine-rust |
| As of November 2024, it is also the fastest cryptography backend for pairing-based cryptography on x86. In particular, with the rise of zkRollups, current non-go BN254 are becoming a bottleneck in Besu (Java), Nethermind (C#), Nimbus-eth1 (Nim) and Reth (Rust) as they are based on the old Zcash implementation (https://github.com/zcash-hackworks/bn) or libff (https://github.com/scipr-lab/libff) which were over 10x slower than state-of-the art already in January 2021 (https://hackmd.io/@gnark/eccbench) | ||
|
|
||
| ## Library organization | ||
| _TODO: tag a new version for audit_ |
There was a problem hiding this comment.
Unresolved TODO in audit-facing document
There is an open _TODO: tag a new version for audit_ note in the Library organization section. Since this document is intended to be shared with auditors and interested parties (Ethereum Foundation, client teams), it should have this resolved or at minimum tracked before the document is considered final.
| - https://github.com/jtraglia/kzg-fuzz | ||
| - Geth: | ||
| - https://github.com/ethereum/go-ethereum/tree/v1.14.12/tests/fuzzers/bn256 | ||
| - https://github.com/ethereum/go-ethereum/tree/v1.14.12/tests/fuzzers/bls12381 No newline at end of file |
There was a problem hiding this comment.
Missing newline at end of file
The file is missing a trailing newline, which is flagged by the diff (\ No newline at end of file). Adding a trailing newline is a standard convention.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This is a tentative scoping document for Constantine's audit.
cc-ing interested parties:
Ethereum Foundation: @JustinDrake, @asanso, @jtraglia, @kevaundray
Consensus Client teams: @arnetheduck, @tersec, @OisinKyne, @sauliusgrigaitis,
Execution Client teams: @arnetheduck, @tersec, @garyschulte, @NickSneo
I've also created a discussion thread to use once scoping is done #482