Gossipsub Tracer RPCSent tracker #4543

kc1116 · 2023-07-06T05:34:03Z

This PR adds a tracker backed by herocache that tracks RPC messages sent from the local node. This is the first part of the iWant flooding https://github.com/dapperlabs/flow-go/issues/6472 . This tracker is intended to be used during RPC inspection iWant validation to ensure iWant control messages received are in response to iHave messages that have been sent.

…have-rpc-tracker

- track iHave messages on each RPC sent

codecov-commenter · 2023-07-06T05:40:26Z

Codecov Report

Merging #4543 (6b6a74a) into master (2528190) will increase coverage by 1.08%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #4543      +/-   ##
==========================================
+ Coverage   56.25%   57.34%   +1.08%     
==========================================
  Files         653      575      -78     
  Lines       64699    57866    -6833     
==========================================
- Hits        36396    33182    -3214     
+ Misses      25362    22236    -3126     
+ Partials     2941     2448     -493

Flag	Coverage Δ
unittests	`57.34% <0.00%> (+1.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
module/metrics/herocache.go	`0.00% <0.00%> (ø)`
module/metrics/network.go	`0.00% <0.00%> (ø)`
module/metrics/noop.go	`0.00% <0.00%> (ø)`
utils/unittest/unittest.go	`7.48% <0.00%> (-0.12%)`	⬇️

... and 408 files with indirect coverage changes

network/p2p/tracer/internal/rpc_sent_tracker.go

network/p2p/tracer/internal/rpc_sent_tracker_test.go

yhassanzadeh13

Overall, looks great. However, please consider incorporating my suggestions to refine your code further. Once these adjustments have been made, please proceed with the merge.

config/default-config.yml

module/metrics/labels.go

network/p2p/tracer/gossipSubMeshTracer.go

yhassanzadeh13 · 2023-07-10T20:11:36Z

network/p2p/tracer/internal/cache.go

+// - bool: true if the record is initialized, false otherwise (i.e.: the record already exists).
+// Note that if init is called multiple times for the same messageID, the record is initialized only once, and the
+// subsequent calls return false and do not change the record (i.e.: the record is not re-initialized).
+func (r *rpcSentCache) init(messageID flow.Identifier, controlMsgType p2pmsg.ControlMessageType) bool {


I propose repositioning the iHaveRPCSentEntityID method to this file and consolidating it with the newRPCSentEntity function as follows:

func rpcSentEntityID(topic string, messageId string, controlMsgType p2pmsg.ControlMessageType) flow.Identifier { return flow.MakeIDFromFingerPrint([]byte(fmt.Sprintf("%s%s%s", topic, messageId, controlMsgType))) }

This modification promotes efficient computing by saving one hash computation per message. Currently, we're processing two hashes for each message, which seems superfluous.

In addition, I recommend a substantial refactor of the init method to improve its alignment with common software architecture principles such as cohesion, coupling, and simplicity:

The method name init should be changed to add. This aligns with the standard Add operations found in our mempools, which will improve understandability due to its familiar behavior.

Its signature should be modified to add(topic string, messageId string, controlMsgType p2pmsg.ControlMessageType). This change enhances cohesion and reduces confusion by clarifying the type of records it maintains. The current init(messageID flow.Identifier, controlMsgType p2pmsg.ControlMessageType) bool signature displays low cohesion and high coupling, requiring the caller to comprehend and compute the messageID externally. Given that the messageID is merely used as a key to keep records, this understanding should be internal to the cache. The current setup expects the caller to comprehend this internal aspect, which is not aligned with the best practices. Furthermore, the messageID as a flow.Identifier provides a generic representation, leaving ambiguity about its actual meaning. This issue can be resolved by utilizing the rpcSentEntityID method defined above to compute the messageID.

Such changes adhere to the principles of software architecture, namely encapsulation, cohesion, and low coupling. By hiding internal details like the computation of messageID, we achieve better encapsulation. The refactoring also improves cohesion because the new add method will perform a more focused, single responsibility: adding a record based on specific input parameters. Additionally, the coupling is reduced as the new method is less reliant on the external computation of messageID.

yhassanzadeh13 · 2023-07-10T20:15:59Z

network/p2p/tracer/internal/cache.go

+// - flow.Identifier: the messageID to store the rpc control message.
+// Returns:
+// - bool: true if the RPC has been cache indicating it was sent from the local node.
+func (r *rpcSentCache) has(messageId flow.Identifier) bool {


Building upon the previous comment, I suggest refactoring the has function to:

has(topic string, messageId string, controlMsgType p2pmsg.ControlMessageType) bool { }

This adjustment ensures a consistent and clear signature across all methods, enhancing code readability and maintenance.

The external call would then be presented as follows:

func (t *RPCSentTracker) WasIHaveRPCSent(topicID, messageID string) bool { return t.cache.has(topicID, messageID, p2pmsg.CtrlMsgIHave) }

This representation aligns with the refactored add method, ensuring consistency throughout the interface. It also simplifies the interaction by providing an intuitive method signature, reinforcing the principle of least surprise in software design.

yhassanzadeh13 · 2023-07-10T20:17:33Z

network/p2p/tracer/internal/cache_test.go

Tests looks good, but please refactor them based on the comments on the signature of add and has methods.

yhassanzadeh13 · 2023-07-10T20:28:41Z

network/p2p/tracer/gossipSubMeshTracer.go

@@ -139,6 +151,15 @@ func (t *GossipSubMeshTracer) Prune(p peer.ID, topic string) {
 	lg.Info().Hex("flow_id", logging.ID(id.NodeID)).Str("role", id.Role.String()).Msg("pruned peer")
 }

+// SendRPC is called when a RPC is sent. Currently, the GossipSubMeshTracer tracks iHave RPC messages that have been sent.
+// This function can be updated to track other control messages in the future as required.
+func (t *GossipSubMeshTracer) SendRPC(rpc *pubsub.RPC, _ peer.ID) {


I'd like to propose a refinement for consideration in a separate PR, and you're welcome to open an issue and address it independently:

The current setup involves the SendRPC function being invoked by GossipSub in a blocking manner. This implies that any processing performed here will result in a temporary pause of outgoing RPCs. This pause can negatively impact the performance and correctness of GossipSub message dissemination, potentially leading to dropped outgoing RPCs due to a 'context deadline exceeded' error that results from blocking the processing of an RPC. In addition, this can create lock contention between tracking sent RPCs and evaluating incoming RPCs when checking if an iWant corresponds to a previously advertised iHave.

To mitigate these issues, I suggest implementing a non-blocking approach for processing RPCs. This can be accomplished by utilizing a worker queue and dispatching a copy of the RPC to the queue for further processing while promptly returning from the function. This strategy also eliminates the aforementioned lock contention.

You might draw inspiration from the following example when implementing this solution: https://github.com/onflow/flow-go/blob/master/network/alsp/manager/manager.go#L228-L258.

By reducing contention, we can minimize the risk of deadlocks and improve the overall response time of the system.

Follow up issue https://github.com/dapperlabs/flow-internal/issues/1899

yhassanzadeh13 · 2023-07-10T20:31:38Z

network/p2p/tracer/internal/rpc_sent_tracker_test.go

+	t.Run("WasIHaveRPCSent should return true for iHave message after it is tracked with OnIHaveRPCSent", func(t *testing.T) {
+		topicID := channels.PushBlocks.String()
+		messageID1 := unittest.IdentifierFixture().String()
+		iHaves := []*pb.ControlIHave{{
+			TopicID:    &topicID,
+			MessageIDs: []string{messageID1},
+		}}
+		rpc := rpcFixture(withIhaves(iHaves))
+		tracker.OnIHaveRPCSent(rpc.GetControl().GetIhave())
+		require.True(t, tracker.WasIHaveRPCSent(topicID, messageID1))
+
+		// manipulate last byte of message ID ensure false positive not returned
+		messageID2 := []byte(messageID1)
+		messageID2[len(messageID2)-1] = 'X'
+		require.False(t, tracker.WasIHaveRPCSent(topicID, string(messageID2)))
+	})
+}


Please also add a test case tracking multiple messages on multiple topics.

Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>

…w-go into khalil/1897-ihave-rpc-tracker

…age ID's

kc1116 added 3 commits July 5, 2023 12:34

add control message rpc sent tracker for use in gossip sub tracer

4c635c5

Merge branch 'master' of github.com:onflow/flow-go into khalil/1897-i…

cefc60b

…have-rpc-tracker

add rpc sent tracker to gossipsub mesh tracer

2850cac

- track iHave messages on each RPC sent

kc1116 requested a review from peterargue July 6, 2023 05:34

kc1116 requested a review from yhassanzadeh13 as a code owner July 6, 2023 05:34

peterargue reviewed Jul 7, 2023

View reviewed changes

network/p2p/tracer/internal/rpc_sent_tracker.go Outdated Show resolved Hide resolved

network/p2p/tracer/internal/rpc_sent_tracker_test.go Show resolved Hide resolved

kc1116 added 3 commits July 7, 2023 12:32

use MakeIDFromFingerPrint

1e75800

update WasIHaveRPCSent test ensure false positive not returned

c7119f9

Update rpc_sent_tracker.go

33681ec

kc1116 requested a review from peterargue July 7, 2023 16:42

Merge branch 'master' into khalil/1897-ihave-rpc-tracker

3dcbf9b

peterargue approved these changes Jul 7, 2023

View reviewed changes

yhassanzadeh13 approved these changes Jul 10, 2023

View reviewed changes

kc1116 and others added 15 commits July 11, 2023 09:30

Update config/default-config.yml

c7a4d58

Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>

Update module/metrics/labels.go

252dd2d

Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>

Update network/p2p/tracer/gossipSubMeshTracer.go

647065b

Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>

document irrecoverable error in NewGossipSubMeshTracer

72e6b7d

Update network/p2p/tracer/gossipSubMeshTracer.go

1dcb3a4

Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>

use cache size from default config

2b2d2fc

Update network/p2p/tracer/internal/cache.go

6b6a74a

Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>

rename RPCSentCacheConfig -> rpcCtrlMsgSentCacheConfig

4588903

Merge branch 'khalil/1897-ihave-rpc-tracker' of github.com:onflow/flo…

c52f26a

…w-go into khalil/1897-ihave-rpc-tracker

add godoc for rpcCtrlMsgSentCacheConfig

8d95bb4

remove error return from NewGossipSubMeshTracer func signature

cfc3672

rename messageId -> messageEntityID

d270551

remove unused funcs remove & ids

8edb744

improve cache func signature cohesion

60b764d

update WasIHaveRPCSent to check multiple topic IDs with multiple mess…

64f629d

…age ID's

kc1116 merged commit 36c7855 into master Jul 11, 2023

kc1116 deleted the khalil/1897-ihave-rpc-tracker branch July 11, 2023 23:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gossipsub Tracer RPCSent tracker #4543

Gossipsub Tracer RPCSent tracker #4543

kc1116 commented Jul 6, 2023

codecov-commenter commented Jul 6, 2023 •

edited

Loading

yhassanzadeh13 left a comment

yhassanzadeh13 Jul 10, 2023

kc1116 Jul 11, 2023

yhassanzadeh13 Jul 10, 2023

kc1116 Jul 11, 2023

yhassanzadeh13 Jul 10, 2023

kc1116 Jul 11, 2023

yhassanzadeh13 Jul 10, 2023

kc1116 Jul 11, 2023

yhassanzadeh13 Jul 10, 2023

kc1116 Jul 11, 2023

Gossipsub Tracer RPCSent tracker #4543

Gossipsub Tracer RPCSent tracker #4543

Conversation

kc1116 commented Jul 6, 2023

codecov-commenter commented Jul 6, 2023 • edited Loading

Codecov Report

yhassanzadeh13 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 6, 2023 •

edited

Loading