Skip to content

feat(vm): optimize voting power management (TIP-944) - #7068

Open
yanghang8612 wants to merge 5 commits into
release_v4.8.3from
feature/tip-944-voting-power
Open

yanghang8612 wants to merge 5 commits into
release_v4.8.3from
feature/tip-944-voting-power

Conversation

@yanghang8612

@yanghang8612 yanghang8612 commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

Implement TIP-944 behind ALLOW_TVM_VOTE_POWER_OPTIMIZATION (103), aligning legacy voting-power state transitions for TVM voting, Stake 1.0 freezing/unfreezing, and self-destruct stake receipt and cleanup.

Why are these changes required?

Keep TVM account-state transitions consistent with native NewResourceModel rules.

Related to TIP-944.

This PR has been tested by:

  • ./gradlew clean build -x test on macOS ARM64 / JDK 17.
  • Focused regression suite: 68 tests, 67 passed and 1 existing benchmark skipped, including static configuration restoration, real VM execution, enclosing-call rollback, and Stake 1.0 switch combinations.
  • Required Checkstyle tasks and .github/scripts/check_math_usage.sh.

Follow up

Coordinate on-chain activation. Existing feature prerequisites and voting-power selection conditions remain applicable; no historical account-state migration is introduced.

Extra details

Targets release_v4.8.3. Proposal validation, application, and parameter output place 103 after 102. TIP-945 has independent runtime activation.

assertEquals(1, dbManager.getDynamicPropertiesStore().getAllowTvmVotePowerOptimization());
assertThrows(ContractValidateException.class, () -> ProposalUtil.validator(
dbManager.getDynamicPropertiesStore(), fork, 103, 1));
ConfigLoader.disable = false;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The governance test is a nice end-to-end check: fork gating, one-way activation, ProposalService application and both ConfigLoader modes are all exercised in one place.

One thing worth tightening: the test sets ConfigLoader.disable = false and finishes while the global VMConfig snapshot still reports allowTvmVotePowerOptimization == true. Both are static, so this state can leak into other test classes that share the same JVM fork and make unrelated assertions pass or fail for the wrong reason. Would it be worth restoring ConfigLoader.disable and the global snapshot in an @After so the class is self-contained?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking the test isolation. Tip944Test inherits the method-level VMConfigRule from BaseTest. That rule already deep-copies and restores the global snapshot and ConfigLoader.disable, and clears the thread-local view, including when setup or the test fails.

I've added three focused regressions covering normal completion, setup failure, and assertion failure, with in-place flag mutation and snapshot replacement. They verify all snapshot fields, loader state, and local-view cleanup. The focused suites pass on both TIP branches, including the governance tests.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification and the additional tests. I checked VMConfigRule and confirmed that it restores the configuration after each test, including when setup or an assertion fails. This addresses my concern.

@liang3885

Copy link
Copy Markdown

Could this leave a historical account with stale oldTronPower unhandled?

A possible pre-activation sequence, assuming the relevant feature flags are enabled:

  1. Contract A has 100 TRX of Stake 1.0 stake, oldTronPower == 0, and no votes.
  2. A freezes 1 additional TRX through Stake V2. FreezeBalanceV2Processor initializes oldTronPower from the existing stake before adding the new stake, so the snapshot becomes 100 TRX.
  3. After the Stake 1.0 stake expires, A executes SELFDESTRUCT to B through the path that preserves an existing contract.
  4. Since A has no votes, the old withdrawRewardAndCancelVote() skips the branch that resets oldTronPower. Its stake is transferred/cleared, potentially leaving A with zero stake but positive oldTronPower.

This PR appears to prevent that state from being created by the same sequence after activation. However, for an account already in that state, both new self-destruct invalidation checks appear to be skipped because they require positive stake. The optimized vote-cleanup path also no longer resets oldTronPower.

Could such an account retain the stale snapshot and subsequently vote through getAllTronPower()? Is there another invariant or cleanup path that makes this sequence unreachable?

I have not reproduced this end to end or confirmed that such accounts exist on-chain. Would it be worth adding a regression test starting with zero stake and positive oldTronPower, followed by SELFDESTRUCT and another voting attempt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic:vm VM, smart contract

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants