[staking] persist cand center to statedb and patchfile #3661

dustinxie · 2022-10-12T16:26:23Z

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)

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

dustinxie · 2022-10-12T17:47:24Z

action/protocol/staking/candidate_center_extra.go

+func (cb *candBase) ownersList() CandidateList {
+	cb.lock.RLock()
+	defer cb.lock.RUnlock()
+	if len(cb.owners) == 0 {


zhi: delete

if ownerList() is called before recordOwner(), cb.owners is nil, so need to populate it

dustinxie · 2022-10-12T17:49:38Z

action/protocol/staking/candidate_center_extra.go

+	}
+	for i, d := range cb.owners {
+		if d.Owner.String() == c.Owner.String() {
+			cb.owners[i] = c.Clone()


zhi: if nameMap and operatorMap could be type of map, why owners is a list?

b/c outside calls GetByName() and GetByOperator(), map better suits that need.
owners is only updated by candidate update/register tx, a list is easier and suffices for the use-case

dustinxie · 2022-10-12T17:52:21Z

action/protocol/staking/candidate_statemanager.go

-		delCandBucketIndex(addr address.Address, index uint64) error
+		putCandidate(*Candidate) error
+		delCandidate(address.Address) error
+		putCandidateList(CandidateList, []byte) error


zhi: why putCandidateList is a function of interface?

b/c we are calling it from the CandidateStateManager interface, for the same reason why putCandidate is a func of interface here

no need to add. They are under the same pkg

dustinxie · 2022-10-12T17:54:13Z

action/protocol/staking/handlers.go

@@ -684,6 +684,7 @@ func (p *Protocol) handleCandidateRegister(ctx context.Context, act *action.Cand
 	if err := csm.Upsert(c); err != nil {
 		return log, nil, csmErrorToHandleError(owner.String(), err)
 	}
+	csm.DirtyView().candCenter.base.recordOwner(c)


zhi: put into csm.Upsert

I also hesitate to do this, but we need to change to Upsert(*Candidate, bool)

a bool flag to indicate a specific purpose (save the correct candidate) is kind of weird here

too many changes

Let me know if you have a better option, or Upsert(*Candidate, bool) is fine, I can modify

dustinxie · 2022-10-12T17:58:08Z

action/protocol/staking/protocol.go

+var (
+	_nameKey     = []byte("name")
+	_operatorKey = []byte("oper")
+	_ownerKey    = []byte("owne")


zhi: using names, operators, owners won't take too many disk spaces

right, disk space is not a concern here
I prefer for these meta keys to have the same length, similar to namespace in filedaov2

im also confused here

dustinxie · 2022-10-12T18:00:45Z

action/protocol/staking/protocol.go

+	name := base.candsInNameMap()
+	op := base.candsInOperatorMap()
+	owners := base.ownersList()
+	if len(name) == 0 || len(op) == 0 || len(owners) == 0 {


zhi: invalid checking

for our real use-case, it is true.
I'd just keep it for safety

dustinxie · 2022-10-12T18:04:19Z

blockchain/config.go

@@ -76,6 +81,7 @@ var (
 		ChainDBPath:            "/var/data/chain.db",
 		TrieDBPatchFile:        "/var/data/trie.db.patch",
 		TrieDBPath:             "/var/data/trie.db",
+		CandsMapPatchFile:      "/var/data/candsmap.patch",


@CoderZhi is this name good? it will create file like candsmap.patch-19xxxx

Liuhaai · 2022-10-12T19:15:58Z

action/protocol/staking/candidate_center_extra.go

+
+package staking
+
+func (cb *candBase) clone() *candBase {


don't create two files for same class.Create another class if a new file is wanted

codecov · 2022-10-12T22:28:47Z

Codecov Report

Merging #3661 (263ff82) into master (e9732a1) will decrease coverage by 0.48%.
The diff coverage is 65.92%.

❗ Current head 263ff82 differs from pull request most recent head cfc602e. Consider uploading reports for the commit cfc602e to get more accurate results

@@            Coverage Diff             @@
##           master    #3661      +/-   ##
==========================================
- Coverage   74.95%   74.47%   -0.49%     
==========================================
  Files         269      269              
  Lines       23819    23925     +106     
==========================================
- Hits        17854    17818      -36     
- Misses       5039     5174     +135     
- Partials      926      933       +7

Impacted Files	Coverage Δ
action/protocol/protocol.go	`0.00% <ø> (ø)`
action/protocol/staking/candidate_statemanager.go	`98.59% <ø> (+1.36%)`	⬆️
blockchain/config.go	`74.54% <ø> (ø)`
db/config.go	`100.00% <ø> (ø)`
db/trie/mptrie/node.go	`100.00% <ø> (ø)`
ioctl/newcmd/action/action.go	`89.09% <ø> (+9.55%)`	⬆️
ioctl/newcmd/node/nodedelegate.go	`70.19% <0.00%> (-0.95%)`	⬇️
ioctl/newcmd/node/nodeprobationlist.go	`90.90% <0.00%> (-4.33%)`	⬇️
action/protocol/staking/candidate_center_extra.go	`13.55% <13.55%> (ø)`
server/itx/server.go	`60.00% <25.00%> (-2.21%)`	⬇️
... and 35 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

dustinxie · 2022-10-13T18:14:12Z

action/protocol/staking/protocol.go

@@ -81,6 +83,9 @@ type (
 		candBucketsIndexer *CandidatesBucketsIndexer
 		voteReviser        *VoteReviser
 		patch              *PatchStore
+		_nameHash          string
+		_opHash            string
+		_ownerHash         string


debug code, will be removed before check-in

dustinxie · 2022-10-13T18:14:18Z

action/protocol/staking/protocol.go

+			println("======= same owner list", p._ownerHash)
+		}
+	}
+


debug code, will be removed before check-in

CoderZhi · 2022-10-13T22:45:26Z

action/protocol/staking/protocol.go

-			BootstrapCandidates:   cfg.BootstrapCandidates,
+			BootstrapCandidates:   cfg.Staking.BootstrapCandidates,
+			PersistCandsMapBlock:  cfg.PersistCandsMapBlock,
+			CandsMapPatchDir:      cfg.CandsMapPatchDir,


delete

}, patch: NewPatchStore(cfg.CandsMapPatchDir),

CoderZhi · 2022-10-13T22:45:52Z

action/protocol/staking/protocol.go

+	if p.patch == nil {
+		p.patch = NewPatchStore(p.config.CandsMapPatchDir)
+	}


CoderZhi · 2022-10-13T23:09:35Z

blockchain/config.go

@@ -28,6 +28,7 @@ type (
 		ChainDBPath            string           `yaml:"chainDBPath"`
 		TrieDBPatchFile        string           `yaml:"trieDBPatchFile"`
 		TrieDBPath             string           `yaml:"trieDBPath"`
+		CandsMapPatchDir       string           `yaml:"candsMapPatchDir"`


StakingPatchDir string yaml:"stakingPatchDir"`

CoderZhi · 2022-10-13T23:13:19Z

blockchain/config.go

@@ -67,6 +68,8 @@ type (
 		WorkingSetCacheSize uint64 `yaml:"workingSetCacheSize"`
 		// StreamingBlockBufferSize
 		StreamingBlockBufferSize uint64 `yaml:"streamingBlockBufferSize"`
+		// PersistCandsMapBlock is the block to persist candidates map
+		PersistCandsMapBlock uint64 `yaml:"persistCandsMapBlock"`


PersistStakingPatchBlock

CoderZhi · 2022-10-13T23:16:00Z

action/protocol/staking/candidate_center_extra.go

+func (cb *candBase) ownersList() CandidateList {
+	cb.lock.RLock()
+	defer cb.lock.RUnlock()
+	return cb.owners


for read, if we do clone in https://github.com/iotexproject/iotex-core/pull/3661/files#diff-17d187e581661db50bb29a2354c11c2d8e05245a5d4ad4a27f9619f6affb6ee7R56, https://github.com/iotexproject/iotex-core/pull/3661/files#diff-17d187e581661db50bb29a2354c11c2d8e05245a5d4ad4a27f9619f6affb6ee7R42, what about here?

name, op := candsInNameMap(), candsInOperatorMap()
name, op will be serialized and saved to DB, so actually no need to clone() for the 2 places you mentioned, clone() is for safety I believe since we don't want to touch the *Candidate in these 2 maps.
for ownerList, the only use of it is to serialize and save to DB, so I think no need to clone the list here (b/c when we add a *Candidate to ownerList, we make a clone, L77 below)

CoderZhi · 2022-10-13T23:17:25Z

action/protocol/staking/candidate_center_extra.go

+		if err := d.Validate(); err != nil {
+			return err
+		}
+		cb.nameMap[d.Name] = d


for write, if we do clone in https://github.com/iotexproject/iotex-core/pull/3661/files#diff-17d187e581661db50bb29a2354c11c2d8e05245a5d4ad4a27f9619f6affb6ee7R72 and https://github.com/iotexproject/iotex-core/pull/3661/files#diff-17d187e581661db50bb29a2354c11c2d8e05245a5d4ad4a27f9619f6affb6ee7R77, we may do the same thing in https://github.com/iotexproject/iotex-core/pull/3661/files#diff-17d187e581661db50bb29a2354c11c2d8e05245a5d4ad4a27f9619f6affb6ee7R77 and https://github.com/iotexproject/iotex-core/pull/3661/files#diff-17d187e581661db50bb29a2354c11c2d8e05245a5d4ad4a27f9619f6affb6ee7R97 as well

dustinxie · 2022-10-14T00:51:32Z

action/protocol/staking/candidate_center_extra.go

+		}
+		cb.operatorMap[d.Operator.String()] = d
+	}
+	cb.owners = owners


owners at right side is created by de-serializing data read from DB, it is a new instance of list, so no need to clone here

sonarcloud · 2022-10-14T03:41:25Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

dustinxie requested a review from a team as a code owner October 12, 2022 16:26

dustinxie force-pushed the mapfix branch 2 times, most recently from d5a0db6 to 69fc63f Compare October 12, 2022 17:46

dustinxie commented Oct 12, 2022

View reviewed changes

raullenchai approved these changes Oct 12, 2022

View reviewed changes

Liuhaai reviewed Oct 12, 2022

View reviewed changes

dustinxie force-pushed the mapfix branch from 69fc63f to ebecbd4 Compare October 12, 2022 22:18

dustinxie force-pushed the mapfix branch from 29a94d7 to 7d34006 Compare October 13, 2022 18:13

dustinxie commented Oct 13, 2022

View reviewed changes

dustinxie force-pushed the mapfix branch from 7d34006 to f2e2dbc Compare October 13, 2022 20:18

CoderZhi approved these changes Oct 13, 2022

View reviewed changes

dustinxie commented Oct 14, 2022

View reviewed changes

dustinxie and others added 3 commits October 13, 2022 21:00

add PersistStakingPatchBlock into blockchain.Config

02f8190

[protocol] add PreCommit() interface

ebf4e0c

[staking] persist name/operator map and owner list to stateDB

af8a83b

dustinxie force-pushed the mapfix branch from f2e2dbc to af7b7f1 Compare October 14, 2022 02:17

[staking] patch for staking protocol (#3661)

cfc602e

dustinxie force-pushed the mapfix branch from af7b7f1 to cfc602e Compare October 14, 2022 03:40

dustinxie merged commit cfc602e into master Oct 14, 2022

dustinxie deleted the mapfix branch October 14, 2022 03:57

dustinxie mentioned this pull request Oct 14, 2022

add precommit #3660

Closed

dustinxie mentioned this pull request Oct 14, 2022

1.8.4 patch db #3658

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[staking] persist cand center to statedb and patchfile #3661

[staking] persist cand center to statedb and patchfile #3661

dustinxie commented Oct 12, 2022

dustinxie Oct 12, 2022

dustinxie Oct 12, 2022

dustinxie Oct 13, 2022

dustinxie Oct 12, 2022

dustinxie Oct 12, 2022

dustinxie Oct 12, 2022

dustinxie Oct 12, 2022

Liuhaai Oct 12, 2022

dustinxie Oct 13, 2022

dustinxie Oct 12, 2022

dustinxie Oct 12, 2022 •

edited

Loading

dustinxie Oct 12, 2022

dustinxie Oct 12, 2022

Liuhaai Oct 12, 2022

dustinxie Oct 13, 2022

dustinxie Oct 12, 2022

dustinxie Oct 12, 2022

dustinxie Oct 12, 2022

Liuhaai Oct 12, 2022 •

edited

Loading

codecov bot commented Oct 12, 2022 •

edited

Loading

dustinxie Oct 13, 2022

dustinxie Oct 13, 2022

CoderZhi Oct 13, 2022

CoderZhi Oct 13, 2022

CoderZhi Oct 13, 2022

CoderZhi Oct 13, 2022

CoderZhi Oct 13, 2022

dustinxie Oct 14, 2022 •

edited

Loading

CoderZhi Oct 13, 2022

dustinxie Oct 14, 2022

sonarcloud bot commented Oct 14, 2022

[staking] persist cand center to statedb and patchfile #3661

[staking] persist cand center to statedb and patchfile #3661

Conversation

dustinxie commented Oct 12, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinxie Oct 12, 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

Liuhaai Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 12, 2022 • 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

dustinxie Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 14, 2022

dustinxie Oct 12, 2022 •

edited

Loading

Liuhaai Oct 12, 2022 •

edited

Loading

codecov bot commented Oct 12, 2022 •

edited

Loading

dustinxie Oct 14, 2022 •

edited

Loading