[rolldpos] refactor rolldpos.ChainManager #3516

huof6829 · 2022-07-11T11:12:32Z

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #3462

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

Liuhaai · 2022-07-11T19:49:58Z

Should the old interface be removed?

codecov · 2022-07-12T03:16:23Z

Codecov Report

Merging #3516 (de562f2) into master (a20e489) will decrease coverage by 1.30%.
The diff coverage is 56.99%.

❗ Current head de562f2 differs from pull request most recent head 8ba04c9. Consider uploading reports for the commit 8ba04c9 to get more accurate results

@@            Coverage Diff             @@
##           master    #3516      +/-   ##
==========================================
- Coverage   75.43%   74.12%   -1.31%     
==========================================
  Files         247      254       +7     
  Lines       22845    23404     +559     
==========================================
+ Hits        17233    17349     +116     
- Misses       4685     5129     +444     
+ Partials      927      926       -1

Impacted Files	Coverage Δ
action/protocol/staking/read_state.go	`15.38% <0.00%> (ø)`
api/grpcserver.go	`79.49% <ø> (+0.55%)`	⬆️
blockchain/blockchain.go	`0.89% <0.00%> (ø)`
blockchain/filedao/filedao_legacy.go	`85.80% <ø> (ø)`
blockchain/filedao/filedao_v2.go	`98.51% <ø> (ø)`
blockchain/pubsubmanager.go	`0.00% <0.00%> (ø)`
config/config.go	`94.06% <ø> (+9.31%)`	⬆️
db/db_bolt.go	`72.51% <ø> (-0.19%)`	⬇️
db/kvstore_impl.go	`61.64% <ø> (ø)`
db/kvstorewithcache.go	`0.00% <0.00%> (ø)`
... and 69 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

dustinxie · 2022-07-12T23:08:04Z

blockchain/blockchain.go

+		// BlockProposeTime return propose time by height
+		BlockProposeTime(uint64) (time.Time, error)
+		// BlockCommitTime return commit time by height
+		BlockCommitTime(uint64) (time.Time, error)


no need to change the main Blockchain interface?

Yes, no need,

dustinxie · 2022-07-13T00:24:57Z

consensus/scheme/rolldpos/rolldpos.go

+		// ChainAddress returns chain address on parent chain, the root chain return empty.
+		ChainAddress() string
+	}
+)


add an internal struct here to implement ChainManager interface

struct xxx { bc blockchain.Blockchain } func (c *xxx) BlockProposeTime() func (c *xxx) BlockCommitTime()

right, internal struct is better

dustinxie · 2022-07-13T00:25:58Z

consensus/scheme/rolldpos/rolldpos.go

+		// BlockCommitTime return commit time by height
+		BlockCommitTime(uint64) (time.Time, error)
+		// Genesis returns the genesis
+		Genesis() genesis.Genesis


according to issue, can delete Genesis() if it's not used

dustinxie · 2022-07-18T17:01:31Z

consensus/scheme/rolldpos/rolldpos.go

-		// BlockProposeTime return propose time by height
-		BlockProposeTime(uint64) (time.Time, error)
-		// BlockCommitTime return commit time by height
-		BlockCommitTime(uint64) (time.Time, error)


still keep these 2

and add GenesisTime()

dustinxie · 2022-07-18T17:01:56Z

consensus/scheme/rolldpos/roundcalculator.go

+
+type blockTime struct {
+	bc blockchain.Blockchain
+}


make this struct implement ChainManager interface

and rename to chainManager

dustinxie · 2022-07-18T17:03:48Z

consensus/scheme/rolldpos/roundcalculator.go

