Large message handling combined #1234

ufarooqstatus · 2025-01-10T12:44:26Z

We use message preamble to detect incoming messages. This information is used to:

Announce the messages we are receiving to full-message mesh using IMReceiving messages.
Make IWANT requests only if we are not already receiving the same message
Limit outstanding IWANT requests for a message to 1
Stagger (only add small delay) mesh transmissions

PoC implementation for shadow simulation. Not intended for merging
Reduces duplicates to less than 1.5 for large messages

More details here

… issuing iwant

updating local branch

…imeouts

…r iwants

github-actions · 2025-01-10T12:44:41Z

Pull requests titles must follow the Conventional Commits specification

github-actions · 2025-01-10T12:44:50Z

Commits must follow the Conventional Commits specification
Please fix these commit messages:

some coments added
preamble added in IWANT replies also
topicID added in preamble
sem is one (sequential send in staggering)
Added control preamble, and staggering. Still need to add preamble for iwants
stagger send during publish and validateAndRelay. Still need to add timeouts
staggering added in message relay, still need to adjust timeouts
corrected dup_during_validation count
Merge branch 'master' into lma_merge_imreceiving_iwant_1 updating local branch
we make only one iwant request
set num_finds to 1
imreceiving handling merged with iwant optimization
IMReceiving message added
stats places, warmup messages added

richard-ramos

Very interesting PR!

Is this PR also implementing this part of the spec?:

if the message ID is found in the ongoing_receives list,
the peer should postpone sending the IWANT request for a defer_interval.
The defer_interval may be based on the message download time.

richard-ramos · 2025-01-14T12:46:01Z

libp2p/protocols/pubsub/gossipsub.nim

@@ -226,6 +253,7 @@ method init*(g: GossipSub) =
  g.codecs &= GossipSubCodec_12
  g.codecs &= GossipSubCodec_11
  g.codecs &= GossipSubCodec_10


Would this change result in a GossipSubCodec_14 as described in libp2p/specs#654 ?

richard-ramos · 2025-01-14T12:51:17Z

libp2p/protocols/pubsub/gossipsub.nim

