[evm] add transaction log for Suicide() #4171

dustinxie · 2024-03-07T00:43:14Z

Description

When Suicide() is called upon destruction, the contract's balance will be transferred to a beneficiary. This needs to be captured in a transaction log

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

millken · 2024-03-07T02:40:36Z

action/protocol/execution/evm/evmstatedbadapter.go

@@ -406,6 +412,17 @@ func (stateDB *StateDBAdapter) Suicide(evmAddr common.Address) bool {
 		stateDB.logError(err)
 		return false
 	}
+	// before calling Suicide, EVM will transfer the contract's balance to beneficiary
+	// need to create a transaction log on successful suicide
+	if len(stateDB.lastAddBalanceAmount.Bytes()) > 0 {


should be check if stateDB.lastAddBalanceAmount != nil && stateDB.lastAddBalanceAmount.Cmp(big.NewInt(0))>0 to prevent nil or negative amount

in AddBalance(), nil or negative amount will be caught and return error, in other words, lastAddBalanceAmount must be valid positive number here

add initialization of the value lastAddBalanceAmount in new to prevent being nil

millken · 2024-03-07T04:36:39Z

action/protocol/execution/evm/evmstatedbadapter.go

@@ -242,6 +245,7 @@ func (stateDB *StateDBAdapter) SubBalance(evmAddr common.Address, amount *big.In

 // AddBalance adds balance to account
 func (stateDB *StateDBAdapter) AddBalance(evmAddr common.Address, amount *big.Int) {
+	stateDB.lastAddBalanceAmount.SetUint64(0)


the change is only in AddBalance, does SubBalance also need it

CoderZhi · 2024-03-07T13:27:23Z

action/protocol/execution/evm/evmstatedbadapter.go

@@ -242,6 +245,7 @@ func (stateDB *StateDBAdapter) SubBalance(evmAddr common.Address, amount *big.In

 // AddBalance adds balance to account
 func (stateDB *StateDBAdapter) AddBalance(evmAddr common.Address, amount *big.Int) {
+	stateDB.lastAddBalanceAmount.SetUint64(0)


what's the purpose?

to clear the amount for each AddBalance(), consider the following:

AddBalance(4000)

AddBalance(0)

Inside Suicide, it calls AddBalance(4000) to beneficiary but failed, if the amount is not cleared, it will mistakenly think the amount match and issue a tx log with amount=4000, but the 4000 is in step 1 and is mistaken

CoderZhi · 2024-03-07T13:30:26Z

action/protocol/execution/evm/evmstatedbadapter.go

+	// need to create a transaction log on successful suicide
+	if len(stateDB.lastAddBalanceAmount.Bytes()) > 0 {
+		from, _ := address.FromBytes(evmAddr[:])
+		stateDB.addTransactionLogs(&action.TransactionLog{


if lastAddBalanceAmount is not set or the amount is different from balance, panic?

fixed in latest commit

envestcc

I think it is not a safe enough way to handle EVM upgrade in the future. If EVM has unexpected calls to AddBalance and SubBalance in the future, it would be difficult for us to detect.

Here is my proposal:

Basically, operations on AddBalance and SubBalance not triggered by MakeTransfer are unexpected behaviors in terms of recording transaction logs, and these operations should be append to a UnexpectedOpList.
Based on the current logic of the EVM code, when Suicide is called, we can remove unexpected AddBalance records with the same address and amount from the list, thereby generating a transaction log.
After the action is executed, we should check the UnexpectedOpList. If it is not empty, we should return an error or panic.

dustinxie · 2024-03-11T03:25:08Z

I think it is not a safe enough way to handle EVM upgrade in the future. If EVM has unexpected calls to AddBalance and SubBalance in the future, it would be difficult for us to detect.

Here is my proposal:

Basically, operations on AddBalance and SubBalance not triggered by MakeTransfer are unexpected behaviors in terms of recording transaction logs, and these operations should be append to a UnexpectedOpList.

Based on the current logic of the EVM code, when Suicide is called, we can remove unexpected AddBalance records with the same address and amount from the list, thereby generating a transaction log.

After the action is executed, we should check the UnexpectedOpList. If it is not empty, we should return an error or panic.

good suggestion, will take care in further PRs

codecov · 2024-03-11T04:29:05Z

Codecov Report

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

Project coverage is 76.18%. Comparing base (e1f0636) to head (461ef9f).
Report is 198 commits behind head on master.

Files	Patch %	Lines
action/protocol/execution/evm/evmstatedbadapter.go	87.23%	4 Missing and 2 partials ⚠️
action/protocol/execution/evm/evm.go	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4171      +/-   ##
==========================================
+ Coverage   75.38%   76.18%   +0.80%     
==========================================
  Files         303      340      +37     
  Lines       25923    29094    +3171     
==========================================
+ Hits        19541    22165    +2624     
- Misses       5360     5829     +469     
- Partials     1022     1100      +78

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

dustinxie · 2024-03-11T04:35:12Z

action/protocol/execution/evm/evmstatedbadapter.go

+			})
+		}
+	} else {
+		if stateDB.suicideTxLogMismatchPanic {


for safety, we gate the panic with a flag
after hard-fork, we can run a fullsync w/o the flag, and remove this flag if fullsync pass

https://github.com/iotexproject/iotex-core/pull/4171/files#r1519122441 if we panic here, what's the purpose of resetting the mount?

I gave an example above, if we don't reset the amount, it is possible to generate a mistaken txLog for an earlier AddBalance() whose amount happens to equal to suicide address

action/protocol/execution/evm/evmstatedbadapter.go

CoderZhi · 2024-03-11T06:16:50Z

action/protocol/context.go

@@ -261,6 +262,7 @@ func WithFeatureCtx(ctx context.Context) context.Context {
 			SharedGasWithDapp:                       g.IsToBeEnabled(height),
 			CandidateRegisterMustWithStake:          !g.IsToBeEnabled(height),
 			DisableDelegateEndorsement:              !g.IsToBeEnabled(height),
+			SuicideTxLogMismatchPanic:               g.IsToBeEnabled(height),


default value should be the value after hard fork

is this a must? to me using a positive (enable after height) flag name will help improve code readability and reduce chance of programming mistake

CoderZhi · 2024-03-11T09:11:40Z

action/protocol/execution/evm/evmstatedbadapter.go

+	// before calling Suicide, EVM will transfer the contract's balance to beneficiary
+	// need to create a transaction log on successful suicide
+	if stateDB.lastAddBalanceAmount.Cmp(actBalance) == 0 {
+		if len(stateDB.lastAddBalanceAmount.Bytes()) > 0 {


ignore efficiency and use stateDB.lastAddBalanceAmount.Cmp(big.NewInt(0)) > 0

CoderZhi · 2024-03-11T09:12:11Z

action/protocol/execution/evm/evmstatedbadapter.go

+			})
+		}
+	} else {
+		if stateDB.suicideTxLogMismatchPanic {


https://github.com/iotexproject/iotex-core/pull/4171/files#r1519122441 if we panic here, what's the purpose of resetting the mount?

sonarcloud · 2024-03-11T23:06:34Z

Quality Gate failed

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

See analysis details on SonarCloud

Liuhaai · 2024-03-12T00:26:03Z

action/protocol/execution/evm/evmstatedbadapter.go

@@ -48,6 +48,8 @@ type (
 		err                        error
 		blockHeight                uint64
 		executionHash              hash.Hash256
+		lastAddBalanceAddr         string


Retrieving the info from statedb/snapshot ( getStateObject(addr common.Address) in geth(https://github.com/ethereum/go-ethereum/blob/ebf9e11af2ff701d0961623e817d37b421b96802/core/state/statedb.go#L540) ) seems more reasonable than two new variables under StateDBAdapter for me

can you elaborate? we cannot get the beneficiary's address in StateDBAdapter, so capture it in AddBalance every time. also we need the transfer amount, not the beneficiary's entire balance

dustinxie requested review from CoderZhi, Liuhaai and envestcc as code owners March 7, 2024 00:43

millken reviewed Mar 7, 2024

View reviewed changes

dustinxie force-pushed the enter branch from 64afa5d to 041bb26 Compare March 7, 2024 03:44

millken reviewed Mar 7, 2024

View reviewed changes

CoderZhi reviewed Mar 7, 2024

View reviewed changes

envestcc reviewed Mar 8, 2024

View reviewed changes

dustinxie force-pushed the enter branch from 041bb26 to 28922a9 Compare March 11, 2024 03:21

dustinxie force-pushed the enter branch from 28922a9 to df763eb Compare March 11, 2024 04:13

dustinxie commented Mar 11, 2024

View reviewed changes

envestcc reviewed Mar 11, 2024

View reviewed changes

action/protocol/execution/evm/evmstatedbadapter.go Show resolved Hide resolved

CoderZhi reviewed Mar 11, 2024

View reviewed changes

dustinxie added 3 commits March 11, 2024 16:05

[evm] add transaction log for Suicide()

705d52d

panic when suicide amount not match + fix test

279e47e

address comment

461ef9f

dustinxie force-pushed the enter branch from df763eb to 461ef9f Compare March 11, 2024 23:05

Liuhaai reviewed Mar 12, 2024

View reviewed changes

CoderZhi approved these changes Mar 12, 2024

View reviewed changes

envestcc approved these changes Mar 12, 2024

View reviewed changes

dustinxie merged commit 135d9c1 into iotexproject:master Mar 12, 2024
3 of 5 checks passed

BrewTestBot mentioned this pull request Mar 25, 2024

ioctl 1.14.0 Homebrew/homebrew-core#167116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[evm] add transaction log for Suicide() #4171

[evm] add transaction log for Suicide() #4171

dustinxie commented Mar 7, 2024

millken Mar 7, 2024

dustinxie Mar 7, 2024

millken Mar 7, 2024

CoderZhi Mar 7, 2024

dustinxie Mar 11, 2024

CoderZhi Mar 7, 2024

dustinxie Mar 11, 2024

envestcc left a comment

dustinxie commented Mar 11, 2024

codecov bot commented Mar 11, 2024 •

edited

Loading

dustinxie Mar 11, 2024

CoderZhi Mar 11, 2024

dustinxie Mar 11, 2024

CoderZhi Mar 11, 2024

dustinxie Mar 11, 2024

CoderZhi Mar 11, 2024

dustinxie Mar 11, 2024

CoderZhi Mar 11, 2024

sonarcloud bot commented Mar 11, 2024

Liuhaai Mar 12, 2024 •

edited

Loading

dustinxie Mar 12, 2024

[evm] add transaction log for Suicide() #4171

[evm] add transaction log for Suicide() #4171

Conversation

dustinxie commented Mar 7, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

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

envestcc left a comment

Choose a reason for hiding this comment

dustinxie commented Mar 11, 2024

codecov bot commented Mar 11, 2024 • edited Loading

Codecov Report

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 Mar 11, 2024

Quality Gate failed

Liuhaai Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 11, 2024 •

edited

Loading

Liuhaai Mar 12, 2024 •

edited

Loading