blake2f and point evaluation precompile#2828
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let z_fr = Fr::from_be_bytes_mod_order(z); | ||
| let y_fr = Fr::from_be_bytes_mod_order(y); |
There was a problem hiding this comment.
[HIGH] Reject non-canonical BLS field elements before verification
from_be_bytes_mod_order silently reduces the caller-provided z and y modulo the BLS scalar field. The point-evaluation precompile must reject encodings >= BLS_MODULUS; otherwise calldata containing z + r or y + r verifies against the reduced field element while the raw precompile input is not the claimed EIP-4844 field element. Contracts that rely on this precompile for strict blob/KZG verification can accept malformed proofs.
Parse these with a canonical field-element check before constructing Fr values. At the same input boundary, make the length check exact (input.len() == 192) rather than accepting trailing bytes.
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE BASELINE scrutiny: author has write permission, substantial subtensor contribution history, no Gittensor allowlist hit, and no committer/author mismatch; branch blake2f-point-evaluation -> devnet-ready. Static-only Skeptic review: I did not build, test, or execute PR code. No review-infrastructure files were modified. The new Blake2F dependency is pinned to the same RaoFoundation/frontier revision already used by existing EVM precompile dependencies, and the other runtime precompile dependencies are existing workspace dependencies. The point-evaluation input path rejects non-192-byte inputs before fixed-range copies/slices. FindingsNo findings. Prior-comment reconciliation
ConclusionI found no malicious behavior or security vulnerability in the current diff under static Skeptic review. 🔍 AI Review — Auditor (domain review)VERDICT: 👎 Gittensor: LIKELY by subtensor-heavy PR history; author has write permission and substantial prior contributions, with no direct allowlist hit. Spec-version bump is present ( The prior point-evaluation test concern appears addressed: the current tests include a mainnet success vector and the go-kzg/EEST vector set exercising successful proofs against production Findings
Prior-comment reconciliation
ConclusionBlocking because Blake2F remains exposed only at non-canonical 📜 Previous run (superseded)
|
|
🔄 AI review updated — Skeptic: VULNERABLE |
| let z_fr = Fr::from_be_bytes_mod_order(z); | ||
| let y_fr = Fr::from_be_bytes_mod_order(y); |
There was a problem hiding this comment.
[HIGH] Reject non-canonical BLS field elements before verification
EIP-4844 point-evaluation calldata requires z and y to be canonical BLS scalar field elements. Fr::from_be_bytes_mod_order reduces any 32-byte value modulo the field order, so out-of-range encodings are accepted as aliases for valid field elements and can make malformed calldata verify. Reject z/y >= BLS_MODULUS_BYTES before converting them.
| let z_fr = Fr::from_be_bytes_mod_order(z); | |
| let y_fr = Fr::from_be_bytes_mod_order(y); | |
| let deserialize_bls_scalar = |bytes: &[u8]| { | |
| if bytes >= BLS_MODULUS_BYTES.as_slice() { | |
| return Err(PrecompileFailure::Error { | |
| exit_status: ExitError::Other("invalid field element".into()), | |
| }); | |
| } | |
| Ok(Fr::from_be_bytes_mod_order(bytes)) | |
| }; | |
| let z_fr = deserialize_bls_scalar(z)?; | |
| let y_fr = deserialize_bls_scalar(y)?; |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| a if a == hash(10) => Some(Blake2F::execute(handle)), | ||
| a if a == hash(11) => Some(PointEvaluation::execute(handle)), |
There was a problem hiding this comment.
[HIGH] Wire these precompiles to the canonical Ethereum addresses
Ethereum assigns BLAKE2F to 0x0000000000000000000000000000000000000009 and EIP-4844 point evaluation to 0x000000000000000000000000000000000000000a. This registers BLAKE2F at hash(10) and point evaluation at hash(11), so standard contracts and clients calling the blob proof precompile at 0x0a will execute Blake2F instead, while point evaluation is only reachable at the non-standard 0x0b address. Please either wire the canonical addresses with an explicit compatibility plan for the existing non-standard slots, or make this a documented Bittensor-only address extension with tests proving that behavior.
| #[test] | ||
| fn point_evaluation_requires_192_bytes() { | ||
| new_test_ext().execute_with(|| { | ||
| let result = execute_point_eval(vec![0u8; 100]); | ||
| assert!(result.is_err()); | ||
| }); | ||
| } | ||
|
|
||
| #[test] | ||
| fn point_evaluation_rejects_invalid_versioned_hash() { | ||
| new_test_ext().execute_with(|| { | ||
| let mut input = vec![0u8; 192]; | ||
| input[0] = 0x00; | ||
| let result = execute_point_eval(input); | ||
| assert!(result.is_err()); | ||
| }); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
[MEDIUM] Add a successful KZG proof vector test
The new point-evaluation precompile only has malformed-input tests. That would still pass if the verifier rejected every valid proof, returned the wrong success payload, or was unreachable through the intended precompile address. Add at least one canonical EIP-4844 success vector asserting the 64-byte return value, plus a registry/routing test for the address that contracts are expected to call.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| let z = &input[32..64]; | ||
| let y = &input[64..96]; | ||
| let commitment_bytes = &input[96..144]; | ||
| let proof_bytes = &input[144..196]; |
There was a problem hiding this comment.
[CRITICAL] Fix out-of-bounds proof slice before exposing the precompile
INPUT_LEN is 192, but this slices input[144..196]. Because the length check above requires exactly 192 bytes, any call with the expected input length panics here before returning an EVM error. This is runtime/precompile code, so an externally submitted EVM call can trigger a chain-bricking panic. The proof occupies the remaining 48 bytes, so the end offset should be 192.
| let proof_bytes = &input[144..196]; | |
| let proof_bytes = &input[144..192]; |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| let z = &input[32..64]; | ||
| let y = &input[64..96]; | ||
| let commitment_bytes = &input[96..144]; | ||
| let proof_bytes = &input[144..196]; |
There was a problem hiding this comment.
[CRITICAL] Fix out-of-bounds proof slice before exposing the precompile
INPUT_LEN is exactly 192, but this slice takes input[144..196]. Rust slice indexing panics when the end bound exceeds the array length, so a caller can submit a 192-byte input and trigger a runtime panic in the EVM precompile path instead of receiving an EVM error. The KZG proof is 48 bytes, so the range should end at 192.
| let proof_bytes = &input[144..196]; | |
| let proof_bytes = &input[144..192]; |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| a if a == hash(8) => Some(Bn128Pairing::execute(handle)), | ||
| a if a == hash(9) => Some(Bn128Add::execute(handle)), | ||
| a if a == hash(10) => Some(PointEvaluation::execute(handle)), | ||
| a if a == hash(11) => Some(Blake2F::execute(handle)), |
There was a problem hiding this comment.
[HIGH] Resolve non-canonical Blake2F routing before merge
This still exposes Blake2F at 0x0b, while EIP-152 callers use 0x09. I see the existing table already uses 0x09 for Bn128Add, so this may require an explicit compatibility decision rather than a one-line swap, but as written the PR does not provide the standard Blake2F precompile address that EVM contracts expect. Either wire Blake2F to the EIP address through a documented compatibility plan, or make the non-standard 0x0b semantics explicit in code/docs/tests so this is not presented as a standard Ethereum precompile.
| input[1] = 0x42; | ||
| let result = execute_point_eval(input); | ||
| assert!(result.is_err()); | ||
| }); |
There was a problem hiding this comment.
[MEDIUM] Add a successful KZG proof vector test
The new point-evaluation precompile only tests rejection paths. That leaves the pairing equation, hard-coded G2 SRS, compressed point decoding, versioned-hash handling, and returned constants without a positive regression check. Add a known-good EIP-4844 point-evaluation vector that succeeds and asserts the exact 64-byte output (FIELD_ELEMENTS_PER_BLOB_BYTES || BLS_MODULUS_BYTES).
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| a if a == hash(8) => Some(Bn128Pairing::execute(handle)), | ||
| a if a == hash(9) => Some(Bn128Add::execute(handle)), | ||
| a if a == hash(10) => Some(PointEvaluation::execute(handle)), | ||
| a if a == hash(11) => Some(Blake2F::execute(handle)), |
There was a problem hiding this comment.
[HIGH] Resolve non-canonical Blake2F routing before merge
This adds Blake2F only at 0x0b, while Ethereum clients and contracts call Blake2F at 0x09. The PR description notes that 0x09 is already occupied here, but leaving the new precompile only at a non-EIP address means standard Ethereum tooling still calls the existing Bn128Add route instead of Blake2F. Before merge, resolve the compatibility strategy explicitly: either provide a backward-compatible canonical 0x09 route, document and test an intentional Subtensor-only address mapping, or split the larger address-layout migration out so this PR does not claim Ethereum Blake2F compatibility that it does not provide.
| #[test] | ||
| fn point_evaluation_rejects_versioned_hash_mismatch() { | ||
| new_test_ext().execute_with(|| { | ||
| let mut input = vec![0u8; 192]; | ||
| input[0] = 0x01; | ||
| input[1] = 0x42; | ||
| let result = execute_point_eval(input); | ||
| assert!(result.is_err()); |
There was a problem hiding this comment.
[MEDIUM] Add a successful KZG proof vector test
The new point-evaluation precompile currently has only rejection-path tests: short input, invalid version byte, and hash mismatch. That leaves the pairing equation, output payload, scalar boundary handling, and canonical commitment/proof deserialization untested on the one path this precompile is supposed to expose. Add at least one known-good EIP-4844/KZG vector that returns the expected 64-byte output, preferably sourced from upstream consensus/EIP test fixtures rather than generated from the implementation under test.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| a if a == hash(8) => Some(Bn128Pairing::execute(handle)), | ||
| a if a == hash(9) => Some(Bn128Add::execute(handle)), | ||
| a if a == hash(10) => Some(PointEvaluation::execute(handle)), | ||
| a if a == hash(11) => Some(Blake2F::execute(handle)), |
There was a problem hiding this comment.
[HIGH] Resolve non-canonical Blake2F routing before merge
This PR adds Blake2F only at 0x0b, but the Ethereum Blake2F precompile is 0x09. The PR description acknowledges the conflict with the existing Bn128Add route, but merging this as-is means Ethereum-compatible callers that use 0x09 will still hit the wrong precompile and fail. Resolve the address compatibility plan before merge: either make Blake2F available at the canonical address without silently breaking existing users, or explicitly split this from the Ethereum-compatibility claim with migration documentation and tests for the chosen routing behavior.
| // Constant polynomial f(x) = 100. | ||
| // Commitment = 100 * G1, so C - y*G1 = 0. | ||
| // Proof = identity (G1 point at infinity). |
There was a problem hiding this comment.
[MEDIUM] Add a nontrivial successful KZG proof vector
The new success test uses a constant polynomial and an identity proof, so C - y*G1 and the proof are both the identity. That makes the pairing equation pass without exercising the hardcoded G2_SRS, the S - z term, or a real non-zero proof. Please add a fixed successful EIP-4844/KZG vector with a non-identity proof and assert the exact 64-byte output, so the test would fail if the SRS constant, sign convention, or pairing inputs are wrong.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| a if a == hash(8) => Some(Bn128Pairing::execute(handle)), | ||
| a if a == hash(9) => Some(Bn128Add::execute(handle)), | ||
| a if a == hash(10) => Some(PointEvaluation::execute(handle)), | ||
| a if a == hash(11) => Some(Blake2F::execute(handle)), |
There was a problem hiding this comment.
[HIGH] Resolve non-canonical Blake2F routing before merge
This still exposes Blake2F only at 0x0b, even though EIP-152 callers use 0x09. The PR body correctly notes that 0x09 is already occupied here, but merging this as-is means standard Ethereum tooling and contracts that call the Blake2F precompile will fail on Subtensor. Please resolve the address conflict explicitly before merge, for example by introducing a compatibility plan that provides canonical Blake2F behavior without silently breaking the existing hash(9) users, or by narrowing the PR/issue so this is documented as a non-Ethereum-compatible custom precompile rather than the EVM Blake2F precompile.
| assert_eq!(result.unwrap().output, EXPECTED_OUTPUT); | ||
|
|
||
| // Verify the real precompile rejects it (wrong SRS) | ||
| let real_reject = PointEvaluation::execute_impl(&input, &G2_SRS); |
There was a problem hiding this comment.
[MEDIUM] Add a nontrivial success vector for the production trusted setup
The new nontrivial test proves the pairing equation with a synthetic test-only SRS, then asserts the same vector is rejected by the production G2_SRS. That is useful for the algebra, but it still does not catch a wrong hardcoded production trusted setup, serialization mismatch, or sign/order mismatch for real EIP-4844 KZG vectors. Add at least one official/nontrivial point-evaluation fixture that succeeds through PointEvaluation::execute with the hardcoded G2_SRS; the current identity-success case passes independently of the SRS and is too weak for this precompile.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| a if a == hash(8) => Some(Bn128Pairing::execute(handle)), | ||
| a if a == hash(9) => Some(Bn128Add::execute(handle)), | ||
| a if a == hash(10) => Some(PointEvaluation::execute(handle)), | ||
| a if a == hash(11) => Some(Blake2F::execute(handle)), |
There was a problem hiding this comment.
[HIGH] Resolve non-canonical Blake2F routing before merge
This routes Blake2F only at 0x0b, while the Ethereum/EIP-152 Blake2F precompile address is 0x09. Standard contracts, libraries, and conformance tests will call 0x09; on this runtime that address still dispatches to Bn128Add, so the new precompile is not actually available at the expected EVM ABI address.
The PR body notes the existing conflict, but this needs a resolved compatibility decision before merge: either move/alias the precompile set so Blake2F is available at canonical 0x09, or explicitly document and test a chain-specific non-canonical address plan instead of presenting this as Ethereum precompile compatibility.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
open-junius
requested review from
UnArbosFive,
UnArbosFour,
UnArbosSeven,
UnArbosSix,
UnArbosThree and
UnArbosTwo
| a if a == hash(8) => Some(Bn128Pairing::execute(handle)), | ||
| a if a == hash(9) => Some(Bn128Add::execute(handle)), | ||
| a if a == hash(10) => Some(PointEvaluation::execute(handle)), | ||
| a if a == hash(11) => Some(Blake2F::execute(handle)), |
There was a problem hiding this comment.
[HIGH] Resolve non-canonical Blake2F routing before merge
This still exposes Blake2F only at 0x0b, while Ethereum/EIP-152 clients call Blake2F at 0x09. Because 0x09 is currently occupied by the existing BN128 add mapping, this needs an explicit compatibility decision before merge: either migrate/fix the existing canonical address layout, provide a backward-compatible aliasing plan, or stop presenting this as the standard Blake2F precompile. As written, standard callers will fail to reach the feature this PR claims to add.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
UnArbosTwo
left a comment
UnArbosTwo
left a comment
There was a problem hiding this comment.
Looks good but can you make a doc with a table with all the existing and new precompiles and their addresses.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2 participants
Description
The PR add the blake2f and point evaluation precompiles.
According to EVM, the address 0x09 is for blake2f precompile. But it is already used before.
So the address 0x0b is assigned to blake2f.
Related Issue(s)
Type of Change
Breaking Change
If this PR introduces a breaking change, please provide a detailed description of the impact and the migration path for existing applications.
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctlyScreenshots (if applicable)
Please include any relevant screenshots or GIFs that demonstrate the changes made.
Additional Notes
Please provide any additional information or context that may be helpful for reviewers.