Skip to content

Equivalent messages filter on consensus topic #5666

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Dec 21, 2023
Merged

Equivalent messages filter on consensus topic #5666

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0e51612
added equivalent messages filter + debugger
sstanculeanu Oct 24, 2023
b18146b
added EquivalentMessagesDebuggerStub
sstanculeanu Oct 24, 2023
901309d
fixes after first review
sstanculeanu Oct 25, 2023
0f793fe
fix after review, process invalid equivalent message received
sstanculeanu Nov 13, 2023
0babda5
fix after review, better mutex handling
sstanculeanu Nov 13, 2023
d4e7634
fix after review, extract to new method
sstanculeanu Nov 13, 2023
cba26d3
added activation flag for equivalent messages
sstanculeanu Dec 7, 2023
1ba03ab
fixed epochs config test
sstanculeanu Dec 7, 2023
71ce841
fix after review: small refactor, in order to keep the mutex locked o…
sstanculeanu Dec 13, 2023
7bf2591
Merge branch 'feat/equivalent-messages' of https://github.com/multive…
sstanculeanu Dec 14, 2023
57072fa
fixes after merge
sstanculeanu Dec 14, 2023
918f099
further fixes after review + todos
sstanculeanu Dec 18, 2023
c9fe2aa
added one more todo
sstanculeanu Dec 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions consensus/spos/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,8 +398,10 @@ func (wrk *Worker) ProcessReceivedMessage(message p2p.MessageP2P, fromConnectedP

msgType := consensus.MessageType(cnsMsg.MsgType)

err = wrk.processEquivalentMessage(msgType, cnsMsg.BlockHeaderHash)
wrk.mutEquivalentMessages.Lock()
Copy link
Contributor

@iulianpascalau iulianpascalau Nov 13, 2023

Choose a reason for hiding this comment

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

this solves the problem, hopefully, as it will only call once checkConsensusMessageValidity per round if the messages are correct and there are no edge cases of multiple broadcasts or message misses I've talked about.
However, I would extract L401-L422 in a new function: checkValidityAndProcessEquivalentMessages and use the defer mechanism for the mutex unlocking

Copy link
Collaborator Author

@sstanculeanu sstanculeanu Nov 13, 2023

Choose a reason for hiding this comment

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

updated as suggested

err = wrk.processEquivalentMessageUnprotected(msgType, cnsMsg.BlockHeaderHash)
if err != nil {
wrk.mutEquivalentMessages.Unlock()
return err
}

Expand All @@ -413,9 +415,11 @@ func (wrk *Worker) ProcessReceivedMessage(message p2p.MessageP2P, fromConnectedP

err = wrk.consensusMessageValidator.checkConsensusMessageValidity(cnsMsg, message.Peer())
if err != nil {
wrk.processInvalidEquivalentMessage(msgType, cnsMsg.BlockHeaderHash)
wrk.processInvalidEquivalentMessageUnprotected(msgType, cnsMsg.BlockHeaderHash)
wrk.mutEquivalentMessages.Unlock()
return err
}
wrk.mutEquivalentMessages.Unlock()

wrk.networkShardingCollector.UpdatePeerIDInfo(message.Peer(), cnsMsg.PubKey, wrk.shardCoordinator.SelfId())

Expand Down Expand Up @@ -729,17 +733,14 @@ func (wrk *Worker) ResetConsensusMessages() {
wrk.consensusMessageValidator.resetConsensusMessages()
}

func (wrk *Worker) processEquivalentMessage(msgType consensus.MessageType, blockHeaderHash []byte) error {
func (wrk *Worker) processEquivalentMessageUnprotected(msgType consensus.MessageType, blockHeaderHash []byte) error {
// early exit if the message is not with final info
if !wrk.consensusService.IsMessageWithFinalInfo(msgType) {
return nil
}

hdrHash := string(blockHeaderHash)

wrk.mutEquivalentMessages.Lock()
defer wrk.mutEquivalentMessages.Unlock()

// if an equivalent message was seen before, return error to stop further broadcasts
Copy link
Contributor

@iulianpascalau iulianpascalau Oct 24, 2023

Choose a reason for hiding this comment

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

slightly optimization for L743 - L49

numMessages := wrk.equivalentMessages[hdrHash]
wrk.equivalentMessages[hdrHash] = numMessages+1
if numMessages > 0 {
    return ErrEquivalentMessageAlreadyReceived
}

as this one does one read & one write on the map. The original code does 2 reads & one write

Copy link
Collaborator Author

@sstanculeanu sstanculeanu Oct 24, 2023

Choose a reason for hiding this comment

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

applied

numMessages := wrk.equivalentMessages[hdrHash]
wrk.equivalentMessages[hdrHash] = numMessages + 1
Expand All @@ -750,16 +751,13 @@ func (wrk *Worker) processEquivalentMessage(msgType consensus.MessageType, block
return nil
}

func (wrk *Worker) processInvalidEquivalentMessage(msgType consensus.MessageType, blockHeaderHash []byte) {
func (wrk *Worker) processInvalidEquivalentMessageUnprotected(msgType consensus.MessageType, blockHeaderHash []byte) {
if !wrk.consensusService.IsMessageWithFinalInfo(msgType) {
return
}

hdrHash := string(blockHeaderHash)

wrk.mutEquivalentMessages.Lock()
defer wrk.mutEquivalentMessages.Unlock()

delete(wrk.equivalentMessages, hdrHash)
Copy link
Contributor

@iulianpascalau iulianpascalau Nov 13, 2023

Choose a reason for hiding this comment

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

I do not think this is the proper way to do this.
Reason: what happens in this scenario?

  1. receive the correct message
  2. receive the same block final info but for some reason checkConsensusMessageValidity returns an error. We delete the equivalent message for the hdrHash
  3. we receive the correct message again and we re-broadcast it

Copy link
Collaborator Author

@sstanculeanu sstanculeanu Nov 13, 2023

Choose a reason for hiding this comment

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

pushed

}

Expand Down