-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: discard the code_hash and trx_hash fields passed in during contract deployment
#6945
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -217,6 +217,9 @@ public void execute(Object object) throws ContractExeException { | |
| } else { | ||
| result.spendEnergy(saveCodeEnergy); | ||
| if (VMConfig.allowTvmConstantinople()) { | ||
| CreateSmartContract createContract = | ||
| ContractCapsule.getSmartContractFromTransaction(trx); | ||
| checkContractHashFields(createContract.getNewContract()); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is placing this check immediately before saveCode intentional so that the restriction is applied at the code-persistence boundary, while the preceding contract validation flow keeps its existing behavior?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. The check is intentionally performed when the deployment reaches the code-persistence stage. If either field is populated after activation, execution is stopped before the deployed code is saved, while earlier validation behavior remains unchanged. |
||
| rootRepository.saveCode(program.getContractAddress().getNoLeadZeroesData(), code); | ||
| } | ||
| } | ||
|
|
@@ -330,6 +333,7 @@ private void create() | |
| if (contract == null) { | ||
| throw new ContractValidateException("Cannot get CreateSmartContract from transaction"); | ||
| } | ||
|
|
||
| SmartContract newSmartContract; | ||
| if (VMConfig.allowTvmCompatibleEvm()) { | ||
| newSmartContract = contract.getNewContract().toBuilder().setVersion(1).build(); | ||
|
|
@@ -455,6 +459,12 @@ private void create() | |
|
|
||
| } | ||
|
|
||
| static void checkContractHashFields(SmartContract contract) { | ||
| if (!contract.getCodeHash().isEmpty() || !contract.getTrxHash().isEmpty()) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Covering both fields with a single || and relying on proto3 isEmpty() correctly handles unset vs. empty — normal deployments are untouched. |
||
| MUtil.checkCPUTimeForContractHashFields(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * ** | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,12 @@ public static boolean isNotNullOrEmpty(String str) { | |
| return !isNullOrEmpty(str); | ||
| } | ||
|
|
||
| public static void checkCPUTimeForContractHashFields() { | ||
| if (ForkController.instance().pass(Parameter.ForkBlockVersionEnum.VERSION_4_8_2_2)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the activation check intentionally kept inside this helper so that the call site can remain unconditional once the persistence path is reached, with the helper becoming a no-op before activation?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. Keeping the activation decision in the helper centralizes the fork-dependent behavior. Before activation it returns normally, and after activation it applies the new restriction. |
||
| throw new OutOfTimeException("CPU timeout for contract hash fields"); | ||
| } | ||
| } | ||
|
|
||
| public static void checkCPUTime() { | ||
| if (ForkController.instance().pass(Parameter.ForkBlockVersionEnum.VERSION_4_7_1)) { | ||
| throw new OutOfTimeException("CPU timeout for 0x0a executing"); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| package org.tron.core.actuator; | ||
|
|
||
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertThrows; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| import com.google.protobuf.ByteString; | ||
| import org.junit.Test; | ||
| import org.mockito.MockedStatic; | ||
| import org.mockito.Mockito; | ||
| import org.tron.common.utils.ForkController; | ||
| import org.tron.core.config.Parameter.ForkBlockVersionEnum; | ||
| import org.tron.core.vm.program.Program.OutOfTimeException; | ||
| import org.tron.protos.contract.SmartContractOuterClass.SmartContract; | ||
|
|
||
| public class ContractHashValidationTest { | ||
|
|
||
| @Test | ||
| public void acceptsHashFieldsBeforeActivation() { | ||
| SmartContract contract = SmartContract.newBuilder() | ||
| .setCodeHash(ByteString.copyFromUtf8("code")) | ||
| .setTrxHash(ByteString.copyFromUtf8("transaction")) | ||
| .build(); | ||
|
|
||
| runWithActivation(false, () -> VMActuator.checkContractHashFields(contract)); | ||
| } | ||
|
|
||
| @Test | ||
| public void rejectsCodeHashAfterActivation() { | ||
| SmartContract contract = SmartContract.newBuilder() | ||
| .setCodeHash(ByteString.copyFromUtf8("code")) | ||
| .build(); | ||
|
|
||
| OutOfTimeException exception = assertThrows(OutOfTimeException.class, | ||
| () -> runWithActivation(true, () -> VMActuator.checkContractHashFields(contract))); | ||
|
|
||
| assertEquals("CPU timeout for contract hash fields", exception.getMessage()); | ||
| } | ||
|
|
||
| @Test | ||
| public void rejectsTransactionHashAfterActivation() { | ||
| SmartContract contract = SmartContract.newBuilder() | ||
| .setTrxHash(ByteString.copyFromUtf8("transaction")) | ||
| .build(); | ||
|
|
||
| OutOfTimeException exception = assertThrows(OutOfTimeException.class, | ||
| () -> runWithActivation(true, () -> VMActuator.checkContractHashFields(contract))); | ||
|
|
||
| assertEquals("CPU timeout for contract hash fields", exception.getMessage()); | ||
| } | ||
|
|
||
| @Test | ||
| public void acceptsEmptyHashFieldsAfterActivation() { | ||
| runWithActivation(true, | ||
| () -> VMActuator.checkContractHashFields(SmartContract.getDefaultInstance())); | ||
| } | ||
|
|
||
| private void runWithActivation(boolean activated, Runnable action) { | ||
| ForkController controller = mock(ForkController.class); | ||
| when(controller.pass(ForkBlockVersionEnum.VERSION_4_8_2_2)).thenReturn(activated); | ||
| try (MockedStatic<ForkController> controllerMock = Mockito.mockStatic(ForkController.class)) { | ||
| controllerMock.when(ForkController::instance).thenReturn(controller); | ||
| action.run(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question: since
create()already unpacked thisCreateSmartContractfromtrx, does callinggetSmartContractFromTransactionagain here add any measurable cost on the deployment path?