[evm] panic on unrecoverable error #4178

dustinxie · 2024-03-12T06:27:23Z

Description

revert to the same snapshot version again should cause a panic(). make it the same behavior as in ethereum
panic on unrecoverable errors (like store state/account fail)

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)

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

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

codecov · 2024-03-12T06:43:24Z

Codecov Report

Attention: Patch coverage is 35.13514% with 48 lines in your changes are missing coverage. Please review.

Project coverage is 76.51%. Comparing base (e1f0636) to head (d716bb9).
Report is 224 commits behind head on master.

❗ Current head d716bb9 differs from pull request most recent head 7a8f280. Consider uploading reports for the commit 7a8f280 to get more accurate results

Files	Patch %	Lines
action/protocol/execution/evm/evmstatedbadapter.go	35.21%	20 Missing and 26 partials ⚠️
action/protocol/execution/evm/evm.go	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4178      +/-   ##
==========================================
+ Coverage   75.38%   76.51%   +1.12%     
==========================================
  Files         303      340      +37     
  Lines       25923    29273    +3350     
==========================================
+ Hits        19541    22397    +2856     
- Misses       5360     5761     +401     
- Partials     1022     1115      +93

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CoderZhi · 2024-03-12T16:32:01Z

action/protocol/context.go

@@ -263,6 +264,7 @@ func WithFeatureCtx(ctx context.Context) context.Context {
 			CandidateRegisterMustWithStake:          !g.IsToBeEnabled(height),
 			DisableDelegateEndorsement:              !g.IsToBeEnabled(height),
 			SuicideTxLogMismatchPanic:               g.IsToBeEnabled(height),
+			PanicOnDuplicateRevert:                  g.IsToBeEnabled(height),


default value (false) as the value after hard fork

ok, to me using a positive (enable after height) flag name is more natural, which help improve code readability and reduce chance of programming mistake, IMO that's more valuable than keeping the default value = false after hard-fork

readability is related to https://github.com/iotexproject/iotex-core/pull/4178/files#diff-8d0192f25f9daf6c1bfa70c3b75949444fe5d1d47193e6b7c1210c39a19ed52aR409, but not the default value in the feature context

CoderZhi · 2024-03-12T16:45:27Z

action/protocol/execution/evm/evmstatedbadapter.go

@@ -561,16 +570,19 @@ func (stateDB *StateDBAdapter) Empty(evmAddr common.Address) bool {

 // RevertToSnapshot reverts the state factory to the state at a given snapshot
 func (stateDB *StateDBAdapter) RevertToSnapshot(snapshot int) {
+	ds, ok := stateDB.suicideSnapshot[snapshot]
+	if !ok && stateDB.panicOnDuplicateRevert {
+		log.L().Panic("Failed to revert to snapshot.", zap.Int("snapshot", snapshot))


according to offline discussion, this won't happen. Thus, hard fork may not be needed. Add a TODO if you insist.

Two more places we need to handle, especially the second one, which was observed before.
https://github.com/iotexproject/iotex-core/pull/4178/files#diff-d4f14fe3911e27971891bedc8bb75529dfdad400cf2beaf785e93e81f1746fc0R581
https://github.com/iotexproject/iotex-core/pull/4178/files#diff-d4f14fe3911e27971891bedc8bb75529dfdad400cf2beaf785e93e81f1746fc0R653

for safety add hard-fork, can remove after hard-fork, added TODO
Two more places:

the first will be taken care of by this change? after hard-fork it will panic, code won't be able to reach here

the second is related to fixSnapshotOrder flag (which is mostly fixed by trie.db.patch), but I think there are still some cases that error will happen (mostly seen in 13-16m block heights). so we should keep the code as-is, or what is your suggestion here?

adding TODO is fine

if we believe so, using log.Panic instead of log.Error?

We need a follow up issue to capture all the cases and fix them

CoderZhi · 2024-03-13T03:16:01Z

action/protocol/context.go

@@ -263,6 +264,7 @@ func WithFeatureCtx(ctx context.Context) context.Context {
 			CandidateRegisterMustWithStake:          !g.IsToBeEnabled(height),
 			DisableDelegateEndorsement:              !g.IsToBeEnabled(height),
 			SuicideTxLogMismatchPanic:               g.IsToBeEnabled(height),
+			PanicOnDuplicateRevert:                  g.IsToBeEnabled(height),


readability is related to https://github.com/iotexproject/iotex-core/pull/4178/files#diff-8d0192f25f9daf6c1bfa70c3b75949444fe5d1d47193e6b7c1210c39a19ed52aR409, but not the default value in the feature context

CoderZhi · 2024-03-13T03:18:04Z

action/protocol/execution/evm/evmstatedbadapter.go

@@ -561,16 +570,19 @@ func (stateDB *StateDBAdapter) Empty(evmAddr common.Address) bool {

 // RevertToSnapshot reverts the state factory to the state at a given snapshot
 func (stateDB *StateDBAdapter) RevertToSnapshot(snapshot int) {
+	ds, ok := stateDB.suicideSnapshot[snapshot]
+	if !ok && stateDB.panicOnDuplicateRevert {
+		log.L().Panic("Failed to revert to snapshot.", zap.Int("snapshot", snapshot))


adding TODO is fine

if we believe so, using log.Panic instead of log.Error?

We need a follow up issue to capture all the cases and fix them

envestcc · 2024-03-28T02:45:28Z

action/protocol/execution/evm/evmstatedbadapter.go

@@ -220,14 +241,11 @@ func (stateDB *StateDBAdapter) accountCreationOpts() []state.AccountCreationOpti
 // CreateAccount creates an account in iotx blockchain
 func (stateDB *StateDBAdapter) CreateAccount(evmAddr common.Address) {
 	addr, err := address.FromBytes(evmAddr.Bytes())
-	if err != nil {
-		log.L().Error("Failed to convert evm address.", zap.Error(err))
+	if stateDB.assertError(err, "Failed to convert evm address.", zap.Error(err)) {


it will call stateDB.logError(err) in new behaviours, does it matters?

it does not matter much. stateDB.logError(err) just updates the most recent error of EVM, does not affect consensus

envestcc · 2024-03-28T02:50:54Z

action/protocol/execution/evm/evmstatedbadapter.go

+	ds, ok := stateDB.suicideSnapshot[snapshot]
+	if !ok && stateDB.panicUnrecoverableError {
+		log.L().Panic("Failed to revert to snapshot.", zap.Int("snapshot", snapshot))
+	}
 	if err := stateDB.sm.Revert(snapshot); err != nil {


panic if it returns error?

dustinxie · 2024-03-29T01:26:37Z

action/protocol/execution/evm/evmstatedbadapter.go

@@ -499,11 +499,11 @@ func (stateDB *StateDBAdapter) Exist(evmAddr common.Address) bool {
 		return true
 	}
 	recorded, err := accountutil.Recorded(stateDB.sm, addr)
-	if !recorded {
-		log.L().Debug("Account does not exist.", zap.String("address", addr.String()))
+	if stateDB.assertError(err, "Account does not exist.", zap.Error(err), zap.String("address", evmAddr.Hex())) {


if error happens, it returns false, err, so should check error first

CoderZhi · 2024-04-06T06:54:43Z

action/protocol/execution/evm/evmstatedbadapter.go

-	if err != nil {
-		log.L().Error("Failed to create account.", zap.Error(err))
-		stateDB.logError(err)
+	if stateDB.assertError(err, "Failed to create account.", zap.Error(err), zap.String("address", evmAddr.Hex())) {


zap.Error(err) could be found in all places which calls stateDB.assertError(, thus, it could be added inside of the function given err.

it's possible, yet inside assertError(), would have to manually add it to the beginning of ...fields

var f []zap.Field f = append(zap.Error(err), fields...) log.L().Panic(msg, f)

which is kind of inefficient
so would keep it as-is for now, can refactor if we feel there's a need later

sonarcloud · 2024-04-09T17:17:31Z

Quality Gate failed

Failed conditions
8.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

dustinxie requested review from CoderZhi, Liuhaai and envestcc as code owners March 12, 2024 06:27

envestcc approved these changes Mar 12, 2024

View reviewed changes

CoderZhi reviewed Mar 12, 2024

View reviewed changes

CoderZhi approved these changes Mar 13, 2024

View reviewed changes

dustinxie force-pushed the enter branch from 6e1f529 to d716bb9 Compare March 20, 2024 00:57

dustinxie changed the title ~~[evm] panic on duplicate revert version~~ [evm] panic on unrecoverable error Mar 20, 2024

envestcc reviewed Mar 28, 2024

View reviewed changes

millken approved these changes Mar 28, 2024

View reviewed changes

dustinxie commented Mar 29, 2024

View reviewed changes

CoderZhi reviewed Apr 6, 2024

View reviewed changes

dustinxie added 2 commits April 9, 2024 10:04

[evm] panic on duplicate revert version

3184c44

[evm] panic on unrecoverable error (iotexproject#4178)

7a8f280

dustinxie force-pushed the enter branch from 562d2d7 to 7a8f280 Compare April 9, 2024 17:16

dustinxie merged commit 7a8f280 into iotexproject:master Apr 9, 2024
2 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[evm] panic on unrecoverable error #4178

[evm] panic on unrecoverable error #4178

dustinxie commented Mar 12, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading

CoderZhi Mar 12, 2024

dustinxie Mar 12, 2024 •

edited

Loading

CoderZhi Mar 13, 2024

CoderZhi Mar 12, 2024

dustinxie Mar 12, 2024

CoderZhi Mar 13, 2024

CoderZhi Mar 13, 2024

CoderZhi Mar 13, 2024

envestcc Mar 28, 2024

dustinxie Mar 28, 2024

envestcc Mar 28, 2024

dustinxie Mar 29, 2024

CoderZhi Apr 6, 2024

dustinxie Apr 9, 2024 •

edited

Loading

sonarcloud bot commented Apr 9, 2024

[evm] panic on unrecoverable error #4178

[evm] panic on unrecoverable error #4178

Conversation

dustinxie commented Mar 12, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Mar 12, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

dustinxie Mar 12, 2024 • edited Loading

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

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

dustinxie Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Apr 9, 2024

Quality Gate failed

dustinxie commented Mar 12, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading

dustinxie Mar 12, 2024 •

edited

Loading

dustinxie Apr 9, 2024 •

edited

Loading