Clean up candidates #3696

CoderZhi · 2022-11-17T23:22:09Z

After hardfork, fix logic bug of updating candidate name/operator
Clean up candidate names and operator addresses

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 · 2022-11-17T23:32:57Z

Codecov Report

Merging #3696 (b158f8d) into master (263ff82) will decrease coverage by 0.73%.
The diff coverage is 87.24%.

@@            Coverage Diff             @@
##           master    #3696      +/-   ##
==========================================
- Coverage   74.47%   73.73%   -0.74%     
==========================================
  Files         269      276       +7     
  Lines       23925    24187     +262     
==========================================
+ Hits        17818    17835      +17     
- Misses       5174     5444     +270     
+ Partials      933      908      -25

Impacted Files	Coverage Δ
action/protocol/context.go	`66.40% <0.00%> (-1.60%)`	⬇️
action/protocol/staking/candidate_statereader.go	`53.94% <ø> (-3.38%)`	⬇️
db/db_bolt.go	`68.53% <0.00%> (-3.60%)`	⬇️
ioctl/newcmd/action/actiontransfer.go	`100.00% <ø> (ø)`
ioctl/newcmd/action/stake2withdraw.go	`95.74% <ø> (ø)`
ioctl/newcmd/action/xrc20transferfrom.go	`100.00% <ø> (ø)`
ioctl/newcmd/did/didgethash.go	`100.00% <ø> (ø)`
ioctl/newcmd/did/didgeturi.go	`100.00% <ø> (ø)`
server/itx/server.go	`62.20% <ø> (+2.20%)`	⬆️
action/protocol/staking/handlers.go	`74.47% <33.33%> (-0.64%)`	⬇️
... and 23 more

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

dustinxie · 2022-11-18T00:40:49Z

action/protocol/staking/candidate_statemanager.go

