Skip to content

Commit

Permalink
Fix data race in fsm (#982)
Browse files Browse the repository at this point in the history
* Fix data race in several places accessing fsm and fix copy validators function.

* tidy linter.

* Fix tests after changes and user RLock.
  • Loading branch information
0xSasaPrsic authored Dec 1, 2022
1 parent f33af57 commit 4828279
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 3 deletions.
10 changes: 10 additions & 0 deletions consensus/polybft/consensus_runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,9 @@ func (c *consensusRuntime) FSM() error {
"endOfSprint", isEndOfSprint,
)

c.lock.Lock()
c.fsm = ff
c.lock.Unlock()

return nil
}
Expand Down Expand Up @@ -997,6 +999,9 @@ func (c *consensusRuntime) IsValidBlock(proposal []byte) bool {
}

func (c *consensusRuntime) IsValidSender(msg *proto.Message) bool {
c.lock.RLock()
defer c.lock.RUnlock()

err := c.fsm.ValidateSender(msg)
if err != nil {
c.logger.Error("invalid sender", "error", err)
Expand All @@ -1008,6 +1013,9 @@ func (c *consensusRuntime) IsValidSender(msg *proto.Message) bool {
}

func (c *consensusRuntime) IsProposer(id []byte, height, round uint64) bool {
c.lock.RLock()
defer c.lock.RUnlock()

nextProposer, err := c.fsm.validators.CalcProposer(round)
if err != nil {
c.logger.Error("cannot calculate proposer", "error", err)
Expand Down Expand Up @@ -1117,6 +1125,8 @@ func (c *consensusRuntime) HasQuorum(
messages []*proto.Message,
msgType proto.MessageType,
) bool {
c.lock.RLock()
defer c.lock.RUnlock()
// extract the addresses of all the signers of the messages
ppIncluded := false
signers := make(map[types.Address]struct{}, len(messages))
Expand Down
2 changes: 1 addition & 1 deletion consensus/polybft/validator_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ func (v *validatorSet) Copy() *validatorSet {
func validatorListCopy(valList []*ValidatorAccount) []*ValidatorAccount {
valCopy := make([]*ValidatorAccount, len(valList))
for i, val := range valList {
valCopy[i] = NewValidator(val.Metadata, val.ProposerPriority)
valCopy[i] = NewValidator(val.Metadata.Copy(), val.ProposerPriority)
}

return valCopy
Expand Down
7 changes: 5 additions & 2 deletions consensus/polybft/validator_set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -408,8 +408,11 @@ func TestValidatorSetTotalVotingPowerErrorOnOverflow(t *testing.T) {
func TestUpdatesForNewValidatorSet(t *testing.T) {
t.Parallel()

v1 := &ValidatorMetadata{Address: types.Address{0x1}, VotingPower: 100}
v2 := &ValidatorMetadata{Address: types.Address{0x2}, VotingPower: 100}
keys, err := bls.CreateRandomBlsKeys(2)
require.NoError(t, err)

v1 := &ValidatorMetadata{Address: types.Address{0x1}, BlsKey: keys[0].PublicKey(), VotingPower: 100}
v2 := &ValidatorMetadata{Address: types.Address{0x2}, BlsKey: keys[1].PublicKey(), VotingPower: 100}
accountSet := []*ValidatorMetadata{v1, v2}
valSet, err := NewValidatorSet(accountSet, hclog.NewNullLogger())
require.NoError(t, err)
Expand Down

0 comments on commit 4828279

Please sign in to comment.