Updated minimum staking related logic #920

jeongkyun-oh · 2021-03-12T00:05:24Z

Proposed changes

Updated the minimum staking update flag available
Removed the validators who do not have minimum staking amount of KLAY from committees/proposers
- Case 1. Some members do not have enough KLAYs
  - Members who do not have enough KLAYs cannot be committee as well as proposers
- Case 2. No validator has enough KLAYs
  - All members become the committee and a proposer is chosen from them
- Case 3. Governing node in single mode doesn't have enough KLAYs
  - No one can vote until governing node has enough KLAYs
  - This case will be handled in another PR. In single mode, the governing node will always be in proposers/committee.
Updated for backward-incompatible change

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

consensus/istanbul/backend/engine.go

aidan-kwon · 2021-03-16T05:33:43Z

Case 3. Governing node in single mode doesn't have enough KLAYs

No one can vote until governing node has enough KLAYs

@KimKyungup @jeongkyun-oh The third policy probably becomes a huddle for Service Chain or Private Chain operators. How about letting "Governing node in single mode" propose a block?

consensus/istanbul/validator/weighted.go

blockchain/genesis.go

params/config.go

consensus/istanbul/validator/weighted.go

consensus/istanbul/validator/validator.go

KimKyungup · 2021-03-22T00:21:28Z

The third policy probably becomes a huddle for Service Chain or Private Chain operators. How about letting "Governing node in single mode" propose a block?

@jeongkyun-oh @aidan-kwon
Does this feature affect to all governing modes?
If the default minimum staking amount is zero, I think it's ok. But the feature of governing mode should be documented and discussed.

ehnuje · 2021-03-22T04:35:44Z

governance/default.go

+	if minimumStakingAmount, ok := new(big.Int).SetString(gov.MinimumStake(), 10); ok {
+		params.SetMinimumStakingAmount(minimumStakingAmount)
+	}


Same as updateMinimumStakingAmount?

Right, but updateMinimumStakingAmount is a trigger function for voting. There is no proper key and value for this.

consensus/istanbul/validator/weighted.go

ehnuje · 2021-03-22T04:47:10Z

consensus/istanbul/backend/engine.go

