feat(mempool/dog): optimize sending of ResetRoute

[najeal]

Closes #4617

PR checklist

• Tests written/updated
• Changelog entry added in .changelog (we use unclog to manage our changelog)
• Updated relevant documentation (docs/ or spec/) and code comments

[najeal]

I suppose @jmalicevic is concerned as she opened the issue and @hvanz as he has been pinged in.

[BastienFaivre]

Hi @najeal, I am implementing the DOG algorithm in Rust and came across the same issue. I had the same idea as you for the solution. However, when implementing it, I realized an interesting point. Suppose the following topology:

A --- C
 \   / \
  ---   E
 /   \ /
B --- D

I'm sorry for my poor drawing skills, but here are the edges: (A, C), (A, D), (B, C), (B, D), (C, E), (D, E).

In this situation, we can imagine node E receiving duplicates for transactions originating from A and B. Now, suppose node E sends two have_tx messages (for both A's and B's transactions) to the same node: either C or D. If we follow the HashSet solution (map[p2p.ID]struct{}) you propose to store the peer to which we've sent a have_tx message, we lose the information that we've sent two have_tx messages to the same peer!.
Consequently, if we send a reset route message to this peer, we lose its entry in the HashSet, and the local node will think that all routes are enabled. However, this is not the case. Therefore, if we imagine a situation where the redundancy is still too small, the local node cannot take appropriate action to increase it as it thinks all routes are enabled.

So, I suggest keeping track of all the have_tx sent, not just the peers. Something like map[p2p.ID]uint, for example, where would be aware of the number of disabled routes on a given peer.

Let me know what you think about this approach!

[najeal]

Hi @BastienFaivre ! Sorry for the delay !
I don't see where we loose information. Supposing E have sent to C an HaveTx message concerning both A & B. When E is picking C from the HashSet to send him a ResetRoute Message, C will reset all the restricting routes pointing to E (both from A and B). There is no reason for E to send again a ResetRoute to C. Hence C will be deleted from the HashSet.

[BastienFaivre]

Thank you for your response, @najeal! I now see better why we reasoned differently.

I think the meaning of an have_tx message is misunderstood. From the documentation, have_tx messages are origin's specific, which means that upon receiving an have_tx message from a node X, a given node will ONLY stop forwarding to X transactions whose origin is the same as the transaction specified in the have_tx message, and not block all traffic to X! There is no such a single have_tx message concerning multiple nodes (as you mention in your reasoning). Moreover, a reset_route message does not reset all the restricting routes to a given node, but only one at random.

Therefore, have_tx messages are independent, forcing us to track them all.

Let me know if I missed one of your points!

[najeal]

@BastienFaivre in CometBFT, the SenderNode will also continue to delivers some messages when specific route has not been restricted. HaveTx concerns one SourceNode whereas a ResetRoute does not concern a specific Node 👍
You are right the Picking of node to deliver the ResetRoute message is random 👍