Skip to content
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

Fix gas cost enforcement for constructors and make --enforce-gas-cost-min-value actually work #12282

Merged
merged 5 commits into from
Nov 22, 2021
Merged

Fix gas cost enforcement for constructors and make --enforce-gas-cost-min-value actually work #12282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cf52827
Fix gas mismatch in constructor not being counted as a failure
cameel Nov 15, 2021
077b3a0
Fix `--enforce-gas-cost-min-value` being applied only when the cost e…
cameel Nov 15, 2021
f42180e
Rename gas constants in ExecutionFramework
cameel Nov 15, 2021
032f2d8
Update gas expectations
cameel Nov 15, 2021
27dc77b
Fix typo in test name: ihneritance -> inheritance
cameel Nov 15, 2021
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
6 changes: 3 additions & 3 deletions test/ExecutionFramework.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,15 +182,15 @@ void ExecutionFramework::sendMessage(bytes const& _data, bool _isCreation, u256
message.kind = EVMC_CALL;
message.destination = EVMHost::convertToEVMC(m_contractAddress);
}
message.gas = m_gas.convert_to<int64_t>();
message.gas = InitialGas.convert_to<int64_t>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Constants should be either full caps or just regular variables. UpperCamelCase is reserved for types.

Copy link
Member Author

Choose a reason for hiding this comment

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

So how about full caps? Can we start using that everywhere? The m_ was especially misleading in this case but I often find myself wishing we had some convention for constants to easily distinguish them from normal variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the coding style guide says c_ for constants.

Copy link
Member Author

@cameel cameel Nov 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately it does not. Should that be preferred then? You mentioned that convention before but I don't remember seeing it actually used anywhere in the code base.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, we had that once but removed it because we don't really do it for local variables.


evmc::result result = m_evmcHost->call(message);

m_output = bytes(result.output_data, result.output_data + result.output_size);
if (_isCreation)
m_contractAddress = EVMHost::convertFromEVMC(result.create_address);

m_gasUsed = m_gas - result.gas_left;
m_gasUsed = InitialGas - result.gas_left;
m_transactionSuccessful = (result.status_code == EVMC_SUCCESS);

if (m_showMessages)
Expand All @@ -216,7 +216,7 @@ void ExecutionFramework::sendEther(h160 const& _addr, u256 const& _amount)
message.value = EVMHost::convertToEVMC(_amount);
message.kind = EVMC_CALL;
message.destination = EVMHost::convertToEVMC(_addr);
message.gas = m_gas.convert_to<int64_t>();
message.gas = InitialGas.convert_to<int64_t>();

m_evmcHost->call(message);
}
Expand Down
5 changes: 3 additions & 2 deletions test/ExecutionFramework.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,9 @@ class ExecutionFramework
}

protected:
u256 const GasPrice = 10 * gwei;
u256 const InitialGas = 100000000;

void selectVM(evmc_capabilities _cap = evmc_capabilities::EVMC_CAPABILITY_EVM1);
void reset();

