feat(vm): close shielded TRC20 transaction progressively#79
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
4 issues found across 14 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="common/src/main/java/org/tron/core/config/args/CommitteeConfig.java">
<violation number="1" location="common/src/main/java/org/tron/core/config/args/CommitteeConfig.java:31">
P2: Validate `closeShieldedTRC20Transaction` before copying it through; the new setting is documented as 0-3 but currently accepts any long value.</violation>
</file>
<file name="actuator/src/main/java/org/tron/core/vm/program/Program.java">
<violation number="1" location="actuator/src/main/java/org/tron/core/vm/program/Program.java:1750">
P1: This new runtime-error path does not stop execution before commit, so "closed" shielded precompile calls can still commit state changes before revert is marked.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| private long allowTvmSolidity059 = 0; | ||
| private long forbidTransferToContract = 0; | ||
| private long allowShieldedTRC20Transaction = 0; | ||
| private long closeShieldedTRC20Transaction = 0; |
There was a problem hiding this comment.
P2: Validate closeShieldedTRC20Transaction before copying it through; the new setting is documented as 0-3 but currently accepts any long value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At common/src/main/java/org/tron/core/config/args/CommitteeConfig.java, line 31:
<comment>Validate `closeShieldedTRC20Transaction` before copying it through; the new setting is documented as 0-3 but currently accepts any long value.</comment>
<file context>
@@ -28,6 +28,7 @@ public class CommitteeConfig {
private long allowTvmSolidity059 = 0;
private long forbidTransferToContract = 0;
private long allowShieldedTRC20Transaction = 0;
+ private long closeShieldedTRC20Transaction = 0;
private long allowMarketTransaction = 0;
private long allowTransactionFeePool = 0;
</file context>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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="common/src/main/java/org/tron/core/config/args/CommitteeConfig.java">
<violation number="1" location="common/src/main/java/org/tron/core/config/args/CommitteeConfig.java:31">
P2: Validate `closeShieldedTRC20Transaction` before copying it through; the new setting is documented as 0-3 but currently accepts any long value.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Add committee governance proposal CLOSE_SHIELDED_TRC20_TRANSACTION (id 80, value range 0-3) that progressively disables the shielded TRC20 precompiled contracts: 0 -> off (default, no behavior change) 1 -> disable mint (verifyMintProof) 2 -> disable mint + transfer (verifyTransferProof) 3 -> disable mint + transfer + burn (verifyBurnProof) When a level is engaged, the corresponding proof precompile short-circuits to (success, 32 zero bytes) before any verification so the caller's unused energy is refunded instead of being fully burned; Program tags the call with a runtime error (shield mint/transfer/burn not allowed) and VMActuator turns that into a REVERT with no state change. Validation is gated to fork v4.8.2. The parameter is persisted in DynamicPropertiesStore, applied through ProposalService, loaded into the VM via VMConfig/ConfigLoader, exposed in Wallet.getChainParameters, and configurable through committee.closeShieldedTRC20Transaction (reference.conf). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
f31a7d7 to
7ee8815
Compare
What does this PR do?
Adds a committee governance proposal
CLOSE_SHIELDED_TRC20_TRANSACTION(id 80, value range 0–3) that progressively disables the shielded TRC20 precompiled contracts:01verifyMintProof)2verifyTransferProof)3verifyBurnProof)When a level is engaged, the corresponding proof precompile short-circuits to
(success, 32 zero bytes)before any verification;Programtags the call with a specific runtime error (shield trc20 mint/transfer/burn not allowed), andVMActuatorpreserves that reason and marks the call asREVERT(no state change) instead of overwriting it with the generic"REVERT opcode executed".The parameter is:
DynamicPropertiesStore(CLOSE_SHIELDED_TRC20_TRANSACTION),ProposalService,VMConfig/ConfigLoader,Wallet.getChainParametersasgetCloseShieldedTRC20Transaction,committee.closeShieldedTRC20Transaction(seereference.conf),Design note — why close via "success + revert" instead of failing the precompile (energy cost)
The proof precompile's
execute()deliberately returns success(true, 32 zero bytes)when a level is closed, rather than throwing an exception, returning(false, …), or having the contract lookup returnnull. The reason is energy accounting inProgram.callToPrecompiledAddress:refundEnergy(msg.getEnergy() − requiredEnergy)— only the precompile's fixed cost (getEnergyForData, e.g. 150000) is consumed.execute()/ the contract lookup,execute()returningfalse, or no contract found) is the "spend all energy on failure" path:refundEnergy(0), which burns the entire remaining energy of the call.So if the close were implemented by making the precompile fail, every blocked mint/transfer/burn call would penalize the caller with a full-energy burn. Instead we let
execute()return normally, then setruntimeError(shield mint/transfer/burn not allowed) on the result afterwards and letVMActuatorturn it into aREVERT. This yields the same revert outcome (no state change) while refunding the caller's unused energy, avoiding the all-energy-consumed penalty.Why are these changes required?
Governance needs a controlled, staged way to wind down shielded TRC20 support rather than an all-or-nothing switch. Closing in the order mint → transfer → burn lets the network first stop new shielded notes from being created, then stop moving them, while keeping burn open last so existing holders can still unshield (exit) their funds before the feature is fully closed. Default
0keeps current behavior, so the change is inert until a proposal is approved post-4.8.2.This PR has been tested by:
ProposalUtilTest— proposal 80 is rejected before fork v4.8.2 (even with earlier forks active) and validates the 0–3 range afterwards.ProposalServiceTest— approving the proposal persists the value into the dynamic properties store.PrecompiledContractsTest— each proof precompile short-circuits to(success, zero)at its matching close level.RpcApiServicesTest— store initialization with the new key.checkstyleMain/checkstyleTestreference.confbean-parity, key-name, and comment-coverage gatesFollow up
N/A
Extra details
Adapted to the v4.8.2 codebase:
VMConfiglives incommon, and the committee parameter is wired through theCommitteeConfigbean +reference.conf(replacing the oldConstant.COMMITTEE_*/Args.hasPathstyle). Proposal code80was unused on this branch (it skips 79 → 81), so no renumbering was needed.