@@ -56,7 +56,7 @@ type (
 Upsert(*Candidate) error
 CreditBucketPool(*big.Int) error
 DebitBucketPool(*big.Int, bool) error
- Commit() error
+ Commit(bool) error


this fix should only happen one-time at the HF height, why need to add it as an extra input

dustinxie · 2022-11-18T00:42:28Z

action/protocol/staking/vote_reviser.go

-func (vr *VoteReviser) storeToCache(height uint64, cands CandidateList) {
- vr.cache[height] = cands
+func (vr *VoteReviser) storeToCache(height uint64, dirty, cands CandidateList) {
+ vr.cache[height] = [2]CandidateList{dirty, cands}


cache should only contain the correct candidates after revise, we should be able to do the fix w/o modifying these low-level implementations

dustinxie · 2022-11-18T00:43:14Z

action/protocol/staking/vote_reviser.go

+ return err
+ }
+ log.L().Info("committed revise action",
+ zap.String("name", cand.Name), zap.String("votes", cand.Votes.String()))


we should be able to do the fix w/o modifying these low-level implementations

dustinxie · 2022-11-18T00:45:34Z

action/protocol/staking/candidate_center.go

@@ -440,14 +444,20 @@ func (cb *candBase) all() CandidateList {
 return list
 }

-func (cb *candBase) commit(change *candChange) (int, error) {
+func (cb *candBase) commit(change *candChange, keepAliasBug bool) (int, error) {


if we could have the flag in the candBase, then I think it's inefficient to pass it down every time

dustinxie · 2022-11-18T00:45:50Z

action/protocol/staking/candidate_center.go

@@ -17,7 +17,8 @@ import (
 type (
 // candChange captures the change to candidates
 candChange struct {
- dirty map[string]*Candidate
+ candidates []*Candidate
+ dirty map[string]*Candidate


we should be able to do the fix w/o modifying these low-level implementations

dustinxie · 2022-11-18T16:23:40Z

action/protocol/staking/vote_reviser.go

+ dirty = append(dirty, c)
+ }
+ for _, c := range csm.DirtyView().candCenter.base.operatorMap {
+ dirty = append(dirty, c)


reviser should not bother/deal with candCenter.base.operatorMap

guo · 2022-11-18T22:59:43Z

action/protocol/staking/candidate_center.go

+ return nil
+}
+
+// LegacyCommit writes the change into base with legacy logic


It is cleaner than the bool argument.

Liuhaai · 2022-11-19T07:40:53Z

action/protocol/staking/vote_reviser.go

+ ownerMap[addr] = owner
+ }
+ for _, c := range cands {
+ if owner, ok := ownerMap[c.Owner.String()]; ok {


if owner, ok := csm.DirtyView().candCenter.base.ownerMap[c.Owner.String()]; ok {}

Liuhaai · 2022-11-19T08:15:54Z

Discussed offline. The idea of this pr is easier to understand

dustinxie · 2022-11-20T01:36:14Z

action/protocol/staking/vote_reviser.go

+ }
+ for _, c := range csm.DirtyView().candCenter.base.operatorMap {
+ retval = append(retval, c)
+ }


we should not use name/operatorMap here? don't quite understand the code.

here's my understanding of the intended logic per our discussion during v1.8.4 fix:

input cands is the list of candidates (with vote correctly revised) in the statedb, as done in v1.8.4 fix, we saved the list of candidates that have been called CandidateRegister/Update actions during the affected period, these are candidates that can potentially have an alias in the candidate center

_, _, owners := readCandCenterStateFromStateDB()

so on top of cands, update the name/operator/reward of owners, that is what we need to do

another comment is that VoteReviser logic is clean, it just reads candidates/buckets from statedb, and tally votes again.

now it needs to deal with csm.DirtyView().candCenter.xxx, this adds unnecessary code dependency/coupling

in other words, VoteReviser is designed to work w/o knowing the existence of candCenter, then it should be kept that way

iotex-core/action/protocol/staking/protocol.go

Line 182 in e0578da

name, operator, owners, err := readCandCenterStateFromStateDB(sr, height)

has called and loaded the data.

iotex-core/action/protocol/staking/handlers.go

Line 689 in ee04a05

csm.DirtyView().candCenter.base.recordOwner(c)

candcenter has been used in various places, which is a legacy issue

yes, ideally we should not use csm.DirtyView().candCenter.xxx in protocol.go and handlers.go
so we should not expand it into vote_reviser.go if possible

name, operator, owners, err := readCandCenterStateFromStateDB(sr, height), will NOT be called at next HF height
even if it's read, seems owners is not used in this PR

sonarcloud · 2022-11-28T22:41:10Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

dustinxie added 7 commits October 31, 2022 15:15

[staking] fix name and operator map alias

6a02da4

add test

d72ffec

address comment

ad06fe9

address comment

1a892d7

Merge branch 'master' into mapfix

e939a1b

remove candMap

4940b24

revise candidates

cbc509d

CoderZhi requested a review from a team as a code owner November 17, 2022 23:22

dustinxie reviewed Nov 18, 2022

View reviewed changes

address comments

e14c507

guo reviewed Nov 18, 2022

View reviewed changes

Liuhaai reviewed Nov 19, 2022

View reviewed changes

Liuhaai approved these changes Nov 19, 2022

View reviewed changes

dustinxie reviewed Nov 20, 2022

View reviewed changes

raullenchai approved these changes Nov 28, 2022

View reviewed changes

CoderZhi added 2 commits November 28, 2022 14:34

address comment

93d5a61

Merge branch 'master' into revise_candidates

b158f8d

CoderZhi merged commit c316b81 into master Nov 29, 2022

dustinxie deleted the revise_candidates branch December 17, 2022 04:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up candidates #3696

Clean up candidates #3696

CoderZhi commented Nov 17, 2022

codecov bot commented Nov 17, 2022 •

edited

Loading

dustinxie Nov 18, 2022

dustinxie Nov 18, 2022 •

edited

Loading

dustinxie Nov 18, 2022

dustinxie Nov 18, 2022

dustinxie Nov 18, 2022

dustinxie Nov 18, 2022

guo Nov 18, 2022

Liuhaai Nov 19, 2022

Liuhaai commented Nov 19, 2022

dustinxie Nov 20, 2022 •

edited

Loading

dustinxie Nov 20, 2022 •

edited

Loading

CoderZhi Nov 22, 2022

dustinxie Nov 24, 2022

dustinxie Nov 28, 2022 •

edited

Loading

sonarcloud bot commented Nov 28, 2022

Clean up candidates #3696

Clean up candidates #3696

Conversation

CoderZhi commented Nov 17, 2022

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Nov 17, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

dustinxie Nov 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

Liuhaai commented Nov 19, 2022

dustinxie Nov 20, 2022 • edited Loading

Choose a reason for hiding this comment

dustinxie Nov 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinxie Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Nov 28, 2022

codecov bot commented Nov 17, 2022 •

edited

Loading

dustinxie Nov 18, 2022 •

edited

Loading

dustinxie Nov 20, 2022 •

edited

Loading

dustinxie Nov 20, 2022 •

edited

Loading

dustinxie Nov 28, 2022 •

edited

Loading