Refactor consensus #6483
Refactor consensus #6483
Conversation
| @@ -89,7 +56,7 @@ const ( | |||
| BlockDefaultStringValue = "Undefined message type" | |||
| ) | |||
|
|
|||
| func getStringValue(msgType consensus.MessageType) string { | |||
| func GetStringValue(msgType consensus.MessageType) string { | |||
| type SubroundsFactory interface { | ||
| GenerateSubrounds() error | ||
| SetOutportHandler(driver outport.OutportHandler) | ||
| IsInterfaceNil() bool | ||
| } |
There was a problem hiding this comment.
should be moved to interface.go.. also, it is only used as part of this package, so perhaps unexport it?
| const ( | ||
| ConsensusNone ConsensusStateMachineType = iota | ||
| ConsensusV1 | ||
| ConsensusV2 | ||
| ) |
There was a problem hiding this comment.
missing comments.. also, do we need them exported? perhaps move them to a constants.go file, with L42
There was a problem hiding this comment.
unexported the contants
| IsInterfaceNil() bool | ||
| } | ||
|
|
||
| type ConsensusStateMachineType int |
There was a problem hiding this comment.
missing comment, not sure if it is needed as exported
| ConsensusV2 | ||
| ) | ||
|
|
||
| func NewSubroundsHandler(args *SubroundsHandlerArgs) (*SubroundsHandler, error) { |
There was a problem hiding this comment.
compared with bls/subroundBlock.go from latest rc, no difference ✅
There was a problem hiding this comment.
compared with bls/subroundEndRound.go from latest rc, no difference ✅
There was a problem hiding this comment.
compared with bls/subroundSignature.go from latest rc, no difference ✅
There was a problem hiding this comment.
compared with bls/subroundStartRound.go from latest rc, similar ✅
| return eligibleList | ||
| } | ||
|
|
||
| func CreateEligibleListFromMap(mapKeys map[string]crypto.PrivateKey) []string { |
There was a problem hiding this comment.
missing comments on exported methods
| // SubroundsHandler struct contains the needed data for the SubroundsHandler | ||
| type SubroundsHandler struct { |
There was a problem hiding this comment.
does this needs to be exported?
There was a problem hiding this comment.
I think we should start using exported types for constructors returns, even if we keep the internal fields unexported.
It would make it much simpler to work with these types
| signatureThrottler core.Throttler, | ||
| ) (spos.SubroundsFactory, error) { | ||
| switch consensusType { | ||
| case blsConsensusType: |
There was a problem hiding this comment.
we used blsConsensusType, now we have other versioning, do we need blsConsensusType const anymore? or maybe considering blsConsensusV1 and 2 for newly added versions
There was a problem hiding this comment.
The type here was for the type of votes used during consensus.
both versions use bls signatures as votes.
If we change to something else we could add the switch again, until then I don't think we need it
| sh.currentConsensusType = consensusNone | ||
| sh.EpochStartAction(&testscommon.HeaderHandlerStub{}) | ||
| require.Nil(t, err) | ||
| require.Equal(t, consensusV1, sh.currentConsensusType) | ||
| require.Equal(t, int32(1), startCalled.Load()) | ||
| }) |
There was a problem hiding this comment.
test case here for the transitions scenario from v1 to v2?
There was a problem hiding this comment.
added as part of the initSubroundsForEpoch tests (which will get called by epochStartAction)
| logger "github.com/multiversx/mx-chain-logger-go" | ||
| ) | ||
|
|
||
| var log = logger.GetOrCreate("consensus/spos/bls") |
There was a problem hiding this comment.
| var log = logger.GetOrCreate("consensus/spos/bls") | |
| var log = logger.GetOrCreate("consensus/spos/bls/v1") |
?
| logger "github.com/multiversx/mx-chain-logger-go" | ||
| ) | ||
|
|
||
| var log = logger.GetOrCreate("consensus/spos/bls") |
There was a problem hiding this comment.
| var log = logger.GetOrCreate("consensus/spos/bls") | |
| var log = logger.GetOrCreate("consensus/spos/bls/v2") |
?
| func (cns *ConsensusState) SetRoundCanceled(roundCanceled bool) { | ||
| cns.RoundCanceled = roundCanceled |
There was a problem hiding this comment.
we can now unexport the internal fields, todo for the next PRs
There was a problem hiding this comment.
we can also unexport ConsensusState
There was a problem hiding this comment.
I would unexport maybe just the fields.
| "github.com/stretchr/testify/require" | ||
|
|
||
| chainCommon "github.com/multiversx/mx-chain-go/common" | ||
| mock2 "github.com/multiversx/mx-chain-go/consensus/mock" |
There was a problem hiding this comment.
mock2 -> consensusMock
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?