Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix invalid signers message broadcast on multikey #5817

Merged
merged 7 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 14 additions & 3 deletions consensus/spos/bls/subroundEndRound.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ func (sr *subroundEndRound) receivedInvalidSignersInfo(_ context.Context, cnsDta
return false
}

if sr.IsSelfLeaderInCurrentRound() {
if sr.IsSelfLeaderInCurrentRound() || sr.IsMultiKeyLeaderInCurrentRound() {
return false
}

Expand Down Expand Up @@ -589,20 +589,31 @@ func (sr *subroundEndRound) createAndBroadcastHeaderFinalInfo() {
}

func (sr *subroundEndRound) createAndBroadcastInvalidSigners(invalidSigners []byte) {
isSelfLeader := sr.IsSelfLeaderInCurrentRound() && sr.ShouldConsiderSelfKeyInConsensus()
if !isSelfLeader && !sr.IsMultiKeyLeaderInCurrentRound() {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we change the condition to

if !(isSelfLeader || sr.IsMultiKeyLeaderInCurrentRound()) 

would be easier to read

return
}

leader, errGetLeader := sr.GetLeader()
AdoAdoAdo marked this conversation as resolved.
Show resolved Hide resolved
if errGetLeader != nil {
log.Debug("createAndBroadcastInvalidSigners.GetLeader", "error", errGetLeader)
return
}

cnsMsg := consensus.NewConsensusMessage(
sr.GetData(),
nil,
nil,
nil,
[]byte(sr.SelfPubKey()),
[]byte(leader),
nil,
int(MtInvalidSigners),
sr.RoundHandler().Index(),
sr.ChainID(),
nil,
nil,
nil,
sr.CurrentPid(),
sr.GetAssociatedPid([]byte(leader)),
invalidSigners,
)

Expand Down
112 changes: 93 additions & 19 deletions consensus/spos/bls/subroundEndRound_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1322,7 +1322,7 @@ func TestSubroundEndRound_ReceivedInvalidSignersInfo(t *testing.T) {
assert.False(t, res)
})

t.Run("received message for self leader", func(t *testing.T) {
t.Run("received message from self leader should return false", func(t *testing.T) {
t.Parallel()

container := mock.InitConsensusCore()
Expand All @@ -1339,6 +1339,53 @@ func TestSubroundEndRound_ReceivedInvalidSignersInfo(t *testing.T) {
assert.False(t, res)
})

t.Run("received message from self multikey leader should return false", func(t *testing.T) {
t.Parallel()

container := mock.InitConsensusCore()
keysHandler := &testscommon.KeysHandlerStub{
IsKeyManagedByCurrentNodeCalled: func(pkBytes []byte) bool {
return string(pkBytes) == "A"
},
}
ch := make(chan bool, 1)
consensusState := initConsensusStateWithKeysHandler(keysHandler)
sr, _ := spos.NewSubround(
bls.SrSignature,
bls.SrEndRound,
-1,
int64(85*roundTimeDuration/100),
int64(95*roundTimeDuration/100),
"(END_ROUND)",
consensusState,
ch,
executeStoredMessages,
container,
chainID,
currentPid,
&statusHandler.AppStatusHandlerStub{},
)

srEndRound, _ := bls.NewSubroundEndRound(
sr,
extend,
bls.ProcessingThresholdPercent,
displayStatistics,
&statusHandler.AppStatusHandlerStub{},
&mock.SentSignatureTrackerStub{},
)

srEndRound.SetSelfPubKey("A")

cnsData := consensus.Message{
BlockHeaderHash: []byte("X"),
PubKey: []byte("A"),
}

res := srEndRound.ReceivedInvalidSignersInfo(&cnsData)
assert.False(t, res)
})

t.Run("received hash does not match the hash from current consensus state", func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -1556,29 +1603,56 @@ func TestVerifyInvalidSigners(t *testing.T) {
func TestSubroundEndRound_CreateAndBroadcastInvalidSigners(t *testing.T) {
t.Parallel()

wg := &sync.WaitGroup{}
wg.Add(1)
t.Run("redundancy node should not send", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

"redundancy node should not send while the main is active"

t.Parallel()

expectedInvalidSigners := []byte("invalid signers")
expectedInvalidSigners := []byte("invalid signers")

wasCalled := false
container := mock.InitConsensusCore()
messenger := &mock.BroadcastMessengerMock{
BroadcastConsensusMessageCalled: func(message *consensus.Message) error {
wg.Done()
assert.Equal(t, expectedInvalidSigners, message.InvalidSigners)
wasCalled = true
return nil
},
}
container.SetBroadcastMessenger(messenger)
sr := *initSubroundEndRoundWithContainer(container, &statusHandler.AppStatusHandlerStub{})
container := mock.InitConsensusCore()
nodeRedundancy := &mock.NodeRedundancyHandlerStub{
IsRedundancyNodeCalled: func() bool {
return true
},
}
container.SetNodeRedundancyHandler(nodeRedundancy)
messenger := &mock.BroadcastMessengerMock{
BroadcastConsensusMessageCalled: func(message *consensus.Message) error {
assert.Fail(t, "should have not been called")
return nil
},
}
container.SetBroadcastMessenger(messenger)
sr := *initSubroundEndRoundWithContainer(container, &statusHandler.AppStatusHandlerStub{})

sr.CreateAndBroadcastInvalidSigners(expectedInvalidSigners)
})
t.Run("should work", func(t *testing.T) {
t.Parallel()

wg := &sync.WaitGroup{}
wg.Add(1)

expectedInvalidSigners := []byte("invalid signers")

wasCalled := false
container := mock.InitConsensusCore()
messenger := &mock.BroadcastMessengerMock{
BroadcastConsensusMessageCalled: func(message *consensus.Message) error {
wg.Done()
Copy link
Contributor

Choose a reason for hiding this comment

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

can you do the wg.Done after the wasCalled = true?

assert.Equal(t, expectedInvalidSigners, message.InvalidSigners)
wasCalled = true
return nil
},
}
container.SetBroadcastMessenger(messenger)
sr := *initSubroundEndRoundWithContainer(container, &statusHandler.AppStatusHandlerStub{})

sr.CreateAndBroadcastInvalidSigners(expectedInvalidSigners)
sr.CreateAndBroadcastInvalidSigners(expectedInvalidSigners)

wg.Wait()
wg.Wait()

require.True(t, wasCalled)
require.True(t, wasCalled)
})
}

func TestGetFullMessagesForInvalidSigners(t *testing.T) {
Expand Down