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.

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

Invalid gas computation/fork block in estimateGas #1600

Closed
dimalit opened this issue Jul 19, 2023 · 5 comments · Fixed by #1743
Closed

Invalid gas computation/fork block in estimateGas #1600

dimalit opened this issue Jul 19, 2023 · 5 comments · Fixed by #1743
Assignees
Labels
bug Something isn't working epic:eth-compatibility
Milestone

Comments

@dimalit
Copy link
Contributor

dimalit commented Jul 19, 2023

estimateGas uses DefaultSchedule:
int64_t lowerBound = Transaction::baseGasRequired( !_dest, &_data, EVMSchedule() ); (libethereum/ClientBase.cpp:118)
`
This can lead to overestimation of gas for transactions with non-zero data.

Example:

  1. Send simple transfer transaction with 54000 bytes of data.
  2. It should require 21000+54000*68=3.6M of gas - and this is checked during eth_sendRawTransaction call
  3. In fact, it will consume around 800k of gas.
  4. Reason: skaled uses coefficient 16, but estimateGas uses 68.

See also #1745

@kladkogex
Copy link
Collaborator

This will need to be done as a patch so the state roots do not diverge

@kladkogex kladkogex added this to the 2.3 milestone Jul 21, 2023
@PolinaKiporenko PolinaKiporenko modified the milestones: 2.3, SKALE 2.3 Jul 27, 2023
@PolinaKiporenko PolinaKiporenko added the bug Something isn't working label Jul 27, 2023
@PolinaKiporenko PolinaKiporenko moved this from To Do to Ready For Pickup in SKALE Engineering 🚀 Sep 8, 2023
@dimalit dimalit changed the title Invalid gas computation Invalid gas computation/fork block in estimateGas and checkOutEternalGas Nov 29, 2023
@PolinaKiporenko PolinaKiporenko moved this from Ready For Pickup to In Progress in SKALE Engineering 🚀 Nov 30, 2023
@dimalit
Copy link
Contributor Author

dimalit commented Nov 30, 2023

For QA: need test transaction ('data' field) that has incorrect gas estimation.

@dimalit
Copy link
Contributor Author

dimalit commented Nov 30, 2023

EvmSchedule chronology:

Default: exceptionalFailedCodeDeposit=true, haveDelegateCall = true, txCreateGas = 53000

Frontier: exceptionalFailedCodeDeposit=false, haveDelegateCall = false, txCreateGas = 21000

Homestead = Default

EIP150 = Homestead and: 150mode, extcodesizeGas = 700, extcodecopyGas = 700, balanceGas = 400, sloadGas = 200, callGas = 700, suicideGas = 5000

EIP158 = EIP150 and: 158mode, expByteGas = 50, maxCodeSize = 1024 * 64

Byzantium = EIP158 and: haveRevert, haveReturnData, haveStaticCall, blockRewardOverwrite = { 3 * ether };

EWASM = Byzantium and: maxCodeSize = std::numeric_limits< unsigned >::max

Constantinople = Byzantium and: haveCreate2,haveBitwiseShifting,haveExtcodehash,eip1283Mode,blockRewardOverwrite = { 2 * ether };

ConstantinopleFix = Constantinople and eip1283Mode=false

Istanbul = ConstantinopleFix and: txDataNonZeroGas = 16, sloadGas = 800, balanceGas = 700, extcodehashGas = 700,haveChainID = true, haveSelfbalance = true, sstoreUnchangedGas = 800;

@dimalit dimalit linked a pull request Dec 6, 2023 that will close this issue
@dimalit dimalit changed the title Invalid gas computation/fork block in estimateGas and checkOutEternalGas Invalid gas computation/fork block in estimateGas Dec 8, 2023
@dimalit dimalit moved this from In Progress to Code Review in SKALE Engineering 🚀 Dec 11, 2023
@github-project-automation github-project-automation bot moved this from Code Review to Ready For Release Candidate in SKALE Engineering 🚀 Dec 22, 2023
@DmytroNazarenko
Copy link
Collaborator

skaled: 3.18.0-beta.0
patch: correctForkInPowPatchTimestamp

@DmytroNazarenko DmytroNazarenko moved this from Ready For Release Candidate to Merged To Release Candidate in SKALE Engineering 🚀 Jan 23, 2024
@EvgeniyZZ EvgeniyZZ moved this from Merged To Release Candidate to QA in SKALE Engineering 🚀 Jan 23, 2024
@oleksandrSydorenkoJ
Copy link

Verified on Regression network
skaled: 3.18.0-beta.0
patch: correctForkInPowPatchTimestamp

Skale chain: aged-wry-finished-skale

Updated node
gas estimate is 872792 and equals to gasUsed 872792

2024-01-30 10:32:00.970: Current gas price = 100000
2024-01-30 10:32:00.970: Will call w3.eth.getTransactionCount()...
2024-01-30 10:32:01.380: User 0x71cBE3fedE33905d4D1Bf2Bd51f9d4A62375e659 balance is 999999984369999955943739400000
Index is  1521
TCNT is  1521
gas estimate is  872792
2024-01-30 10:32:02.408: Sending signed method call transaction...
2024-01-30 10:32:05.788: Result receipt: { blockHash: "0x35b06696f8976da85710d09284e1c476a0d3e4d49d6fe270caab1a9524634d19", blockNumber: 46494, contractAddress: null, cumulativeGasUsed: 872792, from: "0x71cbe3fede33905d4d1bf2bd51f9d4a62375e659", gasUsed: 872792, logs: [], logsBloom: "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", status: true, to: "0xa68f946090c600eda6f139783077ee802afeb990", transactionHash: "0xb7c742f0d4e568282acfe1c04f14a5c9de97f097705c43dfe964c2218bf88153", transactionIndex: 0}

not updated node (3.17.1):
gas estimate is 3641116 and it is bigger than gasUsed 872792

gas estimate is  3641116
2024-01-30 10:33:29.554: Sending signed method call transaction...
2024-01-30 10:33:39.076: Result receipt: { blockHash: "0x1d9196077b85080bf2368e4570fdd13365999a50a92d7bfe047c35afb8a1320b", blockNumber: 46501, contractAddress: null, cumulativeGasUsed: 872792, from: "0x71cbe3fede33905d4d1bf2bd51f9d4a62375e659", gasUsed: 872792, logs: [], logsBloom: "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", status: true, to: "0xa68f946090c600eda6f139783077ee802afeb990", transactionHash: "0x4e51301a93defad2c510e89dd173352773e591a49977169c7cd099acdcf4c41e", transactionIndex: 0}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working epic:eth-compatibility
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants