eth: enforce announcement metadatas and drop peers violating the protocol

[karalabe]

In eth/68 we introduced the capability to announce transaction types and sizes along with their hashes. The goal was to allow fetching large transactions and blob transactions via a throttled mechanism to avoid saturating a connection. Whilst we did start announcing the extra data, we never started validating it. This PR adds that component, cross checking delivered transaction bodies with announced metadatas and dropping offending peers. This is a necessary DoS protection, otherwise peers could announce fake small blob txs and cause us to request many concurrently, overloading our bandwidth.

[holiman]

Impressive level of indentation reached here. switch case for if for if if if. I don't have any great suggestions, but if you can make it less that would be supernice

[karalabe]

I won't :D

[holiman]

Well, it's very hard to follow the code here. Like, you check if _, ok := f.waitlist[hash]; ok {, (but never use whatever object it is inside that waitlist, but if it's present, you check the waitslots, otherwise you operate on the announces.

It might make perfect sense to you who wrote the code, but for a reviewer (and maybe even for yourself two years from now), it's pretty hard to ensure that it does what it should, and that all conditions are correct.

[holiman]

For example, couldn't it look something like this? (I mean part of it, there would need to be some more code too)

		case delivery := <-f.cleanup:
			// Independent if the delivery was direct or broadcast, remove all
			// traces of the hash from internal trackers. That said, compare any
			// advertised metadata with the real ones and drop bad peers.
			f.onDeliveries(delivery, func(announced, actual *txMetadata) {
				if announced.kind != actual.kind {
					log.Warn("Announced transaction type mismatch", "peer", peer, "tx", hash, "type", announced.kind, "ann", actual.kind)
					f.dropPeer(peer)
				} else if announced.size != meta.size {
					log.Warn("Announced transaction size mismatch", "peer", peer, "tx", hash, "size", announced.size, "ann", actual.size)
					if math.Abs(float64(announced.size)-float64(actual.size)) > 8 {
						// Normally we should drop a peer considering this is a protocol violation.
						// However, due to the RLP vs consensus format messyness, allow a few bytes
						// wiggle-room where we only warn, but don't drop.
						//
						// TODO(karalabe): Get rid of this relaxation when clients are proven stable.
						f.dropPeer(peer)
					}
				}
			})

[MariusVanDerWijden]

Since this logic is used twice here wouldn't it make sense to combine it into an anonymous function?

verifyMetadata := func(meta *txMetaData) {
    if meta == nil {
       return
    }
    if delivery.metas[i].kind != meta.kind {
    ...

[MariusVanDerWijden]

Ah damn, Martin was quicker :D

[karalabe]

You could probably clean the code up, but at this point I need to get this 5 liner change out of the way and focus on the rest of 4844 and don't want to diverge a month into splitting up the fetcher nice and elegantly. Though TBH, apart from introducing 10 more functions, I don't think it's gonna be much more elegant.

[holiman]

Sure, but I'll take that "You could probably clean the code up" literally, and after we merge this PR, I want to clean it up. For example like this:

https://github.com/karalabe/go-ethereum/compare/enforce-tx-announcement-metas...holiman:go-ethereum:enforce-tx-announcement-metas-mod?expand=1

Even if it means "introducing 10 more functions", I think 10 separate functions is preferrable over a gigantic switch with hundred-liner cases; it increases both readability and testability.

[karalabe]

👍

[karalabe]

The catch with a lot of small methods is that people have the tendency to reuse them. Ok, this sounds weird. But the current logic has a very very strict flow. When there are many methods, people tend to maybe turn one or the other into more generic ones, tweak them, reuse them and then the flow can get completely whacked out because someone forgot some ordering constaint. That's usually the reason I do large methods. It's hard to mess it up in the future.

[holiman]

Just want to double-check the logic here. So, say tx A has size N. Malicious peer A announces it to us, as size M and peer B announces it as size N (correctly).
Now, peer A does not deliver, so we instead ask peer B for it, and peer B delivers a tx with size N.

• Do we hold both meta's, and thus expect A to have M if coming from A and N if coming from B?
• If not, how do we prevent an attack where a malicious announcement is used to punish an honest tx-delivery?

[holiman]

And also, would it make sense to somehow flag (warn) if we notice this behaviour: peers announcing different meta for a transaction.
It could point to shenanigans of this sort, and also of implementation flaws.

[holiman]

Answering my own question here:

• Do we hold both meta's, and thus expect A to have M if coming from A and N if coming from B?

Yes. So attack not possible here

[holiman]

... and furthermore, once the honest peer delivers tx A, the lie from peer A will be discovered and it will be ejected, since we iterate over all announcements in f.announces. Good

[karalabe]

And also, would it make sense to somehow flag (warn) if we notice this behaviour: peers announcing different meta for a transaction.
It could point to shenanigans of this sort, and also of implementation flaws.

In the end, it will turn into a Warn when the bad peer gets dropped. IMO there's no need to find announce clashes at the moment of announcements, when delivery happens we'll know for sure who's right and if anyone's wrong.

I guess what we could do is to add a second warning log where we log that someone was a "bit" off, but not off enough to drop.

[holiman]

Looks somewhat ok ish to me ...

[ngotchac]

Just noting that we are getting spammed (on the latest 1.13.3 release) with

Announced transaction size mismatch

warnings ; always with a delta between announced and actual size of 2 or 3 bytes. As explained in this PR, these warnings are for now ignored (for small deltas), until all implementations behave correctly. Shouldn't we also decrease the log severity?

PS : it seems that these warnings are all for some Erigon peers, if that helps...

[MariusVanDerWijden]

@ngotchac yep we're aware that erigon reports the sizes wrong. Thats why the PR allows for 8 bytes of difference between the announced and actual size. I've opened an issue here: erigontech/erigon#8456