Skip to content

feat: optimize delegate and undelegate instruction handling #6920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ouy95917 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: optimize delegate and undelegate instruction handling #6920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions actuator/src/main/java/org/tron/core/vm/nativecontract/CancelAllUnfreezeV2Processor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.tron.core.vm.VMConstant;
import org.tron.core.vm.nativecontract.param.CancelAllUnfreezeV2Param;
import org.tron.core.vm.repository.Repository;
import org.tron.core.vm.utils.MUtil;
import org.tron.protos.Protocol;

@Slf4j(topic = "VMProcessor")
Expand All @@ -39,6 +40,10 @@ public void validate(CancelAllUnfreezeV2Param param, Repository repo) throws Con
throw new ContractValidateException(
ACCOUNT_EXCEPTION_STR + readableOwnerAddress + NOT_EXIST_STR);
}

if (accountCapsule.hasInvalidDelegatedV2()) {
MUtil.checkCPUTimeForInvalidDelegatedV2Balance();
}
}

public Map<String, Long> execute(CancelAllUnfreezeV2Param param, Repository repo) throws ContractExeException {
Expand Down
5 changes: 5 additions & 0 deletions actuator/src/main/java/org/tron/core/vm/nativecontract/FreezeBalanceV2Processor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.tron.core.store.DynamicPropertiesStore;
import org.tron.core.vm.nativecontract.param.FreezeBalanceV2Param;
import org.tron.core.vm.repository.Repository;
import org.tron.core.vm.utils.MUtil;

@Slf4j(topic = "VMProcessor")
public class FreezeBalanceV2Processor {
Expand Down Expand Up @@ -63,6 +64,10 @@ public void validate(FreezeBalanceV2Param param, Repository repo) throws Contrac
"Unknown ResourceCode, valid ResourceCode[BANDWIDTH、ENERGY]");
}
}

if (repo.isSelfDestructed(ownerAddress)) {
MUtil.checkCPUTimeForFreezeV2AfterSelfDestruct();
}
}

public void execute(FreezeBalanceV2Param param, Repository repo) {
Expand Down
5 changes: 5 additions & 0 deletions actuator/src/main/java/org/tron/core/vm/nativecontract/UnfreezeBalanceV2Processor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.tron.core.vm.config.VMConfig;
import org.tron.core.vm.nativecontract.param.UnfreezeBalanceV2Param;
import org.tron.core.vm.repository.Repository;
import org.tron.core.vm.utils.MUtil;
import org.tron.core.vm.utils.VoteRewardUtil;
import org.tron.protos.Protocol;
import org.tron.protos.contract.Common;
Expand Down Expand Up @@ -86,6 +87,10 @@ public void validate(UnfreezeBalanceV2Param param, Repository repo)
throw new ContractValidateException(
"Invalid unfreeze_balance, [" + param.getUnfreezeBalance() + "] is invalid");
}

if (accountCapsule.hasInvalidDelegatedV2()) {
MUtil.checkCPUTimeForInvalidDelegatedV2Balance();
}
}

private boolean checkUnfreezeBalance(
Expand Down
5 changes: 5 additions & 0 deletions actuator/src/main/java/org/tron/core/vm/nativecontract/WithdrawExpireUnfreezeProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.tron.core.store.DynamicPropertiesStore;
import org.tron.core.vm.nativecontract.param.WithdrawExpireUnfreezeParam;
import org.tron.core.vm.repository.Repository;
import org.tron.core.vm.utils.MUtil;
import org.tron.protos.Protocol;

@Slf4j(topic = "VMProcessor")
Expand Down Expand Up @@ -52,6 +53,10 @@ public void validate(WithdrawExpireUnfreezeParam param, Repository repo) throws
logger.debug(e.getMessage(), e);
throw new ContractValidateException(e.getMessage());
}

if (accountCapsule.hasInvalidDelegatedV2()) {
MUtil.checkCPUTimeForInvalidDelegatedV2Balance();
}
}

