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

Bug/skaled 1745 invalid fork in pow gas #1768

Merged
merged 20 commits into from
Jan 10, 2024

Conversation

dimalit
Copy link
Contributor

@dimalit dimalit commented Dec 19, 2023

Changes:
1 Transaction::checkOutExternalGas() now operates on current Ethereum fork, instead of Constantinople (base gas for 1 byte of data changed from 68 gas/b to 16 gas/b)
2 All changes are activated after correctForkInPowPatchTimestamp. This is true both for external gas check and for eth_estimateGas call.
3 Transaction::checkOutExternalGas() is called one additional time after consensus (needed to be sure that patch activates smoothly). This make possible little decrease of block processing performance (but unlikely).

Testing:
JsonRpcSuite/simplePoWTransaction tests that PoW transaction with incorrect amount of gas fails before patch timestamp and works after it

@dimalit dimalit linked an issue Dec 19, 2023 that may be closed by this pull request
@dimalit dimalit marked this pull request as ready for review December 20, 2023 18:47
Copy link

codecov bot commented Dec 23, 2023

Codecov Report

Attention: 37 lines in your changes are missing coverage. Please review.

Comparison is base (6183faf) 45.52% compared to head (a93c503) 45.56%.
Report is 13 commits behind head on v3.18.0.

❗ Current head a93c503 differs from pull request most recent head 9f1de09. Consider uploading reports for the commit 9f1de09 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           v3.18.0    #1768      +/-   ##
===========================================
+ Coverage    45.52%   45.56%   +0.04%     
===========================================
  Files          356      358       +2     
  Lines        51709    51807      +98     
===========================================
+ Hits         23542    23608      +66     
- Misses       28167    28199      +32     

libethereum/Transaction.cpp Outdated Show resolved Hide resolved
libethereum/Transaction.cpp Show resolved Hide resolved
test/tools/libtesteth/ImportTest.cpp Outdated Show resolved Hide resolved
@dimalit dimalit force-pushed the bug/SKALED-1745-invalid-fork-in-pow-gas branch from 11118c9 to 2feff86 Compare December 28, 2023 16:19
Copy link
Collaborator

@DmytroNazarenko DmytroNazarenko left a comment

Choose a reason for hiding this comment

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

  1. Please, add patch timestamp to estimateGas
  2. Update description with details about new patch timestamp

olehnikolaiev
olehnikolaiev previously approved these changes Jan 10, 2024
@olehnikolaiev olehnikolaiev self-requested a review January 10, 2024 15:16
@DmytroNazarenko DmytroNazarenko merged commit 7377dc7 into v3.18.0 Jan 10, 2024
7 checks passed
@DmytroNazarenko DmytroNazarenko deleted the bug/SKALED-1745-invalid-fork-in-pow-gas branch January 10, 2024 15:19
@github-actions github-actions bot locked and limited conversation to collaborators Jan 10, 2024
@dimalit dimalit restored the bug/SKALED-1745-invalid-fork-in-pow-gas branch January 10, 2024 16:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid gas computation/fork block in checkOutEternalGas
3 participants