Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
79e165c to
c95607d
Compare
bladehan1
left a comment
bladehan1
left a comment
There was a problem hiding this comment.
Automated review by the Codex review pipeline.
Decision: Approve with one concern
Findings: P0=0, P1=1, P2=0, nit=0
NOTE: This review contains AI suggestions; human reviewers retain final judgment.
c95607d to
59af200
Compare
Use TronError with PARAMETER_INIT for invalid startup configuration, remove assertions that are inactive at runtime, and cover the new error paths with unit tests.
59af200 to
48d883f
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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/repository/RepositoryImpl.java">
<violation number="1" location="actuator/src/main/java/org/tron/core/vm/repository/RepositoryImpl.java:976">
P2: This assertion is not inactive/historical code — it guards division by totalEnergyWeight on the very next lines. If totalEnergyWeight is 0, the hardened path throws a bare ArithmeticException and the non-hardened path silently yields an unbounded value from `(long)(... / 0.0)` (Long.MAX_VALUE). The PR limits removal to dead asserts and retains critical ones; this one is live protection in a core resource method, so keeping it (or an explicit zero check) preserves the safety net rather than deleting it.</violation>
</file>
| long totalEnergyLimit = getDynamicPropertiesStore().getTotalEnergyCurrentLimit(); | ||
| long totalEnergyWeight = getDynamicPropertiesStore().getTotalEnergyWeight(); | ||
|
|
||
| assert totalEnergyWeight > 0; |
There was a problem hiding this comment.
P2: This assertion is not inactive/historical code — it guards division by totalEnergyWeight on the very next lines. If totalEnergyWeight is 0, the hardened path throws a bare ArithmeticException and the non-hardened path silently yields an unbounded value from (long)(... / 0.0) (Long.MAX_VALUE). The PR limits removal to dead asserts and retains critical ones; this one is live protection in a core resource method, so keeping it (or an explicit zero check) preserves the safety net rather than deleting it.
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/repository/RepositoryImpl.java, line 976:
<comment>This assertion is not inactive/historical code — it guards division by totalEnergyWeight on the very next lines. If totalEnergyWeight is 0, the hardened path throws a bare ArithmeticException and the non-hardened path silently yields an unbounded value from `(long)(... / 0.0)` (Long.MAX_VALUE). The PR limits removal to dead asserts and retains critical ones; this one is live protection in a core resource method, so keeping it (or an explicit zero check) preserves the safety net rather than deleting it.</comment>
<file context>
@@ -973,8 +972,6 @@ public long calculateGlobalEnergyLimit(AccountCapsule accountCapsule) {
- assert totalEnergyWeight > 0;
-
if (hardenResourceCalculation()) {
return BigInteger.valueOf(energyWeight)
.multiply(BigInteger.valueOf(totalEnergyLimit))
</file context>
There was a problem hiding this comment.
2 issues found across 11 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/common/zksnark/MerklePath.java">
<violation number="1" location="chainbase/src/main/java/org/tron/common/zksnark/MerklePath.java:78">
P3: The deleted assertion was the only enforcement of the invariant that `authenticationPath.size() == index.size()` (path depth == index bit-length). With it removed outright instead of converted to an explicit runtime check, `encode()` now silently produces a malformed encoding when a `MerklePath` is constructed with mismatched sizes, because the public constructor validates nothing and `convertVectorToLong` only bounds-checks `index.size() <= 64` without relating it to `authenticationPath.size()`. An explicit size check costs one `if` and keeps a clear diagnostic for what was previously an always-inactive guard.</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:1048">
P2: TronError extends java.lang.Error, not Exception, so these three throw sites (serverType check, logEmptyError, and the CommitteeConfig postProcess change) change the failure type from a catchable RuntimeException (IllegalArgumentException) to an Error that only the ExitManager uncaught-handler can terminate on. For FullNode startup this is the intended design and exit code 1 is preserved. However, Args.loadDnsPublishConfig/loadDnsPublishParameters are public and documented as callable by tests and external code; any caller that previously wrapped DNS/config loading in catch (Exception) or catch (RuntimeException) for graceful fallback will silently lose that edge and let an Error escape (process exit). Verify no current or future caller relies on catching these failures as Exception.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (!"aws".equalsIgnoreCase(serverType) && !"aliyun".equalsIgnoreCase(serverType)) { | ||
| throw new IllegalArgumentException( | ||
| "Check node.dns.serverType, must be aws or aliyun"); | ||
| throw new TronError( |
There was a problem hiding this comment.
P2: TronError extends java.lang.Error, not Exception, so these three throw sites (serverType check, logEmptyError, and the CommitteeConfig postProcess change) change the failure type from a catchable RuntimeException (IllegalArgumentException) to an Error that only the ExitManager uncaught-handler can terminate on. For FullNode startup this is the intended design and exit code 1 is preserved. However, Args.loadDnsPublishConfig/loadDnsPublishParameters are public and documented as callable by tests and external code; any caller that previously wrapped DNS/config loading in catch (Exception) or catch (RuntimeException) for graceful fallback will silently lose that edge and let an Error escape (process exit). Verify no current or future caller relies on catching these failures as Exception.
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 1048:
<comment>TronError extends java.lang.Error, not Exception, so these three throw sites (serverType check, logEmptyError, and the CommitteeConfig postProcess change) change the failure type from a catchable RuntimeException (IllegalArgumentException) to an Error that only the ExitManager uncaught-handler can terminate on. For FullNode startup this is the intended design and exit code 1 is preserved. However, Args.loadDnsPublishConfig/loadDnsPublishParameters are public and documented as callable by tests and external code; any caller that previously wrapped DNS/config loading in catch (Exception) or catch (RuntimeException) for graceful fallback will silently lose that edge and let an Error escape (process exit). Verify no current or future caller relies on catching these failures as Exception.</comment>
<file context>
@@ -1045,8 +1045,9 @@ private static void loadDnsPublishParameters(NodeConfig.DnsConfig dns,
if (!"aws".equalsIgnoreCase(serverType) && !"aliyun".equalsIgnoreCase(serverType)) {
- throw new IllegalArgumentException(
- "Check node.dns.serverType, must be aws or aliyun");
+ throw new TronError(
+ "Check node.dns.serverType, must be aws or aliyun",
+ TronError.ErrCode.PARAMETER_INIT);
</file context>
| } | ||
|
|
||
| public byte[] encode() throws ZksnarkException { | ||
| assert (authenticationPath.size() == index.size()); |
There was a problem hiding this comment.
P3: The deleted assertion was the only enforcement of the invariant that authenticationPath.size() == index.size() (path depth == index bit-length). With it removed outright instead of converted to an explicit runtime check, encode() now silently produces a malformed encoding when a MerklePath is constructed with mismatched sizes, because the public constructor validates nothing and convertVectorToLong only bounds-checks index.size() <= 64 without relating it to authenticationPath.size(). An explicit size check costs one if and keeps a clear diagnostic for what was previously an always-inactive guard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At chainbase/src/main/java/org/tron/common/zksnark/MerklePath.java, line 78:
<comment>The deleted assertion was the only enforcement of the invariant that `authenticationPath.size() == index.size()` (path depth == index bit-length). With it removed outright instead of converted to an explicit runtime check, `encode()` now silently produces a malformed encoding when a `MerklePath` is constructed with mismatched sizes, because the public constructor validates nothing and `convertVectorToLong` only bounds-checks `index.size() <= 64` without relating it to `authenticationPath.size()`. An explicit size check costs one `if` and keeps a clear diagnostic for what was previously an always-inactive guard.</comment>
<file context>
@@ -75,7 +75,6 @@ private static long convertVectorToLong(List<Boolean> v) throws ZksnarkException
public byte[] encode() throws ZksnarkException {
- assert (authenticationPath.size() == index.size());
List<List<Byte>> pathByteList = Lists.newArrayList();
long indexLong; // 64
for (int i = 0; i < authenticationPath.size(); i++) {
</file context>
1 participant
What does this PR do?
IllegalArgumentExceptionfailures withTronError(PARAMETER_INIT).TrieImpland the Besu-derivedBlake2bfMessageDigestimplementations.chainbase.Why are these changes required?
The selected failures are parameter initialization errors. Classifying them with
PARAMETER_INITmakes startup failures consistent while preserving their messages and exit behavior.The assertion cleanup is limited to historical java-tron code where the assertions are inactive by default. Assertions in
TrieImplare retained because its core implementation is derived from EthereumJ and the checks document internal node-type invariants. The Besu-derived Blake2bf assertion is retained for the same upstream-maintenance reason. Keeping these assertions avoids unnecessary divergence from their source implementations; replacing them with production runtime checks, if required, should be evaluated separately.The two tracked placeholder files are empty and have no source references. Their deletion is moved here so the CI-only PR remains limited to workflow changes.
This PR has been tested by:
./gradlew :common:test --tests org.tron.core.config.args.CommitteeConfigTest./gradlew :framework:test --tests org.tron.core.config.args.ArgsTest./gradlew -g /private/tmp/java-tron-gradle-home :framework:test --tests org.tron.core.tire.TrieTest./gradlew :common:jacocoTestReport :framework:jacocoTestReportCommitteeConfig.java:166andArgs.java:1048,1092../gradlew :framework:checkstyleTestgit diff --checkFollow up
Other startup failures and any replacement of upstream-derived assertions with explicit runtime checks should be evaluated separately according to their semantics and performance impact.
Extra details
TrieImplandBlake2bfMessageDigestare excluded from the assertion-removal scope.