Remove vote when checkVote() fails in HandleGovernanceVote #1367

ian0371 · 2022-05-11T02:43:42Z

Proposed changes

Adding an existing validator via governance.vote("governance.addvalidator", existing_val_addr) has a bug. Refer to the log below.
This is caused by https://github.com/klaytn/klaytn/blob/dev/governance/handler.go#L365 where checkVote() checks if the address to be added already exists in the validator set, in which case it returns WITHOUT REMOVING THE VOTE.
This fix removes the vote when checkVote() fails. (remove in this context means setting Casted to true)

Bug log

Initial setting
For an initial council, we remove one validator so that we can add it back.

> klay.getCouncil()
["0x19c032589b76ac395d3a713088e67f9ce9edf1c6", "0x353a5fd7cd9b8d0fe3e7c65d6c3d229cd0f14533", "0xd4ca4f3f2d80fb5b2f569df45d4c28a98f48a289", "0xe0c91bc72396b6c1e937ef717bf59a90c558f285"]
> addr = klay.getCouncil()[0]
"0x19c032589b76ac395d3a713088e67f9ce9edf1c6"
> governance.vote("governance.removevalidator", addr)
"Your vote is prepared. It will be put into the block header or applied when your node generates a block as a proposer."
> governance.myVotes
[{
    BlockNum: 6,
    Casted: true,
    Key: "governance.removevalidator",
    Value: "0x19c032589b76ac395d3a713088e67f9ce9edf1c6"
}]

add validator (1)

> governance.vote("governance.addvalidator", addr)
"Your vote is prepared. It will be put into the block header or applied when your node generates a block as a proposer."
> governance.myVotes
[{
    BlockNum: 6,
    Casted: true,
    Key: "governance.removevalidator",
    Value: "0x19c032589b76ac395d3a713088e67f9ce9edf1c6"
}, {
    BlockNum: 16,
    Casted: true,
    Key: "governance.addvalidator",
    Value: "0x19c032589b76ac395d3a713088e67f9ce9edf1c6"
}]

add validator (2) with an existing address

> governance.vote("governance.addvalidator", addr)
"Your vote is prepared. It will be put into the block header or applied when your node generates a block as a proposer."
> governance.myVotes
[{
    BlockNum: 0,
    Casted: false,
    Key: "governance.addvalidator",
    Value: "0x19c032589b76ac395d3a713088e67f9ce9edf1c6"
}, {
    BlockNum: 6,
    Casted: true,
    Key: "governance.removevalidator",
    Value: "0x19c032589b76ac395d3a713088e67f9ce9edf1c6"
}]

We can see that the second vote is not cast and it keeps appearing in every block the voter becomes the proposer.

> klay.getBlock(16).voteData
"0xf84294353a5fd7cd9b8d0fe3e7c65d6c3d229cd0f1453397676f7665726e616e63652e61646476616c696461746f729419c032589b76ac395d3a713088e67f9ce9edf1c6"
> klay.getBlock(22).voteData
"0xf84294353a5fd7cd9b8d0fe3e7c65d6c3d229cd0f1453397676f7665726e616e63652e61646476616c696461746f729419c032589b76ac395d3a713088e67f9ce9edf1c6"
> klay.getBlock(25).voteData
"0xf84294353a5fd7cd9b8d0fe3e7c65d6c3d229cd0f1453397676f7665726e616e63652e61646476616c696461746f729419c032589b76ac395d3a713088e67f9ce9edf1c6"
> klay.getBlock(28).voteData
"0xf84294353a5fd7cd9b8d0fe3e7c65d6c3d229cd0f1453397676f7665726e616e63652e61646476616c696461746f729419c032589b76ac395d3a713088e67f9ce9edf1c6"

Types of changes

Please put an x in the boxes related to your change.

Bugfix
New feature or enhancement
Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

I have read the CONTRIBUTING GUIDELINES doc
I have signed the CLA
Lint and unit tests pass locally with my changes ($ make test)
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)
Any dependent changes have been merged and published in downstream modules

Related issues

Please leave the issue numbers or links related to this PR here.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

blukat29 · 2022-05-11T06:05:27Z

