[account] revert ConvertFreshAccountToZeroNonceType func #4163

Liuhaai · 2024-03-05T00:19:20Z

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

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-05T00:41:32Z

Codecov Report

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

Project coverage is 76.14%. Comparing base (e1f0636) to head (ba64dc8).
Report is 199 commits behind head on master.

Files	Patch %	Lines
api/coreservice.go	50.00%	12 Missing and 6 partials ⚠️
state/factory/workingset.go	44.44%	4 Missing and 1 partial ⚠️
action/protocol/execution/evm/evm.go	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4163      +/-   ##
==========================================
+ Coverage   75.38%   76.14%   +0.76%     
==========================================
  Files         303      340      +37     
  Lines       25923    29171    +3248     
==========================================
+ Hits        19541    22212    +2671     
- Misses       5360     5851     +491     
- Partials     1022     1108      +86

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

state/factory/workingset.go

dustinxie · 2024-03-05T03:06:46Z

state/account_test.go

+ require.NoError(err)
+ require.Equal(uint64(1), acct.PendingNonce())
+ require.Error(acct.SetPendingNonce(0))
+ require.Error(acct.SetPendingNonce(3))


see above comment about SetPendingNonce(2)

dustinxie · 2024-03-05T03:07:11Z

state/account.go

+ // so we can convert it to zero-nonce account
+ if st.accountType == 0 && st.nonce == 0 && nonce == 1 {
+ st.accountType = 1
+ }


calling SetPendingNonce(2) will skip this conversion, and remain legacy account.
Do we want to allow this behavior? i.e., the legacy account could convert to zero-nonce type (by sending first tx nonce=0), or remain legacy (by sending first tx nonce=1)

the account isn't a fresh account if its pending nonce is set to 2. And the nonce of a fresh account will be guaranteed by checkNonceContinuity()

envestcc · 2024-03-05T03:08:52Z

state/account.go

+ if st.accountType == 0 && st.nonce == 0 && nonce == 1 {
+ st.accountType = 1
+ }


should be a hard-fork?

https:/iotexproject/iotex-core/pull/4163/files#diff-8d0192f25f9daf6c1bfa70c3b75949444fe5d1d47193e6b7c1210c39a19ed52aR392

dustinxie · 2024-03-05T04:53:20Z

action/protocol/execution/evm/evmstatedbadapter.go

+ pendingNonce = state.PendingNonceConsideringFreshAccount()
+ } else {
+ pendingNonce = state.PendingNonce()
+ }


consider the case, where the legacy address send a first tx with tx.Nonce=2
here it returns pendingNonce = 0

inside EVM code, it calls SetNonce(GetNonce()+1) so it calls SetNonce(1)

then in our SetNonce(), it will hit

if !stateDB.useConfirmedNonce { nonce-- }

nonce=0, and finally calls SetPendingNonce(nonce+1) --> SetPendingNonce(1)
4. SetPendingNonce(1) will trigger the conversion, convert the address to zero-nonce type with s.nonce = 1
5. but the first tx of this address now has nonce=2

For Step 1, the tx.nonce is 0 if state.PendingNonceConsideringFreshAccount() is triggered in the EVM. I might ignore the case it's triggered when tx.nonce is 2. Could you give more details about how to reproduce?
For Step 5, What do you mean the address has nonce=2?

the above steps illustrate a possible attack, someone just sends a tx with tx.Nonce=2, or a malicious delegate just run such a tx, when state.PendingNonceConsideringFreshAccount() is triggered in the EVM, it is GetNonce() = 0, it will pass SetNonce(1) and trigger the conversion
in other words, the first tx of such address will always trigger the conversion, regardless of the tx itself has Nonce=2 or even Nonce=5
and this will be caught by checkNonceContinuity, but it becomes a potential attack

the problem has been settled in offline discussion.

CoderZhi · 2024-03-06T07:05:10Z

action/protocol/execution/evm/evmstatedbadapter.go