@@ -23,6 +24,7 @@ type roundCalculator struct {
 	rp                   *rolldpos.Protocol
 	delegatesByEpochFunc DelegatesByEpochFunc
 	beringHeight         uint64
+	blockTime            *blockTime


no need to add this

dustinxie · 2022-07-18T17:05:04Z

consensus/scheme/rolldpos/rolldposctx.go

@@ -138,6 +138,9 @@ func newRollDPoSCtx(
 		rp:                   rp,
 		timeBasedRotation:    timeBasedRotation,
 		beringHeight:         beringHeight,
+		blockTime: &blockTime{
+			bc: chain.(blockchain.Blockchain),
+		},


no need to add this, modify L137 chain to

chain: &blockTime{ bc: chain.(blockchain.Blockchain), },

CoderZhi · 2022-07-19T17:18:58Z

consensus/scheme/rolldpos/rolldpos.go

@@ -267,8 +326,8 @@ func (b *Builder) SetPriKey(priKey crypto.PrivateKey) *Builder {
 }

 // SetChainManager sets the blockchain APIs
-func (b *Builder) SetChainManager(chain ChainManager) *Builder {
-	b.chain = chain
+func (b *Builder) SetChainManager(bc blockchain.Blockchain) *Builder {


The reason we want to use interface is to prevent referring to blockchain.Blockchain directly (the current implementation is not complete, with other problems). Thus, this parameter type shouldn't be modified, and the chainManager struct shouldn't be defined in this package.

right, still use SetChainManager(chain ChainManager)

CoderZhi · 2022-07-19T17:20:52Z

consensus/scheme/rolldpos/rolldpos.go

+}
+
+// GenesisTime return Genesis time by default
+func (cm *chainManager) GenesisTime() time.Time {


if both BlockCommitTime(0) and BlockProposeTime(0) return bc.Genesis().Timestamp, we can delete this function from the interface.

dustinxie · 2022-07-20T05:45:27Z

consensus/scheme/rolldpos/rolldpos.go

-func (b *Builder) SetChainManager(chain ChainManager) *Builder {
-	b.chain = chain
+func (b *Builder) SetChainManager(bc blockchain.Blockchain) *Builder {
+	b.chain = &chainManager{bc: bc}


define a newChainManager(bc blockchain.Blockchain) ChainManager, so

rolldpos.NewRollDPoSBuilder(). ... .SetChainManager(newChainManager(bc))

dustinxie · 2022-07-20T05:46:17Z

consensus/scheme/rolldpos/rolldposctx_test.go

@@ -44,7 +44,7 @@ func TestRollDPoSCtx(t *testing.T) {
 	})

 	t.Run("case 2:panic because of rp is nil", func(t *testing.T) {
-		_, err := newRollDPoSCtx(consensusfsm.NewConsensusConfig(cfg), dbConfig, true, time.Second, true, b, block.NewDeserializer(0), nil, nil, dummyCandidatesByHeightFunc, "", nil, nil, 0)
+		_, err := newRollDPoSCtx(consensusfsm.NewConsensusConfig(cfg), dbConfig, true, time.Second, true, &chainManager{bc: b}, block.NewDeserializer(0), nil, nil, dummyCandidatesByHeightFunc, "", nil, nil, 0)


newChainManager(b)

Liuhaai · 2022-07-20T17:44:04Z

consensus/scheme/rolldpos/roundcalculator.go

+	bc blockchain.Blockchain
+}
+
+func newBlockTime(bc blockchain.Blockchain) *blockTime {


The arg here has to be ChainManager

dustinxie · 2022-07-21T15:29:22Z

consensus/scheme/rolldpos/rolldposctx.go

@@ -511,14 +511,14 @@ func (ctx *rollDPoSCtx) Commit(msg interface{}) (bool, error) {

 	_consensusDurationMtc.WithLabelValues().Set(float64(time.Since(ctx.round.roundStartTime)))
 	if pendingBlock.Height() > 1 {
-		prevBlkHeader, err := ctx.chain.BlockHeaderByHeight(pendingBlock.Height() - 1)
+		prevBlkProposeTime, err := ctx.roundCalc.chain.BlockProposeTime(pendingBlock.Height() - 1)


it should be ctx.chain, which is set by SetChainManager()

dustinxie · 2022-07-21T15:32:06Z

consensus/scheme/rolldpos/roundcalculator.go

-			if lastBlock, err = c.chain.BlockHeaderByHeight(height - 1); err != nil {
+			var lastBlkProposeTime time.Time
+			lastBlkProposeTime, err = c.chain.BlockProposeTime(height - 1)
+			if err != nil {


write L140, 141 into 1 line, be consistent with L134

dustinxie · 2022-07-21T15:32:34Z

consensus/scheme/rolldpos/roundcalculator.go

-			if lastBlock, err = c.chain.BlockFooterByHeight(height - 1); err != nil {
+			var lastBlkCommitTime time.Time
+			lastBlkCommitTime, err = c.chain.BlockCommitTime(height - 1)
+			if err != nil {


same as above, combine into 1 line

Liuhaai · 2022-07-26T18:23:00Z

consensus/scheme/rolldpos/rolldposctx.go

@@ -511,14 +511,14 @@ func (ctx *rollDPoSCtx) Commit(msg interface{}) (bool, error) {

 	_consensusDurationMtc.WithLabelValues().Set(float64(time.Since(ctx.round.roundStartTime)))
 	if pendingBlock.Height() > 1 {
-		prevBlkHeader, err := ctx.chain.BlockHeaderByHeight(pendingBlock.Height() - 1)
+		prevBlkProposeTime, err := ctx.chain.BlockProposeTime(pendingBlock.Height() - 1)
 		if err != nil {
 			log.L().Error("Error when getting the previous block header.",


logging info needs to be adjusted as well

Liuhaai · 2022-07-26T18:28:11Z

consensus/scheme/rolldpos/rolldpos.go

+	}
+	header, err := cm.bc.BlockHeaderByHeight(height)
+	if err != nil {
+		return time.Now(), errors.Wrapf(


time.Now() is not default value for Time.

add BlockProposeTime BlockCommitTime

919e3b5

huof6829 requested a review from a team as a code owner July 11, 2022 11:12

Merge branch 'master' into fix_3462

02ec7a0

Merge branch 'master' into fix_3462

12eeb71

dustinxie reviewed Jul 12, 2022

View reviewed changes

dustinxie reviewed Jul 13, 2022

View reviewed changes

huof6829 added 3 commits July 14, 2022 23:00

add blockTime struct

1b1aefa

Merge branch 'master' into fix_3462

3bf92ac

Merge branch 'master' into fix_3462

d1b7aec

dustinxie reviewed Jul 18, 2022

View reviewed changes

huof6829 added 3 commits July 19, 2022 10:45

Merge branch 'master' into fix_3462

fb5f503

add chainManager

4d0a616

modify function name

bd5e08a

CoderZhi reviewed Jul 19, 2022

View reviewed changes

dustinxie reviewed Jul 20, 2022

View reviewed changes

huof6829 added 3 commits July 20, 2022 23:03

fix SetChainManager

17de956

Merge branch 'master' into fix_3462

9004327

revert blockchain.go

d95e4f7

Liuhaai reviewed Jul 20, 2022

View reviewed changes

huof6829 added 3 commits July 21, 2022 10:03

add NewChainManager

4d000ed

Merge branch 'master' into fix_3462

246f571

delete GenesisTime

7dc6160

dustinxie reviewed Jul 21, 2022

View reviewed changes

huof6829 and others added 3 commits July 22, 2022 19:27

format

1929890

Merge branch 'master' into fix_3462

b88096b

Merge branch 'master' into fix_3462

f2472fa

dustinxie approved these changes Jul 26, 2022

View reviewed changes

Liuhaai reviewed Jul 26, 2022

View reviewed changes

use ctx.logger

8ba04c9

Liuhaai approved these changes Jul 27, 2022

View reviewed changes

Liuhaai merged commit 0cac228 into iotexproject:master Jul 27, 2022

huof6829 deleted the fix_3462 branch August 4, 2022 14:07

[rolldpos] refactor rolldpos.ChainManager #3516

[rolldpos] refactor rolldpos.ChainManager #3516

Conversation

huof6829 commented Jul 11, 2022

Description

Type of change

How Has This Been Tested?

Checklist:

Liuhaai commented Jul 11, 2022

codecov bot commented Jul 12, 2022 • edited Loading

Codecov Report

dustinxie Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinxie Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

huof6829 Jul 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

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

codecov bot commented Jul 12, 2022 •

edited

Loading

dustinxie Jul 12, 2022 •

edited

Loading

dustinxie Jul 13, 2022 •

edited

Loading

huof6829 Jul 14, 2022 •

edited

Loading

dustinxie Jul 18, 2022 •

edited

Loading

dustinxie Jul 20, 2022 •

edited

Loading