feat: refine contract deployment transaction validation - #6945
Conversation
| } | ||
|
|
||
| static void checkContractHashFields(SmartContract contract) { | ||
| if (!contract.getCodeHash().isEmpty() || !contract.getTrxHash().isEmpty()) { |
There was a problem hiding this comment.
Covering both fields with a single || and relying on proto3 isEmpty() correctly handles unset vs. empty — normal deployments are untouched.
There was a problem hiding this comment.
Yes, exactly. In proto3, an unset bytes field is exposed as ByteString.EMPTY, so isEmpty() treats unset and explicitly empty values consistently. Only a non-empty code_hash or trx_hash reaches the activation-gated restriction, leaving normal contract deployments unchanged.
d63f1cf to
410b33d
Compare
| if (VMConfig.allowTvmConstantinople()) { | ||
| CreateSmartContract createContract = | ||
| ContractCapsule.getSmartContractFromTransaction(trx); | ||
| checkContractHashFields(createContract.getNewContract()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| public static void checkCPUTimeForContractHashFields() { | ||
| if (ForkController.instance().pass(Parameter.ForkBlockVersionEnum.VERSION_4_8_2_2)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| result.spendEnergy(saveCodeEnergy); | ||
| if (VMConfig.allowTvmConstantinople()) { | ||
| CreateSmartContract createContract = | ||
| ContractCapsule.getSmartContractFromTransaction(trx); |
There was a problem hiding this comment.
Quick question: since create() already unpacked this CreateSmartContract from trx, does calling getSmartContractFromTransaction again here add any measurable cost on the deployment path?
There was a problem hiding this comment.
Thanks for checking. We do not have a separate microbenchmark for this parsing step, but it occurs only once when a contract deployment reaches the reaches" and is about to persist its code, rather than inside an execution loop.
Reading it from the original transaction also keeps the check tied to the fields actually supplied by the caller and avoids retaining additional state between the the validation and execution phases. Compared with contract initialization and code persistence writing, the additional protobuf unpacking cost is expected to be negligible.
There was a problem hiding this comment.
Thanks for the detailed explanation, nothing further from me on this line.
410b33d to
cd04a42
Compare
ouy95917
changed the title
code_hash and trx_hash fields passed in during contract deployment5 participants
Why is this needed?
This PR refines two existing validation behaviors for contract deployment transactions.
The
code_hashandtrx_hashfields supplied during contract deployment are unused. They do not participate in deployment processing or affect the deployment result. Since these fields have no functional meaning as deployment inputs, transactions carrying them should be preventively prohibited at the protocol level.Contract name length validation already exists, but its current calculation relies on
String.getBytes(), which depends on the system default charset. The calculation should use the original protobuf byte representation so that the same deployment transaction is evaluated consistently across environments.Historical transaction scan
We performed a complete scan of all historical transactions across the full chain history.
No historical contract deployment transaction was found with either
code_hashortrx_hashset. The number of historical transactions carrying either field is zero.This confirms that existing contract deployments have never relied on these fields. The restriction does not invalidate any historical transaction or alter existing user behavior.
Implementation
After activation:
code_hashortrx_hashis rejected using the existing timeout semantics immediately before its contract code is persisted.Before activation, the existing behavior is preserved.
Normal contract deployments do not populate the hash fields, and ordinary contract names are unaffected by the length-calculation adjustment.
Compatibility
Both changes are controlled by
VERSION_4_8_2_2, with an activation threshold of 70%.This PR currently uses the same fork version introduced by #6920. If the version identifier or activation parameters in #6920 change, this PR should be updated accordingly.
Tests
code_hashandtrx_hashbehavior before and after activation.