p2p: Do not relay banned IP addresses

[sipa]

No description provided.

[gmaxwell]

Concept ACK. utACK, will test.

[naumenkogs]

utACK

[jonasschnelli]

utACK 054d01d

[practicalswift]

utACK 054d01d

[promag]

How can this be tested?

[gmaxwell]

ACK (now tested it, appears to work)

[gmaxwell]

@promag Instrument your node to log the addr message it sends, ban stuff, check that it's not relaying the banned stuff... which is what I did :) (or otherwise, run the patch, and observe that nothing catches fire)

[promag]

@gmaxwell I mean it could have a functional test or something.

[gmaxwell]

I sure hope that when people say they've tested a PR it doesn't mean they just ran its unit test...

[promag]

I hope too, but a test would be a nice addition no?

[practicalswift]

Nit: Do we care about the object slicing going on here (slicing from CAddress to CNetAddr)?

(Note: Slicing was present in this code also before this patch.)

[pstratem]

utACK 054d01d

[jonasschnelli]

I sure hope that when people say they've tested a PR it doesn't mean they just ran its unit test...

Heh. Indeed.
Though, the functional tests are usually a good starting point in how to understand how manual testing can be done...

[laanwj]

I sure hope that when people say they've tested a PR it doesn't mean they just ran its unit test...

I don't hope so either. Would make sense to extend the definition of ACKs in CONTRIBUTING.md to describe that a tested ACK involves change-specific manual testing, and in case it's not obvious how that's done, it should be described in the post.

(not trying to say that running the unit+functional tests locally isn't useful! travis cannot possible cover all possible combinations of hardware and software)

utACK 054d01d

[promag]

My comment was about having this new behavior checked, not about how reviews should be done.

[luke-jr]

utACK

[cculianu]

You guys realize -- this is a DoS vector. This commit introduces effectively a quadratic loop for GETADDR. IsBanned() is a relatively expensive call which linearly scans the entire ban table after taking a lock. This loop does IsBanned for each item in vAddr up to 2500 items max.

What's worse, the ban table can be manipulated and is unbounded in size -- anybody with enough IP addresses can grow it on any target node they can connect to -- to tens of thousands of entries. This is especially true if using IPv6 where thousands of IP addresses are easily obtained by people with even modest resources. See RFC3177 - Recommendations on IPv6 Address Allocations to Sites

I was able to manipulate the ban table on my testnet node. I added some debug/printing to my local build. This is to service just 1 GETADDR message:

2020-06-08T21:09:40Z GETADDR vAddr: 2500 banSz: 47506 ~iters: 118765000(118764972) took: 2010.761 ms