-
Notifications
You must be signed in to change notification settings - Fork 77
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
Possible Bug on GossipSub implementation that makes sharing IHAVE
control messages with empty topics
#361
Comments
Just a note: go-libp2p-pubsub will drop IHAVEs for messages not in a topic it is a part of: https://github.com/libp2p/go-libp2p-pubsub/blob/master/gossipsub.go#L654. This means unless the go-libp2p node is subscribed to the empty topic, it will not handle an IHAVE message with no topic set. |
@MarcoPolo is right, it doesn't seem to have any direct impact on the remote peer's side. However, this would also mean that |
Hm, that looks weird actually... From my memories in the Ethereum network only one implementation was setting up the Just to double check I've added the following test and it passes for me: @Test
fun `check that resulting IHAVE has no topics`() {
val partsQueue = TestGossipQueue(gossipParamsWithLimits)
partsQueue.addIHave(WBytes(ByteArray(10)))
val res = partsQueue.takeMerged().first()
assertThat(res.control.ihaveList.get(0).hasTopicID()).isFalse()
val serialized = res.toByteArray()
val desrializedRpc = Rpc.RPC.parseFrom(serialized)
assertThat(desrializedRpc.control.ihaveList.get(0).hasTopicID()).isFalse()
} I just thought that it could be the case that java proto library treats empty/null field differently |
Does that mean that all seen message_ids are shared on a "single array"?
It doesn't seem to be the case; all the message_ids shared by the
It totally could be. Is there any particular reason not to differentiate all the message_ids shared on IHAVEs by topic? Our only concern here would be that because the topic is empty, as no one is subscribed to an empty topic, those messages are getting directly dropped - expected behaviour, at least at the go implementation. |
Well, this issue is probably worth to be fixed then. Just curious how implementations use the inbound IHAVE topic field? Aside of filtering unknown topics (which basically shouldn't be received at all). |
I would say so
It doesn't seem to be used for anything else that filtering on:
This is how is it handled at the go implementation: ...
iwant := make(map[string]struct{})
for _, ihave := range ctl.GetIhave() {
topic := ihave.GetTopicID()
_, ok := gs.mesh[topic]
if !ok {
continue
}
if !gs.p.peerFilter(p, topic) {
continue
}
for _, mid := range ihave.GetMessageIDs() {
if gs.p.seenMessage(mid) {
continue
}
iwant[mid] = struct{}{}
}
}
... A second thought on this would be to open the question of whether the |
@Nashatyrev can you elaborate on what you mean? The IHAVE primitive is there in order to have a fall back option of hearing about a message that you haven't received through the mesh. It's probabilistic (i.e., you won't necessarily receive IHAVEs for all the messages that have been propagated through the mesh) and messages advertised through IHAVEs should have a topic, otherwise nodes will end up receiving significantly more traffic than interested in (increasing bandwidth and CPU consumption and as a result being an amplification attack vector). |
@yiannisbot as per gossipsub spec a peer should not issue |
For reference, 3 other implemenations besides go-libp2p filters on the IHAVE topic field: rust-libp2p's implementation: nim-libp2p's implementation: |
We have a PR which will fix that incompatibility #365 . I am just wondering about one thing. The protobuf spec for the IHAVE message specifies the topicID as if (msg.hasTopicID() && !mesh.containsKey(msg.topicID)) {
return
} |
Summary
After streaming libp2p traces from our (ProbeLab team) custom libp2p-trace generator tool
Hermes
, we’ve spotted that there are some peers in the Ethereum network sharingIHAVE
messages, includingmsg_ids
, on empty topics.After inspecting this behaviour closely and double-checking that this isn’t a problem or a bug on our end, we’ve been able to verify that:
message_ids
on thoseIHAVE
messages with empty topics do belong to other complete Ethereum topicsAfter checking any existing correlation between the remote peerIDs sending those messages and any particular client or libp2p implementation, we could count a total of 55 unique peers that sent those empty-topic messages over 3.5 hours, where 49/55 of those peers were successfully identified as
teku/teku
nodes, and the 6 missing peers were unable to be identified.There hasn’t been any further investigation on which could be the root problem that generates those buggy
IHAVE
control messages, but it seems that this could be related to an implementation bug, where the topics aren’t mapped correctly tomsg_ids
or aren’t correctly added to the control RPC.Let us know if there is anything else we can help you with :)
Expected behavior
Incoming control GossipSub messages should:
msg_ids
from other topics on the wrong topicsActual behavior
These are trace examples read by the
Hermes
tool:Relevant log output
No response
Possible Solution
No response
Version
The error seems to be coming from the Ethereum’s java client
Teku
which seems to be using the lastest versionV1.1.0-RELEASE
version:Would you like to work on fixing this bug ?
Maybe
CC: @Nashatyrev @ajsutton
The text was updated successfully, but these errors were encountered: