Pseudo slashing for invalid signers #4269

ssd04 · 2022-07-06T08:51:53Z

Pseudo slashing for invalid signers:

added one more field for consensus Message invalidSigners
if there are invalid signers, broadcast the info via a new consensus message type
handle p2p message verification based on a separate component

…marshaller

…sage

codecov-commenter · 2022-07-13T07:55:48Z

Codecov Report

❗ No coverage uploaded for pull request base (feat/optimise-consensus-sigcheck@edf2344). Click here to learn what that means.
Patch has no changes to coverable lines.

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

Additional details and impacted files

@@                         Coverage Diff                         @@
##             feat/optimise-consensus-sigcheck    #4269   +/-   ##
===================================================================
  Coverage                                    ?   71.21%           
===================================================================
  Files                                       ?      663           
  Lines                                       ?    86210           
  Branches                                    ?        0           
===================================================================
  Hits                                        ?    61392           
  Misses                                      ?    20302           
  Partials                                    ?     4516

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

- remove reference to temp pubsub library - restore p2p message changes

…nit test

…core

…core - fix unit tests

…for-invalid-signers

…onsensus-sig-into-pseudo-slashing

…sig-into-pseudo-slashing Merge optimize consensus sig into pseudo slashing

…onsensus-sig-into-pseudo-slashing

…sig-into-pseudo-slashing Merge optimize consensus sig into pseudo slashing

consensus/spos/bls/subroundEndRound_test.go

consensus/spos/bls/subroundEndRound.go

SebastianMarian · 2022-10-18T12:36:36Z

consensus/spos/bls/subroundEndRound.go

+
+		return false
+	}
+


I will also add here the following checks, just to avoid some edge case situations, exactly as they are used in the receivedFinalInfo method:

if sr.IsSelfLeaderInCurrentRound() { return false } if !sr.IsConsensusDataEqual(cnsDta.BlockHeaderHash) { return false } if !sr.CanProcessReceivedMessage(cnsDta, sr.RoundHandler().Index(), sr.Current()) { return false }

consensus/spos/bls/subroundEndRound.go

SebastianMarian · 2022-10-18T13:14:59Z

consensus/spos/bls/subroundEndRound.go

