Large message handling combined by ufarooqstatus · Pull Request #1234 · vacp2p/nim-libp2p

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

@@ -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

  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

+  #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

+  #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

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


No isHighPriority = true?

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

+            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

+      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

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


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

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

+  #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

+    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?

richard-ramos · 2025-09-12T15:23:47Z

@ufarooqstatus should this PR be kept open? or can it be closed

ufarooqstatus · 2025-09-13T12:24:49Z

@ufarooqstatus should this PR be kept open? or can it be closed

yes, it can be closed. it was an early PoC for v1.4

ufarooqstatus added 15 commits September 22, 2024 23:31

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

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

ufarooqstatus mentioned this pull request Apr 11, 2025

GossipSub v1.4: Message preamble + IMReceiving notification to considerably reduce bandwidth & latency for large messages libp2p/specs#654

Open

richard-ramos closed this Mar 17, 2026

github-project-automation Bot moved this from Experimental to done in nim-libp2p Mar 17, 2026

richard-ramos deleted the largeMsg_Merge_IMReceiving_Stagger branch March 30, 2026 14:57

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

Conversation

ufarooqstatus commented Jan 10, 2025

Uh oh!

github-actions Bot commented Jan 10, 2025

Uh oh!

github-actions Bot commented Jan 10, 2025

Uh oh!

richard-ramos left a comment • 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!

richard-ramos Jan 14, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

richard-ramos commented Sep 12, 2025

Uh oh!

ufarooqstatus commented Sep 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

richard-ramos left a comment •

edited

Loading

richard-ramos Jan 14, 2025 •

edited

Loading