account type with zero init nonce #3387

CoderZhi · 2022-05-13T02:52:35Z

Description

Close other PRs per reviewers' request.
In this PR:

Move IsSystemAction into action package as a static function
Set balance via SubBalance and AddBalance
Create account with NewAccount and NewEmptyAccount
Replace state.Nonce with State.PendingNonce
Skip update nonce and gas for system actions
Check nonce value in SetNonce
Introduce new account type

Fixes # (issue)

Type of change

Please delete options that are not relevant.

[] Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)

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

[] Test A
[] Test B

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 · 2022-05-13T06:54:39Z

Codecov Report

Merging #3387 (fe5c397) into master (5ee6a4f) will increase coverage by 0.20%.
The diff coverage is 81.75%.

@@            Coverage Diff             @@
##           master    #3387      +/-   ##
==========================================
+ Coverage   75.21%   75.41%   +0.20%     
==========================================
  Files         244      244              
  Lines       22543    22675     +132     
==========================================
+ Hits        16956    17101     +145     
+ Misses       4664     4649      -15     
- Partials      923      925       +2

Impacted Files	Coverage Δ
action/const.go	`100.00% <ø> (ø)`
state/account.go	`66.37% <63.76%> (-0.29%)`	⬇️
action/protocol/execution/evm/evmstatedbadapter.go	`65.61% <63.79%> (+0.05%)`	⬆️
action/protocol/poll/util.go	`68.18% <75.00%> (-0.05%)`	⬇️
action/protocol/rewarding/protocol.go	`79.41% <79.41%> (+2.28%)`	⬆️
actpool/actpool.go	`87.09% <95.65%> (+0.18%)`	⬆️
action/protocol/account/protocol.go	`86.90% <100.00%> (+0.65%)`	⬆️
action/protocol/account/transfer.go	`85.10% <100.00%> (+1.01%)`	⬆️
action/protocol/context.go	`67.47% <100.00%> (+30.28%)`	⬆️
action/protocol/execution/evm/evm.go	`45.09% <100.00%> (+1.30%)`	⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ee6a4f...fe5c397. Read the comment docs.

dustinxie · 2022-05-18T01:15:29Z