private long getTotalWithdrawUnfreeze(List<Protocol.Account.UnFreezeV2> unfrozenV2List, long now) {
Expand Down
10 changes: 10 additions & 0 deletions actuator/src/main/java/org/tron/core/vm/program/ContractState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,16 @@ public boolean isNewContract(byte[] address) {
return repository.isNewContract(address);
}

@Override
public void markSelfDestruct(byte[] address) {
repository.markSelfDestruct(address);
}

@Override
public boolean isSelfDestructed(byte[] address) {
return repository.isSelfDestructed(address);
}

@Override
public void updateAccount(byte[] address, AccountCapsule accountCapsule) {
repository.updateAccount(address, accountCapsule);
Expand Down
11 changes: 10 additions & 1 deletion actuator/src/main/java/org/tron/core/vm/program/Program.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,12 +512,18 @@ public void suicide(DataWord obtainerAddress) {
internalTx.setValue(internalTx.getValue() + expireUnfrozenBalance);
}
}

getContractState().markSelfDestruct(owner);
getResult().addDeleteAccount(this.getContractAddress());
}

public void suicide2(DataWord obtainerAddress) {

byte[] owner = getContextAddress();

if (getContractState().isSelfDestructed(obtainerAddress.toTronAddress())) {
MUtil.checkCPUTimeForSelfDestructedBeneficiary();
}

boolean isNewContract = getContractState().isNewContract(owner);
if (isNewContract) {
suicide(obtainerAddress);
Expand All @@ -540,6 +546,7 @@ public void suicide2(DataWord obtainerAddress) {
"suicide", nonce, getContractState().getAccount(owner).getAssetMapV2());

if (FastByteComparisons.isEqual(owner, obtainer)) {
getContractState().markSelfDestruct(owner);
return;
}

Expand Down Expand Up @@ -579,6 +586,8 @@ public void suicide2(DataWord obtainerAddress) {
internalTx.setValue(internalTx.getValue() + expireUnfrozenBalance);
}
}

getContractState().markSelfDestruct(owner);
}