Expand Down Expand Up @@ -302,8 +305,6 @@ class ExecutionFramework
bool m_transactionSuccessful = true;
util::h160 m_sender = account(0);
util::h160 m_contractAddress;
u256 const m_gasPrice = 10 * gwei;
u256 const m_gas = 100000000;
bytes m_output;
u256 m_gasUsed;
};
Expand Down
11 changes: 7 additions & 4 deletions test/libsolidity/SemanticTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,10 @@ TestCase::TestResult SemanticTest::runTest(
if (m_transactionSuccessful == test.call().expectations.failure)
success = false;
if (success && !checkGasCostExpectation(test, _isYulRun))
{
success = false;
m_gasCostFailure = true;
}

test.setFailure(!m_transactionSuccessful);
test.setRawBytes(bytes());
Expand Down Expand Up @@ -562,14 +565,14 @@ bool SemanticTest::checkGasCostExpectation(TestFunctionCall& io_test, bool _comp
// We don't check gas if enforce gas cost is not active
// or test is run with abi encoder v1 only
// or gas used less than threshold for enforcing feature
// or the test has used up all available gas (test will fail anyway)
// or setting is "ir" and it's not included in expectations
// or if the called function is an isoltest builtin e.g. `smokeTest` or `storageEmpty`
if (
!m_enforceGasCost ||
(
(setting == "ir" || m_gasUsed < m_enforceGasCostMinValue || m_gasUsed >= m_gas) &&
io_test.call().expectations.gasUsed.count(setting) == 0
) ||
m_gasUsed < m_enforceGasCostMinValue ||
m_gasUsed >= InitialGas ||
(setting == "ir" && io_test.call().expectations.gasUsed.count(setting) == 0) ||
io_test.call().kind == FunctionCall::Kind::Builtin
)
return true;
Expand Down
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/array/constant_var_as_array_length.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ contract C {
// compileViaYul: also
// ----
// constructor(): 1, 2, 3 ->
// gas irOptimized: 143598
// gas legacy: 183490
// gas legacyOptimized: 151938
// a(uint256): 0 -> 1
// a(uint256): 1 -> 2
// a(uint256): 2 -> 3
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/array/fixed_arrays_in_constructors.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,8 @@ contract Creator {
// compileViaYul: also
// ----
// constructor(): 1, 2, 3, 4 ->
// gas irOptimized: 132278
// gas legacy: 176789
// gas legacyOptimized: 129585
// r() -> 4
// ch() -> 3
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/constructor/bytes_in_constructors_unpacker.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,8 @@ contract Test {
// compileViaYul: also
// ----
// constructor(): 7, 0x40, 78, "abcdefghijklmnopqrstuvwxyzabcdef", "ghijklmnopqrstuvwxyzabcdefghijkl", "mnopqrstuvwxyz" ->
// gas irOptimized: 291443
// gas legacy: 309842
// gas legacyOptimized: 260801
// m_x() -> 7
// m_s() -> 0x20, 78, "abcdefghijklmnopqrstuvwxyzabcdef", "ghijklmnopqrstuvwxyzabcdefghijkl", "mnopqrstuvwxyz"
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/constructor/constructor_arguments_external.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,8 @@ contract Main {
// compileViaYul: also
// ----
// constructor(): "abc", true
// gas irOptimized: 112563
// gas legacy: 145838
// gas legacyOptimized: 104017
// getFlag() -> true
// getName() -> "abc"
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/constructor/constructor_static_array_argument.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ contract C {
// compileViaYul: also
// ----
// constructor(): 1, 2, 3, 4 ->
// gas irOptimized: 180731
// gas legacy: 221377
// gas legacyOptimized: 177671
// a() -> 1
// b(uint256): 0 -> 2
// b(uint256): 1 -> 3
Expand Down
1 change: 1 addition & 0 deletions test/libsolidity/semanticTests/constructor_inheritance_init_order.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ contract B is A {
// compileViaYul: true
// ----
// constructor() ->
// gas irOptimized: 122233
// y() -> 42
3 changes: 3 additions & 0 deletions .../constructor_ihneritance_init_order_2.sol → .../constructor_inheritance_init_order_2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,7 @@ contract B is A {
// compileViaYul: also
// ----
// constructor() ->
// gas irOptimized: 122233
// gas legacy: 135046
// gas legacyOptimized: 116176
// y() -> 42
2 changes: 2 additions & 0 deletions test/libsolidity/semanticTests/constructor_with_params.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,7 @@ contract C {
// compileViaYul: also
// ----
// constructor(): 2, 0 ->
// gas irOptimized: 104227
// gas legacy: 117158
// i() -> 2
// k() -> 0
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/constructor_with_params_diamond_inheritance.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ contract D is B, C {
// compileViaYul: also
// ----
// constructor(): 2, 0 ->
// gas irOptimized: 159542
// gas legacy: 170665
// gas legacyOptimized: 145396
// i() -> 2
// j() -> 2
// k() -> 1
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/constructor_with_params_inheritance.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,8 @@ contract D is C {
// compileViaYul: also
// ----
// constructor(): 2, 0 ->
// gas irOptimized: 124844
// gas legacy: 139250
// gas legacyOptimized: 119367
// i() -> 2
// k() -> 1
4 changes: 3 additions & 1 deletion test/libsolidity/semanticTests/events/event_emit_from_other_contract.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ contract C {
// compileViaYul: also
// ----
// constructor() ->
// gas legacy: 249112
// gas irOptimized: 177344
// gas legacy: 250376
// gas legacyOptimized: 174522
// deposit(bytes32), 18 wei: 0x1234 ->
// ~ emit Deposit(address,bytes32,uint256) from 0xf01f7809444bd9a93a854361c6fae3f23d9e23db: #0x0fdd67305928fcac8d213d1e47bfa6165cd0b87b, #0x1234, 0x00
2 changes: 1 addition & 1 deletion test/libsolidity/semanticTests/events/event_signature_in_library.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,4 @@ contract C {
// ----
// constructor()
// ~ emit E((uint8,int16),(uint8,int16)): #0xad3228b676f7d3cd4284a5443f17f1962b36e491b30a40b2405849e597ba5fb5, 0x00, 0x00
// gas legacy: 150662
// gas legacy: 150602
2 changes: 1 addition & 1 deletion test/libsolidity/semanticTests/externalContracts/FixedFeeRegistrar.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ contract FixedFeeRegistrar is Registrar {
// compileViaYul: also
// ----
// constructor()
// gas irOptimized: 425623
// gas irOptimized: 426283
// gas legacy: 936897
// gas legacyOptimized: 490983
// reserve(string), 69 ether: 0x20, 3, "abc" ->
Expand Down
6 changes: 3 additions & 3 deletions test/libsolidity/semanticTests/externalContracts/deposit_contract.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,9 +178,9 @@ contract DepositContract is IDepositContract, ERC165 {
// compileViaYul: also
// ----
// constructor()
// gas irOptimized: 1558013
// gas legacy: 2580394
// gas legacyOptimized: 1775403
// gas irOptimized: 1558001
// gas legacy: 2436584
// gas legacyOptimized: 1776483
// supportsInterface(bytes4): 0x0 -> 0
// supportsInterface(bytes4): 0xffffffff00000000000000000000000000000000000000000000000000000000 -> false # defined to be false by ERC-165 #
// supportsInterface(bytes4): 0x01ffc9a700000000000000000000000000000000000000000000000000000000 -> true # ERC-165 id #
Expand Down
4 changes: 2 additions & 2 deletions test/libsolidity/semanticTests/externalContracts/prbmath_signed.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ contract test {
// compileViaYul: also
// ----
// constructor()
// gas irOptimized: 1924584
// gas legacy: 2602700
// gas irOptimized: 1924392
// gas legacy: 2480887
// gas legacyOptimized: 1874490
// div(int256,int256): 3141592653589793238, 88714123 -> 35412542528203691288251815328
// gas irOptimized: 22137
Expand Down
2 changes: 1 addition & 1 deletion test/libsolidity/semanticTests/externalContracts/prbmath_unsigned.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ contract test {
// ----
// constructor()
// gas irOptimized: 1778342
// gas legacy: 2356230
// gas legacy: 2250130
// gas legacyOptimized: 1746528
// div(uint256,uint256): 3141592653589793238, 88714123 -> 35412542528203691288251815328
// gas irOptimized: 22004
Expand Down
2 changes: 1 addition & 1 deletion test/libsolidity/semanticTests/externalContracts/ramanujan_pi.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ contract test {
// ----
// constructor()
// gas irOptimized: 465357
// gas legacy: 733634
// gas legacy: 672749
// gas legacyOptimized: 479606
// prb_pi() -> 3141592656369545286
// gas irOptimized: 57478
Expand Down
2 changes: 1 addition & 1 deletion test/libsolidity/semanticTests/externalContracts/strings.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ contract test {
// ----
// constructor()
// gas irOptimized: 702619
// gas legacy: 1188228
// gas legacy: 1130761
// gas legacyOptimized: 750416
// toSlice(string): 0x20, 11, "hello world" -> 11, 0xa0
// gas irOptimized: 22660
Expand Down
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/functionCall/creation_function_call_with_args.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,7 @@ contract D {
// compileViaYul: also
// ----
// constructor(): 2 ->
// gas irOptimized: 200295
// gas legacy: 245842
// gas legacyOptimized: 195676
// f() -> 2
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/functionCall/creation_function_call_with_salt.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,7 @@ contract D {
// compileViaYul: also
// ----
// constructor(): 2 ->
// gas irOptimized: 200458
// gas legacy: 246202
// gas legacyOptimized: 195914
// f() -> 2
3 changes: 2 additions & 1 deletion test/libsolidity/semanticTests/functionCall/external_call_to_nonexisting.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ contract C {
// compileViaYul: also
// ----
// constructor(), 1 ether ->
// gas irOptimized: 308423
// gas legacy: 465314
// gas legacyOptimized: 510004
// gas legacyOptimized: 304481
// f(uint256): 0 -> FAILURE
// f(uint256): 1 -> FAILURE
// f(uint256): 2 -> FAILURE
Expand Down
2 changes: 2 additions & 0 deletions test/libsolidity/semanticTests/functionCall/external_call_to_nonexisting_debugstrings.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ contract C {
// revertStrings: debug
// ----
// constructor(), 1 ether ->
// gas irOptimized: 448383
// gas legacy: 834272
// gas legacyOptimized: 510004
// f(uint256): 0 -> FAILURE, hex"08c379a0", 0x20, 37, "Target contract does not contain", " code"
// f(uint256): 1 -> FAILURE, hex"08c379a0", 0x20, 37, "Target contract does not contain", " code"
Expand Down
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/functionCall/gas_and_value_basic.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ contract test {
// compileViaYul: also
// ----
// constructor(), 20 wei ->
// gas irOptimized: 285350
// gas legacy: 402654
// gas legacyOptimized: 274470
// sendAmount(uint256): 5 -> 5
// outOfGas() -> FAILURE # call to helper should not succeed but amount should be transferred anyway #
// checkState() -> false, 15
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/functionCall/gas_and_value_brace_syntax.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ contract test {
// compileViaYul: also
// ----
// constructor(), 20 wei ->
// gas irOptimized: 285350
// gas legacy: 402654
// gas legacyOptimized: 274470
// sendAmount(uint256): 5 -> 5
// outOfGas() -> FAILURE # call to helper should not succeed but amount should be transferred anyway #
// checkState() -> false, 15
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/functionCall/send_zero_ether.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,7 @@ contract Main {
// compileViaYul: also
// ----
// constructor(), 20 wei ->
// gas irOptimized: 102862
// gas legacy: 116691
// gas legacyOptimized: 100361
// s() -> true
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/immutable/use_scratch.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,8 @@ contract C {
// compileViaYul: also
// ----
// constructor(): 3 ->
// gas irOptimized: 137184
// gas legacy: 209361
// gas legacyOptimized: 139324
// f() -> 84, 23
// m(uint256): 3 -> 7
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/inheritance/value_for_constructor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ contract Main {
// compileViaYul: also
// ----
// constructor(), 22 wei ->
// gas irOptimized: 288778
// gas legacy: 402045
// gas legacyOptimized: 266772
// getFlag() -> true
// getName() -> "abc"
// getBalances() -> 12, 10
6 changes: 3 additions & 3 deletions test/libsolidity/semanticTests/isoltestTesting/balance_other_contract.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ contract ClientReceipt {
// compileViaYul: also
// ----
// constructor(), 2000 wei ->
// gas irOptimized: 191881
// gas legacy: 235167
// gas legacyOptimized: 180756
// gas irOptimized: 184076
// gas legacy: 235195
// gas legacyOptimized: 176766
// balance -> 1500
// gas irOptimized: 191881
// gas legacy: 235167
Expand Down
4 changes: 3 additions & 1 deletion test/libsolidity/semanticTests/smoke/constructor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ contract C {
// compileViaYul: also
// ----
// constructor(), 2 wei: 3 ->
// gas legacy: 148000
// gas irOptimized: 111723
// gas legacy: 151416
// gas legacyOptimized: 108388
// state() -> 3
// balance() -> 2
// balance -> 2
Expand Down
3 changes: 3 additions & 0 deletions test/libsolidity/semanticTests/state/blockhash_basic.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ contract C {
// compileViaYul: also
// ----
// constructor()
// gas irOptimized: 119839
// gas legacy: 155081
// gas legacyOptimized: 107997
// genesisHash() -> 0x3737373737373737373737373737373737373737373737373737373737373737
// currentHash() -> 0
// f(uint256): 0 -> 0x3737373737373737373737373737373737373737373737373737373737373737
Expand Down
2 changes: 1 addition & 1 deletion test/libsolidity/semanticTests/userDefinedValueType/erc20.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ contract ERC20 {
// constructor()
// ~ emit Transfer(address,address,uint256): #0x00, #0x1212121212121212121212121212120000000012, 0x14
// gas irOptimized: 442239
// gas legacy: 861547
// gas legacy: 861559
// gas legacyOptimized: 420959
// totalSupply() -> 20
// gas irOptimized: 23415
Expand Down
5 changes: 4 additions & 1 deletion test/libsolidity/semanticTests/various/address_code.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ contract C {
function h() public view returns (uint) { return address(1).code.length; }
}
// ====
// compileViaYul: also
// compileToEwasm: also
// compileViaYul: also
// ----
// constructor() ->
// gas irOptimized: 199687
// gas legacy: 241124
// gas legacyOptimized: 155549
// initCode() -> 0x20, 0
// f() -> true
// g() -> 0
Expand Down
3 changes: 2 additions & 1 deletion test/libsolidity/semanticTests/various/code_length.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,9 @@ contract C {
}
}
// ====
// compileViaYul: also
// compileToEwasm: also
// compileViaYul: also
// ----
// constructor()
// gas legacy: 126455
// f(): true, true -> true, true
1 change: 1 addition & 0 deletions test/libsolidity/semanticTests/various/codebalance_assembly.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ contract C {
// compileViaYul: also
// ----
// constructor(), 23 wei ->
// gas legacy: 100517
// f() -> 0
// g() -> 1
// h() -> 23
Loading