Invalid signers cache #6873

sstanculeanu · 2025-03-12T10:00:46Z

Reasoning behind the pull request

as now all participants can aggregate signatures and send final info, the network might get flooded with the invalid signers messages

Proposed changes

added a mechanism similar to equivalent proofs to avoid further broadcast in case of known invalid signers message

Testing procedure

with rc 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?

…th the same message

andreibancioiu · 2025-03-12T11:53:39Z

consensus/spos/invalidSignersCache.go

+	cache.invalidSignersMap[hash] = struct{}{}
+}
+
+// HasInvalidSigners check whether the provided hash exists in int internal map or not


Typo in int 🙃

andreibancioiu · 2025-03-12T11:54:51Z

consensus/spos/invalidSignersCache.go

+	sync.RWMutex
+	invalidSignersMap map[string]struct{}


Not sure, maybe sync.Map would have also worked (shorter code)?

back to map + mut due to reset method

andreibancioiu · 2025-03-12T11:59:03Z

consensus/spos/worker.go

+func (wrk *Worker) verifyMessageWithInvalidSigners(cnsMsg *consensus.Message) error {
+	// No need to guard this method by verification of common.EquivalentMessagesFlag as invalidSignersCache will have entries only for consensus v2
+	invalidSignersHash := wrk.hasher.Compute(string(cnsMsg.InvalidSigners))
+	if wrk.invalidSignersCache.HasInvalidSigners(string(invalidSignersHash)) {
+		// return error here to avoid further broadcast of this message
+		return ErrInvalidSignersAlreadyReceived
+	}
+
+	return nil
+}


So all good if someone sends a clandestine message with this topic shortly before Andromeda's activation?

added check for flag to be active. used IsFlagEnabled as header epoch is unknown for this message

raduchis · 2025-03-12T12:15:37Z

