[staking] fix weighted votes of contract staking bucket #3936

envestcc · 2023-10-10T11:21:30Z

Description

Currently, the IIP-13 contract staking bucket does not take into account the impact of the staking duration and stake-lock when calculating the voting power. It only considers the amount of IOTX staked. However, it should actually use the similar voting logic as the native staking bucket.

It's a hard-fork at RedseaBlockHeight.

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 · 2023-10-10T11:37:13Z

Codecov Report

Merging #3936 (553fbd9) into master (e1f0636) will increase coverage by 0.71%.
Report is 86 commits behind head on master.
The diff coverage is 74.34%.

❗ Current head 553fbd9 differs from pull request most recent head abc14e6. Consider uploading reports for the commit abc14e6 to get more accurate results

@@            Coverage Diff             @@
##           master    #3936      +/-   ##
==========================================
+ Coverage   75.38%   76.09%   +0.71%     
==========================================
  Files         303      328      +25     
  Lines       25923    27959    +2036     
==========================================
+ Hits        19541    21275    +1734     
- Misses       5360     5591     +231     
- Partials     1022     1093      +71

Files	Coverage Δ
action/candidate_register.go	`90.00% <100.00%> (ø)`
action/candidate_update.go	`89.01% <100.00%> (+0.12%)`	⬆️
action/claimreward.go	`90.62% <100.00%> (-0.42%)`	⬇️
action/depositreward.go	`87.50% <100.00%> (-0.56%)`	⬇️
action/protocol/context.go	`67.91% <100.00%> (+0.73%)`	⬆️
action/protocol/execution/evm/evmstatedbadapter.go	`66.77% <ø> (ø)`
action/protocol/execution/protocol.go	`42.10% <100.00%> (ø)`
action/protocol/rewarding/reward.go	`90.07% <100.00%> (+0.60%)`	⬆️
...tion/protocol/staking/candidate_buckets_indexer.go	`89.31% <100.00%> (-0.47%)`	⬇️
...col/staking/ethabi/stake_composite_bucketscount.go	`100.00% <100.00%> (ø)`
... and 66 more

... and 5 files with indirect coverage changes

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

CoderZhi · 2023-10-11T01:05:53Z

action/protocol/staking/vote_bucket.go

 	remainingTime := v.StakedDuration.Seconds()
+	if !v.isNative() {
+		// according to produce one block per 5 seconds
+		remainingTime = float64(v.StakedDurationBlockNumber) * 5


don't hard code constant

yes, by maintain the StakedDuration field also for the nft bucket, we can remove the additional case here.

CoderZhi · 2023-10-11T01:08:25Z

blockindex/contractstaking/indexer.go

 	}
 )

 // NewContractStakingIndexer creates a new contract staking indexer
