test(crypto): add regression baseline for crypto#35
Open
Federico2014 wants to merge 3 commits into
Open
Conversation
Add direct unit coverage for the alt_bn128 (BN254) primitives behind the ecAdd/ecMul/ecPairing precompiles: BN128Fp/BN128G1/BN128G2 point validation, G1 order-r / subgroup behaviour, the G1-vs-G2 subgroup-check asymmetry, and PairingCheck correctness against the EIP-197 generators. Previously these classes had no direct tests; only gas accounting was exercised via Solidity in IstanbulTest. Closes the high-priority gap noted in the Q2 crypto test baseline and related to HackerOne #3769516.
Add direct execute() coverage for two crypto precompiles that previously had no asserting unit test: - ECRecoverTest (0x01): recovered address matches the signer on the success path; bad recovery id, non-zero v padding, zero r/s, and short input all return an empty result without throwing. - Blake2FTest (EIP-152, 0x09): pins the canonical vector 4 input/output pair, plus length / finalization-flag validation and round-count gas pricing. Extends the Q2 crypto precompile test baseline (signature, verification).
📝 WalkthroughWalkthroughThis PR adds three independent JUnit regression test suites for cryptographic precompiles: BN128 validates alt_bn128 curve primitives (G1/G2 point acceptance, subgroup membership, pairing correctness); Blake2FTest exercises the BLAKE2F compression function with canonical vectors and edge cases; and ECRecoverTest verifies ECRECOVER signature recovery and graceful error handling on invalid inputs. ChangesCryptographic precompile regression tests
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Federico2014
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/src/test/java/org/tron/common/runtime/vm/Blake2FTest.java`:
- Around line 74-89: For both rejectsWrongLength and rejectsInvalidFinalFlag,
extend the assertions to ensure that when BLAKE2F.execute(input) returns false
the output bytes are empty/stale-free: after calling BLAKE2F.execute(input)
assert result.getLeft() is false and assert that result.getRight() is either
null or contains only zero bytes (no non-zero/stale data); keep the existing
energy assertion in rejectsInvalidFinalFlag (BLAKE2F.getEnergyForData(input) ==
0L). This uses the existing BLAKE2F.execute and result.getRight() to validate no
output is produced on rejection.
In `@framework/src/test/java/org/tron/common/runtime/vm/ECRecoverTest.java`:
- Around line 96-97: The test currently only asserts that the output is empty
for the polluted-`v` case but omits verifying the execution status; update the
ECRecoverTest polluted-`v` case to assert the boolean execution result
(result.getLeft()) is false like the other malformed-input tests so the contract
is enforced when calling EC_RECOVER.execute(input); locate the Pair<Boolean,
byte[]> result variable and add the same result.getLeft() assertion used
elsewhere.
- Around line 69-72: The test currently only compares the trailing bytes by
extracting tail from result.getRight(), which misses verifying the left padding;
replace that check by constructing a full 32-byte expected word (left-pad
key.getAddress() with zeros to length 32) and assertArrayEquals that full
expected 32-byte array against result.getRight(); update/removing references to
the temporary tail variable and keep using result.getRight() as the actual
value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b1ff9dc-f403-4146-b0f7-df314881f18c
📒 Files selected for processing (3)
framework/src/test/java/org/tron/common/crypto/zksnark/BN128Test.javaframework/src/test/java/org/tron/common/runtime/vm/Blake2FTest.javaframework/src/test/java/org/tron/common/runtime/vm/ECRecoverTest.java
Comment on lines
+74
to
+89
| public void rejectsWrongLength() { | ||
| // 212 bytes — one short of the required 213. | ||
| byte[] input = new byte[212]; | ||
| Pair<Boolean, byte[]> result = BLAKE2F.execute(input); | ||
| assertFalse("incorrect length must fail", result.getLeft()); | ||
| } | ||
|
|
||
| @Test | ||
| public void rejectsInvalidFinalFlag() { | ||
| byte[] input = VECTOR_4_INPUT.clone(); | ||
| input[212] = 0x02; // flag must be 0x00 or 0x01 | ||
|
|
||
| Pair<Boolean, byte[]> result = BLAKE2F.execute(input); | ||
| assertFalse("invalid finalization flag must fail", result.getLeft()); | ||
| assertEquals("invalid flag must price to zero", 0L, BLAKE2F.getEnergyForData(input)); | ||
| } |
There was a problem hiding this comment.
Assert empty output on rejected inputs.
These failure-path tests only check the status bit. If execute() ever returns false but still emits stale/non-empty bytes, this suite would miss it.
Suggested test tightening
`@Test`
public void rejectsWrongLength() {
// 212 bytes — one short of the required 213.
byte[] input = new byte[212];
Pair<Boolean, byte[]> result = BLAKE2F.execute(input);
assertFalse("incorrect length must fail", result.getLeft());
+ assertEquals("failed precompile must not return output", 0, result.getRight().length);
}
@@
public void rejectsInvalidFinalFlag() {
byte[] input = VECTOR_4_INPUT.clone();
input[212] = 0x02; // flag must be 0x00 or 0x01
Pair<Boolean, byte[]> result = BLAKE2F.execute(input);
assertFalse("invalid finalization flag must fail", result.getLeft());
+ assertEquals("failed precompile must not return output", 0, result.getRight().length);
assertEquals("invalid flag must price to zero", 0L, BLAKE2F.getEnergyForData(input));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void rejectsWrongLength() { | |
| // 212 bytes — one short of the required 213. | |
| byte[] input = new byte[212]; | |
| Pair<Boolean, byte[]> result = BLAKE2F.execute(input); | |
| assertFalse("incorrect length must fail", result.getLeft()); | |
| } | |
| @Test | |
| public void rejectsInvalidFinalFlag() { | |
| byte[] input = VECTOR_4_INPUT.clone(); | |
| input[212] = 0x02; // flag must be 0x00 or 0x01 | |
| Pair<Boolean, byte[]> result = BLAKE2F.execute(input); | |
| assertFalse("invalid finalization flag must fail", result.getLeft()); | |
| assertEquals("invalid flag must price to zero", 0L, BLAKE2F.getEnergyForData(input)); | |
| } | |
| public void rejectsWrongLength() { | |
| // 212 bytes — one short of the required 213. | |
| byte[] input = new byte[212]; | |
| Pair<Boolean, byte[]> result = BLAKE2F.execute(input); | |
| assertFalse("incorrect length must fail", result.getLeft()); | |
| assertEquals("failed precompile must not return output", 0, result.getRight().length); | |
| } | |
| `@Test` | |
| public void rejectsInvalidFinalFlag() { | |
| byte[] input = VECTOR_4_INPUT.clone(); | |
| input[212] = 0x02; // flag must be 0x00 or 0x01 | |
| Pair<Boolean, byte[]> result = BLAKE2F.execute(input); | |
| assertFalse("invalid finalization flag must fail", result.getLeft()); | |
| assertEquals("failed precompile must not return output", 0, result.getRight().length); | |
| assertEquals("invalid flag must price to zero", 0L, BLAKE2F.getEnergyForData(input)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/src/test/java/org/tron/common/runtime/vm/Blake2FTest.java` around
lines 74 - 89, For both rejectsWrongLength and rejectsInvalidFinalFlag, extend
the assertions to ensure that when BLAKE2F.execute(input) returns false the
output bytes are empty/stale-free: after calling BLAKE2F.execute(input) assert
result.getLeft() is false and assert that result.getRight() is either null or
contains only zero bytes (no non-zero/stale data); keep the existing energy
assertion in rejectsInvalidFinalFlag (BLAKE2F.getEnergyForData(input) == 0L).
This uses the existing BLAKE2F.execute and result.getRight() to validate no
output is produced on rejection.
Comment on lines
+69
to
+72
| // The precompile left-pads the 21-byte TRON address into a 32-byte word. | ||
| byte[] expected = key.getAddress(); | ||
| byte[] tail = Arrays.copyOfRange(result.getRight(), 32 - expected.length, 32); | ||
| assertArrayEquals("recovered address must match the signer", expected, tail); |
There was a problem hiding this comment.
Assert the full 32-byte return word.
This only checks the tail bytes, so a regression that returns the right address with non-zero left padding would still pass. Compare against the fully padded 32-byte expected word instead.
Proposed change
- // The precompile left-pads the 21-byte TRON address into a 32-byte word.
- byte[] expected = key.getAddress();
- byte[] tail = Arrays.copyOfRange(result.getRight(), 32 - expected.length, 32);
- assertArrayEquals("recovered address must match the signer", expected, tail);
+ // The precompile left-pads the 21-byte TRON address into a 32-byte word.
+ byte[] expected = new byte[32];
+ byte[] address = key.getAddress();
+ System.arraycopy(address, 0, expected, 32 - address.length, address.length);
+ assertArrayEquals("recovered address must match the signer", expected, result.getRight());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // The precompile left-pads the 21-byte TRON address into a 32-byte word. | |
| byte[] expected = key.getAddress(); | |
| byte[] tail = Arrays.copyOfRange(result.getRight(), 32 - expected.length, 32); | |
| assertArrayEquals("recovered address must match the signer", expected, tail); | |
| // The precompile left-pads the 21-byte TRON address into a 32-byte word. | |
| byte[] expected = new byte[32]; | |
| byte[] address = key.getAddress(); | |
| System.arraycopy(address, 0, expected, 32 - address.length, address.length); | |
| assertArrayEquals("recovered address must match the signer", expected, result.getRight()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/src/test/java/org/tron/common/runtime/vm/ECRecoverTest.java` around
lines 69 - 72, The test currently only compares the trailing bytes by extracting
tail from result.getRight(), which misses verifying the left padding; replace
that check by constructing a full 32-byte expected word (left-pad
key.getAddress() with zeros to length 32) and assertArrayEquals that full
expected 32-byte array against result.getRight(); update/removing references to
the temporary tail variable and keep using result.getRight() as the actual
value.
Comment on lines
+96
to
+97
| Pair<Boolean, byte[]> result = EC_RECOVER.execute(input); | ||
| assertEquals("non-zero v padding must yield empty output", 0, result.getRight().length); |
There was a problem hiding this comment.
Check the execution status in the polluted-v case.
This case currently passes on any empty output, even if execute() starts reporting failure. Add the same result.getLeft() assertion used by the other malformed-input tests so the regression baseline matches the intended contract.
Proposed change
Pair<Boolean, byte[]> result = EC_RECOVER.execute(input);
+ assertTrue(result.getLeft());
assertEquals("non-zero v padding must yield empty output", 0, result.getRight().length);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Pair<Boolean, byte[]> result = EC_RECOVER.execute(input); | |
| assertEquals("non-zero v padding must yield empty output", 0, result.getRight().length); | |
| Pair<Boolean, byte[]> result = EC_RECOVER.execute(input); | |
| assertTrue(result.getLeft()); | |
| assertEquals("non-zero v padding must yield empty output", 0, result.getRight().length); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/src/test/java/org/tron/common/runtime/vm/ECRecoverTest.java` around
lines 96 - 97, The test currently only asserts that the output is empty for the
polluted-`v` case but omits verifying the execution status; update the
ECRecoverTest polluted-`v` case to assert the boolean execution result
(result.getLeft()) is false like the other malformed-input tests so the contract
is enforced when calling EC_RECOVER.execute(input); locate the Pair<Boolean,
byte[]> result variable and add the same result.getLeft() assertion used
elsewhere.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds a focused regression baseline for the cryptography-related precompiled contracts that previously lacked asserting unit coverage. No production code is changed — test files and JSON vector resources only.
Two layers:
BN128TestECRecoverTestBlake2FTestframework/src/test/resources/precompiles/, reusing the existing{Input, Expected, Gas, Name}JSON schema already used byp256verify_test_vectors.json:bn128add_test_vectors.jsonbn128mul_test_vectors.jsonbn128pairing_test_vectors.jsonblake2f_test_vectors.jsonPrecompileVectorTestloads these and asserts both output and gas. Adding more vectors needs only a JSON append — no test-code change.Why are these changes required?
The crypto precompiles had coverage gaps:
BN128*/PairingCheckhad no direct unit test (only gas accounting was exercised through Solidity inIstanbulTest);ECRECOVERhad a single non-asserting invocation;BLAKE2Fwas only tested indirectly via a Solidity contract. TheBN128Testorder-r and pairing-negation checks also lock in the G1 cofactor=1 property relevant to HackerOne #3769516.This PR has been tested by:
./gradlew :framework:test: 25/25 pass./gradlew :framework:checkstyleTest: passVector correctness is not hand-typed: BN128 vectors are computed against the curve math in a separate script, BLAKE2F vectors are captured from actual
Blake2F.execute()output, and all are re-asserted byPrecompileVectorTestagainst the live precompile (output + gas).Compatibility note
TRON's ECRECOVER (0x01) returns a 21-byte TRON address (
sha3omit12, keccak[11:32] with the leading byte replaced by the TRON prefix0x41), not Ethereum's 20-byte address. Standard go-ethereum ecrecover vectors therefore cannot be reused directly — which is why ECRECOVER keeps a programmatic self-verifying test (deterministic signature → assert recovered address equals the signer) rather than a JSON vector file.Follow up
Optional: extend the vector set with more go-ethereum bn256/blake2f boundary cases and official modExp/sha256 vectors under the same
resources/precompiles/directory.