Equivalent messages filter on consensus topic #5666

sstanculeanu · 2023-10-24T11:26:08Z

Reasoning behind the pull request

implement equivalent messages filter on consensus topic, in order to stop further broadcasts of messages containing same block final info

Proposed changes

added filter for equivalent messages
added specific debugger

Testing procedure

with feat branch

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

iulianpascalau · 2023-10-24T13:17:49Z

consensus/spos/interface.go

+
+// EquivalentMessagesDebugger defines the specific debugger for equivalent messages
+type EquivalentMessagesDebugger interface {
+	DisplayEquivalentMessagesStatistics(getDataHandler func() map[string]uint64)


let's make the signature more straightforward. Why not declare the function like

DisplayEquivalentMessagesStatistics(messages map[string]uint64)

I tried to avoid copying the map when we don't need the log.. with the function pointer approach, it is copied only when the debugger is active

iulianpascalau · 2023-10-24T13:24:54Z

consensus/spos/worker.go

+	wrk.mutEquivalentMessages.Lock()
+	defer wrk.mutEquivalentMessages.Unlock()
+
+	// if an equivalent message was seen before, return error to stop further broadcasts


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

iulianpascalau · 2023-10-24T13:25:22Z

consensus/spos/worker.go

+	wrk.mutEquivalentMessages.RLock()
+	defer wrk.mutEquivalentMessages.RUnlock()
+
+	equivalentMessagesCopy := make(map[string]uint64, len(wrk.equivalentMessages))


iulianpascalau · 2023-10-24T13:26:32Z

consensus/spos/worker_test.go

+		equivMsgFrom,
+		&p2pmocks.MessengerStub{},
+	)
+	assert.Equal(t, spos.ErrEquivalentMessageAlreadyReceived, err)


test also the inner counter? Or a special unit test for the reset & read of the counters?

improved the test

codecov · 2023-10-25T11:39:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (feat/equivalent-messages@8145826). Click here to learn what that means.

❗ Current head 918f099 differs from pull request most recent head c9fe2aa. Consider uploading reports for the commit c9fe2aa to get more accurate results

Additional details and impacted files

@@                     Coverage Diff                     @@
##             feat/equivalent-messages    #5666   +/-   ##
===========================================================
  Coverage                            ?   80.37%           
===========================================================
  Files                               ?      712           
  Lines                               ?    94410           
  Branches                            ?        0           
===========================================================
  Hits                                ?    75879           
  Misses                              ?    13223           
  Partials                            ?     5308

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

consensus/spos/worker.go

iulianpascalau · 2023-11-13T15:21:25Z

consensus/spos/worker.go

+	wrk.mutEquivalentMessages.Lock()
+	defer wrk.mutEquivalentMessages.Unlock()
+
+	delete(wrk.equivalentMessages, hdrHash)


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

receive the correct message

receive the same block final info but for some reason checkConsensusMessageValidity returns an error. We delete the equivalent message for the hdrHash

we receive the correct message again and we re-broadcast it

iulianpascalau · 2023-11-13T17:45:30Z

consensus/spos/worker.go

 	msgType := consensus.MessageType(cnsMsg.MsgType)

-	err = wrk.processEquivalentMessage(msgType, cnsMsg.BlockHeaderHash)
+	wrk.mutEquivalentMessages.Lock()


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

updated as suggested

consensus/spos/worker.go

…nly when necessary + added equivalent message signature verification

…rsx/mx-chain-go into equivalent_messages_filter # Conflicts: # cmd/node/config/enableEpochs.toml # common/enablers/enableEpochsHandler.go # common/enablers/enableEpochsHandler_test.go # common/enablers/epochFlags.go # common/interface.go # config/epochConfig.go # config/tomlConfig_test.go # consensus/spos/bls/blsSubroundsFactory.go # consensus/spos/bls/subroundStartRound.go # consensus/spos/bls/subroundStartRound_test.go # consensus/spos/errors.go # consensus/spos/interface.go # testscommon/enableEpochsHandlerMock/enableEpochsHandlerStub.go

common/enablers/enableEpochsHandler_test.go

config/epochConfig.go

consensus/spos/bls/subroundStartRound.go

consensus/spos/worker.go

AdoAdoAdo · 2023-12-19T09:54:03Z

consensus/spos/debug/equivalentMessagesDebugger.go

 		line := []string{
 			hash,
-			fmt.Sprintf("%d", cnt),
+			fmt.Sprintf("%d", info.NumMessages),


this will need to display new fields, most importantly if any equivalent proof was validated, can be done on the next PR.

AdoAdoAdo · 2023-12-19T10:05:16Z

consensus/spos/worker.go

-	}
-
-	return nil
+	// TODO[Sorin]: after flag enabled, VerifySignature on previous hash, with the signature and bitmap from the proof on cnsMsg


verify on current header with the signature and bitmap from cnsMsg, but anyway would be removed in next PR.

added equivalent messages filter + debugger

0e51612

sstanculeanu added type:feature New feature or request area:processing impact:low interested:protocol complexity:medium labels Oct 24, 2023

sstanculeanu self-assigned this Oct 24, 2023

added EquivalentMessagesDebuggerStub

b18146b

iulianpascalau self-requested a review October 24, 2023 12:46

iulianpascalau reviewed Oct 24, 2023

View reviewed changes

AdoAdoAdo self-requested a review October 24, 2023 13:31

fixes after first review

901309d

AdoAdoAdo reviewed Nov 1, 2023

View reviewed changes

consensus/spos/worker.go Outdated Show resolved Hide resolved

fix after review, process invalid equivalent message received

0f793fe

iulianpascalau reviewed Nov 13, 2023

View reviewed changes

fix after review, better mutex handling

0babda5

iulianpascalau reviewed Nov 13, 2023

View reviewed changes

fix after review, extract to new method

d4e7634

iulianpascalau previously approved these changes Nov 13, 2023

View reviewed changes

AdoAdoAdo requested changes Dec 7, 2023

View reviewed changes

consensus/spos/worker.go Show resolved Hide resolved

added activation flag for equivalent messages

cba26d3

sstanculeanu dismissed iulianpascalau’s stale review via cba26d3 December 7, 2023 10:27

fixed epochs config test

1ba03ab

iulianpascalau previously approved these changes Dec 7, 2023

View reviewed changes

AdoAdoAdo requested changes Dec 12, 2023

View reviewed changes

consensus/spos/worker.go Show resolved Hide resolved

fix after review: small refactor, in order to keep the mutex locked o…

71ce841

…nly when necessary + added equivalent message signature verification

sstanculeanu dismissed iulianpascalau’s stale review via 71ce841 December 13, 2023 09:58

sstanculeanu added 2 commits December 14, 2023 17:58

fixes after merge

57072fa

AdoAdoAdo reviewed Dec 18, 2023

View reviewed changes

sstanculeanu added 2 commits December 18, 2023 16:01

further fixes after review + todos

918f099

added one more todo

c9fe2aa

AdoAdoAdo approved these changes Dec 19, 2023

View reviewed changes

iulianpascalau approved these changes Dec 21, 2023

View reviewed changes

sstanculeanu merged commit bd628de into feat/equivalent-messages Dec 21, 2023

sstanculeanu deleted the equivalent_messages_filter branch December 21, 2023 14:31

Equivalent messages filter on consensus topic #5666

Equivalent messages filter on consensus topic #5666

Uh oh!

Conversation

sstanculeanu commented Oct 24, 2023

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sstanculeanu Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sstanculeanu Oct 24, 2023 •

edited

Loading

codecov bot commented Oct 25, 2023 •

edited

Loading