Possible Bug on GossipSub implementation that makes sharing `IHAVE` control messages with empty topics

cortze commented 4 months ago

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 sharing IHAVE messages, including msg_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:

our node is not subscribed to an empty topic
our node isn’t sending any control messages on an empty topic
the shared message_ids on those IHAVE messages with empty topics do belong to other complete Ethereum topics

After 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 to msg_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:

first, include only topics that we are subscribed to ↔ we weren’t subscribed to an empty topic
Shouldn’t advertise msg_ids from other topics on the wrong topics

Actual behavior

These are trace examples read by the Hermes tool:

# Traced message
{
    'Type': 'RECV_RPC', 
    'PeerID': '16Uiu2HAm5BX98oN6Z8Rb...StKkyrNdNsdKZyw4hXutE', 
    'Timestamp': '2024-04-11T19:53:25.725929846+02:00', 
    'Data': {
        'PeerID': '16Uiu2HAmVfzYfKQvjqY...2oouvW1XVAF7eKttz9',
        'Control': {
            'IHave': [{
                'TopicID': '', 
                'MsgIDs': ['252e9507e347568769c8e1d2ae43bdf2f117ec64', '5d821f26cccbe6b3cc8c6b339282e21524c4e5ce', 'e0bbad506fb5e5ce69a8fadad5fd7518a5bb2bce', '688cbce6758cf605e2c4dd720db6a3a4b61a4af5', 'de4f5140c5853477ebdf4511287e700fb9625a4d', '5426c45d73eaed8ec9d69f61cf9c68fb1f1b59f0', 'd49d54be0bc38b498a175a5472e92de5bda32978', 'bf977641e97982ae3fd3deacc949afe5f64b77ad', '73f6198493b464fa61a7f4c880fc0f5fa04cea78', '37e73805c92d1e8b845ec6fed417601e7ea6fbb2', '7b8d699e52663316145f99406c32fea1a9fe11a5', '081ce1f8956edbd3d6fba83a9596e77b1153af8a', '7de595e3bc9ce484e7c7b974b90a584f5a3c6c46', '0b79dfc7e06194cb326641b4dcab2761295d995a', '6d0a07b0571fc6085c104384621f816c6be3b045']
            }]
        }
    }
}
# Comming from peer
peer:16Uiu2HAmVfzYfKQvjqY...2oouvW1XVAF7eKttz9 -> agent_version:teku/teku/v24.3.1/linux-x86_64/-eclipseadoptium-openjdk64bitservervm-java-21

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 version V1.1.0-RELEASE version:

dependency 'io.libp2p:jvm-libp2p:1.1.0-RELEASE'

Would you like to work on fixing this bug ?

Maybe

CC: @Nashatyrev @ajsutton

MarcoPolo commented 4 months ago

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.

cortze commented 4 months ago

@MarcoPolo is right, it doesn't seem to have any direct impact on the remote peer's side.

However, this would also mean that teku in this case, wouldn't be helping their mesh peers on the Gossiping feature of GossipSub, as these empty topic message_ids would be dropped

Nashatyrev commented 4 months ago

Hm, that looks weird actually... In jvm libp2p we are just ignoring IHAVE topic field. That means we never set it for outbound messages and just ignoring them on inbound messages. (however we should not send IHAVEs to those peers who are not subscribed to a corresponding topic. )

From my memories in the Ethereum network only one implementation was setting up the topic field for IHAVEs. Others were ignoring it as well.

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 @cortze by any chance do you have a message dump? I would check that option

cortze commented 4 months ago

In jvm libp2p we are just ignoring IHAVE topic field. That means we never set it for outbound messages and just ignoring them on inbound messages.

Does that mean that all seen message_ids are shared on a "single array"?

(however we should not send IHAVEs to those peers who are not subscribed to a corresponding topic. )

It doesn't seem to be the case; all the message_ids shared by the jvm-libp2p peers belong to topics we were subscribed to. However, this seems to be an implementation mismatch, as at least these do share message_ids by topics:

go-libp2p (Prysm)
rust-libp2p (Lighthouse)
nim-libp2p (Nimbus)

I just thought that it could be the case that java proto library treats empty/null field differently

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. This makes peers using the JVM-libp2p host not contribute to the Gossip part of GossipSub.

Nashatyrev commented 4 months ago

This makes peers using the JVM-libp2p host not contribute to the Gossip part of GossipSub.

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).

cortze commented 4 months ago

Well, this issue is probably worth to be fixed then.

I would say so

Just curious how implementations use the inbound IHAVE topic field? Aside of filtering unknown topics (which basically shouldn't be received at all).

It doesn't seem to be used for anything else that filtering on:

whether we are subscribed to the topic
whether the peer is valid for that topic

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 peerscore of a peer should be decreased if it sends IHAVE messages on topics we are not subscribed to. But this would be a separate topic to discuss at the GossipSub spec level.

yiannisbot commented 4 months ago

Just curious how implementations use the inbound IHAVE topic field?

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

Nashatyrev commented 4 months ago

@yiannisbot as per gossipsub spec a peer should not issue IHAVEs to a peer for topics this peer is not subscribed. Thus if you see a messageId which is not in your seen cache then it should mean you are missing a message from a topic you are subscribed to.

MarcoPolo commented 4 months ago

Just curious how implementations use the inbound IHAVE topic field?

For reference, 3 other implemenations besides go-libp2p filters on the IHAVE topic field:

rust-libp2p's implementation:

https://github.com/libp2p/rust-libp2p/blob/master/protocols/gossipsub/src/behaviour.rs#L1210-L1219

nim-libp2p's implementation:

https://github.com/vacp2p/nim-libp2p/blob/c4da9be32cc01efa2de066c396fe9ef1c7769aa1/libp2p/protocols/pubsub/gossipsub/behavior.nim#L252

cpp-libp2p: https://github.com/libp2p/cpp-libp2p/blob/f27f9fb9a2da69ecfb2d469c0775e2c2a9114eb1/src/protocol/gossip/impl/gossip_core.cpp#L228

StefanBratanov commented 3 months ago

We have a PR which will fix that incompatibility https://github.com/libp2p/jvm-libp2p/pull/365 . I am just wondering about one thing. The protobuf spec for the IHAVE message specifies the topicID as optional. Is that intentional? Essentially as of now, seems like all client implementation filter out IHAVEs which don't have this optional field set. We implement this a bit differently in the PR and allow IHAVEs without a set topic in order to be compatible with other Teku nodes who wouldn't use the new version of jvm-libp2p which we will release.

if (msg.hasTopicID() && !mesh.containsKey(msg.topicID)) {
 return
}

libp2p / jvm-libp2p