@@ -782,7 +873,25 @@ method publish*(g: GossipSub, topic: string, data: seq[byte]): Future[int] {.asy

  g.mcache.put(msgId, msg)

-  g.broadcast(peers, RPCMsg(messages: @[msg]), isHighPriority = true)
+  #g.broadcast(peers, RPCMsg(messages: @[msg]), isHighPriority = true)


✂️ ?

Suggested change

#g.broadcast(peers, RPCMsg(messages: @[msg]), isHighPriority = true)

richard-ramos · 2025-01-14T12:55:19Z

libp2p/protocols/pubsub/gossipsub.nim

+  #g.broadcast(peers, RPCMsg(messages: @[msg]), isHighPriority = true)
+  let sem = newAsyncSemaphore(1)
+  var staggerPeers = toSeq(peers)
+  g.rng.shuffle(staggerPeers)


Why are the peers randomly sorted?

richard-ramos · 2025-01-14T13:03:57Z

libp2p/protocols/pubsub/gossipsub.nim

+  #We send message immediately after sending preamble to each peer
+  proc sendToOne(p: PubSubPeer) {.async.} =
+    g.broadcast(@[p], RPCMsg(control: some(ControlMessage(
+        preamble: @[ControlIHave(topicID: topic, messageIDs: @[msgId])]
+      ))), isHighPriority = true)
+
+    await sem.acquire()
+    defer: sem.release()
+
+    g.broadcast(@[p], RPCMsg(messages: @[msg]), isHighPriority = false)
+
+  for p in staggerPeers:
+    asyncSpawn sendToOne(p)
+


Using the AsyncSemaphore(1) means that the preambles are being sent sequentially to each peer, right? Why is this the case? Shouldn't we try to get the preamble messages being sent concurrently to all peers?

richard-ramos · 2025-01-14T13:04:50Z

libp2p/protocols/pubsub/gossipsub.nim

+    await sem.acquire()
+    defer: sem.release()
+
+    g.broadcast(@[p], RPCMsg(messages: @[msg]), isHighPriority = false)


No isHighPriority = true?

richard-ramos · 2025-01-14T13:08:13Z

libp2p/protocols/pubsub/gossipsub/behavior.nim

+            return result
          continue
        let msg = g.mcache.get(mid).valueOr:
          libp2p_gossipsub_received_iwants.inc(1, labelValues = ["unknown"])
          continue
        libp2p_gossipsub_received_iwants.inc(1, labelValues = ["correct"])
-        messages.add(msg)
-  return messages
+        result.messages.add(msg)
+        result.ids.add(mid)


It's probably better to define a messages and ids variables, instead of using result. See https://status-im.github.io/nim-style-guide/language.result.html

richard-ramos · 2025-01-14T13:14:54Z

libp2p/protocols/pubsub/gossipsub/behavior.nim

+      if peer.heIsReceivings[^1].len > 1000: break
+      if messageId.len > 100: continue


When do messageIds exceed 100? I'm wondering if this condition should be added in other places as i haven't seen length checking for messageIds using 100. (Probably a good idea to extract this into a constant too)

richard-ramos · 2025-01-14T13:29:41Z

libp2p/protocols/pubsub/rpc/messages.nim

+    preamble*: seq[ControlIHave]
+    imreceiving*: seq[ControlIWant]


Shouldn't these have their own type? ControlPreamble and ControlImReceiving?

richard-ramos · 2025-01-14T13:31:33Z

libp2p/protocols/pubsub/gossipsub.nim

+  #We send message immediately after sending preamble to each peer
+  proc sendToOne(p: PubSubPeer) {.async.} =
+    g.broadcast(@[p], RPCMsg(control: some(ControlMessage(
+        preamble: @[ControlIHave(topicID: topic, messageIDs: @[msgId])]


Sending the message length is still pending, right?

richard-ramos · 2025-01-14T13:34:02Z

libp2p/protocols/pubsub/gossipsub.nim

+    await sem.acquire()
+    defer: sem.release()
+
+    g.broadcast(@[p], RPCMsg(messages: @[msg]), isHighPriority = false)


Reading in the specs, I see the following:

Adding a message preamble may increase control overhead for small messages.
Therefore, it is preferable to use it only for messages that exceed the preamble_threshold.

The way it is coded right now, it will send the preamble always, right? Is this because this PR is for experiments?

ufarooqstatus added 15 commits September 22, 2024 23:31

added stats counters, still to check message receives from mesh after…

Loading
Loading status checks…

35d1876

… issuing iwant

stats places, warmup messages added

8377eb0

IMReceiving message added

f1b78f6

imreceiving handling merged with iwant optimization

8bb6215

set num_finds to 1

4b691b6

we make only one iwant request

b2a75fc

Merge branch 'master' into lma_merge_imreceiving_iwant_1

64ef502

updating local branch

corrected dup_during_validation count

1fe2bae

staggering added in message relay, still need to adjust timeouts

ab0feb3

stagger send during publish and validateAndRelay. Still need to add t…

0677ab4

…imeouts

Added control preamble, and staggering. Still need to add preamble fo…

43f1f0a

…r iwants

sem is one (sequential send in staggering)

58470a7

topicID added in preamble

0acd8be

preamble added in IWANT replies also

e779185

some coments added

ac9e1e6

github-actions bot assigned ufarooqstatus Jan 10, 2025

ufarooqstatus marked this pull request as draft January 10, 2025 12:47

ufarooqstatus requested a review from richard-ramos January 10, 2025 14:13

richard-ramos reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Large message handling combined #1234

Large message handling combined #1234

ufarooqstatus commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

richard-ramos left a comment •

edited

Loading

richard-ramos Jan 14, 2025

richard-ramos Jan 14, 2025

richard-ramos Jan 14, 2025

richard-ramos Jan 14, 2025 •

edited

Loading

richard-ramos Jan 14, 2025

richard-ramos Jan 14, 2025

richard-ramos Jan 14, 2025

richard-ramos Jan 14, 2025

richard-ramos Jan 14, 2025

richard-ramos Jan 14, 2025

		if peer.heIsReceivings[^1].len > 1000: break
		if messageId.len > 100: continue

Large message handling combined #1234

Are you sure you want to change the base?

Large message handling combined #1234

Conversation

ufarooqstatus commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

richard-ramos left a comment • edited Loading

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

richard-ramos Jan 14, 2025 • edited Loading

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

richard-ramos left a comment •

edited

Loading

richard-ramos Jan 14, 2025 •

edited

Loading