+		// they will not be in the committee as well as the proposer candidates list.
+		pHeader := chain.GetHeaderByNumber(params.CalcStakingBlockNumber(snap.Number + 1))
+		if pHeader != nil {
+			if err := snap.ValSet.TransitionValidators(pHeader.Hash(), pHeader.Number.Uint64()); err != nil {


Is this an error we can ignore and does not make any problem?

The logic is updated and moved to into Refresh function. Please take another look.

consensus/istanbul/validator/weighted.go

ehnuje · 2021-03-22T04:59:03Z

consensus/istanbul/validator/weighted.go

+	}
+
+	if len(newWeightedValidators) <= 0 {
+		return newWeightedDemoted, newDemotedStaking, []*weightedValidator{}, []float64{}


Why do we return demoted validators and their staking info in this place?

If no validator has enough KLAYs, the demoted validators can still propose blocks to prevent them from not creating blocks.

consensus/istanbul/validator/weighted.go

ehnuje · 2021-03-22T05:20:29Z

consensus/istanbul/validator/weighted.go

@@ -607,17 +656,62 @@ func (valSet *weightedCouncil) Refresh(hash common.Hash, blockNum uint64) error
 	return nil
 }

+// updateValidators converts weighted validator slice to istanbul.Validators and sets them to the council.
+func (valSet *weightedCouncil) updateValidators(validators []*weightedValidator, demoted []*weightedValidator) {


Just a minor comment, but I think set would be more appropriate in this case.

set -> does nothing or little inside the function

update -> does something inside the function

Updated. Thanks!

consensus/istanbul/validator/weighted.go

jeongkyun-oh · 2021-04-13T09:17:15Z

@aidan-kwon As we talked f2f, I will update the following cases.

Comment: The method NewValidatorSet shouldn't be called.
Comment: The validatorMu is used for validators and demotedValidators.
The Refresh method should be called for every block, so it would be better to move into apply method.
The unit test for snapshot is added.

jeongkyun-oh · 2021-04-15T01:32:54Z

@aidan-kwon @ehnuje @KimKyungup @yoomee1313 PTAL

KimKyungup · 2021-04-15T05:50:39Z

consensus/istanbul/backend/snapshot.go

 		snap.ValSet, snap.Votes, snap.Tally = gov.HandleGovernanceVote(snap.ValSet, snap.Votes, snap.Tally, header, validator, addr)
+		if policy == uint64(params.WeightedRandom) {
+			// Snapshot of block N (Snapshot_N) should contain proposers for N+1 and following blocks.
+			// And proposers for Block N+1 can be calculated from the nearest previous proposersUpdateInterval block.
+			// Let's refresh proposers in Snapshot_N using previous proposersUpdateInterval block for N+1, if not updated yet.
+			pHeader := chain.GetHeaderByNumber(params.CalcProposerBlockNumber(number + 1))
+			if pHeader != nil {
+				if err := snap.ValSet.Refresh(pHeader.Hash(), pHeader.Number.Uint64(), chain.Config()); err != nil {
+					// There are three error cases and they just don't refresh proposers
+					// (1) no validator at all
+					// (2) invalid formatted hash
+					// (3) no staking info available
+					logger.Trace("Skip refreshing proposers while creating snapshot", "snap.Number", snap.Number, "pHeader.Number", pHeader.Number.Uint64(), "err", err)
+				}
+			} else {
+				logger.Trace("Can't refreshing proposers while creating snapshot due to lack of required header", "snap.Number", snap.Number)
+			}
+		}


Do we need a lock for atomicity of snap.ValSet? @jeongkyun-oh

I think no need to lock snap.ValSet which is accessed synchronously.snap.ValSet is an interface for weightedCouncil. If you worry about the validators and demotedValidators, the validatorsMu protects them in weightedCouncil.

jeongkyun-oh self-assigned this Mar 12, 2021

jeongkyun-oh requested review from aidan-kwon, ehnuje, KimKyungup, joowon-byun and yoomee1313 as code owners March 12, 2021 00:05

KimKyungup reviewed Mar 12, 2021

View reviewed changes

consensus/istanbul/backend/engine.go Outdated Show resolved Hide resolved

KimKyungup reviewed Mar 16, 2021

View reviewed changes

consensus/istanbul/validator/weighted.go Outdated Show resolved Hide resolved

KimKyungup reviewed Mar 16, 2021

View reviewed changes

consensus/istanbul/validator/weighted.go Outdated Show resolved Hide resolved

KimKyungup reviewed Mar 16, 2021

View reviewed changes

consensus/istanbul/validator/weighted.go Outdated Show resolved Hide resolved

KimKyungup reviewed Mar 16, 2021

View reviewed changes

consensus/istanbul/validator/weighted.go Outdated Show resolved Hide resolved

aidan-kwon reviewed Mar 16, 2021

View reviewed changes

blockchain/genesis.go Outdated Show resolved Hide resolved

aidan-kwon reviewed Mar 16, 2021

View reviewed changes

params/config.go Outdated Show resolved Hide resolved

aidan-kwon reviewed Mar 16, 2021

View reviewed changes

consensus/istanbul/validator/weighted.go Outdated Show resolved Hide resolved

aidan-kwon reviewed Mar 17, 2021

View reviewed changes

consensus/istanbul/validator/weighted.go Outdated Show resolved Hide resolved

aidan-kwon reviewed Mar 17, 2021

View reviewed changes

consensus/istanbul/validator/weighted.go Outdated Show resolved Hide resolved

aidan-kwon reviewed Mar 17, 2021

View reviewed changes

consensus/istanbul/validator/weighted.go Show resolved Hide resolved

jeongkyun-oh requested review from KimKyungup and aidan-kwon March 19, 2021 05:34

jeongkyun-oh force-pushed the KLT-273-remove-poor-validator3 branch from 80e11ec to 1aac62b Compare March 19, 2021 06:06

aidan-kwon reviewed Mar 19, 2021

View reviewed changes

consensus/istanbul/validator/validator.go Outdated Show resolved Hide resolved

jeongkyun-oh force-pushed the KLT-273-remove-poor-validator3 branch from 1aac62b to 077152e Compare March 22, 2021 07:41

ehnuje reviewed Mar 23, 2021

View reviewed changes

jeongkyun-oh force-pushed the KLT-273-remove-poor-validator3 branch 5 times, most recently from 63db488 to 2c8e809 Compare March 29, 2021 06:13

jeongkyun-oh added 12 commits April 14, 2021 22:01

Fixed bug

57864b9

Updated minimum staking flag to work

4c68631

Implemented demoted validators for minimum staking

3490ed2

Added demoted validators to snapshot persistence

6ddbf93

Updated addvalidator/removevalidator

c32cc16

Make minimum staking vote working

18ee26e

Added hard-fork logic for refresh method

eae67bc

Simplified incompatible change logic

dd99cfc

Reflected the review from Melvin

b2d08a5

Add demoted validator as valid peer

a55f39e

Moved Refresh method to apply

56f5595

Added comments

71ff89a

jeongkyun-oh dismissed stale reviews from yoomee1313, ehnuje, and KimKyungup via 71ff89a April 14, 2021 13:01

jeongkyun-oh force-pushed the KLT-273-remove-poor-validator3 branch from 4ccdb5a to 71ff89a Compare April 14, 2021 13:01

jeongkyun-oh requested review from ehnuje, KimKyungup and yoomee1313 April 14, 2021 23:50

KimKyungup reviewed Apr 15, 2021

View reviewed changes

KimKyungup approved these changes Apr 15, 2021

View reviewed changes

ehnuje approved these changes Apr 16, 2021

View reviewed changes

yoomee1313 approved these changes Apr 16, 2021

View reviewed changes

jeongkyun-oh merged commit 398a28d into klaytn:dev Apr 26, 2021

jeongkyun-oh deleted the KLT-273-remove-poor-validator3 branch April 26, 2021 08:12

jeongkyun-oh mentioned this pull request May 7, 2021

Updated istanbul consensus engine to verify headers synchronously #957

Merged

9 tasks

yoomee1313 added the istanbul-compatible label May 20, 2021

aidan-kwon mentioned this pull request Oct 18, 2021

[Master] release/v1.7.0 QA Sign-off #1043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated minimum staking related logic #920

Updated minimum staking related logic #920

jeongkyun-oh commented Mar 12, 2021 •

edited

aidan-kwon commented Mar 16, 2021

KimKyungup commented Mar 22, 2021

ehnuje Mar 22, 2021

jeongkyun-oh Mar 25, 2021 •

edited

ehnuje Mar 22, 2021

jeongkyun-oh Mar 25, 2021

ehnuje Mar 22, 2021

jeongkyun-oh Mar 25, 2021

This comment was marked as resolved.

ehnuje Mar 22, 2021

jeongkyun-oh Mar 25, 2021

jeongkyun-oh commented Apr 13, 2021 •

edited

jeongkyun-oh commented Apr 15, 2021

KimKyungup Apr 15, 2021

jeongkyun-oh Apr 15, 2021

Updated minimum staking related logic #920

Updated minimum staking related logic #920

Conversation

jeongkyun-oh commented Mar 12, 2021 • edited

Proposed changes

Types of changes

Checklist

aidan-kwon commented Mar 16, 2021

KimKyungup commented Mar 22, 2021

Choose a reason for hiding this comment

jeongkyun-oh Mar 25, 2021 • 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

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeongkyun-oh commented Apr 13, 2021 • edited

jeongkyun-oh commented Apr 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeongkyun-oh commented Mar 12, 2021 •

edited

jeongkyun-oh Mar 25, 2021 •

edited

jeongkyun-oh commented Apr 13, 2021 •

edited