api/coreservice.go

 	accountMeta := &iotextypes.AccountMeta{
 		Address:      addrStr,
 		Balance:      state.Balance.String(),
-		Nonce:        state.Nonce,
-		PendingNonce: pendingNonce,
+		Nonce:        nonce,


why not use state.Nonce as before? so line 260 to 263 is not necessary?

action/protocol/rewarding/protocol.go

dustinxie · 2022-05-18T01:21:52Z

state/account.go

 	// other actions' nonces start from 1
-	Nonce        uint64
+	nonce        uint64


i think it's OK to leave it as it, or is there a particular reason to make nonce unexported?

dustinxie · 2022-05-18T01:23:46Z

state/account.go

+	switch st.Type {
+	case 1:
+		return st.nonce
+	default: // 0


maybe

case 0: return nonce + 1 default: panic("unknown account type")

action/protocol/execution/evm/evmstatedbadapter.go

dustinxie · 2022-05-18T01:46:22Z

action/protocol/execution/evm/evmstatedbadapter.go

-		nonce++
+		nonce = state.PendingNonce()
+		if !stateDB.usePendingNonce {
+			nonce--


is it possible a new account (type = 1) will go to here? then nonce would be 0-1

dustinxie · 2022-05-18T01:51:13Z

action/protocol/execution/evm/evmstatedbadapter.go

@@ -318,11 +342,14 @@ func (stateDB *StateDBAdapter) SetNonce(evmAddr common.Address, nonce uint64) {
 	log.L().Debug("Called SetNonce.",
 		zap.String("address", addr.String()),
 		zap.Uint64("nonce", nonce))
-	s.Nonce = nonce
-	if err := accountutil.StoreAccount(stateDB.sm, addr, s); err != nil {
+	if err := accountutil.SetNonce(s, nonce); err != nil {


i don't recall, in the case stateDB.usePendingNonce == true, why in GetNonce() need to do nonce++ and in SetNonce() do nonce-- to bring it back (line 340 above)

does this logic still hold for the new account (type = 1)?

dustinxie · 2022-06-13T23:33:11Z

action/protocol/account/protocol_test.go


 	// create account
 	require.NoError(createAccount(sm, addrv1.String(), big.NewInt(5)))
 	s, err = accountutil.LoadAccount(sm, addrv1)
 	require.NoError(err)
 	require.Equal("5", s.Balance.String())
-	require.Equal(uint64(0x0), s.Nonce)


add test to create new type account, assert s.PendingNonce() == 0

Are you sure to add unit test to this PR?

dustinxie · 2022-06-13T23:38:06Z

action/protocol/account/util/util.go

+	case state.ErrStateNotExist:
+		return state.NewAccount(opts...)
+	case nil:
+		return account, nil


nit: move case nil: to the first case, to be consistent with other places

dustinxie · 2022-06-13T23:39:40Z

action/protocol/account/util/util.go

+	switch errors.Cause(err) {
+	case nil:
+		fallthrough
+	case state.ErrStateNotExist:


nit: case nil, state.ErrStateNotExist is more straightforward

state/factory/workingset.go

Liuhaai · 2022-06-28T03:43:38Z

state/account.go

-func EmptyAccount() Account {
-	return Account{
+// NewAccount creates a new account with options
+func NewAccount(opts ...AccountCreationOption) (*Account, error) {


move to the head

action/protocol/poll/util.go

Liuhaai · 2022-06-28T05:04:46Z

action/protocol/account/transfer.go

 	if recipientAcct.IsContract() {
-		// update sender Nonce
-		accountutil.SetNonce(tsf, sender)
+		if fixNonce || tsf.Nonce() != 0 {


SetPendingNonce can be moved out of if condition?

dustinxie · 2022-06-29T02:53:36Z

state/account.go

+}
+
+// SetPendingNonce sets the pending nonce
+func (st *Account) SetPendingNonce(nonce uint64) error {


IMO, we should keep using SetNonce(nonce), this way SetPendingNonce(nonce+1) feels more confusing

dustinxie · 2022-06-29T22:36:24Z

action/protocol/execution/evm/evmstatedbadapter_test.go

-				for _, addr := range stateDB.cachedContractAddrs() {
-					c := stateDB.cachedContract[addr]
+			if stateDB.disableSortCachedContracts {
+				for _, c := range stateDB.cachedContract {


IMO it's not necessary to flip the flag? it increases the risk of making accidental error, and make it hard to review
if we really want to do it, better do it in a separate PR, since this is not related to zero-nonce

dustinxie · 2022-06-29T22:38:30Z

action/protocol/execution/testdata-london/basic-token.json

@@ -26,7 +26,7 @@
        "rawGasLimit": 1000000,
        "rawGasPrice": "0",
        "rawAccessList": [{
-            "address": "06abb2ca03834bd4f62c610ca7627e3dd12d9a97",
+            "address": "675f1057F81e9e768e33faddbd5609C09F4c0a5C",


why would the address change?

dustinxie · 2022-06-29T22:43:26Z

api/coreservice.go

 	accountMeta := &iotextypes.AccountMeta{
 		Address:      addrStr,
 		Balance:      state.Balance.String(),
-		Nonce:        state.Nonce,


for safety better do this in another PR as TODO mentioned, my worry is that this might breaking the API behavior, some user may be using meta.Nonce from returned value and this will break their code?

dustinxie · 2022-06-29T22:47:39Z

state/account.go

+		return st.nonce
+	default: // 0
+		return st.nonce + 1
+	}


same as in SetNonce:

case 0: return st.nonce + 1 default: return ErrUnknownAccountType

* upstream/master: (24 commits) account type with zero init nonce (iotexproject#3387) [api] Separate Server and Server Handler (iotexproject#3485) [ioctl] Build hdwallet derive command line into new ioctl (iotexproject#3418) [ioctl] Build hdwallet create command line into new ioctl (iotexproject#3470) [makefile] add go mod tidy (iotexproject#3471) [api] update chain metrics (iotexproject#3484) remove config.EVMNetworkID() (iotexproject#3460) [filedao] remove checkMasterChainDBFile() (iotexproject#3463) [api] add crashlog (iotexproject#3456) [api] Move generateBlockMeta to grpcserver.go (iotexproject#3303) [ioctl] Build action hash command line into new ioctl (iotexproject#3425) [ioctl] Build hdwallet export command line into new ioctl (iotexproject#3423) [ioctl] Refactor nodereward command in new ioctl (iotexproject#3416) [ioctl] Cleanup TestNewNodeDelegateCmd (iotexproject#3421) [blockchain] Remove BoltDBDaoOption (iotexproject#3465) remove InMemDaoOption (iotexproject#3464) [action] add evm london test (iotexproject#3402) [ioctl] create main for ioctl/newcmd (iotexproject#3296) [ioctl] Build block bucket command line into new ioctl (iotexproject#3386) [ioctl] Build hdwallet import command line into new ioctl (iotexproject#3419) ...

CoderZhi force-pushed the nonce branch 3 times, most recently from 146f7da to d63b9a1 Compare May 15, 2022 07:35

dustinxie reviewed May 18, 2022

View reviewed changes

action/protocol/rewarding/protocol.go Outdated Show resolved Hide resolved

dustinxie reviewed May 18, 2022

View reviewed changes

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

dustinxie reviewed May 18, 2022

View reviewed changes

CoderZhi marked this pull request as ready for review May 18, 2022 06:23

CoderZhi requested a review from a team as a code owner May 18, 2022 06:23

CoderZhi force-pushed the nonce branch 9 times, most recently from 6881b43 to fb78e66 Compare May 23, 2022 17:26

CoderZhi force-pushed the nonce branch 3 times, most recently from 27fb160 to 1bc02b7 Compare June 10, 2022 07:05

CoderZhi closed this Jun 13, 2022

CoderZhi reopened this Jun 13, 2022

dustinxie reviewed Jun 13, 2022

View reviewed changes

Liuhaai reviewed Jun 27, 2022

View reviewed changes

state/factory/workingset.go Show resolved Hide resolved

dustinxie linked an issue Jun 28, 2022 that may be closed by this pull request

nonce-0 account #3477

Closed

Liuhaai reviewed Jun 28, 2022

View reviewed changes

action/protocol/poll/util.go Show resolved Hide resolved

Liuhaai reviewed Jun 28, 2022

View reviewed changes

Liuhaai approved these changes Jun 28, 2022

View reviewed changes

dustinxie reviewed Jun 29, 2022

View reviewed changes

iotexproject deleted a comment from CoderZhi Jun 29, 2022

dustinxie reviewed Jun 29, 2022

View reviewed changes

dustinxie approved these changes Jul 1, 2022

View reviewed changes

CoderZhi added 11 commits June 30, 2022 17:57

set balance via SubBalance and AddBalance

99f3ffb

create account with NewAccount and NewEmptyAccount

8598056

refactor state.nonce

478d264

skip update nonce and gas for system actions

b397bcb

check nonce value

878c583

account type

2bae970

read state with context

524115e

address comments

79aeaee

fix unit test

59509d1

address comment

877cf1a

address comment

50c36f9

CoderZhi force-pushed the nonce branch from 77f9539 to 50c36f9 Compare July 1, 2022 01:01

Merge branch 'master' into nonce

fe5c397

CoderZhi merged commit 7382ab7 into master Jul 2, 2022

CoderZhi deleted the nonce branch July 2, 2022 05:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

account type with zero init nonce #3387

account type with zero init nonce #3387

CoderZhi commented May 13, 2022 •

edited

Loading

codecov bot commented May 13, 2022 •

edited

Loading

dustinxie May 18, 2022 •

edited

Loading

dustinxie May 18, 2022

dustinxie May 18, 2022 •

edited

Loading

dustinxie May 18, 2022

dustinxie May 18, 2022

dustinxie Jun 13, 2022

CoderZhi Jun 13, 2022

dustinxie Jun 13, 2022

dustinxie Jun 13, 2022

Liuhaai Jun 28, 2022

Liuhaai Jun 28, 2022

dustinxie Jun 29, 2022

dustinxie Jun 29, 2022 •

edited

Loading

dustinxie Jun 29, 2022

dustinxie Jun 29, 2022

dustinxie Jun 29, 2022

account type with zero init nonce #3387

account type with zero init nonce #3387

Conversation

CoderZhi commented May 13, 2022 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented May 13, 2022 • edited Loading

Codecov Report

dustinxie May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinxie May 18, 2022 • 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

dustinxie Jun 29, 2022 • 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

CoderZhi commented May 13, 2022 •

edited

Loading

codecov bot commented May 13, 2022 •

edited

Loading

dustinxie May 18, 2022 •

edited

Loading

dustinxie May 18, 2022 •

edited

Loading

dustinxie Jun 29, 2022 •

edited

Loading