-
Notifications
You must be signed in to change notification settings - Fork 977
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
EIP-7594: Passive sampling #3717
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to defeat the purpose of only subscribing to gossip from custodied subnets. If you subscribe to an extra
SAMPLES_PER_SLOT
column subnets before each slot, it is equivalent to increasing the size of the total subnets custodied. I don't think subscribing/unsubscribing quickly makes much of a difference hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, how about defining "passive sampling" as receiving samples from the custodied subnets (instead of extra subnets) and decide that the data is available if it receives such samples. This hasn't been specified anywhere in the spec yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like the wrong term, I don't think being part of a gossip subnet can be thought as sampling . The level of amplification is 8x on a subnet vs a simple req/resp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't really have to be 8x. You can just connects to a single node as a mesh peer, so the bandwidth used will be just the same as req/resp.
I disagree on this. Sampling means take some portion of something. Subscribing to some columns/subnets means taking some columns of all the columns, so I think the term still makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to change my mind on this. I think it's okay to be subscribed to extra subnets.
It's not really equivalent because you don't keep the past samples for the extra subnets. You just get them and throw them away.
In terms of the bandwidth usage, as I mentioned in the previous comment, you can reduce the mesh degree to 1 so that you use as much bandwidth as req/resp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires us to be able to have dynamic mesh sizes for separate topics, currently in
go-libp2p-pubsub
and possibly in other language implementations all topics have the same mesh size. Also having a mesh size of 1 can be problematic for the general network as you would have increased latency on the propagation of a message. A remote peer will not know if a connected peer has a mesh size of 8 or 1. On some paths, data columns might take a lot longer to be propagatedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for this usecase you would want a new protocol message ? Where instead of random gossip, a peer simply returns the most recent seen message ids for a particular topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's okay because, even though the message is delayed or not received at all, the passive sampling acts as only a complement to active sampling.
Notice that, without passive sampling, the sampling node has to wait until the sampling time to request the samples. With passive sampling (even with a mesh size of 1), there will be a very likely chance that it will get the samples before the sampling time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I have an upgrade to GossipSub in mind which will probably help on this issue. I will create a PR on that in a few days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it is libp2p/specs#617