consensus/spos/bls/v2/subroundEndRound.go

 func (sr *subroundEndRound) handleInvalidSignersOnAggSigFail() ([]byte, []byte, error) {
 	ctx, cancel := context.WithTimeout(context.Background(), sr.RoundHandler().TimeDuration())
 	invalidPubKeys, err := sr.verifyNodesOnAggSigFail(ctx)
 	cancel()


not related to the changes from this PR, but this cancel() doesn't seem right to call it after the "intensive". Maybe move it up 1 line and call it on a defer?

raduchis · 2025-03-12T12:32:00Z

consensus/spos/bls/v2/subroundEndRound.go


+	invalidSignersHash := sr.Hasher().Compute(string(cnsDta.InvalidSigners))
+	invalidSignersCache := sr.InvalidSignersCache()
+	if invalidSignersCache.HasInvalidSigners(string(invalidSignersHash)) {


if there are different number of invalidSigners: first time 2: A and B, and in on a second message it gets only A, it seems that the message will be propagated 2 times?

consensus/spos/worker.go

andreibancioiu · 2025-03-13T13:50:02Z

consensus/spos/bls/v2/subroundEndRound.go

+		}
+
+		if len(pubKey) > 0 {
+			pubKeys = append(pubKeys, pubKey)


So we only collect the public keys of the good signers? And if there's any invalid one, we collect nothing. Should we find a better name for the function e.g. findGoodSigners?

not really, this method checks whether the invalid signers provided an invalid signature.. so this appends the invalid signers that we confirmed they had an invalid signature

andreibancioiu · 2025-03-13T13:50:52Z

consensus/spos/bls/v2/subroundEndRound.go

-			return err
+		pubKey, errVerify := sr.verifyInvalidSigner(msg)
+		if errVerify != nil {
+			return nil, errVerify


Thus, here we break & return, all good?

yes, it means we were not able to verify if the provided invalid signers indeed had invalid signatures

andreibancioiu · 2025-03-13T13:51:53Z

consensus/spos/bls/v2/subroundEndRound.go

 	err := sr.MessageSigningHandler().Verify(msg)
 	if err != nil {
-		return err
+		return "", err


Note to ourselves - this is the raw byte pubkey, not the hex-encoded one (from what I understand). No change needed.

andreibancioiu · 2025-03-13T14:03:01Z

consensus/spos/invalidSignersCache.go

+type invalidSignersCache struct {
+	sync.RWMutex
+	invalidSignersHashesMap    map[string]struct{}
+	invalidSignersForHeaderMap map[string]map[string]struct{}


So, for each header, we retain the public keys of the "bad" signers (as a map, for easy lookup), correct?

andreibancioiu · 2025-03-13T14:04:10Z

consensus/spos/invalidSignersCache.go

+	for _, pk := range invalidPublicKeys {
+		cache.invalidSignersForHeaderMap[string(headerHash)][pk] = struct{}{}
+	}


Nice automatic de-duplication, in case we get different sets of bad signers for the same header 👍

andreibancioiu · 2025-03-13T14:07:00Z

consensus/spos/invalidSignersCache.go

+	invalidSignersHash := cache.hasher.Compute(string(invalidSigners))
+	_, hasSameInvalidSigners := cache.invalidSignersHashesMap[string(invalidSignersHash)]
+	if hasSameInvalidSigners {
+		return true


Early answer if we've previously received the exact same message, correct? I mean, this is just an optimization - is it a crucial one?

I am thinking out loud - if this node already knows about this exact set of invalid signers, for the given block, it will not forward the message / it will stop the gossip. Also, if the node does not know about this exact set, but knows about a superset of invalid signers (which include this set), it will also stop the gossip.

Correct?

for the first question, it indeed is just an optimization to avoid the Deserialization

for the second one, correct

andreibancioiu · 2025-03-13T14:08:51Z

consensus/spos/invalidSignersCache.go

+		cnsMsg := &consensus.Message{}
+		err = cache.marshaller.Unmarshal(cnsMsg, msg.Data())
+		if err != nil {
+			return false


Unfortunate / rare circumstance? If so, maybe we should log it (as warning perhaps)?

hm.. not sure it is needed. worst case, the node will process twice the same message

andreibancioiu · 2025-03-13T14:11:33Z

consensus/spos/invalidSignersCache.go

+		return false
+	}
+
+	messages, err := cache.signingHandler.Deserialize(invalidSigners)


Maybe can be renamed from messages to signers. Or even invalidSigners, while the existing variable can be renamed to serializedInvalidSigners.

applied suggestion, renamed to invalidSignersP2PMessages

andreibancioiu · 2025-03-13T14:13:35Z

consensus/spos/invalidSignersCache.go

+		}
+	}
+
+	return knownInvalidSigners == len(messages)


Opinion (sorry if mistaken, I might have missed something): we can also return early if we find an unknown one, and skip the counting logic.

good suggestion, applied

andreibancioiu · 2025-03-13T14:14:10Z

consensus/spos/invalidSignersCache.go

+	return knownInvalidSigners == len(messages)
+}
+
+// Reset clears the internal map


consensus/spos/bls/v2/subroundEndRound.go

consensus/spos/invalidSignersCache.go

consensus/spos/worker.go

added invalid signers cache in order to avoid flooding the network wi…

b329c94

…th the same message

sstanculeanu self-assigned this Mar 12, 2025

andreibancioiu reviewed Mar 12, 2025

View reviewed changes

raduchis self-requested a review March 12, 2025 12:01

raduchis requested changes Mar 12, 2025

View reviewed changes

sstanculeanu added 4 commits March 12, 2025 15:25

fixes after review

03edfe7

back to map + mut

d34dde1

improved the invalidSignersCache mechanism

50d43da

fix failing test

4665ec6

andreibancioiu reviewed Mar 13, 2025

View reviewed changes

further fixes after review

8e7319a

andreibancioiu previously approved these changes Mar 13, 2025

View reviewed changes

raduchis reviewed Mar 14, 2025

View reviewed changes

fixes after review

95dde91

sstanculeanu dismissed andreibancioiu’s stale review via 95dde91 March 14, 2025 13:46

andreibancioiu approved these changes Mar 14, 2025

View reviewed changes

Merge branch 'feat/andromeda-fixes' into invalid_signers_cache

40304bc

raduchis approved these changes Mar 17, 2025

View reviewed changes

consensus/spos/worker.go Show resolved Hide resolved

consensus/spos/worker.go Show resolved Hide resolved

sstanculeanu merged commit f7f185c into feat/andromeda-fixes Mar 17, 2025
4 checks passed

sstanculeanu deleted the invalid_signers_cache branch March 17, 2025 09:00

Invalid signers cache #6873

Invalid signers cache #6873

Uh oh!

Conversation

sstanculeanu commented Mar 12, 2025

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development