+			log.Debug("doEndRoundJobByLeader.getFullMessagesForInvalidSigners", "error", err.Error())
+			return false
+		}
+
 		bitmap, sig, err = sr.computeAggSigOnValidNodes()
 		if err != nil {
 			log.Debug("doEndRoundJobByLeader.computeAggSigOnValidNodes", "error", err.Error())


Also in the case of insufficient threshold of valid signatures I think we should call sr.createAndBroadcastInvalidSigners(invalidSigners). I think this method should be called mandatory if we have invalidSigners set, no matter if we will exit from the current method earlier.

One suggestion is to replace L313-L317 with the code below and remove L365-L367 and L300-L301 as method getFullMessagesForInvalidSigners could return only two parameters, not needed the middle one.

invalidSigners, err := sr.getFullMessagesForInvalidSigners(invalidPubKeys) if err != nil { log.Debug("doEndRoundJobByLeader.getFullMessagesForInvalidSigners", "error", err.Error()) return false } if len(invalidSigners) > 0 { sr.createAndBroadcastInvalidSigners(invalidSigners) }

SebastianMarian · 2022-10-18T13:24:33Z

consensus/spos/bls/subroundEndRound.go

@@ -337,7 +438,7 @@ func (sr *subroundEndRound) verifyNodesOnAggSigVerificationFail() error {

 			err = sr.SetJobDone(pk, SrSignature, false)
 			if err != nil {
-				return err
+				return nil, err


L445 should be like this I think, as we should punish the validator who sent this, not just to revert things to his last honesty level:
decreaseFactor := -spos.ValidatorPeerHonestyIncreaseFactor + spos.ValidatorPeerHonestyDecreaseFactor

Exactly as we do in subRoundSignature -> receivedSignature method, for nodes who are not in consensus but they are sending signatures. Anyway the invalid signatures are much worse than valid ones which are sent when is not necessary:

if !sr.IsNodeInConsensusGroup(node) { sr.PeerHonestyHandler().ChangeScore( node, spos.GetConsensusTopicID(sr.ShardCoordinator()), spos.ValidatorPeerHonestyDecreaseFactor, ) return false }

SebastianMarian · 2022-10-18T14:28:38Z

consensus/spos/consensusState.go

@@ -63,6 +67,7 @@ func (cns *ConsensusState) ResetConsensusState() {
 	cns.Data = nil

 	cns.initReceivedHeaders()
+	cns.initReceivedMessagesWithSig()


...WithInvalidSigners ?

SebastianMarian · 2022-10-18T14:30:12Z

consensus/spos/consensusState.go

@@ -94,6 +105,22 @@ func (cns *ConsensusState) GetReceivedHeaders() []data.HeaderHandler {
 	return receivedHeaders
 }

+// AddMessageWithSignature will add the p2p message to received list of messages
+func (cns *ConsensusState) AddMessageWithSignature(key string, message p2p.MessageP2P) {


...WithInvalidSigners ?

SebastianMarian · 2022-10-18T14:30:24Z

consensus/spos/consensusState.go

+}
+
+// GetMessageWithSignature will get the p2p message based on key
+func (cns *ConsensusState) GetMessageWithSignature(key string) (p2p.MessageP2P, bool) {


...WithInvalidSigners ?

SebastianMarian · 2022-10-18T14:33:44Z

consensus/spos/interface.go

@@ -53,6 +53,8 @@ type ConsensusCoreHandler interface {
 	PrivateKey() crypto.PrivateKey
 	// SingleSigner returns the single signer stored in the ConsensusStore used for randomness and leader's signature generation
 	SingleSigner() crypto.SingleSigner
+	// KeyGenerator returns the key generator stored in the ConsensusStore


ConsensusCore instead ConsensusStore in the whole file and also in some other files (2 files)

SebastianMarian · 2022-10-18T14:35:37Z

consensus/spos/interface.go

@@ -63,6 +65,8 @@ type ConsensusCoreHandler interface {
 	NodeRedundancyHandler() consensus.NodeRedundancyHandler
 	// ScheduledProcessor returns the scheduled txs processor
 	ScheduledProcessor() consensus.ScheduledProcessor
+	// MessageSignerHandler


Finalize this comment

SebastianMarian · 2022-10-18T15:02:41Z

consensus/mock/consensusDataContainerMock.go

@@ -191,6 +193,11 @@ func (ccm *ConsensusCoreMock) SingleSigner() crypto.SingleSigner {
 	return ccm.blsSingleSigner
 }

+// KeyGenerator -


L191: ConsensusCore instead ConsensusStore and also in some other files (2 more files)

SebastianMarian · 2022-10-20T07:44:21Z

consensus/spos/interface.go

@@ -65,7 +65,7 @@ type ConsensusCoreHandler interface {
 	NodeRedundancyHandler() consensus.NodeRedundancyHandler
 	// ScheduledProcessor returns the scheduled txs processor
 	ScheduledProcessor() consensus.ScheduledProcessor
-	// MessageSignerHandler
+	// MessageSignerHandler return the p2p signing handler


AdoAdoAdo · 2022-10-21T07:24:36Z

consensus/spos/bls/subroundEndRound.go

@@ -442,7 +456,7 @@ func (sr *subroundEndRound) verifyNodesOnAggSigVerificationFail() ([]string, err
 			}

 			// use increase factor since it was added optimistically, and it proved to be wrong
-			decreaseFactor := -spos.ValidatorPeerHonestyIncreaseFactor
+			decreaseFactor := -spos.ValidatorPeerHonestyIncreaseFactor + spos.ValidatorPeerHonestyDecreaseFactor


ssd04 added 5 commits July 4, 2022 10:53

added new field for consensus message - InvalidSigners

8da8b53

first implementation for pseudo slashing

765a83b

temp version for p2p pubsub

514622f

add custom type for invalid signers

6dcfb39

first implementation for pseudo slashing: fix unit tests

7b78941

ssd04 self-assigned this Jul 7, 2022

ssd04 added 10 commits July 7, 2022 16:26

validity checks for invalisSigners consensus message type + use json …

947f8b8

…marshaller

p2p: added additional field with marshalled pubsub message to p2p mes…

044b165

…sage

use original marshalled p2p message instead of json marshalling

c2dece2

added new field for consensus message - number of InvalidSigners

c70ac05

reverted new field for consensus message - number of InvalidSigners

d5e629f

added interface for p2p message handling

46968f9

use p2p message signing component in subround endround

fbdc098

subround endround: unit test for invalid signers verification

5e2d918

p2p: remove marshalled data field

53e3bec

consensus: use p2p message instead of marshalled data

d01da9f

ssd04 added 5 commits July 13, 2022 12:08

unit tests for consensus core and message validator

73c46eb

cleanup unused core:

9919dc3

- remove reference to temp pubsub library - restore p2p message changes

consensus: more unit tests for end round + comment updates

a0b5f11

flag check for invalid signers + peer honesty change

fccb94d

fix should send flag on error

6cd0194

ssd04 marked this pull request as ready for review July 13, 2022 10:35

ssd04 added 6 commits July 19, 2022 10:43

bls worker: more checks for invalid signers consensus message

d185fe3

bls worker: more checks for invalid signers consensus message - fix u…

4736186

…nit test

use single signer to verify sig share; add keyGenerator to consensus …

887d6ed

…core

use single signer to verify sig share; add keyGenerator to consensus …

c818111

…core - fix unit tests

fix mocks: remove reference to peer blacklist

9250e77

add message signer mock

86e1b51

AdoAdoAdo self-requested a review July 19, 2022 13:46

ssd04 added 8 commits August 2, 2022 11:04

consensus state: rlock for get message with signature

4f3b53f

Merge branch 'feat/optimise-consensus-sigcheck' into pseudo-slashing-…

984ac90

…for-invalid-signers

spos endround: comments and log updates

67f385a

consensus worker: remove TODO comment

e6609d0

Merge branch 'feat/optimise-consensus-sigcheck' into merge-optimize-c…

d759b60

…onsensus-sig-into-pseudo-slashing

Merge pull request #4509 from ElrondNetwork/merge-optimize-consensus-…

33334b0

…sig-into-pseudo-slashing Merge optimize consensus sig into pseudo slashing

Merge branch 'feat/optimise-consensus-sigcheck' into merge-optimize-c…

f850519

…onsensus-sig-into-pseudo-slashing

Merge pull request #4557 from ElrondNetwork/merge-optimize-consensus-…

f60633f

…sig-into-pseudo-slashing Merge optimize consensus sig into pseudo slashing

SebastianMarian self-requested a review October 17, 2022 12:25

AdoAdoAdo requested changes Oct 18, 2022

View reviewed changes

SebastianMarian reviewed Oct 18, 2022

View reviewed changes

ssd04 added 3 commits October 19, 2022 21:43

fixes after review: more check, small refactoring, comments update

4272475

fix linter issue

d92dc33

added more unit tests for received invalid signers info

387e6f9

SebastianMarian reviewed Oct 20, 2022

View reviewed changes

fix comment typo

63d7e98

SebastianMarian approved these changes Oct 20, 2022

View reviewed changes

AdoAdoAdo approved these changes Oct 21, 2022

View reviewed changes

AdoAdoAdo merged commit 405148c into feat/optimise-consensus-sigcheck Oct 21, 2022

AdoAdoAdo deleted the pseudo-slashing-for-invalid-signers branch October 21, 2022 07:41

schimih added the type:improvement label May 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pseudo slashing for invalid signers #4269

Pseudo slashing for invalid signers #4269

ssd04 commented Jul 6, 2022 •

edited

codecov-commenter commented Jul 13, 2022 •

edited

SebastianMarian Oct 18, 2022

ssd04 Oct 19, 2022

SebastianMarian Oct 18, 2022 •

edited

ssd04 Oct 19, 2022

SebastianMarian Oct 18, 2022 •

edited

ssd04 Oct 19, 2022

SebastianMarian Oct 18, 2022 •

edited

SebastianMarian Oct 18, 2022 •

edited

SebastianMarian Oct 18, 2022 •

edited

SebastianMarian Oct 18, 2022

ssd04 Oct 19, 2022

SebastianMarian Oct 18, 2022

ssd04 Oct 19, 2022

SebastianMarian Oct 18, 2022

ssd04 Oct 19, 2022

SebastianMarian Oct 20, 2022

ssd04 Oct 20, 2022

AdoAdoAdo Oct 21, 2022

Pseudo slashing for invalid signers #4269

Pseudo slashing for invalid signers #4269

Conversation

ssd04 commented Jul 6, 2022 • edited

codecov-commenter commented Jul 13, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianMarian Oct 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianMarian Oct 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianMarian Oct 18, 2022 • edited

Choose a reason for hiding this comment

SebastianMarian Oct 18, 2022 • edited

Choose a reason for hiding this comment

SebastianMarian Oct 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssd04 commented Jul 6, 2022 •

edited

codecov-commenter commented Jul 13, 2022 •

edited

SebastianMarian Oct 18, 2022 •

edited

SebastianMarian Oct 18, 2022 •

edited

SebastianMarian Oct 18, 2022 •

edited

SebastianMarian Oct 18, 2022 •

edited

SebastianMarian Oct 18, 2022 •

edited