[evm] manually correct gas refund in case opcode execution returns Er… #3690

dustinxie · 2022-11-08T04:08:53Z

…rWriteProtection

Description

as title

Fixes #(issue)

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
[] New feature (non-breaking change which adds functionality)
[] Code refactor or improvement
[] Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)
[] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

make test
fullsync
[] Other test (please specify)

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

dustinxie · 2022-11-08T04:10:23Z

go.mod

@@ -200,6 +200,6 @@ require (
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )

-replace github.com/ethereum/go-ethereum => github.com/iotexproject/go-ethereum v0.4.1
+replace github.com/ethereum/go-ethereum => github.com/iotexproject/go-ethereum v1.7.4-0.20221107225611-7cbc562f7437


iotexproject/go-ethereum@9c1b37b

once verified fullsync, it will become a tag/version

it seems i cannot create a PR using this commit ( atari branch against v0.4.1 tag)

codecov · 2022-11-08T04:17:34Z

Codecov Report

Merging #3690 (0352587) into master (263ff82) will decrease coverage by 0.50%.
The diff coverage is 84.06%.

@@            Coverage Diff             @@
##           master    #3690      +/-   ##
==========================================
- Coverage   74.47%   73.97%   -0.51%     
==========================================
  Files         269      291      +22     
  Lines       23925    24691     +766     
==========================================
+ Hits        17818    18265     +447     
- Misses       5174     5494     +320     
+ Partials      933      932       -1

Impacted Files	Coverage Δ
action/protocol/context.go	`66.40% <0.00%> (-1.60%)`	⬇️
action/protocol/rewarding/reward.go	`90.03% <ø> (-0.04%)`	⬇️
action/protocol/staking/candidate_statereader.go	`53.94% <ø> (-3.38%)`	⬇️
action/protocol/types.go	`0.00% <0.00%> (ø)`
api/web3server.go	`27.59% <0.00%> (-34.51%)`	⬇️
db/db_bolt.go	`68.53% <0.00%> (-3.60%)`	⬇️
ioctl/newcmd/action/actiontransfer.go	`100.00% <ø> (ø)`
ioctl/newcmd/action/stake2withdraw.go	`95.74% <ø> (ø)`
ioctl/newcmd/action/xrc20transferfrom.go	`100.00% <ø> (ø)`
ioctl/newcmd/did/didgethash.go	`100.00% <ø> (ø)`
... and 44 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…rWriteProtection

Liuhaai · 2022-11-14T17:59:17Z

lgtm, let me know if the testings pass

millken · 2022-11-24T02:26:54Z

lgtm, let me know if the testings pass

fullsync passed

dustinxie · 2022-11-30T02:23:13Z

action/protocol/execution/protocol_test.go


