chore: add safety guard for negative cycle index#6527
chore: add safety guard for negative cycle index#6527operagxsasha wants to merge 6 commits intotronprotocol:developfrom
Conversation
| } | ||
| if (beginCycle < endCycle) { | ||
| for (Pair<byte[], Long> vote : srAddresses) { | ||
| if (beginCycle == 0) { |
There was a problem hiding this comment.
beginCycle will be start with 1
There was a problem hiding this comment.
Correct, this prevents beginCycle from being 0
There was a problem hiding this comment.
I agree with @bladehan1's observation. After tracing the call sites of computeReward(beginCycle, endCycle, accountCapsule):
- In
withdrawReward()(line 94):beginCycle = delegationStore.getBeginCycle(address), and the delegation cycle starts from 1. - Before reaching line 215,
beginCyclehas already been validated (beginCycle > currentCyclereturns early at line 98-100), and may have been incremented (beginCycle += 1at line 116). - In
queryReward()(line 142): same pattern —beginCycleis loaded from store and validated before use.
So beginCycle == 0 cannot occur in normal execution flow. Additionally, even as a defensive check, using continue inside the vote loop is incorrect — it skips individual votes rather than handling the invalid cycle at the method level. A proper guard would be placed before the for-loop, e.g., if (beginCycle <= 0) return 0;.
Also noting that the indentation of the added code is inconsistent with the surrounding code style.
| if (beginCycle < endCycle) { | ||
| for (Pair<byte[], Long> vote : srAddresses) { | ||
| if (beginCycle == 0) { | ||
| continue; |
There was a problem hiding this comment.
BTW, the indentation looks off this line. Please fix to match project style (4 spaces).
chainbase/src/main/java/org/tron/core/service/MortgageService.java
Outdated
Show resolved
Hide resolved
| if (beginCycle < endCycle) { | ||
| for (Pair<byte[], Long> vote : srAddresses) { | ||
|
|
||
| for (Pair<byte[], Long> vote : srAddresses) { |
There was a problem hiding this comment.
The PR title mentions adding a "safety guard for negative cycle index", but the diff only shows formatting changes.
Could you double-check if the intended logic changes are missing from this PR?
4 participants
What does this PR do?
Why are these changes required?
This PR has been tested by:
Follow up
Extra details