eth/protocols/eth: reject messages with duplicated txs

[jsvisa]

implement #32724

Add deduplication to avoid the same txs multiple times in a single message

[rjl493456442]

We can have it in the next release. I think the issue is legit

[fjl]

Would be nice to have a test for this in cmd/devp2p/internal/ethtest

[cskiraly]

I'm wondering what should be the goal here: while we could deduplicate easily, we could also make this a protocol violation. I do not see any reasons for a peer to send duplicates in the same message except for programming error, so I would prefer to do the duplicate check and disconnect with an error code.

[cskiraly]

More specifically, we have this in the spec:
https://github.com/ethereum/devp2p/blob/bc76b9809a30e6dc5c8dcda996273f0f9bcf7108/caps/eth.md#L371-L372

Nodes must not resend the same transaction to a peer in the same session and must not
relay transactions to a peer they received that transaction from.

Based on this we can disconnect nodes that send duplicates in the same message.

[cskiraly]

I've updated this to return error and thus disconnect on duplicate in the same message. Let me know if you agree.

We are still not disconnecting if the duplicate is in separate packets. While the spec says nodes must not do it, let's discuss first whether there could be some side effects.

[rjl493456442]

The spec https://github.com/ethereum/devp2p/blob/master/caps/eth.md#pooledtransactions-0x0a says:

The response must be aligned with the request. What if the request contains the duplicated tx hashes?

Unfortunately it's not specified in the spec whether it violates the protocol or not. So i will assume it's
allowed.

https://github.com/ethereum/devp2p/blob/master/caps/eth.md#getpooledtransactions-0x09

[cskiraly]

But it is us doing the request, and we are not sending requests repeating the same hash multiple times.

[jsvisa]

Although the spec does not specify how to handle the situation of duplicate hashes.
As normal nodes, they should not send such requests for duplicated hashes.
Here, treating them as non-conventional nodes and disconnecting them makes sense.

However, this would not be compatible with the previous situation. Would there be any issues?

[cskiraly]

Issues could only happen if there would be nodes sending duplicates in the same message.

• that's not allowed by the spec
• I don't think we can do that, but better check in code
• I don't think other clients do that, but we can do a quick check running a node with extra logging on mainnet

On our side,

• request: TxFetcher.Notify is deduplicating, so we can't request twice the same
• propagate: we only propagate after promote, and we can't promote the same TX twice

[cskiraly]

I was also running this on mainnet for a while, haven't seen any any issues (as expected).