+func TestSmartContractTest(t *testing.T) {


can move to a t.Run() like those above?

this is a debug code, has been removed

CoderZhi

i am still not convinced that the unit tests cover all cases. try your best to enrich the unit tests if possible.

CoderZhi · 2022-11-29T22:04:44Z

action/protocol/execution/testdata-istanbul/write-protection-002.json

+        "expectedLogs": [{}],
+        "hasReturnValue": true,
+        "rawReturnValue": "",
+        "comment": "call make"


wrong comment

CoderZhi · 2022-11-29T22:04:58Z

action/protocol/execution/testdata-istanbul/write-protection-001.json

+        "expectedLogs": [{},{}],
+        "hasReturnValue": true,
+        "rawReturnValue": "",
+        "comment": "call make"


wrong comment

CoderZhi · 2022-11-29T22:06:28Z

action/protocol/execution/testdata-istanbul/write-protection-003.json

+        "expectedLogs": [{}],
+        "hasReturnValue": true,
+        "rawReturnValue": "",
+        "comment": "call make"


wrong comment

CoderZhi · 2022-11-29T22:12:35Z

action/protocol/execution/testdata-istanbul/write-protection-004.json

+        "expectedLogs": [{}],
+        "hasReturnValue": true,
+        "rawReturnValue": "",
+        "comment": "call make"


wrong comment

CoderZhi · 2022-11-30T19:07:49Z

action/protocol/execution/evm/evm.go

-		refundLastSnapshot = stateDB.RefundAtLastSnapshot()
-		currentRefund      = stateDB.GetRefund()
-		featureCtx         = protocol.MustGetFeatureCtx(ctx)
+		deltaRefundByDynamicGas = evm.DeltaRefundByDynamicGas


txcontext is not an ideal place to store the deltarefund. StateDB should be the only place to interact with evm from outside. we may add an interface to set delta refund value. Consequently, we can all stateDB.GetRefundWithDelta, and do not to fetch feature context here.

the interface (SetDelta, GetRefundWithDelta) needs to be implemented in go-ethereum and iotex-core, but not necessary for go-ethereum. the current deltaRefund is just a temporary variable, I think there is no problem.

TxContext.Origin and TxContext.GasPrice are also used in our iotex-core code, so i think it's fine to use it

sonarcloud · 2022-12-02T09:45:27Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
7.3% Duplication

dustinxie requested a review from a team as a code owner November 8, 2022 04:08

dustinxie commented Nov 8, 2022

View reviewed changes

dustinxie force-pushed the testing branch from 6b0a73c to fd5a1a0 Compare November 9, 2022 05:23

[evm] manually correct gas refund in case opcode execution returns Er…

4bce536

…rWriteProtection

dustinxie force-pushed the testing branch from fd5a1a0 to 4bce536 Compare November 12, 2022 05:25

millken added 3 commits November 23, 2022 12:04

Merge branch 'master' into testing

19e66ce

fix gas refund, update tests

bb3c901

fix gas refund, update tests

c9eb62b

fix gas refund on testnet 12117999

d2cb675

Liuhaai approved these changes Nov 28, 2022

View reviewed changes

raullenchai approved these changes Nov 28, 2022

View reviewed changes

millken added 4 commits November 29, 2022 09:54

Merge branch 'master' into testing

6b40bfe

Merge branch 'master' into testing

b1aaf5c

update testdata

323cf28

fix tests

b6d04fc

dustinxie commented Nov 30, 2022

View reviewed changes

Merge branch 'master' into testing

f696353

CoderZhi reviewed Nov 30, 2022

View reviewed changes

millken added 5 commits December 1, 2022 09:50

Merge branch 'master' into testing

adc64f7

fix comment in json

86b5d52

add more test cases

baf3c27

add more test cases

c5a64f6

Merge branch 'master' into testing

0352587

dustinxie merged commit 31371a9 into master Dec 2, 2022

dustinxie mentioned this pull request Dec 5, 2022

[evm] manually correct gas refund in case opcode execution returns Er… #3655

Closed

11 tasks

millken mentioned this pull request Dec 8, 2022

add more cases for gas refund #3707

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[evm] manually correct gas refund in case opcode execution returns Er… #3690

[evm] manually correct gas refund in case opcode execution returns Er… #3690

dustinxie commented Nov 8, 2022

dustinxie Nov 8, 2022 •

edited

Loading

dustinxie Nov 8, 2022

codecov bot commented Nov 8, 2022 •

edited

Loading

Liuhaai commented Nov 14, 2022

millken commented Nov 24, 2022

dustinxie Nov 30, 2022

millken Nov 30, 2022

CoderZhi left a comment

CoderZhi Nov 29, 2022

CoderZhi Nov 29, 2022

CoderZhi Nov 29, 2022

CoderZhi Nov 29, 2022

CoderZhi Nov 30, 2022

millken Dec 2, 2022

dustinxie Dec 2, 2022

sonarcloud bot commented Dec 2, 2022

[evm] manually correct gas refund in case opcode execution returns Er… #3690

[evm] manually correct gas refund in case opcode execution returns Er… #3690

Conversation

dustinxie commented Nov 8, 2022

Description

Type of change

How Has This Been Tested?

Checklist:

dustinxie Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 8, 2022 • edited Loading

Codecov Report

Liuhaai commented Nov 14, 2022

millken commented Nov 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoderZhi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 2, 2022

dustinxie Nov 8, 2022 •

edited

Loading

codecov bot commented Nov 8, 2022 •

edited

Loading