[staking] Candidate Register without Staking #4059

envestcc · 2024-01-18T02:50:31Z

Description

We expanded the action CandidateRegister to support registration without staking. However, in the absence of self-staking, they will not be selected as consensus nodes.

Type of change

Please delete options that are not relevant.

Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)

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 · 2024-01-18T03:05:13Z

Codecov Report

Attention: Patch coverage is 56.92308% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 76.36%. Comparing base (e1f0636) to head (ef0e99a).
Report is 193 commits behind head on master.

Files	Patch %	Lines
action/signedaction.go	0.00%	20 Missing ⚠️
action/protocol/staking/handlers.go	80.00%	6 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4059      +/-   ##
==========================================
+ Coverage   75.38%   76.36%   +0.98%     
==========================================
  Files         303      340      +37     
  Lines       25923    29022    +3099     
==========================================
+ Hits        19541    22163    +2622     
- Misses       5360     5754     +394     
- Partials     1022     1105      +83

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CoderZhi · 2024-01-20T10:54:18Z

action/protocol/staking/validations.go

@@ -66,7 +70,11 @@ func (p *Protocol) validateCandidateRegister(ctx context.Context, act *action.Ca
 	}

 	if act.Amount().Cmp(p.config.RegistrationConsts.MinSelfStake) < 0 {
-		return errors.Wrap(action.ErrInvalidAmount, "self staking amount is not valid")
+		featureCtx := protocol.MustGetFeatureCtx(ctx)
+		if featureCtx.CandidateRegisterMustWithStake || act.Amount().Cmp(big.NewInt(0)) != 0 {


if featureCtx.CandidateRegisterMustWithStake always return error?

yes, only featureCtx.CandidateRegisterMustWithStake==false && act.Amount()==0 is valid if act.Amount() < MinSelfStake

!featureCtx.CandidateRegisterMustWithStake && act.Amount()==0 is easier to read than featureCtx.CandidateRegisterMustWithStake || act.Amount().Cmp(big.NewInt(0)) != 0

dustinxie · 2024-01-24T19:57:53Z

action/protocol/staking/handlers.go

+	// register with self-stake
+	bucketIdx := uint64(candidateNoSelfStakeBucketIndex)
+	var bucketLogs []*action.TransactionLog
+	votes := big.NewInt(0)


use var () to put 3 together

dustinxie · 2024-01-24T19:59:11Z

action/protocol/staking/candidate_center.go

@@ -529,7 +532,7 @@ func (cb *candBase) collision(d *Candidate) (address.Address, address.Address, a
 		oper = c.Owner
 	}

-	if c, hit := cb.selfStkBucketMap[d.SelfStakeBucketIdx]; hit && !address.Equal(c.Owner, d.Owner) {
+	if c, hit := cb.selfStkBucketMap[d.SelfStakeBucketIdx]; hit && d.SelfStakeBucketIdx != candidateNoSelfStakeBucketIndex && !address.Equal(c.Owner, d.Owner) {


change is this file should use what's in PR4061, right?

yes, it's part of PR4061

dustinxie · 2024-01-24T20:02:03Z

action/protocol/staking/handlers.go

+		if err != nil {
+			return log, nil, err
+		}
+		bucketIdx = bktIdx


L672: bucketIdx, err = csm.xxx, no need to have a new var bktIdx here

dustinxie · 2024-01-24T20:08:53Z

action/protocol/staking/handlers.go

-			Amount:    registrationFee,
-		},
-	}, nil
+	txLogs := append(bucketLogs, &action.TransactionLog{


no need to define a new txLogs? can use existing bucketLogs
i think rename bucketLogs to txLogs to make the name more clear

dustinxie · 2024-01-24T23:36:03Z

action/protocol/staking/handlers.go

+			Amount:    act.Amount(),
+		})
+		votes = p.calculateVoteWeight(bucket, true)
+		log.AddTopics(byteutil.Uint64ToBytesBigEndian(bucketIdx), owner.Bytes())


this needs to be outside of if{} -- registration without self-stake should also create a topic in the log

sonarcloud · 2024-01-30T13:29:50Z

Quality Gate failed

Failed conditions

4.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Liuhaai · 2024-02-06T17:24:40Z

action/protocol/staking/validations.go

-		return errors.Wrap(action.ErrInvalidAmount, "self staking amount is not valid")
+		featureCtx := protocol.MustGetFeatureCtx(ctx)
+		if featureCtx.CandidateRegisterMustWithStake || act.Amount().Cmp(big.NewInt(0)) != 0 {
+			log.L().Warn("Candidate register amount is less than the minimum requirement", zap.Bool("feature", featureCtx.CandidateRegisterMustWithStake), zap.String("amount", act.Amount().String()))


i think the error has been logged upstream

Liuhaai · 2024-02-06T17:33:29Z

action/protocol/staking/handlers.go

+		votes     = big.NewInt(0)
+		err       error
+	)
+	if act.Amount().Sign() > 0 {


no hardfork here?

Before the hard fork, the act.Amount must be greater than 0 (garunteed by action validation), behaves the same as in the old version.

if act.Amount().Sign() <= 0 { should be return error?

already checked at https://github.com/iotexproject/iotex-core/pull/4059/files#diff-49d3555713cf037eb3ff80b28e13bd7329cc276b8fb0d60e4798b055a7e4aad8R70

dustinxie · 2024-03-08T07:11:34Z

action/protocol/staking/handlers.go

-	bucketIdx, err := csm.putBucketAndIndex(bucket)
-	if err != nil {
-		return log, nil, err
+	// register with self-stake


move to L713

dustinxie · 2024-03-08T07:13:22Z

action/protocol/staking/handlers.go

+			Recipient: address.StakingBucketPoolAddr,
+			Amount:    act.Amount(),
+		})
+		votes = p.calculateVoteWeight(bucket, true)
 	}


think adding an else make the code more explicit

else { // register w/o self-stake, waiting to be endorsed bucketIdx = uint64(candidateNoSelfStakeBucketIndex) votes = big.NewInt(0) }

dustinxie · 2024-03-08T07:18:56Z

action/protocol/staking/handlers.go

+		bucketIdx = uint64(candidateNoSelfStakeBucketIndex)
+		txLogs    []*action.TransactionLog
+		votes     = big.NewInt(0)
+		err       error


withSelfStake = act.Amount().Sign() > 0

err error not ncessary?

dustinxie · 2024-03-08T07:19:16Z

action/protocol/staking/handlers.go

+		votes     = big.NewInt(0)
+		err       error
+	)
+	if act.Amount().Sign() > 0 {


if withSelfStake {

dustinxie · 2024-03-08T07:19:32Z

action/protocol/staking/handlers.go

-		return log, nil, &handleError{
-			err:           errors.Wrapf(err, "failed to update staking bucket pool %s", err.Error()),
-			failureStatus: iotextypes.ReceiptStatus_ErrWriteAccount,
+	if act.Amount().Sign() > 0 {


if withSelfStake {

sonarcloud · 2024-03-11T03:03:44Z

Quality Gate failed

Failed conditions
5.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

envestcc requested review from CoderZhi, dustinxie and Liuhaai as code owners January 18, 2024 02:50

envestcc force-pushed the pr-registernonstake branch from 7ab6ddf to 669b5ac Compare January 18, 2024 04:04

CoderZhi reviewed Jan 20, 2024

View reviewed changes

dustinxie reviewed Jan 24, 2024

View reviewed changes

envestcc force-pushed the pr-registernonstake branch from 669b5ac to a6c1518 Compare January 25, 2024 14:56

envestcc added 3 commits January 30, 2024 21:28

register without stake

9eeb590

add test

f024880

address comment

fca42c3

envestcc force-pushed the pr-registernonstake branch from a6c1518 to fca42c3 Compare January 30, 2024 13:29

CoderZhi approved these changes Feb 5, 2024

View reviewed changes

Liuhaai reviewed Feb 6, 2024

View reviewed changes

envestcc and others added 3 commits March 7, 2024 11:14

Merge branch 'master' into pr-registernonstake

0179f56

address comments

163be84

Merge branch 'master' into pr-registernonstake

8c609b3

dustinxie reviewed Mar 8, 2024

View reviewed changes

action/protocol/staking/handlers.go Outdated

votes = big.NewInt(0)

err error

)

if act.Amount().Sign() > 0 {

Copy link

Member

dustinxie Mar 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if withSelfStake {

dustinxie reviewed Mar 8, 2024

View reviewed changes

envestcc added 2 commits March 8, 2024 22:15

address comments

ff11122

address comments

2f7bed0

address comments

9a01979

dustinxie approved these changes Mar 11, 2024

View reviewed changes

dustinxie and others added 3 commits March 10, 2024 18:07

Merge branch 'master' into pr-registernonstake

be85af5

Merge branch 'master' into pr-registernonstake

ffc9fb7

add test

ef0e99a

envestcc requested a review from a team as a code owner March 11, 2024 03:02

envestcc merged commit 9505d91 into iotexproject:master Mar 11, 2024
2 of 5 checks passed

envestcc deleted the pr-registernonstake branch March 11, 2024 03:35

BrewTestBot mentioned this pull request Mar 25, 2024

ioctl 1.14.0 Homebrew/homebrew-core#167116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[staking] Candidate Register without Staking #4059

[staking] Candidate Register without Staking #4059

envestcc commented Jan 18, 2024 •

edited

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading

CoderZhi Jan 20, 2024

envestcc Jan 22, 2024

Liuhaai Feb 6, 2024

dustinxie Jan 24, 2024

dustinxie Jan 24, 2024

envestcc Jan 25, 2024

dustinxie Jan 24, 2024

dustinxie Jan 24, 2024

dustinxie Jan 24, 2024

sonarcloud bot commented Jan 30, 2024

Liuhaai Feb 6, 2024

Liuhaai Feb 6, 2024

envestcc Mar 7, 2024 •

edited

Loading

millken Mar 8, 2024

envestcc Mar 8, 2024

dustinxie Mar 8, 2024

dustinxie Mar 8, 2024

dustinxie Mar 8, 2024

dustinxie Mar 8, 2024

dustinxie Mar 8, 2024

sonarcloud bot commented Mar 11, 2024

[staking] Candidate Register without Staking #4059

[staking] Candidate Register without Staking #4059

Conversation

envestcc commented Jan 18, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Jan 18, 2024 • 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

sonarcloud bot commented Jan 30, 2024

Quality Gate failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

envestcc Mar 7, 2024 • 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

sonarcloud bot commented Mar 11, 2024

Quality Gate failed

envestcc commented Jan 18, 2024 •

edited

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading

envestcc Mar 7, 2024 •

edited

Loading