[state] convert clean address to zero-nonce type #3991

dustinxie · 2023-11-22T00:00:27Z

Description

If a legacy account has never initiated an outgoing tx, we convert it to a zero-nonce type account. This enables us to do deterministic deployment

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

state/account.go

CoderZhi

as discussed, this feature could be deleted after hardfork if the accountType is updated from 0 to 1

dustinxie · 2023-11-30T07:48:46Z

action/protocol/account/transfer.go

@@ -66,6 +66,7 @@ func (p *Protocol) handleTransfer(ctx context.Context, tsf *action.Transfer, sm

 	if fCtx.FixGasAndNonceUpdate || tsf.Nonce() != 0 {
 		// update sender Nonce
+		sender.CheckConvertToZeroNonceType(tsf.Nonce())


don't need a hard-fork flag here, b/c this would only happen after the hard-fork when the nonce of clean address is changed by AdjustNonceForCleanAddress() to 0
for now, the first nonce of a clean address remains == 1, so this does no harm

dustinxie · 2023-11-30T07:50:43Z

just realized more changes are needed for evmstateadaptor.GetNonce/SetNonce, since inside evm.Create it also calls those 2, will update later

codecov · 2023-11-30T08:00:00Z

Codecov Report

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

Comparison is base (e1f0636) 75.38% compared to head (122d409) 76.18%.
Report is 156 commits behind head on master.

Files	Patch %	Lines
blockindex/contractstaking/event_handler.go	67.23%	45 Missing and 13 partials ⚠️
action/protocol/staking/staking_statereader.go	69.76%	35 Missing and 17 partials ⚠️
action/protocol/execution/evm/evm.go	48.38%	47 Missing and 1 partial ⚠️
api/coreservice.go	62.74%	33 Missing and 5 partials ⚠️
api/web3server.go	79.24%	30 Missing and 3 partials ⚠️
action/candidate_endorsement.go	0.00%	31 Missing ⚠️
action/protocol/staking/protocol.go	33.33%	28 Missing ⚠️
action/candidate_activate.go	0.00%	25 Missing ⚠️
...tion/protocol/staking/contractstake_bucket_type.go	0.00%	24 Missing ⚠️
action/protocol/rewarding/fund.go	23.07%	19 Missing and 1 partial ⚠️
... and 36 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3991      +/-   ##
==========================================
+ Coverage   75.38%   76.18%   +0.80%     
==========================================
  Files         303      337      +34     
  Lines       25923    28731    +2808     
==========================================
+ Hits        19541    21888    +2347     
- Misses       5360     5731     +371     
- Partials     1022     1112      +90

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

CoderZhi · 2023-12-01T02:00:52Z

api/coreservice.go

+		return nonce, err
+	}
+	g := core.Genesis()
+	if nonce > 1 || !g.IsSumatra(core.TipHeight()) {


why IsSumatra? what's the difference from UseZeroNonceForFreshAccount?

IsSumatra() is the same flag as UseZeroNonceForFreshAccount , here we don't have a ctx

API service does not affect past blocks, so after the hard-fork, this check will be removed

if so, be consistent with g.IsToBeEnabled(core.TipHeight())

CoderZhi · 2023-12-01T02:03:12Z

action/protocol/execution/evm/evmstatedbadapter.go

@@ -344,6 +345,9 @@ func (stateDB *StateDBAdapter) GetNonce(evmAddr common.Address) uint64 {
 		}
 		pendingNonce--
 	}
+	if pendingNonce == 1 && stateDB.convertFreshAddress {


could we remove pendingNonce == 1? same question for all the other places making a similar check outside of function AdjustNonceForFreshAccount

CoderZhi · 2023-12-01T02:06:01Z

state/account.go

@@ -178,6 +187,16 @@ func (st *Account) PendingNonce() uint64 {
 	}
 }

+// AdjustNonceForFreshAccount adjust the nonce for a fresh legacy account
+func (st *Account) AdjustNonceForFreshAccount(nonce uint64) uint64 {


Better to be a util function, instead of a struct function

change the name to make it more correct

action/protocol/staking/protocol.go

Liuhaai · 2023-12-02T20:58:05Z

action/protocol/account/transfer.go

@@ -66,6 +66,7 @@ func (p *Protocol) handleTransfer(ctx context.Context, tsf *action.Transfer, sm

 	if fCtx.FixGasAndNonceUpdate || tsf.Nonce() != 0 {
 		// update sender Nonce
+		sender.ConvertFreshAccountToZeroNonceType(tsf.Nonce())


if acc.IsNewbieAccount() && acc.AccountType() == 0 && useZeroNonce { acc.ConvertFreshAccountToZeroNonceType() } // Make sure the value matches if acc.PendingNonce() is called

Other places related to setpendingnonce/pendingnonce are similar

acc.IsNewbieAccount() && acc.AccountType() == 0 is duplicate with what's checked inside ConvertFreshAccountToZeroNonceType

added the useZeroNonce flag check

action/protocol/execution/evm/evmstatedbadapter.go

CoderZhi · 2023-12-07T00:06:35Z

api/coreservice.go

+		return nonce, err
+	}
+	g := core.Genesis()
+	if nonce > 1 || !g.IsSumatra(core.TipHeight()) {


if so, be consistent with g.IsToBeEnabled(core.TipHeight())

Liuhaai · 2023-12-16T01:01:15Z

state/factory/workingset.go

@@ -366,7 +370,11 @@ func (ws *workingSet) checkNonceContinuity(ctx context.Context, accountNonceMap
 			return errors.Wrapf(err, "failed to get the confirmed nonce of address %s", srcAddr)
 		}
 		sort.Slice(receivedNonces, func(i, j int) bool { return receivedNonces[i] < receivedNonces[j] })
-		pendingNonce := confirmedState.PendingNonce()
+		if useZeroNonce {
+			pendingNonce = confirmedState.PendingNonceConsideringFreshAccount()


PendingNonceConsideringFreshAccount is conditional? The name indicates the judging is done inside the func

envestcc · 2023-12-28T11:24:50Z

state/account.go

@@ -178,6 +187,16 @@ func (st *Account) PendingNonce() uint64 {
 	}
 }

+// PendingNonceConsideringFreshAccount return the pending nonce considering fresh legacy account
+func (st *Account) PendingNonceConsideringFreshAccount() uint64 {


How about naming it PendingNonce() and renaming the original LegacyPendingNonce()

SetPendingNonce() should also be updated for this change

PendingNonce has too many uses in the code
if we want to do this, can do in a separate PR

envestcc · 2023-12-28T11:44:30Z

state/account_test.go

@@ -175,3 +175,45 @@ func TestClone(t *testing.T) {
 	require.Equal(big.NewInt(200), ss.Balance)
 	require.Equal(big.NewInt(200+100), account.Balance)
 }
+
+func TestConvertFreshAddress(t *testing.T) {


e2etest is also required to check working well in evm

added in latest commit

api/coreservice.go

api/web3server.go

envestcc · 2024-01-05T06:46:12Z

e2etest/local_transfer_test.go

+			make([]byte, 4),
+			uint64(200000), big.NewInt(1),
+			TsfSuccess, "",
+			"Transfer with nonce 0",


Why behavior changes when this feature not enabled

the test is "transfer with nonce 0". Before enable this, legacy account first tx has nonce=1, so the test will fail. Now with this feature enabled, the first tx can have nonce=0 so the test can pass

envestcc · 2024-01-08T03:31:11Z

blockchain/integrity/integrity_test.go

+	} {
+		a, err := accountutil.AccountState(ctx, sf, v.a)
+		require.NoError(err)
+		require.EqualValues(1, a.AccountType())


add case to check if first tx nonce==1

envestcc · 2024-01-08T03:39:12Z

action/protocol/account/transfer.go

@@ -66,6 +66,9 @@ func (p *Protocol) handleTransfer(ctx context.Context, tsf *action.Transfer, sm

 	if fCtx.FixGasAndNonceUpdate || tsf.Nonce() != 0 {
 		// update sender Nonce
+		if fCtx.UseZeroNonceForFreshAccount {
+			sender.ConvertFreshAccountToZeroNonceType(tsf.Nonce())


maybe can do before protocol.Handle

see latest commit

dustinxie · 2024-01-10T05:18:47Z

state/factory/workingset.go

+				return nil, errors.Wrapf(err, "failed to store converted sender %s", actCtx.Caller.String())
+			}
+		}
+	}


do the check/conversion before action.Handle()

dustinxie · 2024-01-10T05:19:04Z

action/protocol/account/transfer.go

@@ -66,9 +66,6 @@ func (p *Protocol) handleTransfer(ctx context.Context, tsf *action.Transfer, sm

 	if fCtx.FixGasAndNonceUpdate || tsf.Nonce() != 0 {
 		// update sender Nonce
-		if fCtx.UseZeroNonceForFreshAccount {
-			sender.ConvertFreshAccountToZeroNonceType(tsf.Nonce())
-		}


revert change in 1st commit

dustinxie · 2024-01-10T05:19:14Z

action/protocol/execution/evm/evm.go

@@ -405,9 +405,6 @@ func prepareStateDB(ctx context.Context, sm protocol.StateManager) (*StateDBAdap
 	if !featureCtx.CorrectGasRefund {
 		opts = append(opts, ManualCorrectGasRefundOption())
 	}
-	if featureCtx.UseZeroNonceForFreshAccount {
-		opts = append(opts, ConvertFreshAddressOption())
-	}


revert change in 1st commit

dustinxie · 2024-01-10T05:19:24Z

action/protocol/execution/evm/evmstatedbadapter.go

@@ -373,9 +358,6 @@ func (stateDB *StateDBAdapter) SetNonce(evmAddr common.Address, nonce uint64) {
 	log.L().Debug("Called SetNonce.",
 		zap.String("address", addr.String()),
 		zap.Uint64("nonce", nonce))
-	if stateDB.convertFreshAddress {
-		s.ConvertFreshAccountToZeroNonceType(nonce)
-	}


revert change in 1st commit

dustinxie · 2024-01-10T05:19:32Z

action/protocol/rewarding/protocol.go

 		accountCreationOpts = append(accountCreationOpts, state.LegacyNonceAccountTypeOption())
 	}
 	acc, err := accountutil.LoadOrCreateAccount(sm, addr, accountCreationOpts...)
 	if err != nil {
 		return err
 	}
 	if !skipSetNonce {
-		if fCtx.UseZeroNonceForFreshAccount {
-			acc.ConvertFreshAccountToZeroNonceType(nonce)
-		}


revert change in 1st commit

dustinxie · 2024-01-10T05:19:39Z

action/protocol/staking/protocol.go

 		accountCreationOpts = append(accountCreationOpts, state.LegacyNonceAccountTypeOption())
 	}
 	acc, err := accountutil.LoadAccount(sm, actionCtx.Caller, accountCreationOpts...)
 	if err != nil {
 		return nil, err
 	}
-	if fCtx.UseZeroNonceForFreshAccount {
-		acc.ConvertFreshAccountToZeroNonceType(actionCtx.Nonce)
-	}


revert change in 1st commit

Liuhaai · 2024-01-10T21:01:07Z

actpool/actqueue.go

 		balance = new(big.Int).Set(confirmedState.Balance)
 		acts    = make([]action.SealedEnvelope, 0, len(q.items))
 	)
 	q.mu.RLock()
 	defer q.mu.RUnlock()
+	if confirmedState.IsLegacyFreshAccount() && nonce == 0 {
+		// for a legacy fresh account, the nonce of first tx could be either 0 or 1


The nonce of 1st tx is undeterministic

updated in latest commit

CoderZhi

let's further simplify the logic to: "the nonce of the first action for a fresh account with legacy type could only be 0 after hard fork no matter how". The only problem for the simplified logic is that a fresh account with legacy type may send an action of nonce 1 may need to resend an action of nonce 0, which is not a big problem.

CoderZhi · 2024-01-10T01:02:32Z

state/account.go

@@ -166,6 +171,15 @@ func (st *Account) SetPendingNonce(nonce uint64) error {
 	return nil
 }

+// ConvertFreshAccountToZeroNonceType converts a fresh legacy account to zero-nonce account
+func (st *Account) ConvertFreshAccountToZeroNonceType(nonce uint64) {


delete parameter nonce

this is to make sure the conversion only happens for a tx with nonce = 0

dustinxie · 2024-01-12T06:55:08Z

let's further simplify the logic to: "the nonce of the first action for a fresh account with legacy type could only be 0 after hard fork no matter how". The only problem for the simplified logic is that a fresh account with legacy type may send an action of nonce 1 may need to resend an action of nonce 0, which is not a big problem.

yes updated this in latest commit

CoderZhi · 2024-01-13T12:46:45Z

api/coreservice.go

+		return nonce, err
+	}
+	g := core.Genesis()
+	if nonce > 1 || !g.IsToBeEnabled(core.TipHeight()) {


nonce returned in line 494 is from ap. if nonce == 1 && after hard fork, shall we return 1?

fixed in latest commit

CoderZhi · 2024-01-14T03:50:36Z

state/account.go

@@ -166,6 +171,17 @@ func (st *Account) SetPendingNonce(nonce uint64) error {
 	return nil
 }

+// ConvertFreshAccountToZeroNonceType converts a fresh legacy account to zero-nonce account
+func (st *Account) ConvertFreshAccountToZeroNonceType(nonce uint64) bool {


could this conversion be moved to SetPendingNonce as:

switch st.accountType { case 1: ... case 0: if st.accountType == 0 && st.nonce == 0 && nonce == 1 { st.accountType = 1 } else { .... } default: ... } st.nonce++

The hard fork feature and nonce has been checked in checkNonceContinuity, thus we can delete this function as well as the place calling it

it's very hard to do insideSetPendingNonce alone, because EVM use emvstateadaptor's GetNonce and SetNonce to access the nonce value.
In order to achieve nonce == 1 in the example you gave, we'll need add code inside GetNonce and SetNonce -- which is basically the code moved to workingset.go in commit c03396e

Liuhaai · 2024-01-15T05:38:15Z

I prefer changes around /action/protocol/account/util/util.go, but the current implementation is acceptable for me

CoderZhi

Three places could be changed to achieve the conversion:

When returning a confirmed state of an account, passing the hard fork symbol
Modify account.PendingNonce returns the right pending nonce
SetPendingNonce sets the right pending nonce and modify the account type accordingly

This PR tries to resolve the problem by modifying the process but not the object containing the feature. We need a followup PR to do it in an OO way.

sonarcloud · 2024-01-17T01:04:31Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
No data about Coverage
2.4% Duplication on New Code

See analysis details on SonarCloud

dustinxie requested review from CoderZhi, Liuhaai, envestcc, millken and a team as code owners November 22, 2023 00:00

Liuhaai reviewed Nov 22, 2023

View reviewed changes

state/account.go Outdated Show resolved Hide resolved

CoderZhi reviewed Nov 27, 2023

View reviewed changes

dustinxie force-pushed the track branch 2 times, most recently from c9bf2a9 to bb20925 Compare November 30, 2023 07:46

dustinxie commented Nov 30, 2023

View reviewed changes

dustinxie force-pushed the track branch from bb20925 to 5e17a17 Compare November 30, 2023 23:09

CoderZhi reviewed Dec 1, 2023

View reviewed changes

Liuhaai reviewed Dec 2, 2023

View reviewed changes

dustinxie force-pushed the track branch 2 times, most recently from 4d9fb01 to fc7e6cb Compare December 4, 2023 03:58

CoderZhi reviewed Dec 7, 2023

View reviewed changes

Liuhaai reviewed Dec 16, 2023

View reviewed changes

envestcc reviewed Dec 28, 2023

View reviewed changes

dustinxie force-pushed the track branch 2 times, most recently from 16fb160 to 2d763e0 Compare January 5, 2024 05:44

envestcc reviewed Jan 5, 2024

View reviewed changes

api/coreservice.go Outdated Show resolved Hide resolved

envestcc reviewed Jan 5, 2024

View reviewed changes

api/web3server.go Show resolved Hide resolved

envestcc reviewed Jan 5, 2024

View reviewed changes

envestcc reviewed Jan 8, 2024

View reviewed changes

dustinxie force-pushed the track branch from 2d763e0 to c1fa391 Compare January 10, 2024 05:16

dustinxie commented Jan 10, 2024

View reviewed changes

envestcc approved these changes Jan 10, 2024

View reviewed changes

Liuhaai reviewed Jan 10, 2024

View reviewed changes

dustinxie force-pushed the track branch from c1fa391 to b613eec Compare January 11, 2024 01:04

CoderZhi reviewed Jan 12, 2024

View reviewed changes

dustinxie added 3 commits January 11, 2024 22:40

[state] convert fresh legacy address to zero-nonce type

21c1892

address comment 1

6c45123

add test

9ac8244

dustinxie force-pushed the track branch from b613eec to dc55b35 Compare January 12, 2024 06:53

CoderZhi reviewed Jan 13, 2024

View reviewed changes

move conversion check to workingset

c03396e

dustinxie force-pushed the track branch from dc55b35 to 2183a9c Compare January 13, 2024 20:42

CoderZhi reviewed Jan 14, 2024

View reviewed changes

address comment

e9a3670

dustinxie force-pushed the track branch from 2183a9c to e9a3670 Compare January 14, 2024 07:06

Liuhaai approved these changes Jan 15, 2024

View reviewed changes

CoderZhi approved these changes Jan 17, 2024

View reviewed changes

Merge branch 'master' into track

122d409

dustinxie merged commit dbd7a5b into iotexproject:master Jan 17, 2024
4 of 5 checks passed

dustinxie added a commit that referenced this pull request Jan 29, 2024

[state] convert clean address to zero-nonce type (#3991)

593dba7

BrewTestBot mentioned this pull request Feb 7, 2024

ioctl 1.13.0 Homebrew/homebrew-core#161991

Merged

[state] convert clean address to zero-nonce type #3991

[state] convert clean address to zero-nonce type #3991

Conversation

dustinxie commented Nov 22, 2023

Description

Type of change

How Has This Been Tested?

Checklist:

CoderZhi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinxie commented Nov 30, 2023

codecov bot commented Nov 30, 2023 • edited

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

Liuhaai Dec 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Liuhaai Dec 16, 2023 • edited

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

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 Jan 11, 2024 • edited

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

dustinxie commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinxie Jan 15, 2024 • edited

Choose a reason for hiding this comment

Liuhaai commented Jan 15, 2024

CoderZhi left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 17, 2024

Quality Gate passed

codecov bot commented Nov 30, 2023 •

edited

Liuhaai Dec 2, 2023 •

edited

Liuhaai Dec 16, 2023 •

edited

dustinxie Jan 11, 2024 •

edited

dustinxie Jan 15, 2024 •

edited