Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: idontwant on publish #1230

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: idontwant on publish #1230

wants to merge 1 commit into from

Conversation

richard-ramos
Copy link
Member

@richard-ramos richard-ramos commented Dec 17, 2024

Closes #1224

The following changes were done:

  1. Once a message is broadcasted, if it exceeds the threeshold to consider it a large message it will immediatly send an IDONTWANT message
    if isLargeMessage(msg, msgId):
    g.sendIDontWant(msg, msgId, peers)
  2. Extracts the logic to broadcast an IDONTWANT message and to verify the size of a message into separate functions
  3. Adds a template to filter values of a hashset without modifying the original

@richard-ramos richard-ramos force-pushed the idontwant-publish branch 2 times, most recently from c8d250c to 43104a0 Compare December 18, 2024 13:32
@richard-ramos richard-ramos marked this pull request as ready for review December 18, 2024 14:29
@@ -784,6 +797,9 @@ method publish*(g: GossipSub, topic: string, data: seq[byte]): Future[int] {.asy

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

if isLargeMessage(msg, msgId):
g.sendIDontWant(msg, msgId, peers)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... why should we send IDONTWANT after broadcasting the message? If the other peers receive the message, they already know that I don't want it.

There exists a case where sending it before broadcasting makes sense, though I'm not sure how well it has been studied, ie it needs research: when streaming a large message to a peer, this takes time - in the meantime, the peer might start streaming the same message to us - an IDONTWANT before sending the message tells the other peer not to do that.

@arnetheduck
Copy link
Contributor

I think there's some confusion about #1224 here - what that issue addresses is the case where a message exists "out of band" and is published on gossipsub - the aim is to tell my neighbours that I have received the message out of band already, and they should not waste bandwidth on sending me an extra copy.

In this particular case, the message is a "blob" from a blob transaction - most of the time, blobs are in the mempool before they are added in a block so if I have the blob in the mempool, there's no point receiving it over gossipsub also.

@richard-ramos
Copy link
Member Author

richard-ramos commented Dec 20, 2024

the aim is to tell my neighbours that I have received the message out of band already

Would you say that in this case I should instead provide a way for someone using gossipsub to indicate that they don't want to receive some message? (i.e. something like pubsub.IDontWant(someMessageId))?

If so, do you think it makes sense to also keep the changes i'm proposing (and changing the order so the IDONTWANT is sent before the broadcast? or should that be researched first?

@richard-ramos
Copy link
Member Author

Other implementations do seem to send it before it's published unless i understood wrong:

@arnetheduck
Copy link
Contributor

Would you say that in this case I should instead provide a way for someone using gossipsub to indicate that they don't want to receive some message? (i.e. something like pubsub.IDontWant(someMessageId))?

I think a function like that is broadly useful, yes - though the docs should make clear that it's a "hint".

If so, do you think it makes sense to also keep the changes i'm proposing (and changing the order so the IDONTWANT is sent before the broadcast? or should that be researched first?

they certainly have to be reordered - the policy for when to send a pre-emptive idontwant is however not clear to me - the only time it is useful is during the aforementioned case where the other peer makes the decision to start sending the message while we are also sending it .. it happens, but ... only when streaming time is large enough - all other times, it's a waste of time and bandwidth, and I'm not sure the threshold is well understood. Perhaps this has been answered already somewhere but doing a bit of digging might be the right course of action here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Implement IDONTWANT API (or verify that it works)
3 participants