libp2p / libp2p/jvm-libp2p

hanvleIWant ignores IDONTWANT, so we send messages go-libp2p and rust-libp2p would skip

Open Beginner friendly
#527 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Kotlin
Stars
366
Forks
85
Avg merge
2d 39m
Merged PRs (30d)
6

Description

Problem

handleIWant never consults the per-peer IDONTWANT set. If a peer sends us IDONTWANT for a message id and then we get an IWANT for that same id, we serve the full message anyway. go-libp2p and rust-libp2p both skip it.

We honor IDONTWANT everywhere else. peerDoesNotWantMessage (GossipRouter.kt:805) is applied on both broadcast paths, at GossipRouter.kt:526 for inbound forwarding and GossipRouter.kt:550 for local publish. The IWANT response path is the one place it is missing.

This is not a vulnerability. An attacker gains nothing by telling us not to send a message and then asking for it. The cost falls on honest peers, and since we only emit IDONTWANT for messages at or above iDontWantMinMessageSizeThreshold (16 KiB by default), every message affected is a large one.

Root cause

GossipRouter.kt:380-387:

private fun handleIWant(msg: Rpc.ControlIWant, peer: PeerHandler) {
    val peerScore = score.score(peer.peerId)
    if (peerScore < scoreParams.gossipThreshold) return
    msg.messageIDsList
        .mapNotNull { mCache.getMessageForPeer(peer.peerId, it.toWBytes()) }
        .filter { it.sentCount < params.gossipRetransmission }
        .forEach { submitPublishMessageSilently(peer, it.msg) }
}

Score gate, cache lookup, retransmission cap, send. No IDONTWANT check.

Cross-implementation comparison

go-libp2p-pubsub, gossipsub.go:1023, first check inside the id loop:

// Check if that peer has sent IDONTWANT before, if so don't send them the message
if _, ok := gs.unwanted[p][computeChecksum(mid)]; ok {
    continue
}

rust-libp2p, behaviour.rs handle_iwant, checked after the retransmission cap:

if let Some(peer) = self.connected_peers.get_mut(peer_id)
    && peer.dont_send.contains_key(&id) {
    tracing::debug!(%peer_id, message_id=%id, "Peer already sent IDONTWANT for this message");
    continue;
}

We are the only one of the three that skips this check.

Spec

gossipsub v1.2 scopes the MUST to relaying to the mesh, which we already satisfy:

When later relaying the messageId message to the mesh the peers found in dont_send_message_ids MUST be skipped.

The IWANT path is covered separately, under "Cancelling IWANT", at SHOULD/MAY strength:

If a node requested a message via IWANT and then occasionally receives the message from other peer it MAY try to cancel its IWANT requests with the corresponding IDONTWANT message. It may work in cases when a peer delays/queues IWANT requests and the IWANT request SHOULD be removed from the queue if not processed yet

So this is not a spec violation. It is the spec's stated use case for IDONTWANT cancelling an IWANT, and we cannot honor it at all today because the set is never read on that path.

Impact

Reachability: Any mesh peer, no special capability. Reached on the normal path, by a peer that requests a message via IWANT, receives it from someone faster, and cancels with IDONTWANT.

Cost: One redundant transmission of a >=16 KiB message per (peer, message id), up to gossipRetransmission (3) times, inside the iDontWantTTL window (3s). The receiver discards it as a duplicate.

Where it bites: Exactly the case IDONTWANT exists to fix. Large messages, blob sidecars in particular, where the whole point of the extension is to avoid paying for a transmission the peer has already said it does not need.

Suggested fix

Add the filter to the existing chain in handleIWant:

msg.messageIDsList
    .map { it.toWBytes() }
    .filterNot { peerDoesNotWantMessage(peer, it) }
    .mapNotNull { mCache.getMessageForPeer(peer.peerId, it) }
    .filter { it.sentCount < params.gossipRetransmission }
    .forEach { submitPublishMessageSilently(peer, it.msg) }

Filtering before getMessageForPeer also stops a skipped id from burning a retransmission slot. That matches go, where the IDONTWANT check runs before GetForPeer. rust checks after. go's order is the better one: a message we deliberately did not send should not count against the peer's retransmission budget.

No protocol.supportsIDontWant() guard is needed. peerIDontWant is only populated in handleIDontWant, which already returns early for peers that do not support the extension (GossipRouter.kt:390).

Test to add: send IDONTWANT for an id, then IWANT for the same id, and assert nothing is sent and the retransmission counter is untouched.

Related

  • #522, #519, #526, from the same cross-implementation review pass.
  • Found while comparing IWANT response-side controls across the three implementations.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in GossipRouter.kt at handleIWant and read peerDoesNotWantMessage, then inspect the existing IDONTWANT handling and broadcast paths. Add coverage for sending IDONTWANT followed by IWANT for the same id; done means no message is sent and the retransmission counter is unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
kotlin
Domain
distributed-systems, networking
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
88/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.