@ian0371 Can you add a scenario test case in TestSnapshot_Validators_AddRemove? (https://github.com/klaytn/klaytn/blob/dev/consensus/istanbul/backend/engine_test.go#L995)
For instance, addval -> addval -> removeval -> removeval case.

I see that unit test for HandleGovernanceVote checks crucial conditions. But having an intuitive scenario as a test would be more comprehensible.

governance/handler.go

hyunsooda · 2022-05-12T04:58:54Z

Let me ask a simple question. Is it invalid removing duplicated vote immediately from Vote()? Why did you delegate it to HandleGovernanceVote()?

hyunsooda · 2022-05-12T04:59:47Z

Each independent test case seems separated by comment. It looks better to make an arbitrary scope case by case or split them into functions. You may consider this later.

ian0371 · 2022-05-12T07:03:19Z

@hyunsooda

Let me ask a simple question. Is it invalid removing duplicated vote immediately from Vote()? Why did you delegate it to HandleGovernanceVote()?

Good question. Validator set can be fetched from istanbul.snapshot, which is inaccessible from governance package (istanbul/backend package imports governance, so the reverse cannot be done. HandleGovernanceVote() receives valset as a parameter for similar reasons).

Each independent test case seems separated by comment. It looks better to make an arbitrary scope case by case or split them into functions. You may consider this later.

I agree that the current readability is not so great. I'll take this into account later :)

hyunsooda · 2022-05-13T04:30:43Z

@ian0371, Okay. then, does not have a side-effect of this post-processing?

ian0371 · 2022-05-13T05:21:24Z

does not have a side-effect of this post-processing?

No, it does not have a side effect. The removing vote is rarely executed in the real world (i.e., when adding an existing validator).

hyunsooda · 2022-05-13T06:12:00Z

Okay.. thank you.

ehnuje · 2022-05-15T23:00:12Z

consensus/istanbul/backend/engine_test.go

-						nodeKeys = append(nodeKeys, allNodeKeys[i])
-						addrs = append(addrs, allAddrs[i])
+						includeNode(allAddrs[i], allNodeKeys[i])


Why are these lines changed?

@ehnuje Without them, the same node ends up twice in nodeKeys/addrs, resulting in "invalid committed seals".

@ian0371 What I meant to say was that changing the test code because of the core code change looks strange. Do we need to sync the test code logic and the core code logic manually for this test?

@ehnuje Changing these lines is inevitable because the existing code cannot handle the new test cases. You can consider it "fixing a bug".
I could add a new test function to verify them, but instead, I decided to reuse the existing function because most parts (i.e., except for these lines) will be reused. You might not want to go down that road.

governance/handler.go

ian0371 · 2022-05-16T06:14:19Z

Rebased to the latest dev

governance/default_test.go

ehnuje

LGTM, thanks!

zacscoding

Thanks @ian0371 👍

ian0371 requested review from ehnuje, blukat29, hyunsooda, joowon-byun, kjhman21, jimni1222, jiseongnoh, hqjang-pepper, aidan-kwon, aeharvlee, mckim19, yoomee1313, henry-will, JayChoi1736 and zacscoding May 11, 2022 02:43

blukat29 assigned ian0371 May 11, 2022

blukat29 reviewed May 12, 2022

View reviewed changes

governance/handler.go Outdated Show resolved Hide resolved

governance/handler.go Outdated Show resolved Hide resolved

ian0371 requested a review from blukat29 May 13, 2022 05:35

blukat29 previously approved these changes May 13, 2022

View reviewed changes

yoomee1313 previously approved these changes May 13, 2022

View reviewed changes

ehnuje reviewed May 15, 2022

View reviewed changes

ian0371 dismissed stale reviews from yoomee1313 and blukat29 via f1cb3bb May 16, 2022 01:39

ian0371 added 9 commits May 16, 2022 15:12

Refactor code from HandleGovernanceVote into checkValidator

ce410fd

8000

Call removeDuplicatedVote when adding an existing validator

a97a87e

Add addval(existing validator) test for none mode

2fc3e7a

Add addval(existing validator) test for ballot mode

36fb7ce

Check if self is proposer before calling removeDuplicatedVote

177753c

Remove checkValidator function

b786292

Add Validators_AddRemove test in engine_test.go

bea2e71

Update governance.vote return message

c634034

Add warning log before removeDuplicatedVote

80d2ccd

ian0371 force-pushed the fix-addvalidator branch from f1cb3bb to 80d2ccd Compare May 16, 2022 06:12

ian0371 requested review from blukat29, yoomee1313 and ehnuje May 16, 2022 06:53

zacscoding reviewed May 18, 2022

View reviewed changes

governance/default_test.go Show resolved Hide resolved

ian0371 added 2 commits May 18, 2022 10:04

Add removevalidator tests in default_test.go

e268dc1

Add removevalidator-first tests in engine_test.go

5f83554

ehnuje approved these changes May 19, 2022

View reviewed changes

blukat29 approved these changes May 19, 2022

View reviewed changes

zacscoding approved these changes May 23, 2022

View reviewed changes

yoomee1313 approved these changes May 23, 2022

View reviewed changes

ian0371 merged commit 0d5d004 into klaytn:dev May 23, 2022

aidan-kwon mentioned this pull request Aug 22, 2022

[Governance] addvalidator problem #1143

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove vote when checkVote() fails in HandleGovernanceVote #1367

Remove vote when checkVote() fails in HandleGovernanceVote #1367

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Remove vote when checkVote() fails in HandleGovernanceVote #1367

Remove vote when checkVote() fails in HandleGovernanceVote #1367

Uh oh!

Conversation

Proposed changes

Bug log

Types of changes

Checklist

Related issues

Further comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!