+ pendingNonce = state.PendingNonceConsideringFreshAccount()
+ } else {
+ pendingNonce = state.PendingNonce()
+ }


the problem has been settled in offline discussion.

CoderZhi · 2024-03-10T04:14:01Z

action/protocol/context.go

@@ -117,6 +117,7 @@ type (
 UseZeroNonceForFreshAccount bool
 SharedGasWithDapp bool
 DisableDelegateEndorsement bool
+ RefactorFreshAccountConversion bool


default value (false) is the value after hard fork

envestcc

so should use PendingNonceConsideringFreshAccount() to get pending nonce of any account instead of PendingNonce() from now on, right?

Liuhaai · 2024-03-12T20:21:32Z

so should use PendingNonceConsideringFreshAccount() to get pending nonce of any account instead of PendingNonce() from now on, right?

PendingNonce() can be removed in another pr after the activated height since it's used in API now

sonarcloud · 2024-03-12T20:29:27Z

Quality Gate failed

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

See analysis details on SonarCloud

Liuhaai requested review from CoderZhi, dustinxie, envestcc and millken as code owners March 5, 2024 00:19

dustinxie reviewed Mar 5, 2024

View reviewed changes

state/factory/workingset.go Show resolved Hide resolved

dustinxie reviewed Mar 5, 2024

View reviewed changes

envestcc reviewed Mar 5, 2024

View reviewed changes

dustinxie reviewed Mar 5, 2024

View reviewed changes

Liuhaai force-pushed the fix#4156_2 branch from 9ff16d7 to 1fbba38 Compare March 5, 2024 21:02

CoderZhi approved these changes Mar 10, 2024

View reviewed changes

envestcc approved these changes Mar 12, 2024

View reviewed changes

Liuhaai added 4 commits March 12, 2024 13:24

revert ConvertFreshAccountToZeroNonceType func

84e8be1

skip the failed test

b2593a3

add hard fork flag

6e5d287

fix

ba64dc8

Liuhaai force-pushed the fix#4156_2 branch from 1fbba38 to ba64dc8 Compare March 12, 2024 20:28

Liuhaai merged commit 6eb9c5f into iotexproject:master Mar 12, 2024
2 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

[account] revert ConvertFreshAccountToZeroNonceType func #4163

[account] revert ConvertFreshAccountToZeroNonceType func #4163

Liuhaai commented Mar 5, 2024

codecov bot commented Mar 5, 2024 •

edited

Loading

dustinxie Mar 5, 2024 •

edited

Loading

dustinxie Mar 5, 2024 •

edited

Loading

Liuhaai Mar 5, 2024 •

edited

Loading

envestcc Mar 5, 2024

Liuhaai Mar 5, 2024

dustinxie Mar 5, 2024

Liuhaai Mar 5, 2024

dustinxie Mar 5, 2024

CoderZhi Mar 6, 2024

CoderZhi Mar 6, 2024

CoderZhi Mar 10, 2024

envestcc left a comment

Liuhaai commented Mar 12, 2024 •

edited

Loading

sonarcloud bot commented Mar 12, 2024

[account] revert ConvertFreshAccountToZeroNonceType func #4163

[account] revert ConvertFreshAccountToZeroNonceType func #4163

Conversation

Liuhaai commented Mar 5, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Mar 5, 2024 • edited Loading

Codecov Report

dustinxie Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

dustinxie Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Liuhaai Mar 5, 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

envestcc left a comment

Choose a reason for hiding this comment

Liuhaai commented Mar 12, 2024 • edited Loading

sonarcloud bot commented Mar 12, 2024

Quality Gate failed

codecov bot commented Mar 5, 2024 •

edited

Loading

dustinxie Mar 5, 2024 •

edited

Loading

dustinxie Mar 5, 2024 •

edited

Loading

Liuhaai Mar 5, 2024 •

edited

Loading

Liuhaai commented Mar 12, 2024 •

edited

Loading