feat(vm): add optimized BN128 precompiled contracts via IPC#1
Open
Federico2014 wants to merge 1 commit intodevelopfrom
Open
feat(vm): add optimized BN128 precompiled contracts via IPC#1Federico2014 wants to merge 1 commit intodevelopfrom
Federico2014 wants to merge 1 commit intodevelopfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces BN128 elliptic curve operation optimization for the TRON blockchain by adding socket-based IPC support for native EIP-196 operations, along with configuration plumbing, a new chain governance parameter, and comprehensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant VM as VM Execution<br/>(PrecompiledContract)
participant Pool as SocketBN128Client<br/>(Pool Manager)
participant Server as SocketBn128Server<br/>(Listener)
participant Worker as Worker Thread<br/>(Thread Pool)
participant Executor as BN128Executor<br/>(Native Wrapper)
Note over VM,Executor: Optimized BN128 Path (when enabled)
VM->>Pool: bn128Add/Mul/Pairing(byte[] input)
activate Pool
Pool->>Pool: Get/Acquire SocketChannel
Pool->>Server: Send opcode + length + input<br/>(5-byte header + payload)
activate Server
Server->>Server: Parse request header
Server->>Server: Enqueue SocketRequest to worker queue
deactivate Server
Server->>Worker: Dequeue & process request
activate Worker
Worker->>Executor: BN128Executor.add/mul/pairing()
activate Executor
Executor->>Executor: Call LibGnarkEIP196.eip196_perform_operation()
Executor-->>Worker: Pair<Boolean, byte[]> result
deactivate Executor
Worker->>Server: Write response (status + length + payload)
Pool->>Pool: Read 5-byte response header
Pool->>Pool: Read response payload
Pool-->>VM: Return Pair<Boolean, byte[]>
deactivate Worker
deactivate Pool
Note over VM,Executor: Fallback: Non-Optimized Path (native or pure Java)
Note over VM: On socket error/timeout or if optimization disabled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Owner
Author
|
@cubic-dev-ai review |
@Federico2014 I have started the AI code review. It will take a few minutes to complete. |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
17 issues found across 24 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="actuator/src/main/java/org/tron/core/vm/PrecompiledContracts.java">
<violation number="1" location="actuator/src/main/java/org/tron/core/vm/PrecompiledContracts.java:435">
P0: The `SocketBN128Client` is created inside a `static {}` block that runs exactly once at class-load time. Because `VMConfig.ALLOW_OPTIMIZED_BN128` defaults to `false` and is only set later by `ConfigLoader.load()` (called per-transaction from `VMActuator`), the static block will see `false` and skip client creation. After that, even if the chain parameter is enabled through governance, the static block never re-runs, so `client` stays `null` and the optimized path is unreachable.
Consider lazy-initializing the client on first use (double-checked locking or an explicit `init()` method called after `ConfigLoader.load()`), so activation through governance works without a node restart.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/Wallet.java">
<violation number="1" location="framework/src/main/java/org/tron/core/Wallet.java:1512">
P2: Key string `"getAllowOptimizedBn128"` doesn't match the getter method name `getAllowOptimizedBN128()`. Every other chain parameter key in this file mirrors the exact method name. This inconsistency can confuse API consumers who expect the key to correspond to the getter.</violation>
</file>
<file name="crypto/src/main/java/org/tron/common/crypto/bn128/BN128Executor.java">
<violation number="1" location="crypto/src/main/java/org/tron/common/crypto/bn128/BN128Executor.java:64">
P1: Returning `null` here will cause a `NullPointerException` in the callers (`add`, `mul`, `pairing`), since none of them handle a null return. Return `Pair.of(false, EMPTY_BYTE_ARRAY)` to match the error convention used elsewhere, or throw a descriptive exception.</violation>
</file>
<file name="crypto/build.gradle">
<violation number="1" location="crypto/build.gradle:16">
P0: **Supply chain risk**: This dependency pulls native cryptographic binaries from a personal GitHub fork (`federico2014/besu-native`) via JitPack instead of using the official Hyperledger-published artifact (`org.hyperledger.besu`). For a consensus-critical BN128 precompile, a compromised or tampered fork could produce incorrect cryptographic results or introduce backdoors. Consider publishing a vetted fork under the project's own organization with reproducible builds, or using the official `org.hyperledger.besu:besu-native-gnark` artifact from Maven Central.</violation>
</file>
<file name="framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java">
<violation number="1" location="framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java:37">
P3: Class name typo: `BN128PrecompiledContratsTest` → `BN128PrecompiledContractsTest` (missing 'c' in "Contracts").</violation>
<violation number="2" location="framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java:396">
P2: This benchmark test is missing `@Ignore`. Unlike the other benchmark tests in this file (`bn128BenchOptimized`, `bn128BenchUnoptimized`), this test has no `@Ignore` annotation, so it will run on every CI build. It performs 100+ heavy pairing executions with no correctness assertions—only timing logs.</violation>
</file>
<file name="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java">
<violation number="1" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java:89">
P1: Missing validation of `len` read from the client. A negative value causes `NegativeArraySizeException` (since `remaining() >= negative` is always true), and a value exceeding `MAX_PAYLOAD` causes the connection to stall in an infinite compact loop because the buffer can never hold enough data.</violation>
<violation number="2" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java:115">
P1: `workers.shutdown()` will not stop the worker threads because they are blocked on `queue.take()` inside a `while(true)` loop. Use `shutdownNow()` to interrupt them, and handle `InterruptedException` in the worker loop to break out.</violation>
<violation number="3" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java:175">
P1: `ch.write(resp)` on a non-blocking channel may not write all bytes. The return value is ignored, so clients can receive truncated responses. Either loop until `resp.hasRemaining()` is false, or temporarily set the channel to blocking for the write.</violation>
</file>
<file name="crypto/src/main/java/org/tron/common/crypto/bn128/BN128Service.java">
<violation number="1" location="crypto/src/main/java/org/tron/common/crypto/bn128/BN128Service.java:8">
P2: This interface defines `close()` with the same signature as `AutoCloseable.close()` but doesn't extend `AutoCloseable`. Without it, callers can't use try-with-resources to manage the service lifecycle, risking resource leaks if an exception is thrown before an explicit `close()` call.</violation>
<violation number="2" location="crypto/src/main/java/org/tron/common/crypto/bn128/BN128Service.java:13">
P2: Javadoc says "two compressed G1 points" but 128 bytes corresponds to two *uncompressed* G1 points (64 bytes each: 32-byte x + 32-byte y). Compressed points would be ~33 bytes each.</violation>
<violation number="3" location="crypto/src/main/java/org/tron/common/crypto/bn128/BN128Service.java:29">
P2: Javadoc says "192 bytes" but the actual input is a *multiple* of 192 bytes (one or more pairing pairs). This could mislead callers into thinking only a single pair is accepted.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/config/args/Args.java">
<violation number="1" location="framework/src/main/java/org/tron/core/config/args/Args.java:995">
P2: Missing validation for `bn128SocketTimeout` — a zero or negative value could cause socket operations to hang indefinitely or fail. Add a guard similar to `dynamicConfigCheckInterval`.</violation>
</file>
<file name="framework/src/main/java/org/tron/program/FullNode.java">
<violation number="1" location="framework/src/main/java/org/tron/program/FullNode.java:59">
P1: `InterruptedException` from `Thread.currentThread().join()` is caught by the same `catch` block that handles startup failures, causing a misleading error log and `System.exit(1)` on graceful shutdown. Handle the blocking-wait interruption separately so a normal interrupt doesn't look like a startup crash.</violation>
</file>
<file name="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBN128Client.java">
<violation number="1" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBN128Client.java:42">
P1: `Socket.setSoTimeout()` has no effect on `SocketChannel.read()` — the timeout only applies to `Socket.getInputStream().read()`. This means reads will block indefinitely if the server hangs, and the `SocketTimeoutException` catch block is unreachable. Use `Selector` with a non-blocking channel, or read via `ch.socket().getInputStream()` instead.</violation>
<violation number="2" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBN128Client.java:52">
P1: `SocketChannel.read()` is not guaranteed to fill the buffer in a single call. Both the header and body reads must loop until the buffer has no remaining capacity, otherwise partial TCP segments will corrupt the protocol framing and produce silent data errors.
Replace single reads with a loop like:
```java
while (header.hasRemaining()) {
if (ch.read(header) == -1) throw new IOException("Connection closed");
}
```</violation>
<violation number="3" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBN128Client.java:66">
P1: On exception, the possibly-broken `SocketChannel` is returned to the pool via `pool.put(ch)`. Subsequent callers will receive a channel that may be disconnected or have leftover bytes in its read buffer, causing cascading failures. On error, close the broken channel and replace it with a fresh connection before returning it to the pool.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // BN128 client connection pool size | ||
| private static final int BN128_CLIENT_POOL_SIZE = 8; | ||
|
|
||
| static { |
There was a problem hiding this comment.
P0: The SocketBN128Client is created inside a static {} block that runs exactly once at class-load time. Because VMConfig.ALLOW_OPTIMIZED_BN128 defaults to false and is only set later by ConfigLoader.load() (called per-transaction from VMActuator), the static block will see false and skip client creation. After that, even if the chain parameter is enabled through governance, the static block never re-runs, so client stays null and the optimized path is unreachable.
Consider lazy-initializing the client on first use (double-checked locking or an explicit init() method called after ConfigLoader.load()), so activation through governance works without a node restart.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At actuator/src/main/java/org/tron/core/vm/PrecompiledContracts.java, line 435:
<comment>The `SocketBN128Client` is created inside a `static {}` block that runs exactly once at class-load time. Because `VMConfig.ALLOW_OPTIMIZED_BN128` defaults to `false` and is only set later by `ConfigLoader.load()` (called per-transaction from `VMActuator`), the static block will see `false` and skip client creation. After that, even if the chain parameter is enabled through governance, the static block never re-runs, so `client` stays `null` and the optimized path is unreachable.
Consider lazy-initializing the client on first use (double-checked locking or an explicit `init()` method called after `ConfigLoader.load()`), so activation through governance works without a node restart.</comment>
<file context>
@@ -426,6 +428,39 @@ public abstract static class PrecompiledContract {
+ // BN128 client connection pool size
+ private static final int BN128_CLIENT_POOL_SIZE = 8;
+
+ static {
+ if (VMConfig.allowOptimizedBn128()) {
+ try {
</file context>
| dependencies { | ||
| api project(":common") | ||
| api 'net.java.dev.jna:jna:5.12.1' | ||
| api 'com.github.federico2014.besu-native:gnark:1.3.11' |
There was a problem hiding this comment.
P0: Supply chain risk: This dependency pulls native cryptographic binaries from a personal GitHub fork (federico2014/besu-native) via JitPack instead of using the official Hyperledger-published artifact (org.hyperledger.besu). For a consensus-critical BN128 precompile, a compromised or tampered fork could produce incorrect cryptographic results or introduce backdoors. Consider publishing a vetted fork under the project's own organization with reproducible builds, or using the official org.hyperledger.besu:besu-native-gnark artifact from Maven Central.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/build.gradle, line 16:
<comment>**Supply chain risk**: This dependency pulls native cryptographic binaries from a personal GitHub fork (`federico2014/besu-native`) via JitPack instead of using the official Hyperledger-published artifact (`org.hyperledger.besu`). For a consensus-critical BN128 precompile, a compromised or tampered fork could produce incorrect cryptographic results or introduce backdoors. Consider publishing a vetted fork under the project's own organization with reproducible builds, or using the official `org.hyperledger.besu:besu-native-gnark` artifact from Maven Central.</comment>
<file context>
@@ -12,6 +12,8 @@ repositories {
dependencies {
api project(":common")
+ api 'net.java.dev.jna:jna:5.12.1'
+ api 'com.github.federico2014.besu-native:gnark:1.3.11'
}
</file context>
| if (!LibGnarkEIP196.ENABLED) { | ||
| logger.warn("Native BN128 library not available, " | ||
| + "cannot execute optimized path"); | ||
| return null; |
There was a problem hiding this comment.
P1: Returning null here will cause a NullPointerException in the callers (add, mul, pairing), since none of them handle a null return. Return Pair.of(false, EMPTY_BYTE_ARRAY) to match the error convention used elsewhere, or throw a descriptive exception.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/common/crypto/bn128/BN128Executor.java, line 64:
<comment>Returning `null` here will cause a `NullPointerException` in the callers (`add`, `mul`, `pairing`), since none of them handle a null return. Return `Pair.of(false, EMPTY_BYTE_ARRAY)` to match the error convention used elsewhere, or throw a descriptive exception.</comment>
<file context>
@@ -0,0 +1,92 @@
+ if (!LibGnarkEIP196.ENABLED) {
+ logger.warn("Native BN128 library not available, "
+ + "cannot execute optimized path");
+ return null;
+ }
+
</file context>
| if (selector != null) { | ||
| selector.close(); | ||
| } | ||
| workers.shutdown(); |
There was a problem hiding this comment.
P1: workers.shutdown() will not stop the worker threads because they are blocked on queue.take() inside a while(true) loop. Use shutdownNow() to interrupt them, and handle InterruptedException in the worker loop to break out.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java, line 115:
<comment>`workers.shutdown()` will not stop the worker threads because they are blocked on `queue.take()` inside a `while(true)` loop. Use `shutdownNow()` to interrupt them, and handle `InterruptedException` in the worker loop to break out.</comment>
<file context>
@@ -0,0 +1,196 @@
+ if (selector != null) {
+ selector.close();
+ }
+ workers.shutdown();
+ }
+
</file context>
| resp.putInt(result.length); | ||
| resp.put(result); | ||
| resp.flip(); | ||
| ch.write(resp); |
There was a problem hiding this comment.
P1: ch.write(resp) on a non-blocking channel may not write all bytes. The return value is ignored, so clients can receive truncated responses. Either loop until resp.hasRemaining() is false, or temporarily set the channel to blocking for the write.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java, line 175:
<comment>`ch.write(resp)` on a non-blocking channel may not write all bytes. The return value is ignored, so clients can receive truncated responses. Either loop until `resp.hasRemaining()` is false, or temporarily set the channel to blocking for the write.</comment>
<file context>
@@ -0,0 +1,196 @@
+ resp.putInt(result.length);
+ resp.put(result);
+ resp.flip();
+ ch.write(resp);
+ }
+
</file context>
| /** | ||
| * Performs BN128 pairing check. | ||
| * | ||
| * @param input 192 bytes (pairing inputs) |
There was a problem hiding this comment.
P2: Javadoc says "192 bytes" but the actual input is a multiple of 192 bytes (one or more pairing pairs). This could mislead callers into thinking only a single pair is accepted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/common/crypto/bn128/BN128Service.java, line 29:
<comment>Javadoc says "192 bytes" but the actual input is a *multiple* of 192 bytes (one or more pairing pairs). This could mislead callers into thinking only a single pair is accepted.</comment>
<file context>
@@ -0,0 +1,38 @@
+ /**
+ * Performs BN128 pairing check.
+ *
+ * @param input 192 bytes (pairing inputs)
+ * @return Pair of (success, result bytes) - result is {1} if true, {0} if false
+ */
</file context>
Suggested change
| * @param input 192 bytes (pairing inputs) | |
| * @param input multiple of 192 bytes (pairing input pairs: G1 point + G2 point each) |
| /** | ||
| * Performs BN128 point addition. | ||
| * | ||
| * @param input 128 bytes (two compressed G1 points) |
There was a problem hiding this comment.
P2: Javadoc says "two compressed G1 points" but 128 bytes corresponds to two uncompressed G1 points (64 bytes each: 32-byte x + 32-byte y). Compressed points would be ~33 bytes each.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/common/crypto/bn128/BN128Service.java, line 13:
<comment>Javadoc says "two compressed G1 points" but 128 bytes corresponds to two *uncompressed* G1 points (64 bytes each: 32-byte x + 32-byte y). Compressed points would be ~33 bytes each.</comment>
<file context>
@@ -0,0 +1,38 @@
+ /**
+ * Performs BN128 point addition.
+ *
+ * @param input 128 bytes (two compressed G1 points)
+ * @return Pair of (success, result bytes)
+ */
</file context>
Suggested change
| * @param input 128 bytes (two compressed G1 points) | |
| * @param input 128 bytes (two uncompressed G1 points) |
| /** | ||
| * Common interface for BN128 elliptic curve operations via IPC. | ||
| */ | ||
| public interface BN128Service { |
There was a problem hiding this comment.
P2: This interface defines close() with the same signature as AutoCloseable.close() but doesn't extend AutoCloseable. Without it, callers can't use try-with-resources to manage the service lifecycle, risking resource leaks if an exception is thrown before an explicit close() call.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/common/crypto/bn128/BN128Service.java, line 8:
<comment>This interface defines `close()` with the same signature as `AutoCloseable.close()` but doesn't extend `AutoCloseable`. Without it, callers can't use try-with-resources to manage the service lifecycle, risking resource leaks if an exception is thrown before an explicit `close()` call.</comment>
<file context>
@@ -0,0 +1,38 @@
+/**
+ * Common interface for BN128 elliptic curve operations via IPC.
+ */
+public interface BN128Service {
+
+ /**
</file context>
Suggested change
| public interface BN128Service { | |
| public interface BN128Service extends AutoCloseable { |
Comment on lines
+995
to
+996
| PARAMETER.bn128SocketTimeout = config.hasPath(ConfigKey.BN128_SOCKET_TIMEOUT) | ||
| ? config.getInt(ConfigKey.BN128_SOCKET_TIMEOUT) : 30; |
There was a problem hiding this comment.
P2: Missing validation for bn128SocketTimeout — a zero or negative value could cause socket operations to hang indefinitely or fail. Add a guard similar to dynamicConfigCheckInterval.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/config/args/Args.java, line 995:
<comment>Missing validation for `bn128SocketTimeout` — a zero or negative value could cause socket operations to hang indefinitely or fail. Add a guard similar to `dynamicConfigCheckInterval`.</comment>
<file context>
@@ -985,6 +984,17 @@ public static void applyConfigParams(
+ // BN128 Socket configuration
+ PARAMETER.bn128ServerPort = config.hasPath(ConfigKey.BN128_SERVER_PORT)
+ ? config.getInt(ConfigKey.BN128_SERVER_PORT) : 9001;
+ PARAMETER.bn128SocketTimeout = config.hasPath(ConfigKey.BN128_SOCKET_TIMEOUT)
+ ? config.getInt(ConfigKey.BN128_SOCKET_TIMEOUT) : 30;
+
</file context>
Suggested change
| PARAMETER.bn128SocketTimeout = config.hasPath(ConfigKey.BN128_SOCKET_TIMEOUT) | |
| ? config.getInt(ConfigKey.BN128_SOCKET_TIMEOUT) : 30; | |
| PARAMETER.bn128SocketTimeout = config.hasPath(ConfigKey.BN128_SOCKET_TIMEOUT) | |
| ? config.getInt(ConfigKey.BN128_SOCKET_TIMEOUT) : 30; | |
| if (PARAMETER.bn128SocketTimeout <= 0) { | |
| PARAMETER.bn128SocketTimeout = 30; | |
| } |
| import org.tron.core.zksnark.SendCoinShieldTest; | ||
|
|
||
| @Slf4j | ||
| public class BN128PrecompiledContratsTest extends BaseTest { |
There was a problem hiding this comment.
P3: Class name typo: BN128PrecompiledContratsTest → BN128PrecompiledContractsTest (missing 'c' in "Contracts").
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java, line 37:
<comment>Class name typo: `BN128PrecompiledContratsTest` → `BN128PrecompiledContractsTest` (missing 'c' in "Contracts").</comment>
<file context>
@@ -0,0 +1,436 @@
+import org.tron.core.zksnark.SendCoinShieldTest;
+
+@Slf4j
+public class BN128PrecompiledContratsTest extends BaseTest {
+
+ // bn128
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (10)
gradle/verification-metadata.xml (1)
261-276: Add POM checksums for besu-native components to match the standard pattern.The besu-native-common:1.3.11 and gnark:1.3.11 components are missing .pom artifact checksums. JNA and 365 other components include both JAR and POM checksums, so adding them here improves consistency and verification robustness.
Template of missing entries to add
<component group="com.github.federico2014.besu-native" name="besu-native-common" version="1.3.11"> <artifact name="besu-native-common-1.3.11.jar"> <sha256 value="a9f3ed6b1102c4638bdf0c727e96da670ccc5f093b6b002012a0d205d1043c89" origin="Generated by Gradle"/> </artifact> <artifact name="besu-native-common-1.3.11.module"> <sha256 value="ae348bf3c5096afc6f72fb58815f70275456c4d4cfd01ec6b461c684be1eddaa" origin="Generated by Gradle"/> </artifact> + <artifact name="besu-native-common-1.3.11.pom"> + <sha256 value="<!-- fill from Gradle-generated metadata -->" origin="Generated by Gradle"/> + </artifact> </component> <component group="com.github.federico2014.besu-native" name="gnark" version="1.3.11"> <artifact name="gnark-1.3.11.jar"> <sha256 value="44875b21ee3ab679e2dd79e3392ab4ea56a9dd89b25eb01934d05a3c99602f03" origin="Generated by Gradle"/> </artifact> <artifact name="gnark-1.3.11.module"> <sha256 value="9e42c6499e8c2cce2960ce2f61298fbc7402bef74bbc3bddaa69052804e8591e" origin="Generated by Gradle"/> </artifact> + <artifact name="gnark-1.3.11.pom"> + <sha256 value="<!-- fill from Gradle-generated metadata -->" origin="Generated by Gradle"/> + </artifact> </component>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle/verification-metadata.xml` around lines 261 - 276, Add missing .pom artifact checksum entries for the two besu-native components: inside the component with group="com.github.federico2014.besu-native" name="besu-native-common" version="1.3.11" add an artifact entry for besu-native-common-1.3.11.pom with its sha256 value (origin="Generated by Gradle"); likewise inside the component with name="gnark" version="1.3.11" add an artifact entry for gnark-1.3.11.pom with its sha256 value (origin="Generated by Gradle"). Ensure the artifact element mirrors the existing jar/.module entries (artifact name set to the .pom filename, sha256 attribute filled with the actual checksum produced by Gradle, origin="Generated by Gradle") so the file follows the same pattern as other components.crypto/src/main/java/org/tron/common/crypto/bn128/BN128Executor.java (2)
59-65: Returningnullwhen native library is unavailable is inconsistent with the return type contract.The method signature indicates it returns
Pair<Boolean, byte[]>, but returningnullwhen the library is disabled creates an inconsistent API. Callers must handle this special case. Consider returningPair.of(false, EMPTY_BYTE_ARRAY)to maintain a consistent contract, or document the null return behavior.♻️ Proposed fix for consistent return
private static Pair<Boolean, byte[]> executeEIP196Operation( byte operation, byte[] data) { if (!LibGnarkEIP196.ENABLED) { logger.warn("Native BN128 library not available, " + "cannot execute optimized path"); - return null; + return Pair.of(false, EMPTY_BYTE_ARRAY); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypto/src/main/java/org/tron/common/crypto/bn128/BN128Executor.java` around lines 59 - 65, The method executeEIP196Operation currently returns null when LibGnarkEIP196.ENABLED is false, violating its Pair<Boolean, byte[]> contract; change the branch so it returns a consistent Pair (e.g., Pair.of(false, EMPTY_BYTE_ARRAY) or other agreed sentinel byte[]), leave the logger.warn call intact, and ensure callers no longer need to handle a null special case; update any references to EMPTY_BYTE_ARRAY or define one if missing.
86-90: Consider usingArrays.copyOfRangeinstead of customsubArraymethod.This duplicates existing functionality from
java.util.Arrays.copyOfRange(input, start, end)which is already imported and used elsewhere in this file (lines 25, 37).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypto/src/main/java/org/tron/common/crypto/bn128/BN128Executor.java` around lines 86 - 90, The custom subArray(byte[] input, int start, int end) duplicates java.util.Arrays.copyOfRange; replace usages and the method implementation to call Arrays.copyOfRange(input, start, end) (or remove the method and update callers to use Arrays.copyOfRange) so the class uses the standard, tested utility instead of the custom System.arraycopy-based implementation; refer to the BN128Executor.subArray method and any callers in this class when making the change.crypto/src/main/java/org/tron/common/crypto/bn128/BN128Service.java (1)
26-32: Javadoc forbn128Pairinginput size is misleading.The comment states "192 bytes" but pairing operations accept multiples of 192 bytes (0, 192, 384, etc.). This should be clarified.
📝 Proposed fix
/** * Performs BN128 pairing check. * - * `@param` input 192 bytes (pairing inputs) + * `@param` input multiple of 192 bytes (n pairs of G1 and G2 points, 0 to n*192 bytes) * `@return` Pair of (success, result bytes) - result is {1} if true, {0} if false */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypto/src/main/java/org/tron/common/crypto/bn128/BN128Service.java` around lines 26 - 32, The Javadoc for bn128Pairing incorrectly states a fixed "192 bytes"; update the comment for BN128Service.bn128Pairing to state that the input length must be a multiple of 192 bytes (allowed values: 0, 192, 384, ...) and that each 192-byte block represents one pairing input, and if applicable mention that zero-length input is allowed to represent an empty pairing set; also ensure any mention of "192 bytes" in the method contract or docs is replaced with "multiple of 192 bytes" so callers understand the variable-length requirement.crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java (1)
60-60: Consider using a logger instead ofSystem.out.println.For consistency with the rest of the codebase (which uses SLF4J logging), replace the console output with proper logging.
♻️ Proposed fix
Add
@Slf4jannotation and replace:+import lombok.extern.slf4j.Slf4j; + +@Slf4j public class SocketBn128Server { // ... - System.out.println("BN128 Socket Worker started on port " + port); + log.info("BN128 Socket Worker started on port {}", port);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java` at line 60, Replace the System.out.println in SocketBn128Server with SLF4J logging to match the codebase: add a logger (e.g., annotate the class SocketBn128Server with `@Slf4j` or declare a private static final Logger log = LoggerFactory.getLogger(SocketBn128Server.class)) and change the System.out.println("BN128 Socket Worker started on port " + port) call to log.info(...) so startup messages use the project's logging framework.common/src/main/java/org/tron/common/parameter/CommonParameter.java (1)
635-646: Validate the new BN128 IPC settings during config parsing.
bn128ServerPortandbn128SocketTimeoutare externally supplied. Reject ports outside1..65535and non-positive timeouts so bad config fails fast instead of surfacing later as bind/connect errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/org/tron/common/parameter/CommonParameter.java` around lines 635 - 646, The new BN128 IPC fields bn128ServerPort and bn128SocketTimeout need range validation during configuration parsing in CommonParameter; add checks (in the method that loads/parses CommonParameter) to reject/throw/abort if bn128ServerPort is not within 1..65535 or if bn128SocketTimeout is <= 0, and surface a clear error message mentioning bn128ServerPort/bn128SocketTimeout so startup fails fast instead of later bind/connect errors.chainbase/src/main/java/org/tron/core/store/DynamicPropertiesStore.java (1)
2991-2997: Keep node-local BN128 socket settings out ofDynamicPropertiesStore.
getBN128Port()andgetBN128SocketTimeout()just proxyCommonParameter, so this class now mixes consensus state with local process config. Wiring those two values directly fromCommonParameterinConfigLoaderwould keep the boundary clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chainbase/src/main/java/org/tron/core/store/DynamicPropertiesStore.java` around lines 2991 - 2997, DynamicPropertiesStore currently exposes node-local BN128 settings via getBN128Port() and getBN128SocketTimeout(), leaking process config into consensus state; remove these two methods from DynamicPropertiesStore and instead read CommonParameter.getInstance().getBn128ServerPort() and getBn128SocketTimeout() in ConfigLoader (or the configuration wiring layer) and propagate those values to the components that need them (update places that call DynamicPropertiesStore.getBN128Port()/getBN128SocketTimeout() to obtain the values from the ConfigLoader-provided config object instead). Ensure references to the removed methods are updated to use the new config property so DynamicPropertiesStore only contains consensus/state data.framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java (3)
270-276: Consider reducing iteration count in random test for faster CI.This loop runs 21,116 iterations (
5to110 * 192), which may slow down the test suite significantly. If full coverage at every byte length isn't critical, consider sampling a subset of sizes or reducing the upper bound.♻️ Alternative: Sample sizes instead of exhaustive iteration
- for (int i = 5; i <= 110 * 192; i++) { + // Test representative sizes: small, medium, large, and boundary cases + int[] testSizes = {5, 10, 50, 100, 191, 192, 193, 383, 384, 385, 1000, 5000, 110 * 192}; + for (int size : testSizes) { + randomInput = new byte[size]; randomInput = new byte[i]; random.nextBytes(randomInput); result = bn128Pairing.execute(randomInput); Assert.assertFalse(result.getLeft()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java` around lines 270 - 276, The exhaustive loop in BN128PrecompiledContratsTest that calls bn128Pairing.execute over every input length from 5 to 110*192 is heavy; replace it with a sampled set of sizes to keep coverage but speed up CI — e.g., iterate a limited range (reduce upper bound to something like 192*10) or build a small randomized/sample list of lengths (e.g., pick N=200 lengths using random.nextInt(max-5)+5) and run bn128Pairing.execute for each sampled length, ensuring the test still asserts Assert.assertFalse(result.getLeft()) for each sample.
361-380: Consider extracting the pair size constant.The value
192(size of one G1+G2 point pair) and384(two pairs) appear multiple times throughout the file. Extracting these as named constants would improve readability.♻️ Add constants at class level
// At class level, near the address constants: private static final int PAIR_SIZE = 192; // G1 (64 bytes) + G2 (128 bytes) private static final int TWO_PAIRS_SIZE = PAIR_SIZE * 2;Then use
PAIR_SIZEandTWO_PAIRS_SIZEinstead of192and384throughout the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java` around lines 361 - 380, Extract the repeated magic numbers for point pair sizes into class-level constants (e.g., in BN128PrecompiledContratsTest declare private static final int PAIR_SIZE = 192 and private static final int TWO_PAIRS_SIZE = PAIR_SIZE * 2) and replace all occurrences of 192 and 384 in this test method (used when allocating/offsetting twoValidPairs, hundredPairs, hundredTwoPairs and in System.arraycopy calls) with PAIR_SIZE and TWO_PAIRS_SIZE respectively; ensure any arithmetic that uses 100 or 102 remains the same but uses PAIR_SIZE for clarity, so validPointsByte(), twoValidPairs, hundredPairs, hundredTwoPairs and the bn128Pairing.execute call behavior is unchanged.
329-337: Consider simplifying file reading logic.The current approach using
readLines+ streamreduceis verbose. Also,reduce(...).get()will throwNoSuchElementExceptionif the file is empty.♻️ Simplified version using asCharSource
private JSONArray readJsonFile(String fileName) throws Exception { String file1 = SendCoinShieldTest.class.getClassLoader() .getResource("json" + File.separator + fileName).getFile(); - List<String> readLines = Files.readLines(new File(file1), - Charsets.UTF_8); - - return JSONArray - .parseArray(readLines.stream().reduce((s, s2) -> s + s2).get()); + String content = Files.asCharSource(new File(file1), Charsets.UTF_8).read(); + return JSONArray.parseArray(content); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java` around lines 329 - 337, The readJsonFile method currently uses Files.readLines + stream.reduce(...).get(), which is verbose and risks NoSuchElementException on empty files; replace that logic in readJsonFile to read the entire file as a single String using Guava's Files.asCharSource(new File(...), Charsets.UTF_8).read() (or equivalent) and pass that String to JSONArray.parseArray, keeping the same resource lookup (note the current use of SendCoinShieldTest.class.getClassLoader()). This simplifies the code and avoids the empty-file exception while still returning JSONArray from JSONArray.parseArray.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@chainbase/src/main/java/org/tron/core/store/DynamicPropertiesStore.java`:
- Around line 2974-2985: DynamicPropertiesStore currently reads
ALLOW_OPTIMIZED_BN128 from CommonParameter when the key is absent, but nothing
writes a bootstrap value on first run; update the DynamicPropertiesStore
constructor (or its init path) to check for the presence of
ALLOW_OPTIMIZED_BN128 (use getUnchecked/has/get) and, if missing, call
saveAllowOptimizedBn128(CommonParameter.getInstance().getAllowOptimizedBN128())
to persist the bootstrap value without overwriting an existing stored value;
reference methods: DynamicPropertiesStore constructor, getAllowOptimizedBN128(),
and saveAllowOptimizedBn128().
In `@crypto/build.gradle`:
- Around line 15-16: The build declares native libs with the wrong
configuration: change the JNA and gnark dependencies from api to implementation
in the crypto module's Gradle file because these native libraries are internal
to BN128Executor and not part of the module's public API; update the dependency
declarations for 'net.java.dev.jna:jna:5.12.1' and
'com.github.federico2014.besu-native:gnark:1.3.11' to use implementation so they
are not exposed transitively.
In `@crypto/src/main/java/org/tron/common/crypto/bn128/SocketBN128Client.java`:
- Around line 63-67: The finally block in SocketBN128Client currently always
returns the channel via pool.put(ch), risking putting a corrupted socket back
into the pool; change the cleanup to only return healthy channels and
discard/close corrupted ones: in the finally block that references pool.put(ch)
and the local variable ch, detect channel health (e.g.,
ch.isOpen()/ch.isActive() or lack of exception) and call pool.put(ch) only for
healthy channels, otherwise call pool.invalidate(ch) or close ch directly (and
do not put it back); also ensure any caught exceptions (including
SocketTimeoutException and other IOExceptions) mark the channel as invalid so it
is discarded instead of returned to the pool.
- Around line 38-68: The call method uses SocketChannel with setSoTimeout (which
doesn't apply) and assumes full reads; change the SocketChannel to blocking mode
and implement read loops that repeatedly call ch.read(ByteBuffer) until the
header (5 bytes) and then the full body (len bytes) are fully read, handling EOF
(-1) as an error; enforce a real timeout by using
SocketChannel.configureBlocking(true) and tracking elapsed time against
socketTimeout (or use Selector with timeout) and throw a timeout
RuntimeException including socketTimeout when exceeded; ensure pool.put(ch)
still happens in the finally block and reference symbols: call, SocketChannel
ch, pool, socketTimeout, header/body ByteBuffer, and the exception handling for
java.net.SocketTimeoutException.
In `@crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java`:
- Around line 33-34: The MAX_PAYLOAD constant is calculated with the wrong pair
size (196) causing BUFFER_SIZE to be incorrect; change MAX_PAYLOAD to use the
actual BN128 pair size (BN128Executor.PAIR_SIZE or BN128Pairing.PAIR_SIZE, which
is 192) so that MAX_PAYLOAD = 100 * BN128Executor.PAIR_SIZE (or
BN128Pairing.PAIR_SIZE) and leave BUFFER_SIZE = 5 + MAX_PAYLOAD to keep buffer
alignment consistent with the BN128 pair definitions.
- Around line 118-153: Worker threads use while(true) and block on queue.take(),
so workers.shutdown() in stop() won't interrupt them; modify startWorkers() and
stop() so threads can exit on interruption: change the loop to while
(!Thread.currentThread().isInterrupted()), handle InterruptedException around
queue.take() by breaking the loop (or restoring the interrupt and returning),
and keep other exception handling intact; additionally ensure stop() either
calls workers.shutdownNow() or interrupts worker threads so the
InterruptedException is delivered to the queue.take() calls. Ensure references:
startWorkers(), queue.take(), while(true), stop(), and
workers.shutdown()/shutdownNow().
- Around line 169-176: The sendResponse method currently assumes a single
SocketChannel.write will send the entire ByteBuffer; change sendResponse (and
the ByteBuffer resp usage) to loop until resp.hasRemaining() is false, calling
ch.write(resp) repeatedly and handling the return value: if write returns -1
throw an IOException to indicate the channel closed, and otherwise continue
until all bytes are written. Ensure you preserve the existing ByteBuffer.flip()
before the loop and do not allocate a new buffer inside the loop.
In `@framework/src/main/java/org/tron/core/config/args/Args.java`:
- Around line 993-996: Validate BN128 config values immediately after reading
them: when populating PARAMETER.bn128ServerPort from ConfigKey.BN128_SERVER_PORT
reject values <=0 or >65535 and when populating PARAMETER.bn128SocketTimeout
from ConfigKey.BN128_SOCKET_TIMEOUT reject values <=0; if invalid, throw an
IllegalArgumentException (or fail fast) with a clear message including the
invalid value and config key so startup fails early. Locate the assignments to
PARAMETER.bn128ServerPort and PARAMETER.bn128SocketTimeout and add range checks
on the retrieved ints (from config.getInt) before assigning.
In `@framework/src/main/java/org/tron/program/FullNode.java`:
- Around line 45-60: The shutdown hook registration and the self-join are wrong:
move the Runtime.getRuntime().addShutdownHook(...) call so it runs before
server.start() (so the JVM has a hook even if start() blocks) and remove the
Thread.currentThread().join() call (which causes a self-join deadlock). Ensure
the shutdown hook still calls server.stop() and logs errors (referencing
server.start(), Runtime.getRuntime().addShutdownHook(...), server.stop(), and
Thread.currentThread().join() to locate the changes).
In
`@framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java`:
- Line 37: Rename the test class and file to correct the typo: change the class
declaration BN128PrecompiledContratsTest to BN128PrecompiledContractsTest and
update the file name from BN128PrecompiledContratsTest.java to
BN128PrecompiledContractsTest.java; also update any imports or references (test
suites, build files, or other classes) that reference
BN128PrecompiledContratsTest to the new BN128PrecompiledContractsTest identifier
so compilation and test discovery continue to work.
- Around line 205-212: The test contains a misspelled variable name `intput`;
rename all occurrences of `intput` to `input` in BN128PrecompiledContratsTest to
match standard spelling and avoid confusion — update the
declarations/assignments where `input` is set from calls like validPointsByte()
and inValidPointsOutsideSubgroupG2(), and ensure subsequent uses (e.g.,
bn128Pairing.execute(input)) and assertions reference `input` consistently.
In `@framework/src/test/resources/json/bn256ScalarMul.json`:
- Around line 135-138: The JSON test entry is missing required fields causing
inconsistent structure; update the object (the entry with "Input": "" and the
long "Expected" zero string) to include the same keys as other entries: add a
"Name" string (e.g., descriptive like "bn256ScalarMul_empty_input"), a numeric
"Gas" value consistent with similar tests, and a boolean "NoBenchmark"
(true/false as used elsewhere) so the JSON schema matches other entries and
parsing will succeed.
---
Nitpick comments:
In `@chainbase/src/main/java/org/tron/core/store/DynamicPropertiesStore.java`:
- Around line 2991-2997: DynamicPropertiesStore currently exposes node-local
BN128 settings via getBN128Port() and getBN128SocketTimeout(), leaking process
config into consensus state; remove these two methods from
DynamicPropertiesStore and instead read
CommonParameter.getInstance().getBn128ServerPort() and getBn128SocketTimeout()
in ConfigLoader (or the configuration wiring layer) and propagate those values
to the components that need them (update places that call
DynamicPropertiesStore.getBN128Port()/getBN128SocketTimeout() to obtain the
values from the ConfigLoader-provided config object instead). Ensure references
to the removed methods are updated to use the new config property so
DynamicPropertiesStore only contains consensus/state data.
In `@common/src/main/java/org/tron/common/parameter/CommonParameter.java`:
- Around line 635-646: The new BN128 IPC fields bn128ServerPort and
bn128SocketTimeout need range validation during configuration parsing in
CommonParameter; add checks (in the method that loads/parses CommonParameter) to
reject/throw/abort if bn128ServerPort is not within 1..65535 or if
bn128SocketTimeout is <= 0, and surface a clear error message mentioning
bn128ServerPort/bn128SocketTimeout so startup fails fast instead of later
bind/connect errors.
In `@crypto/src/main/java/org/tron/common/crypto/bn128/BN128Executor.java`:
- Around line 59-65: The method executeEIP196Operation currently returns null
when LibGnarkEIP196.ENABLED is false, violating its Pair<Boolean, byte[]>
contract; change the branch so it returns a consistent Pair (e.g.,
Pair.of(false, EMPTY_BYTE_ARRAY) or other agreed sentinel byte[]), leave the
logger.warn call intact, and ensure callers no longer need to handle a null
special case; update any references to EMPTY_BYTE_ARRAY or define one if
missing.
- Around line 86-90: The custom subArray(byte[] input, int start, int end)
duplicates java.util.Arrays.copyOfRange; replace usages and the method
implementation to call Arrays.copyOfRange(input, start, end) (or remove the
method and update callers to use Arrays.copyOfRange) so the class uses the
standard, tested utility instead of the custom System.arraycopy-based
implementation; refer to the BN128Executor.subArray method and any callers in
this class when making the change.
In `@crypto/src/main/java/org/tron/common/crypto/bn128/BN128Service.java`:
- Around line 26-32: The Javadoc for bn128Pairing incorrectly states a fixed
"192 bytes"; update the comment for BN128Service.bn128Pairing to state that the
input length must be a multiple of 192 bytes (allowed values: 0, 192, 384, ...)
and that each 192-byte block represents one pairing input, and if applicable
mention that zero-length input is allowed to represent an empty pairing set;
also ensure any mention of "192 bytes" in the method contract or docs is
replaced with "multiple of 192 bytes" so callers understand the variable-length
requirement.
In `@crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java`:
- Line 60: Replace the System.out.println in SocketBn128Server with SLF4J
logging to match the codebase: add a logger (e.g., annotate the class
SocketBn128Server with `@Slf4j` or declare a private static final Logger log =
LoggerFactory.getLogger(SocketBn128Server.class)) and change the
System.out.println("BN128 Socket Worker started on port " + port) call to
log.info(...) so startup messages use the project's logging framework.
In
`@framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java`:
- Around line 270-276: The exhaustive loop in BN128PrecompiledContratsTest that
calls bn128Pairing.execute over every input length from 5 to 110*192 is heavy;
replace it with a sampled set of sizes to keep coverage but speed up CI — e.g.,
iterate a limited range (reduce upper bound to something like 192*10) or build a
small randomized/sample list of lengths (e.g., pick N=200 lengths using
random.nextInt(max-5)+5) and run bn128Pairing.execute for each sampled length,
ensuring the test still asserts Assert.assertFalse(result.getLeft()) for each
sample.
- Around line 361-380: Extract the repeated magic numbers for point pair sizes
into class-level constants (e.g., in BN128PrecompiledContratsTest declare
private static final int PAIR_SIZE = 192 and private static final int
TWO_PAIRS_SIZE = PAIR_SIZE * 2) and replace all occurrences of 192 and 384 in
this test method (used when allocating/offsetting twoValidPairs, hundredPairs,
hundredTwoPairs and in System.arraycopy calls) with PAIR_SIZE and TWO_PAIRS_SIZE
respectively; ensure any arithmetic that uses 100 or 102 remains the same but
uses PAIR_SIZE for clarity, so validPointsByte(), twoValidPairs, hundredPairs,
hundredTwoPairs and the bn128Pairing.execute call behavior is unchanged.
- Around line 329-337: The readJsonFile method currently uses Files.readLines +
stream.reduce(...).get(), which is verbose and risks NoSuchElementException on
empty files; replace that logic in readJsonFile to read the entire file as a
single String using Guava's Files.asCharSource(new File(...),
Charsets.UTF_8).read() (or equivalent) and pass that String to
JSONArray.parseArray, keeping the same resource lookup (note the current use of
SendCoinShieldTest.class.getClassLoader()). This simplifies the code and avoids
the empty-file exception while still returning JSONArray from
JSONArray.parseArray.
In `@gradle/verification-metadata.xml`:
- Around line 261-276: Add missing .pom artifact checksum entries for the two
besu-native components: inside the component with
group="com.github.federico2014.besu-native" name="besu-native-common"
version="1.3.11" add an artifact entry for besu-native-common-1.3.11.pom with
its sha256 value (origin="Generated by Gradle"); likewise inside the component
with name="gnark" version="1.3.11" add an artifact entry for gnark-1.3.11.pom
with its sha256 value (origin="Generated by Gradle"). Ensure the artifact
element mirrors the existing jar/.module entries (artifact name set to the .pom
filename, sha256 attribute filled with the actual checksum produced by Gradle,
origin="Generated by Gradle") so the file follows the same pattern as other
components.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1dce7f73-e9a5-4adb-9921-337b40193831
📒 Files selected for processing (24)
actuator/src/main/java/org/tron/core/utils/ProposalUtil.javaactuator/src/main/java/org/tron/core/vm/PrecompiledContracts.javaactuator/src/main/java/org/tron/core/vm/config/ConfigLoader.javachainbase/src/main/java/org/tron/core/store/DynamicPropertiesStore.javacommon/src/main/java/org/tron/common/parameter/CommonParameter.javacommon/src/main/java/org/tron/core/config/Parameter.javacommon/src/main/java/org/tron/core/vm/config/VMConfig.javacrypto/build.gradlecrypto/src/main/java/org/tron/common/crypto/bn128/BN128Executor.javacrypto/src/main/java/org/tron/common/crypto/bn128/BN128Service.javacrypto/src/main/java/org/tron/common/crypto/bn128/SocketBN128Client.javacrypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.javaframework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/config/args/Args.javaframework/src/main/java/org/tron/core/config/args/CLIParameter.javaframework/src/main/java/org/tron/core/config/args/ConfigKey.javaframework/src/main/java/org/tron/core/consensus/ProposalService.javaframework/src/main/java/org/tron/program/FullNode.javaframework/src/main/resources/config.confframework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.javaframework/src/test/resources/json/bn256Add.jsonframework/src/test/resources/json/bn256Pairing.jsonframework/src/test/resources/json/bn256ScalarMul.jsongradle/verification-metadata.xml
Comment on lines
+2974
to
+2985
| public void saveAllowOptimizedBn128(long allowOptimizedBn128) { | ||
| this.put(ALLOW_OPTIMIZED_BN128, | ||
| new BytesCapsule(ByteArray.fromLong(allowOptimizedBn128))); | ||
| } | ||
|
|
||
| public long getAllowOptimizedBN128() { | ||
| return Optional.ofNullable(getUnchecked(ALLOW_OPTIMIZED_BN128)) | ||
| .map(BytesCapsule::getData) | ||
| .map(ByteArray::toLong) | ||
| .orElse(CommonParameter.getInstance() | ||
| .getAllowOptimizedBN128()); | ||
| } |
There was a problem hiding this comment.
Bootstrap ALLOW_OPTIMIZED_BN128 into the store instead of relying on fallback reads.
getAllowOptimizedBN128() falls back to CommonParameter when the key is missing, and there is no constructor path that writes this key on first boot. Until a proposal stores it, the effective value can change across restarts just by editing local config. Persist the bootstrap value once during store initialization.
Suggested constructor fix
try {
this.getAllowDynamicEnergy();
} catch (IllegalArgumentException e) {
this.saveAllowDynamicEnergy(CommonParameter.getInstance().getAllowDynamicEnergy());
}
+
+ if (getUnchecked(ALLOW_OPTIMIZED_BN128) == null) {
+ this.saveAllowOptimizedBn128(CommonParameter.getInstance().getAllowOptimizedBN128());
+ }
try {
this.getDynamicEnergyThreshold();
} catch (IllegalArgumentException e) {
this.saveDynamicEnergyThreshold(CommonParameter.getInstance().getDynamicEnergyThreshold());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chainbase/src/main/java/org/tron/core/store/DynamicPropertiesStore.java`
around lines 2974 - 2985, DynamicPropertiesStore currently reads
ALLOW_OPTIMIZED_BN128 from CommonParameter when the key is absent, but nothing
writes a bootstrap value on first run; update the DynamicPropertiesStore
constructor (or its init path) to check for the presence of
ALLOW_OPTIMIZED_BN128 (use getUnchecked/has/get) and, if missing, call
saveAllowOptimizedBn128(CommonParameter.getInstance().getAllowOptimizedBN128())
to persist the bootstrap value without overwriting an existing stored value;
reference methods: DynamicPropertiesStore constructor, getAllowOptimizedBN128(),
and saveAllowOptimizedBn128().
Comment on lines
+15
to
+16
| api 'net.java.dev.jna:jna:5.12.1' | ||
| api 'com.github.federico2014.besu-native:gnark:1.3.11' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that java-library is applied somewhere in Gradle config
rg -n "id ['\"]java-library['\"]|apply plugin:\\s*['\"]java-library['\"]" --glob "**/*.gradle*"
# Quick scan for potential public API leakage of JNA/gnark types
rg -n --type=java "^\\s*public\\s+.*(com\\.sun\\.jna|gnark|besu-native)"Repository: Federico2014/java-tron
Length of output: 113
🏁 Script executed:
# First, let's read the crypto/build.gradle file to see the context
cat -n crypto/build.gradle | head -30Repository: Federico2014/java-tron
Length of output: 842
🏁 Script executed:
# Check if crypto/build.gradle applies java-library plugin
rg -n "apply plugin:|id '" crypto/build.gradle | head -20Repository: Federico2014/java-tron
Length of output: 81
🏁 Script executed:
# Search for public usage of JNA and gnark types in the crypto module
rg -n "public.*jna|public.*gnark|public.*JNA|public.*Gnark" --type=java crypto/Repository: Federico2014/java-tron
Length of output: 48
🏁 Script executed:
# Check if the crypto module exports JNA/gnark types in any public interfaces
rg -n "import.*com\.sun\.jna|import.*gnark|import.*besu-native" --type=java crypto/ | head -20Repository: Federico2014/java-tron
Length of output: 306
🏁 Script executed:
# Check if BN128Executor is a public class and what it exposes
cat -n crypto/src/main/java/org/tron/common/crypto/bn128/BN128Executor.java | head -50Repository: Federico2014/java-tron
Length of output: 1922
🏁 Script executed:
# Check all public methods/fields in crypto module for JNA/gnark type exposure
rg -n "public\s+(class|interface|record|enum|.*\s+(jna|gnark|JNA|Gnark|IntByReference|LibGnark))" --type=java crypto/src/main/java/ -A 3 | head -40Repository: Federico2014/java-tron
Length of output: 3198
Switch to implementation for native dependencies.
The crypto module uses the plain java plugin (line 2), which does not provide the api configuration. Lines 15-16 use api for JNA and gnark dependencies, but since these types are internal implementation details (used only within BN128Executor for native calls, not exposed in public method signatures), they should be declared as implementation instead.
Suggested change
- api 'net.java.dev.jna:jna:5.12.1'
- api 'com.github.federico2014.besu-native:gnark:1.3.11'
+ implementation 'net.java.dev.jna:jna:5.12.1'
+ implementation 'com.github.federico2014.besu-native:gnark:1.3.11'📝 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
| api 'net.java.dev.jna:jna:5.12.1' | |
| api 'com.github.federico2014.besu-native:gnark:1.3.11' | |
| implementation 'net.java.dev.jna:jna:5.12.1' | |
| implementation 'com.github.federico2014.besu-native:gnark:1.3.11' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crypto/build.gradle` around lines 15 - 16, The build declares native libs
with the wrong configuration: change the JNA and gnark dependencies from api to
implementation in the crypto module's Gradle file because these native libraries
are internal to BN128Executor and not part of the module's public API; update
the dependency declarations for 'net.java.dev.jna:jna:5.12.1' and
'com.github.federico2014.besu-native:gnark:1.3.11' to use implementation so they
are not exposed transitively.
Comment on lines
+38
to
+68
| private Pair<Boolean, byte[]> call(byte opcode, byte[] input) throws Exception { | ||
| SocketChannel ch = pool.take(); | ||
| try { | ||
| // Set timeout for socket operations | ||
| ch.socket().setSoTimeout(socketTimeout); | ||
|
|
||
| ByteBuffer req = ByteBuffer.allocate(5 + input.length); | ||
| req.put(opcode); | ||
| req.putInt(input.length); | ||
| req.put(input); | ||
| req.flip(); | ||
| ch.write(req); | ||
|
|
||
| ByteBuffer header = ByteBuffer.allocate(5); | ||
| ch.read(header); | ||
| header.flip(); | ||
| boolean success = header.get() == 0; // status byte: 0=success | ||
| int len = header.getInt(); | ||
|
|
||
| ByteBuffer body = ByteBuffer.allocate(len); | ||
| ch.read(body); | ||
| body.flip(); | ||
| byte[] result = new byte[len]; | ||
| body.get(result); | ||
| return Pair.of(success, result); | ||
| } catch (java.net.SocketTimeoutException e) { | ||
| throw new RuntimeException("BN128 operation timed out after " + socketTimeout + "ms", e); | ||
| } finally { | ||
| pool.put(ch); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Socket timeout is ineffective with SocketChannel, and partial reads are not handled.
Two significant issues:
-
Ineffective timeout:
setSoTimeout()only works with blockingSocket.getInputStream()operations, not with NIOSocketChannel.read(). The timeout will never trigger, and theSocketTimeoutExceptioncatch block at line 63 is unreachable. -
Partial reads not handled:
SocketChannel.read()may return fewer bytes than requested. Lines 52 and 58 assume complete reads which can cause protocol desynchronization.
🐛 Proposed fix using blocking mode with proper read loops
private Pair<Boolean, byte[]> call(byte opcode, byte[] input) throws Exception {
SocketChannel ch = pool.take();
try {
- // Set timeout for socket operations
- ch.socket().setSoTimeout(socketTimeout);
-
ByteBuffer req = ByteBuffer.allocate(5 + input.length);
req.put(opcode);
req.putInt(input.length);
req.put(input);
req.flip();
- ch.write(req);
+ while (req.hasRemaining()) {
+ ch.write(req);
+ }
ByteBuffer header = ByteBuffer.allocate(5);
- ch.read(header);
+ long deadline = System.currentTimeMillis() + socketTimeout;
+ while (header.hasRemaining()) {
+ if (System.currentTimeMillis() > deadline) {
+ throw new RuntimeException("BN128 operation timed out after " + socketTimeout + "ms");
+ }
+ int read = ch.read(header);
+ if (read == -1) {
+ throw new RuntimeException("Connection closed by server");
+ }
+ }
header.flip();
boolean success = header.get() == 0; // status byte: 0=success
int len = header.getInt();
ByteBuffer body = ByteBuffer.allocate(len);
- ch.read(body);
+ while (body.hasRemaining()) {
+ if (System.currentTimeMillis() > deadline) {
+ throw new RuntimeException("BN128 operation timed out after " + socketTimeout + "ms");
+ }
+ int read = ch.read(body);
+ if (read == -1) {
+ throw new RuntimeException("Connection closed by server");
+ }
+ }
body.flip();
byte[] result = new byte[len];
body.get(result);
return Pair.of(success, result);
- } catch (java.net.SocketTimeoutException e) {
- throw new RuntimeException("BN128 operation timed out after " + socketTimeout + "ms", e);
} finally {
pool.put(ch);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crypto/src/main/java/org/tron/common/crypto/bn128/SocketBN128Client.java`
around lines 38 - 68, The call method uses SocketChannel with setSoTimeout
(which doesn't apply) and assumes full reads; change the SocketChannel to
blocking mode and implement read loops that repeatedly call ch.read(ByteBuffer)
until the header (5 bytes) and then the full body (len bytes) are fully read,
handling EOF (-1) as an error; enforce a real timeout by using
SocketChannel.configureBlocking(true) and tracking elapsed time against
socketTimeout (or use Selector with timeout) and throw a timeout
RuntimeException including socketTimeout when exceeded; ensure pool.put(ch)
still happens in the finally block and reference symbols: call, SocketChannel
ch, pool, socketTimeout, header/body ByteBuffer, and the exception handling for
java.net.SocketTimeoutException.
Comment on lines
+63
to
+67
| } catch (java.net.SocketTimeoutException e) { | ||
| throw new RuntimeException("BN128 operation timed out after " + socketTimeout + "ms", e); | ||
| } finally { | ||
| pool.put(ch); | ||
| } |
There was a problem hiding this comment.
Corrupted channel may be returned to pool after exception.
When an exception occurs (e.g., partial read, connection reset), the channel is unconditionally returned to the pool in the finally block. This channel may be in an inconsistent state, causing failures for subsequent operations.
🐛 Proposed fix to discard corrupted channels
private Pair<Boolean, byte[]> call(byte opcode, byte[] input) throws Exception {
SocketChannel ch = pool.take();
+ boolean success = false;
try {
// ... operation code ...
- return Pair.of(success, result);
- } catch (java.net.SocketTimeoutException e) {
- throw new RuntimeException("BN128 operation timed out after " + socketTimeout + "ms", e);
+ Pair<Boolean, byte[]> result = Pair.of(successFlag, resultBytes);
+ success = true;
+ return result;
} finally {
- pool.put(ch);
+ if (success) {
+ pool.put(ch);
+ } else {
+ // Discard corrupted channel and create new one
+ try {
+ ch.close();
+ SocketChannel newCh = SocketChannel.open();
+ newCh.connect(new InetSocketAddress("127.0.0.1", port));
+ pool.put(newCh);
+ } catch (Exception e) {
+ // Pool size reduced, may need reconnection logic
+ }
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crypto/src/main/java/org/tron/common/crypto/bn128/SocketBN128Client.java`
around lines 63 - 67, The finally block in SocketBN128Client currently always
returns the channel via pool.put(ch), risking putting a corrupted socket back
into the pool; change the cleanup to only return healthy channels and
discard/close corrupted ones: in the finally block that references pool.put(ch)
and the local variable ch, detect channel health (e.g.,
ch.isOpen()/ch.isActive() or lack of exception) and call pool.put(ch) only for
healthy channels, otherwise call pool.invalidate(ch) or close ch directly (and
do not put it back); also ensure any caught exceptions (including
SocketTimeoutException and other IOExceptions) mark the channel as invalid so it
is discarded instead of returned to the pool.
Comment on lines
+33
to
+34
| private static final int MAX_PAYLOAD = 100 * 196; | ||
| private static final int BUFFER_SIZE = 5 + MAX_PAYLOAD; |
There was a problem hiding this comment.
MAX_PAYLOAD uses incorrect pair size (196 instead of 192).
The constant uses 100 * 196 but BN128 pairing pairs are 192 bytes each (as defined in BN128Executor.PAIR_SIZE and BN128Pairing.PAIR_SIZE). This inconsistency could cause buffer size misalignment.
🔧 Proposed fix
- private static final int MAX_PAYLOAD = 100 * 196;
+ private static final int PAIR_SIZE = 192;
+ private static final int MAX_PAYLOAD = 100 * PAIR_SIZE;📝 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
| private static final int MAX_PAYLOAD = 100 * 196; | |
| private static final int BUFFER_SIZE = 5 + MAX_PAYLOAD; | |
| private static final int PAIR_SIZE = 192; | |
| private static final int MAX_PAYLOAD = 100 * PAIR_SIZE; | |
| private static final int BUFFER_SIZE = 5 + MAX_PAYLOAD; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java`
around lines 33 - 34, The MAX_PAYLOAD constant is calculated with the wrong pair
size (196) causing BUFFER_SIZE to be incorrect; change MAX_PAYLOAD to use the
actual BN128 pair size (BN128Executor.PAIR_SIZE or BN128Pairing.PAIR_SIZE, which
is 192) so that MAX_PAYLOAD = 100 * BN128Executor.PAIR_SIZE (or
BN128Pairing.PAIR_SIZE) and leave BUFFER_SIZE = 5 + MAX_PAYLOAD to keep buffer
alignment consistent with the BN128 pair definitions.
Comment on lines
+993
to
+996
| PARAMETER.bn128ServerPort = config.hasPath(ConfigKey.BN128_SERVER_PORT) | ||
| ? config.getInt(ConfigKey.BN128_SERVER_PORT) : 9001; | ||
| PARAMETER.bn128SocketTimeout = config.hasPath(ConfigKey.BN128_SOCKET_TIMEOUT) | ||
| ? config.getInt(ConfigKey.BN128_SOCKET_TIMEOUT) : 30; |
There was a problem hiding this comment.
Fail fast on invalid BN128 port/timeout values.
Line 993-Line 996 accepts any integer. Invalid values (e.g., port ≤ 0 / > 65535, timeout ≤ 0) will fail later at runtime with less actionable errors.
🔒 Suggested validation
// BN128 Socket configuration
PARAMETER.bn128ServerPort = config.hasPath(ConfigKey.BN128_SERVER_PORT)
? config.getInt(ConfigKey.BN128_SERVER_PORT) : 9001;
PARAMETER.bn128SocketTimeout = config.hasPath(ConfigKey.BN128_SOCKET_TIMEOUT)
? config.getInt(ConfigKey.BN128_SOCKET_TIMEOUT) : 30;
+
+ if (PARAMETER.bn128ServerPort < 1 || PARAMETER.bn128ServerPort > 65535) {
+ throw new IllegalArgumentException("node.bn128.serverPort must be in [1, 65535]");
+ }
+ if (PARAMETER.bn128SocketTimeout <= 0) {
+ throw new IllegalArgumentException("node.bn128.socketTimeout must be > 0");
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@framework/src/main/java/org/tron/core/config/args/Args.java` around lines 993
- 996, Validate BN128 config values immediately after reading them: when
populating PARAMETER.bn128ServerPort from ConfigKey.BN128_SERVER_PORT reject
values <=0 or >65535 and when populating PARAMETER.bn128SocketTimeout from
ConfigKey.BN128_SOCKET_TIMEOUT reject values <=0; if invalid, throw an
IllegalArgumentException (or fail fast) with a clear message including the
invalid value and config key so startup fails early. Locate the assignments to
PARAMETER.bn128ServerPort and PARAMETER.bn128SocketTimeout and add range checks
on the retrieved ints (from config.getInt) before assigning.
Comment on lines
+45
to
+60
| try { | ||
| server.start(); | ||
| logger.info("BN128 Socket Server started successfully"); | ||
|
|
||
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | ||
| try { | ||
| logger.info("Shutting down BN128 Socket Server..."); | ||
| server.stop(); | ||
| logger.info("BN128 Socket Server stopped successfully"); | ||
| } catch (Exception e) { | ||
| logger.error("Error occurred while stopping BN128 Socket Server", e); | ||
| } | ||
| })); | ||
|
|
||
| Thread.currentThread().join(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Register shutdown hook before start and remove self-join deadlock path.
At Line 46, server.start() is blocking, so the hook at Line 49 is never registered in normal execution. Also, Line 59 (Thread.currentThread().join()) is a self-join deadlock if reached.
🔧 Suggested fix
if (parameter.launchBN128Server) {
logger.info("Starting BN128 Server...");
int bn128Port = Args.getInstance().getBn128ServerPort();
logger.info("BN128 Server config: port={}", bn128Port);
SocketBn128Server server = new SocketBn128Server(bn128Port);
+ Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+ try {
+ logger.info("Shutting down BN128 Socket Server...");
+ server.stop();
+ logger.info("BN128 Socket Server stopped successfully");
+ } catch (Exception e) {
+ logger.error("Error occurred while stopping BN128 Socket Server", e);
+ }
+ }));
try {
server.start();
- logger.info("BN128 Socket Server started successfully");
-
- Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- try {
- logger.info("Shutting down BN128 Socket Server...");
- server.stop();
- logger.info("BN128 Socket Server stopped successfully");
- } catch (Exception e) {
- logger.error("Error occurred while stopping BN128 Socket Server", e);
- }
- }));
-
- Thread.currentThread().join();
} catch (Exception e) {
logger.error("Failed to start BN128 Socket Server", e);
System.exit(1);
}
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@framework/src/main/java/org/tron/program/FullNode.java` around lines 45 - 60,
The shutdown hook registration and the self-join are wrong: move the
Runtime.getRuntime().addShutdownHook(...) call so it runs before server.start()
(so the JVM has a hook even if start() blocks) and remove the
Thread.currentThread().join() call (which causes a self-join deadlock). Ensure
the shutdown hook still calls server.stop() and logs errors (referencing
server.start(), Runtime.getRuntime().addShutdownHook(...), server.stop(), and
Thread.currentThread().join() to locate the changes).
| import org.tron.core.zksnark.SendCoinShieldTest; | ||
|
|
||
| @Slf4j | ||
| public class BN128PrecompiledContratsTest extends BaseTest { |
There was a problem hiding this comment.
Typo in class name: "Contrats" should be "Contracts".
The class is named BN128PrecompiledContratsTest but should be BN128PrecompiledContractsTest. This affects discoverability and consistency with naming conventions.
✏️ Proposed fix
-public class BN128PrecompiledContratsTest extends BaseTest {
+public class BN128PrecompiledContractsTest extends BaseTest {Also rename the file from BN128PrecompiledContratsTest.java to BN128PrecompiledContractsTest.java.
📝 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 class BN128PrecompiledContratsTest extends BaseTest { | |
| public class BN128PrecompiledContractsTest extends BaseTest { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java`
at line 37, Rename the test class and file to correct the typo: change the class
declaration BN128PrecompiledContratsTest to BN128PrecompiledContractsTest and
update the file name from BN128PrecompiledContratsTest.java to
BN128PrecompiledContractsTest.java; also update any imports or references (test
suites, build files, or other classes) that reference
BN128PrecompiledContratsTest to the new BN128PrecompiledContractsTest identifier
so compilation and test discovery continue to work.
Comment on lines
+205
to
+212
| byte[] intput = validPointsByte(); | ||
| Pair<Boolean, byte[]> result = bn128Pairing.execute(intput); | ||
|
|
||
| assertTrue(result.getLeft()); | ||
| Assert.assertArrayEquals(ByteArray.fromHexString( | ||
| "0x0000000000000000000000000000000000000000000000000000000000000001"), result.getRight()); | ||
|
|
||
| intput = inValidPointsOutsideSubgroupG2(); |
There was a problem hiding this comment.
Typo: intput should be input.
The variable name intput is misspelled on lines 205, 206, and 212.
✏️ Proposed fix
- byte[] intput = validPointsByte();
- Pair<Boolean, byte[]> result = bn128Pairing.execute(intput);
+ byte[] input = validPointsByte();
+ Pair<Boolean, byte[]> result = bn128Pairing.execute(input);
assertTrue(result.getLeft());
Assert.assertArrayEquals(ByteArray.fromHexString(
"0x0000000000000000000000000000000000000000000000000000000000000001"), result.getRight());
- intput = inValidPointsOutsideSubgroupG2();
- result = bn128Pairing.execute(intput);
+ input = inValidPointsOutsideSubgroupG2();
+ result = bn128Pairing.execute(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
| byte[] intput = validPointsByte(); | |
| Pair<Boolean, byte[]> result = bn128Pairing.execute(intput); | |
| assertTrue(result.getLeft()); | |
| Assert.assertArrayEquals(ByteArray.fromHexString( | |
| "0x0000000000000000000000000000000000000000000000000000000000000001"), result.getRight()); | |
| intput = inValidPointsOutsideSubgroupG2(); | |
| byte[] input = validPointsByte(); | |
| Pair<Boolean, byte[]> result = bn128Pairing.execute(input); | |
| assertTrue(result.getLeft()); | |
| Assert.assertArrayEquals(ByteArray.fromHexString( | |
| "0x0000000000000000000000000000000000000000000000000000000000000001"), result.getRight()); | |
| input = inValidPointsOutsideSubgroupG2(); | |
| result = bn128Pairing.execute(input); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java`
around lines 205 - 212, The test contains a misspelled variable name `intput`;
rename all occurrences of `intput` to `input` in BN128PrecompiledContratsTest to
match standard spelling and avoid confusion — update the
declarations/assignments where `input` is set from calls like validPointsByte()
and inValidPointsOutsideSubgroupG2(), and ensure subsequent uses (e.g.,
bn128Pairing.execute(input)) and assertions reference `input` consistently.
Comment on lines
+135
to
+138
| { | ||
| "Input": "", | ||
| "Expected": "00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000" | ||
| }, |
There was a problem hiding this comment.
Inconsistent test entry format - missing fields.
This test entry is missing Name, Gas, and NoBenchmark fields that are present in all other entries. This inconsistency could cause parsing issues if the test framework expects uniform JSON structure.
🔧 Proposed fix
{
- "Input": "",
- "Expected": "00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
+ "Input": "",
+ "Expected": "00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
+ "Name": "empty_input",
+ "Gas": 6000,
+ "NoBenchmark": false
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@framework/src/test/resources/json/bn256ScalarMul.json` around lines 135 -
138, The JSON test entry is missing required fields causing inconsistent
structure; update the object (the entry with "Input": "" and the long "Expected"
zero string) to include the same keys as other entries: add a "Name" string
(e.g., descriptive like "bn256ScalarMul_empty_input"), a numeric "Gas" value
consistent with similar tests, and a boolean "NoBenchmark" (true/false as used
elsewhere) so the JSON schema matches other entries and parsing will succeed.
There was a problem hiding this comment.
21 issues found across 24 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="chainbase/src/main/java/org/tron/core/store/DynamicPropertiesStore.java">
<violation number="1" location="chainbase/src/main/java/org/tron/core/store/DynamicPropertiesStore.java:2974">
P2: Naming inconsistency: save method uses `Bn128` but the getter uses `BN128`. Every other save/get pair in this class uses matching capitalization. Rename to `saveAllowOptimizedBN128` for consistency.</violation>
</file>
<file name="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBN128Client.java">
<violation number="1" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBN128Client.java:42">
P1: `setSoTimeout` has no effect on `SocketChannel.read()` — it only applies to `Socket.getInputStream().read()`. This means operations will hang indefinitely if the server stalls, and the `SocketTimeoutException` catch block is unreachable. Consider using `Selector.select(timeout)` or reading from `ch.socket().getInputStream()` instead.</violation>
<violation number="2" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBN128Client.java:52">
P1: SocketChannel.read() may return fewer bytes than the buffer capacity, even in blocking mode. Both the header and body reads need a loop to ensure the buffer is fully filled. A partial header read will cause `BufferUnderflowException` or corrupt the parsed length; a partial body read will return truncated data.</violation>
<violation number="3" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBN128Client.java:66">
P1: After an exception (e.g., partial read, connection reset), the `SocketChannel` may be in an inconsistent protocol state (leftover bytes in the stream). Returning it unconditionally to the pool in `finally` will cause subsequent operations on that channel to read stale/corrupt data. Discard the channel on failure and replace it with a fresh connection.</violation>
</file>
<file name="gradle/verification-metadata.xml">
<violation number="1" location="gradle/verification-metadata.xml:261">
P1: These native cryptographic libraries (`besu-native-common`, `gnark`) are sourced from a personal GitHub fork (`com.github.federico2014.besu-native`) rather than the official Hyperledger repository (`org.hyperledger.besu`). Pulling security-critical BN128 crypto primitives from a personal fork introduces supply chain risk — the fork owner could publish modified artifacts at any time. Consider depending on the official Hyperledger release or vendoring/forking under the project's own organization with proper review controls.</violation>
</file>
<file name="crypto/build.gradle">
<violation number="1" location="crypto/build.gradle:16">
P1: Supply-chain risk: this pulls a **security-critical** cryptographic library (BN128 precompiled contracts) from a personal GitHub fork via JitPack rather than from the official Hyperledger Besu artifact.
JitPack builds from the fork's source on the fly, so the fork owner can push arbitrary changes that will silently flow into this project. For a library that implements elliptic-curve operations used in consensus, this is a high-risk dependency.
Consider using the official artifact from Maven Central (e.g., `org.hyperledger.besu:besu-native-gnark`) or, if a fork is genuinely required, vendoring the fork under the project's own organization with pinned commit verification.</violation>
</file>
<file name="framework/src/main/java/org/tron/program/FullNode.java">
<violation number="1" location="framework/src/main/java/org/tron/program/FullNode.java:49">
P0: `server.start()` blocks indefinitely in its `while (running)` event loop, so the shutdown hook registration after it is **never reached** during normal operation. The server cannot be stopped gracefully. Additionally, `Thread.currentThread().join()` is a self-join that would deadlock if ever reached. Move the shutdown hook registration to before `server.start()`.</violation>
<violation number="2" location="framework/src/main/java/org/tron/program/FullNode.java:54">
P2: `Thread.currentThread().join()` can throw `InterruptedException` after the server has started successfully, but it's caught by the same handler that logs "Failed to start BN128 Socket Server" and calls `System.exit(1)`. This misattributes a normal interruption as a startup failure. Handle `InterruptedException` from `join()` separately.</violation>
</file>
<file name="framework/src/test/resources/json/bn256Pairing.json">
<violation number="1" location="framework/src/test/resources/json/bn256Pairing.json:96">
P2: Test case `ten_point_match_3` is misnamed and is an exact duplicate of `two_point_match_4`. Its Gas value (113000) confirms only 2 pairing pairs, not 10. This gives false confidence about 10-point test coverage.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/Wallet.java">
<violation number="1" location="framework/src/main/java/org/tron/core/Wallet.java:1512">
P2: Key `"getAllowOptimizedBn128"` doesn't match the getter method name `getAllowOptimizedBN128()`. All other chain parameter keys in this method match their getter names exactly. This inconsistency could confuse API consumers looking up parameters by key.</violation>
</file>
<file name="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java">
<violation number="1" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java:33">
P2: `MAX_PAYLOAD` uses `100 * 196` but BN128 pairing pairs are 192 bytes each (as defined in `BN128Executor.PAIR_SIZE` and `BN128Pairing.PAIR_SIZE`). This should be `100 * 192` for consistency. The extra 400 bytes per 100 pairs is unlikely to cause a security issue, but the mismatch signals an error in the calculation.</violation>
<violation number="2" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java:60">
P2: Use SLF4J logger instead of `System.out.println` / `e.printStackTrace()` to align with the project's logging conventions and allow proper log level control.</violation>
<violation number="3" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java:86">
P1: Message framing bug: the 5-byte header is consumed before verifying the full payload is available. If the payload arrives across multiple reads, `compact()` discards the already-consumed header, and subsequent reads misinterpret payload bytes as a new header.
Use `mark()`/`reset()` so the header is only consumed when the full message is present.</violation>
<violation number="4" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java:89">
P1: Missing validation of `len` read from network input. A negative value causes `NegativeArraySizeException` (crashing the selector loop), and a value exceeding `MAX_PAYLOAD` causes the connection to stall forever. Validate `len` and close the channel if out of range.</violation>
<violation number="5" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java:115">
P2: Worker threads never terminate on shutdown. `shutdown()` does not interrupt blocked threads, so `queue.take()` blocks forever. Use `shutdownNow()` and handle `InterruptedException` to allow graceful exit.</violation>
<violation number="6" location="crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java:175">
P1: Incomplete write on non-blocking channel: `ch.write(resp)` may not write all bytes. The return value is ignored, so partial responses can be silently sent, corrupting the framing protocol. Loop until all bytes are written.</violation>
</file>
<file name="actuator/src/main/java/org/tron/core/vm/PrecompiledContracts.java">
<violation number="1" location="actuator/src/main/java/org/tron/core/vm/PrecompiledContracts.java:431">
P2: BN128 client field, static initializer (with socket I/O), and shutdown hook are placed on the abstract `PrecompiledContract` base class, which is the parent of **all** precompiled contracts. Only the three BN128 subclasses use `client`. Consider moving this to a dedicated BN128 helper/holder class to avoid coupling all precompiled contracts to BN128 socket infrastructure.</violation>
</file>
<file name="crypto/src/main/java/org/tron/common/crypto/bn128/BN128Executor.java">
<violation number="1" location="crypto/src/main/java/org/tron/common/crypto/bn128/BN128Executor.java:64">
P1: `executeEIP196Operation` returns `null` when the native library is unavailable, but all three public callers (`add`, `mul`, `pairing`) pass through this return value without a null check. Any caller that doesn't defensively guard for null will get a `NullPointerException`. Return `Pair.of(false, EMPTY_BYTE_ARRAY)` to be consistent with the error-handling pattern used elsewhere in this class.</violation>
</file>
<file name="common/src/main/java/org/tron/common/parameter/CommonParameter.java">
<violation number="1" location="common/src/main/java/org/tron/common/parameter/CommonParameter.java:646">
P1: Default `bn128SocketTimeout = 30` translates to 30 milliseconds (per `Socket.setSoTimeout()` contract). This is extremely short for a socket timeout — even local IPC BN128 pairing operations can exceed 30ms under load. If this is intended to be 30 seconds, the value should be `30000`; otherwise, consider a more reasonable default like `3000` (3 seconds).</violation>
</file>
<file name="framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java">
<violation number="1" location="framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java:37">
P3: Typo in class name: `BN128PrecompiledContratsTest` → `BN128PrecompiledContractsTest` (missing 'c' in "Contracts"). The filename should also be renamed to match.</violation>
<violation number="2" location="framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java:396">
P2: This benchmark test is missing `@Ignore`. The other two benchmark methods (`bn128BenchOptimized`, `bn128BenchUnoptimized`) are `@Ignore`d. Without it, this expensive test (150 iterations of 100-pair pairing) runs in every CI build with no correctness assertions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| server.start(); | ||
| logger.info("BN128 Socket Server started successfully"); | ||
|
|
||
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { |
There was a problem hiding this comment.
P0: server.start() blocks indefinitely in its while (running) event loop, so the shutdown hook registration after it is never reached during normal operation. The server cannot be stopped gracefully. Additionally, Thread.currentThread().join() is a self-join that would deadlock if ever reached. Move the shutdown hook registration to before server.start().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/program/FullNode.java, line 49:
<comment>`server.start()` blocks indefinitely in its `while (running)` event loop, so the shutdown hook registration after it is **never reached** during normal operation. The server cannot be stopped gracefully. Additionally, `Thread.currentThread().join()` is a self-join that would deadlock if ever reached. Move the shutdown hook registration to before `server.start()`.</comment>
<file context>
@@ -36,6 +37,33 @@ public static void main(String[] args) {
+ server.start();
+ logger.info("BN128 Socket Server started successfully");
+
+ Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+ try {
+ logger.info("Shutting down BN128 Socket Server...");
</file context>
| SocketChannel ch = pool.take(); | ||
| try { | ||
| // Set timeout for socket operations | ||
| ch.socket().setSoTimeout(socketTimeout); |
There was a problem hiding this comment.
P1: setSoTimeout has no effect on SocketChannel.read() — it only applies to Socket.getInputStream().read(). This means operations will hang indefinitely if the server stalls, and the SocketTimeoutException catch block is unreachable. Consider using Selector.select(timeout) or reading from ch.socket().getInputStream() instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/common/crypto/bn128/SocketBN128Client.java, line 42:
<comment>`setSoTimeout` has no effect on `SocketChannel.read()` — it only applies to `Socket.getInputStream().read()`. This means operations will hang indefinitely if the server stalls, and the `SocketTimeoutException` catch block is unreachable. Consider using `Selector.select(timeout)` or reading from `ch.socket().getInputStream()` instead.</comment>
<file context>
@@ -0,0 +1,95 @@
+ SocketChannel ch = pool.take();
+ try {
+ // Set timeout for socket operations
+ ch.socket().setSoTimeout(socketTimeout);
+
+ ByteBuffer req = ByteBuffer.allocate(5 + input.length);
</file context>
| ch.write(req); | ||
|
|
||
| ByteBuffer header = ByteBuffer.allocate(5); | ||
| ch.read(header); |
There was a problem hiding this comment.
P1: SocketChannel.read() may return fewer bytes than the buffer capacity, even in blocking mode. Both the header and body reads need a loop to ensure the buffer is fully filled. A partial header read will cause BufferUnderflowException or corrupt the parsed length; a partial body read will return truncated data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/common/crypto/bn128/SocketBN128Client.java, line 52:
<comment>SocketChannel.read() may return fewer bytes than the buffer capacity, even in blocking mode. Both the header and body reads need a loop to ensure the buffer is fully filled. A partial header read will cause `BufferUnderflowException` or corrupt the parsed length; a partial body read will return truncated data.</comment>
<file context>
@@ -0,0 +1,95 @@
+ ch.write(req);
+
+ ByteBuffer header = ByteBuffer.allocate(5);
+ ch.read(header);
+ header.flip();
+ boolean success = header.get() == 0; // status byte: 0=success
</file context>
| <sha256 value="db20239d91307aeafb2599f8c564367eb37d079bab40bb980e6f4c3555cc4882" origin="Generated by Gradle"/> | ||
| </artifact> | ||
| </component> | ||
| <component group="com.github.federico2014.besu-native" name="besu-native-common" version="1.3.11"> |
There was a problem hiding this comment.
P1: These native cryptographic libraries (besu-native-common, gnark) are sourced from a personal GitHub fork (com.github.federico2014.besu-native) rather than the official Hyperledger repository (org.hyperledger.besu). Pulling security-critical BN128 crypto primitives from a personal fork introduces supply chain risk — the fork owner could publish modified artifacts at any time. Consider depending on the official Hyperledger release or vendoring/forking under the project's own organization with proper review controls.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At gradle/verification-metadata.xml, line 261:
<comment>These native cryptographic libraries (`besu-native-common`, `gnark`) are sourced from a personal GitHub fork (`com.github.federico2014.besu-native`) rather than the official Hyperledger repository (`org.hyperledger.besu`). Pulling security-critical BN128 crypto primitives from a personal fork introduces supply chain risk — the fork owner could publish modified artifacts at any time. Consider depending on the official Hyperledger release or vendoring/forking under the project's own organization with proper review controls.</comment>
<file context>
@@ -258,6 +258,22 @@
<sha256 value="db20239d91307aeafb2599f8c564367eb37d079bab40bb980e6f4c3555cc4882" origin="Generated by Gradle"/>
</artifact>
</component>
+ <component group="com.github.federico2014.besu-native" name="besu-native-common" version="1.3.11">
+ <artifact name="besu-native-common-1.3.11.jar">
+ <sha256 value="a9f3ed6b1102c4638bdf0c727e96da670ccc5f093b6b002012a0d205d1043c89" origin="Generated by Gradle"/>
</file context>
| dependencies { | ||
| api project(":common") | ||
| api 'net.java.dev.jna:jna:5.12.1' | ||
| api 'com.github.federico2014.besu-native:gnark:1.3.11' |
There was a problem hiding this comment.
P1: Supply-chain risk: this pulls a security-critical cryptographic library (BN128 precompiled contracts) from a personal GitHub fork via JitPack rather than from the official Hyperledger Besu artifact.
JitPack builds from the fork's source on the fly, so the fork owner can push arbitrary changes that will silently flow into this project. For a library that implements elliptic-curve operations used in consensus, this is a high-risk dependency.
Consider using the official artifact from Maven Central (e.g., org.hyperledger.besu:besu-native-gnark) or, if a fork is genuinely required, vendoring the fork under the project's own organization with pinned commit verification.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/build.gradle, line 16:
<comment>Supply-chain risk: this pulls a **security-critical** cryptographic library (BN128 precompiled contracts) from a personal GitHub fork via JitPack rather than from the official Hyperledger Besu artifact.
JitPack builds from the fork's source on the fly, so the fork owner can push arbitrary changes that will silently flow into this project. For a library that implements elliptic-curve operations used in consensus, this is a high-risk dependency.
Consider using the official artifact from Maven Central (e.g., `org.hyperledger.besu:besu-native-gnark`) or, if a fork is genuinely required, vendoring the fork under the project's own organization with pinned commit verification.</comment>
<file context>
@@ -12,6 +12,8 @@ repositories {
dependencies {
api project(":common")
+ api 'net.java.dev.jna:jna:5.12.1'
+ api 'com.github.federico2014.besu-native:gnark:1.3.11'
}
</file context>
| if (selector != null) { | ||
| selector.close(); | ||
| } | ||
| workers.shutdown(); |
There was a problem hiding this comment.
P2: Worker threads never terminate on shutdown. shutdown() does not interrupt blocked threads, so queue.take() blocks forever. Use shutdownNow() and handle InterruptedException to allow graceful exit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java, line 115:
<comment>Worker threads never terminate on shutdown. `shutdown()` does not interrupt blocked threads, so `queue.take()` blocks forever. Use `shutdownNow()` and handle `InterruptedException` to allow graceful exit.</comment>
<file context>
@@ -0,0 +1,196 @@
+ if (selector != null) {
+ selector.close();
+ }
+ workers.shutdown();
+ }
+
</file context>
| @Getter | ||
| @Setter | ||
| private long vmShouldEndInUs; | ||
| protected static BN128Service client; |
There was a problem hiding this comment.
P2: BN128 client field, static initializer (with socket I/O), and shutdown hook are placed on the abstract PrecompiledContract base class, which is the parent of all precompiled contracts. Only the three BN128 subclasses use client. Consider moving this to a dedicated BN128 helper/holder class to avoid coupling all precompiled contracts to BN128 socket infrastructure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At actuator/src/main/java/org/tron/core/vm/PrecompiledContracts.java, line 431:
<comment>BN128 client field, static initializer (with socket I/O), and shutdown hook are placed on the abstract `PrecompiledContract` base class, which is the parent of **all** precompiled contracts. Only the three BN128 subclasses use `client`. Consider moving this to a dedicated BN128 helper/holder class to avoid coupling all precompiled contracts to BN128 socket infrastructure.</comment>
<file context>
@@ -426,6 +428,39 @@ public abstract static class PrecompiledContract {
@Getter
@Setter
private long vmShouldEndInUs;
+ protected static BN128Service client;
+ // BN128 client connection pool size
+ private static final int BN128_CLIENT_POOL_SIZE = 8;
</file context>
| } | ||
|
|
||
| @Test | ||
| public void bn128Pairing100PairsBenchTest() { |
There was a problem hiding this comment.
P2: This benchmark test is missing @Ignore. The other two benchmark methods (bn128BenchOptimized, bn128BenchUnoptimized) are @Ignored. Without it, this expensive test (150 iterations of 100-pair pairing) runs in every CI build with no correctness assertions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java, line 396:
<comment>This benchmark test is missing `@Ignore`. The other two benchmark methods (`bn128BenchOptimized`, `bn128BenchUnoptimized`) are `@Ignore`d. Without it, this expensive test (150 iterations of 100-pair pairing) runs in every CI build with no correctness assertions.</comment>
<file context>
@@ -0,0 +1,436 @@
+ }
+
+ @Test
+ public void bn128Pairing100PairsBenchTest() {
+ PrecompiledContract bn128Pairing = createPrecompiledContract(altBN128PairingAddr,
+ OWNER_ADDRESS);
</file context>
| private Selector selector; | ||
| private ServerSocketChannel server; | ||
|
|
||
| private static final int MAX_PAYLOAD = 100 * 196; |
There was a problem hiding this comment.
P2: MAX_PAYLOAD uses 100 * 196 but BN128 pairing pairs are 192 bytes each (as defined in BN128Executor.PAIR_SIZE and BN128Pairing.PAIR_SIZE). This should be 100 * 192 for consistency. The extra 400 bytes per 100 pairs is unlikely to cause a security issue, but the mismatch signals an error in the calculation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/common/crypto/bn128/SocketBn128Server.java, line 33:
<comment>`MAX_PAYLOAD` uses `100 * 196` but BN128 pairing pairs are 192 bytes each (as defined in `BN128Executor.PAIR_SIZE` and `BN128Pairing.PAIR_SIZE`). This should be `100 * 192` for consistency. The extra 400 bytes per 100 pairs is unlikely to cause a security issue, but the mismatch signals an error in the calculation.</comment>
<file context>
@@ -0,0 +1,196 @@
+ private Selector selector;
+ private ServerSocketChannel server;
+
+ private static final int MAX_PAYLOAD = 100 * 196;
+ private static final int BUFFER_SIZE = 5 + MAX_PAYLOAD;
+
</file context>
| import org.tron.core.zksnark.SendCoinShieldTest; | ||
|
|
||
| @Slf4j | ||
| public class BN128PrecompiledContratsTest extends BaseTest { |
There was a problem hiding this comment.
P3: Typo in class name: BN128PrecompiledContratsTest → BN128PrecompiledContractsTest (missing 'c' in "Contracts"). The filename should also be renamed to match.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/common/runtime/vm/BN128PrecompiledContratsTest.java, line 37:
<comment>Typo in class name: `BN128PrecompiledContratsTest` → `BN128PrecompiledContractsTest` (missing 'c' in "Contracts"). The filename should also be renamed to match.</comment>
<file context>
@@ -0,0 +1,436 @@
+import org.tron.core.zksnark.SendCoinShieldTest;
+
+@Slf4j
+public class BN128PrecompiledContratsTest extends BaseTest {
+
+ // bn128
</file context>
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?
Why are these changes required?
This PR has been tested by:
Follow up
Extra details
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
Summary by cubic
Add IPC-accelerated BN128 precompile execution for add/mul/pairing using a socket client/server backed by native gnark crypto. The feature is gated by a new chain parameter and fork 4.8.2, and automatically falls back to the existing Java path on errors.
New Features
--bn128-server) withnode.bn128.serverPortandnode.bn128.socketTimeout.VMConfig, dynamic properties, and wallet chain params.Migration
--bn128-serverand configure port/timeout in config.conf.net.java.dev.jna:jna@5.12.1,com.github.federico2014.besu-native:gnark@1.3.11.Written for commit d92a7cc. Summary will update on new commits.