-func NewContractStakingIndexer(kvStore db.KVStore, contractAddr string, contractDeployHeight uint64) (*Indexer, error) {
+func NewContractStakingIndexer(kvStore db.KVStore, contractAddr string, contractDeployHeight uint64, voteConsts genesis.VoteWeightCalConsts) (*Indexer, error) {


pass in the calc function instead

dustinxie · 2023-10-11T04:02:58Z

action/protocol/staking/vote_bucket.go

 	remainingTime := v.StakedDuration.Seconds()
+	if !v.isNative() {


for NFT bucket, does it have v.StakedDuation?

This time, the StakedDuration field is also set for the NFT bucket, based on the StakedDurationBlockNumber* blockInterval.

dustinxie · 2023-10-11T04:10:25Z

blockindex/contractstaking/cache.go

 	}
+
+	calculateVoteWeightFunc func(v *Bucket, selfStake bool) *big.Int


since for NFT buckets, the func is always called withselfStake = false, can remove it from the input param?

calculateVoteWeightFunc func(v *Bucket) *big.Int

yes, it's more concise

dustinxie · 2023-10-11T04:11:11Z

chainservice/builder.go

-	indexer, err := contractstaking.NewContractStakingIndexer(db.NewBoltDB(dbConfig), builder.cfg.Genesis.SystemStakingContractAddress, builder.cfg.Genesis.SystemStakingContractHeight)
+	voteCalcConsts := builder.cfg.Genesis.VoteWeightCalConsts
+	indexer, err := contractstaking.NewContractStakingIndexer(db.NewBoltDB(dbConfig), builder.cfg.Genesis.SystemStakingContractAddress, builder.cfg.Genesis.SystemStakingContractHeight, func(v *staking.VoteBucket, selfStake bool) *big.Int {
+		return staking.CalculateVoteWeight(voteCalcConsts, v, selfStake)


and here

return staking.CalculateVoteWeight(voteCalcConsts, v, false)

Liuhaai · 2023-10-11T22:23:03Z

action/protocol/staking/vote_bucket.go

@@ -223,7 +228,7 @@ func calculateVoteWeight(c genesis.VoteWeightCalConsts, v *VoteBucket, selfStake
 	if remainingTime > 0 {
 		weight += math.Log(math.Ceil(remainingTime/86400)*(1+m)) / math.Log(c.DurationLg) / 100
 	}
-	if selfStake && v.AutoStake && v.StakedDuration >= time.Duration(91)*24*time.Hour {
+	if v.isNative() && selfStake && v.AutoStake && v.StakedDuration >= time.Duration(91)*24*time.Hour {


confused by the condition v.isNative() && selfStake. Does it mean contract can do self staking (isnative: false & selfstake: true) ？

Here, we just want to ensure that the nft bucket does not calculate selfStake weighting. However, this can also be achieved by ensuring that selfStake=false is passed in when bucket is nft. We can revert it.

Liuhaai · 2023-10-11T22:26:14Z

action/protocol/staking/vote_reviser.go

@@ -137,7 +137,7 @@ func (vr *VoteReviser) calculateVoteWeight(csm CandidateStateManager) (Candidate
 		}

 		if cand.SelfStakeBucketIdx == bucket.Index {
-			if err = cand.AddVote(calculateVoteWeight(vr.c, bucket, true)); err != nil {
+			if err = cand.AddVote(CalculateVoteWeight(vr.c, bucket, true)); err != nil {


Will the returned result on L108 be impacted due to the behavior change of CalculateVoteWeight

Liuhaai · 2023-10-11T23:30:06Z

blockindex/contractstaking/cache.go

@@ -74,7 +77,11 @@ func (s *contractStakingCache) CandidateVotes(candidate address.Address, height
 			continue
 		}
 		bt := s.mustGetBucketType(bi.TypeIndex)
-		votes.Add(votes, bt.Amount)
+		if featureCtx.FixContractStakingWeightedVotes {
+			votes.Add(votes, s.config.CalculateVoteWeight(assembleBucket(id, bi, bt, s.config.ContractAddress, s.config.BlockInterval)))


throw error if CalculateVoteWeight is nullptr when Config is empty

Liuhaai · 2023-10-11T23:38:30Z

blockindex/contractstaking/bucket.go

 	"github.com/iotexproject/iotex-core/action/protocol/staking"
 )

 // Bucket defines the bucket struct for contract staking
 type Bucket = staking.VoteBucket

-func assembleBucket(token uint64, bi *bucketInfo, bt *BucketType, contractAddr string) *Bucket {
+func assembleBucket(token uint64, bi *bucketInfo, bt *BucketType, contractAddr string, blockInterval time.Duration) *Bucket {


what are the values of CreateTime, StakeStartTime, and UnstakeStartTime?

they are only for native bucket and keep zero-value for nft bucket. The nft bucket use CreateBlockHeight, StakeStartBlockHeight, UnstakeStartBlockHeight instead.

Liuhaai · 2023-10-11T23:55:13Z

e2etest/contract_staking_test.go

@@ -1868,6 +1876,18 @@ func TestContractStaking(t *testing.T) {
 		})
 	})

+	t.Run("afterRedsea", func(t *testing.T) {


add tests on following heights, like _testRedseaBlockHeight + 10/ + 100, to indicates the change of weighted votes

the votes will not change over time unless unstake or restake.

Liuhaai · 2023-10-11T23:55:32Z

e2etest/contract_staking_test.go

+			ctx := protocol.WithFeatureCtx(protocol.WithBlockCtx(ctx, protocol.BlockCtx{BlockHeight: height}))
+			votes, err := indexer.CandidateVotes(ctx, identityset.Address(7), height)
+			r.NoError(err)
+			r.EqualValues(12245, votes.Int64())


@dustinxie is the value 12245 expected?

millken · 2023-10-11T07:11:06Z

blockindex/contractstaking/cache.go

@@ -74,7 +81,11 @@ func (s *contractStakingCache) CandidateVotes(candidate address.Address, height
 			continue
 		}
 		bt := s.mustGetBucketType(bi.TypeIndex)
-		votes.Add(votes, bt.Amount)
+		if featureCtx.FixContractStakingWeightedVotes {
+			votes.Add(votes, s.calculateVoteWeight(assembleBucket(id, bi, bt, s.contractAddress), false))


selfStake should check bucket ownerAddress == delegate，can not be use false here

selfStake is only for candidate register, not enable to nft bucket

dustinxie · 2023-10-12T18:14:14Z

blockindex/contractstaking/bucket.go

 	vb := Bucket{
 		Index:                     token,
 		StakedAmount:              bt.Amount,
+		StakedDuration:            time.Duration(bt.Duration) * blockInterval,


pls make sure all NFT buckets are processed by this assembleBucket before being passed to CalculateVoteWeight
otherwise v.StakedDuration is not correct

dustinxie · 2023-10-13T04:21:30Z

blockindex/contractstaking/cache.go

@@ -78,6 +78,9 @@ func (s *contractStakingCache) CandidateVotes(ctx context.Context, candidate add
 		}
 		bt := s.mustGetBucketType(bi.TypeIndex)
 		if featureCtx.FixContractStakingWeightedVotes {
+			if s.config.CalculateVoteWeight == nil {
+				return nil, errors.New("calculate vote weight function is not set")
+			}


if we add this check, it should be added at creation time, when the func is passed to Newxxx

sonarqubecloud · 2023-10-13T07:34:30Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
14.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

envestcc added 4 commits October 10, 2023 19:10

add ctx param for CandidateVotes()

0637d79

hard-fork to calculate weighted votes

064d49a

update tests

4703474

update tests

65eb100

envestcc added 2 commits October 10, 2023 19:57

update tests

a3dc2dc

update tests

fbb4680

envestcc marked this pull request as ready for review October 10, 2023 13:47

envestcc requested review from CoderZhi, dustinxie, Liuhaai and a team as code owners October 10, 2023 13:47

CoderZhi requested changes Oct 11, 2023

View reviewed changes

address comments

f88f90d

dustinxie reviewed Oct 11, 2023

View reviewed changes

address comments

dacbaab

envestcc force-pushed the iip13_votes branch from d970600 to dacbaab Compare October 11, 2023 09:40

use DardanellesUpgrade.BlockInterval

878a0df

Liuhaai reviewed Oct 12, 2023

View reviewed changes

millken reviewed Oct 12, 2023

View reviewed changes

dustinxie reviewed Oct 12, 2023

View reviewed changes

enhance robustness

40268f3

dustinxie reviewed Oct 13, 2023

View reviewed changes

address comments

abc14e6

CoderZhi approved these changes Oct 13, 2023

View reviewed changes

dustinxie approved these changes Oct 13, 2023

View reviewed changes

dustinxie merged commit bd7b5e4 into iotexproject:master Oct 13, 2023
2 of 3 checks passed

BrewTestBot mentioned this pull request Oct 27, 2023

ioctl 1.12.0 Homebrew/homebrew-core#152539

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[staking] fix weighted votes of contract staking bucket #3936

[staking] fix weighted votes of contract staking bucket #3936

envestcc commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading

CoderZhi Oct 11, 2023

envestcc Oct 11, 2023

CoderZhi Oct 11, 2023

envestcc Oct 11, 2023

dustinxie Oct 11, 2023

envestcc Oct 11, 2023

dustinxie Oct 11, 2023

envestcc Oct 11, 2023

dustinxie Oct 11, 2023

Liuhaai Oct 11, 2023

envestcc Oct 12, 2023

Liuhaai Oct 11, 2023

Liuhaai Oct 11, 2023

envestcc Oct 12, 2023

Liuhaai Oct 11, 2023

envestcc Oct 12, 2023

Liuhaai Oct 11, 2023

envestcc Oct 13, 2023

Liuhaai Oct 11, 2023

millken Oct 11, 2023

envestcc Oct 12, 2023

dustinxie Oct 12, 2023 •

edited

Loading

dustinxie Oct 13, 2023

envestcc Oct 13, 2023

sonarqubecloud bot commented Oct 13, 2023

		remainingTime := v.StakedDuration.Seconds()
		if !v.isNative() {

		}

		calculateVoteWeightFunc func(v Bucket, selfStake bool) big.Int

[staking] fix weighted votes of contract staking bucket #3936

[staking] fix weighted votes of contract staking bucket #3936

Conversation

envestcc commented Oct 10, 2023 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Oct 10, 2023 • 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

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

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, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 13, 2023

envestcc commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading

dustinxie Oct 12, 2023 •

edited

Loading