refactor(config): improve startup errors and remove inactive assertions - #45
refactor(config): improve startup errors and remove inactive assertions#45bladehan1 wants to merge 2 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus 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>
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.