public Repository getContractState() {
Expand Down
4 changes: 4 additions & 0 deletions actuator/src/main/java/org/tron/core/vm/repository/Repository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ public interface Repository {

boolean isNewContract(byte[] address);

void markSelfDestruct(byte[] address);

boolean isSelfDestructed(byte[] address);

void updateAccount(byte[] address, AccountCapsule accountCapsule);

void updateDynamicProperty(byte[] word, BytesCapsule bytesCapsule);
Expand Down
31 changes: 31 additions & 0 deletions actuator/src/main/java/org/tron/core/vm/repository/RepositoryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ public class RepositoryImpl implements Repository {
private final HashMap<Key, Value<DelegatedResourceAccountIndex>> delegatedResourceAccountIndexCache = new HashMap<>();
private final HashBasedTable<Key, Key, Value<byte[]>> transientStorage = HashBasedTable.create();
private final HashSet<Key> newContractCache = new HashSet<>();
private final HashSet<Key> selfDestructCache = new HashSet<>();

public static void removeLruCache(byte[] address) {
}
Expand Down Expand Up @@ -572,6 +573,29 @@ public boolean isNewContract(byte[] address) {
}
}

@Override
public void markSelfDestruct(byte[] address) {
selfDestructCache.add(Key.create(address));
}

@Override
public boolean isSelfDestructed(byte[] address) {
Key key = Key.create(address);
if (selfDestructCache.contains(key)) {
return true;
}

if (parent != null) {
boolean isSelfDestructed = parent.isSelfDestructed(address);
if (isSelfDestructed) {
selfDestructCache.add(key);

@lily309 lily309 Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is caching the positive result from the parent repository intentional here? My understanding is that the self-destruct state is monotonic within the transaction, while a negative result is left uncached so that a later parent update can still be observed.

@yanghang8612 yanghang8612 Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, that is the intention. Once an address is marked, it remains self-destructed for the current transaction, so caching a positive result avoids repeated parent lookups. Negative results are not cached because the parent repository may receive the marker later.

}
return isSelfDestructed;
} else {
return false;
}
}

@Override
public void updateAccount(byte[] address, AccountCapsule accountCapsule) {
accountCache.put(Key.create(address),
Expand Down Expand Up @@ -780,6 +804,7 @@ public void commit() {
commitDelegatedResourceAccountIndexCache(repository);
commitTransientStorage(repository);
commitNewContractCache(repository);
commitSelfDestructCache(repository);
}

@Override
Expand Down Expand Up @@ -1142,6 +1167,12 @@ public void commitNewContractCache(Repository deposit) {
}
}

private void commitSelfDestructCache(Repository deposit) {
if (deposit != null) {
selfDestructCache.forEach(key -> deposit.markSelfDestruct(key.getData()));

@lily309 lily309 Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does propagating the marker only during commit mean that it follows the same lifecycle as the other repository state, so a reverted child repository requires no separate cleanup?

@yanghang8612 yanghang8612 Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Correct. The marker is propagated to the parent only when the child repository commits. If the child execution is reverted, the child repository and its marker are discarded together.

}
}

/**
* Get the block id from the number.
*/
Expand Down
19 changes: 19 additions & 0 deletions actuator/src/main/java/org/tron/core/vm/utils/MUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,23 @@ public static void checkCPUTimeForModExp() {
throw new OutOfTimeException("CPU timeout for modExp executing");
}
}

public static void checkCPUTimeForFreezeV2AfterSelfDestruct() {
if (ForkController.instance().pass(Parameter.ForkBlockVersionEnum.VERSION_4_8_2_2)) {
throw new OutOfTimeException("CPU timeout for FreezeBalanceV2 after SELFDESTRUCT");
}
}

public static void checkCPUTimeForSelfDestructedBeneficiary() {
if (ForkController.instance().pass(Parameter.ForkBlockVersionEnum.VERSION_4_8_2_2)) {
throw new OutOfTimeException(
"CPU timeout for SELFDESTRUCT with selfdestructed beneficiary");
}
}

public static void checkCPUTimeForInvalidDelegatedV2Balance() {
if (ForkController.instance().pass(Parameter.ForkBlockVersionEnum.VERSION_4_8_2_2)) {
throw new OutOfTimeException("CPU timeout for invalid delegated V2 balance");
}
}
}
4 changes: 4 additions & 0 deletions chainbase/src/main/java/org/tron/core/capsule/AccountCapsule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1336,6 +1336,10 @@ public void clearDelegatedResource() {
this.account = builder.build();
}

public boolean hasInvalidDelegatedV2() {
return getDelegatedFrozenV2BalanceForBandwidth() < 0 || getDelegatedFrozenV2BalanceForEnergy() < 0;
}

public void importAsset(byte[] key) {
this.account = AssetUtil.importAsset(this.account, key);
}
Expand Down
5 changes: 3 additions & 2 deletions common/src/main/java/org/tron/core/config/Parameter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ public enum ForkBlockVersionEnum {
VERSION_4_8_0_1(33, 1596780000000L, 70),
VERSION_4_8_1(34, 1596780000000L, 80),
VERSION_4_8_1_1(35, 1596780000000L, 70),
VERSION_4_8_2(36, 1596780000000L, 80);
VERSION_4_8_2(36, 1596780000000L, 80),
VERSION_4_8_2_2(37, 1596780000000L, 70);
// if add a version, modify BLOCK_VERSION simultaneously

@Getter
Expand Down Expand Up @@ -79,7 +80,7 @@ public class ChainConstant {
public static final int SINGLE_REPEAT = 1;
public static final int BLOCK_FILLED_SLOTS_NUMBER = 128;
public static final int MAX_FROZEN_NUMBER = 1;
public static final int BLOCK_VERSION = 36;
public static final int BLOCK_VERSION = 37;
public static final long FROZEN_PERIOD = 86_400_000L;
public static final long DELEGATE_PERIOD = 3 * 86_400_000L;
public static final long TRX_PRECISION = 1000_000L;
Expand Down
32 changes: 32 additions & 0 deletions framework/src/test/java/org/tron/common/runtime/vm/OperationsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static org.junit.Assert.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.tron.core.config.Parameter.ChainConstant.FROZEN_PERIOD;
import static org.tron.core.config.Parameter.ForkBlockVersionEnum.VERSION_4_8_2_2;

import java.util.List;
import java.util.Locale;
Expand All @@ -16,6 +17,7 @@
import org.junit.BeforeClass;
import org.junit.Ignore;
import org.junit.Test;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.util.StringUtils;
Expand All @@ -24,6 +26,7 @@
import org.tron.common.parameter.CommonParameter;
import org.tron.common.runtime.InternalTransaction;
import org.tron.common.utils.DecodeUtil;
import org.tron.common.utils.ForkController;
import org.tron.core.Constant;
import org.tron.core.Wallet;
import org.tron.core.capsule.AccountCapsule;
Expand All @@ -42,6 +45,7 @@
import org.tron.core.vm.config.ConfigLoader;
import org.tron.core.vm.config.VMConfig;
import org.tron.core.vm.program.Program;
import org.tron.core.vm.program.Program.OutOfTimeException;
import org.tron.core.vm.program.invoke.ProgramInvokeMockImpl;
import org.tron.core.vm.repository.Repository;
import org.tron.protos.Protocol;
Expand Down Expand Up @@ -1182,6 +1186,7 @@ public void testSuicideAction() throws ContractValidateException {

program.suicide(new DataWord(
dbManager.getAccountStore().getBlackhole().getAddress().toByteArray()));
Assert.assertTrue(program.getContractState().isSelfDestructed(program.getContextAddress()));

DecodeUtil.addressPreFixByte = prePrefixByte;
VMConfig.initAllowEnergyAdjustment(0);
Expand Down Expand Up @@ -1240,6 +1245,7 @@ public void testSuicideAction2() throws ContractValidateException {
OperationActions.suicideAction2(program);

Assert.assertEquals(1, program.getResult().getDeleteAccounts().size());
Assert.assertTrue(program.getContractState().isSelfDestructed(contractAddr));


invoke = new ProgramInvokeMockImpl(StoreFactory.getInstance(), new byte[0], contractAddr);
Expand All @@ -1256,6 +1262,7 @@ public void testSuicideAction2() throws ContractValidateException {
dbManager.getAccountStore().getBlackhole().getAddress().toByteArray()));

Assert.assertEquals(0, spyProgram.getResult().getDeleteAccounts().size());
Assert.assertTrue(spyProgram.getContractState().isSelfDestructed(contractAddr));

DecodeUtil.addressPreFixByte = prePrefixByte;
VMConfig.initAllowEnergyAdjustment(0);
Expand All @@ -1266,6 +1273,31 @@ public void testSuicideAction2() throws ContractValidateException {
VMConfig.initAllowTvmVote(0);
}

@Test
public void testSuicide2RejectsSelfDestructedBeneficiaryAfterFork()

@CodeNinjaEvan CodeNinjaEvan Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could we also add a pre-fork case asserting that suicide2() succeeds when VERSION_4_8_2_2 is not activated? Since the beneficiary check is invoked unconditionally and gated inside MUtil, this would explicitly protect backward compatibility before the fork.

@yanghang8612 yanghang8612 Sep 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for pointing this out. Before VERSION_4_8_2_2 is activated, checkCPUTimeForSelfDestructedBeneficiary() is a no-op and execution continues through the existing suicide2() path. The existing testSuicideAction2() still covers the successful legacy flow, while the newly added test focuses specifically on the rejection behavior introduced after activation.

Given that the behavioral change is confined to the activated branch, we would prefer to keep the current test scope unless there is another pre-activation side effect that needs to be covered.

@CodeNinjaEvan CodeNinjaEvan Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could we also add a pre-fork case asserting that suicide2() succeeds when VERSION_4_8_2_2 is not activated? Since the beneficiary check is invoked unconditionally and gated inside MUtil, this would explicitly protect backward compatibility before the fork.

@yanghang8612 yanghang8612 Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks. This appears to be the same concern as the discussion above. The pre-activation check returns without throwing, so the existing suicide2() behavior remains unchanged. I have added the detailed explanation in the other thread.

throws ContractValidateException {
byte[] contractAddr = Hex.decode("41471fd3ad3e9eeadeec4608b92d16ce6b500704cc");
byte[] beneficiary = Hex.decode("411111111111111111111111111111111111111111");
invoke = new ProgramInvokeMockImpl(StoreFactory.getInstance(), new byte[0], contractAddr);
program = new Program(null, null, invoke,
new InternalTransaction(
Protocol.Transaction.getDefaultInstance(),
InternalTransaction.TrxType.TRX_UNKNOWN_TYPE));
program.getContractState().markSelfDestruct(beneficiary);

ForkController forkController = Mockito.mock(ForkController.class);
try (MockedStatic<ForkController> fork = Mockito.mockStatic(ForkController.class)) {
fork.when(ForkController::instance).thenReturn(forkController);
Mockito.when(forkController.pass(VERSION_4_8_2_2)).thenReturn(true);

OutOfTimeException exception = Assert.assertThrows(OutOfTimeException.class,
() -> program.suicide2(new DataWord(beneficiary)));
Assert.assertEquals(
"CPU timeout for SELFDESTRUCT with selfdestructed beneficiary",
exception.getMessage());
}
}

@Test
public void testVoteWitnessCost() throws ContractValidateException {
// Build stack environment, the stack from top to bottom is 0x00, 0x80, 0x00, 0x80
Expand Down
Loading
Loading