addrman: Do not propagate obviously poor addresses onto the network #4632

pull jgarzik wants to merge 1 commits into bitcoin:master from jgarzik:2014_addr_expire changing 1 files +10 −4

jgarzik commented at 7:20 PM on August 4, 2014: contributor

This is a first pass at filtering bad addresses.

ETA: Although this might not be a perfect solution, we need to stop the practice of returning long dead garbage addresses to other peers. This addresses one of the longstanding Things That Bug Me About Bitcoin.
jgarzik commented at 11:45 PM on August 4, 2014: contributor

Updated to fill up to nNodes. Should now be "feature complete."
Diapolo commented at 8:50 AM on August 5, 2014: none

Looks like a very nice idea, will test this later.
Diapolo commented at 12:03 PM on August 5, 2014: none

Compiles fine and I have a testnet and mainnet node running this patch now. What about a debug message, which addresses are dropped?
laanwj added the label P2P on Aug 6, 2014
jgarzik added the label Priority Medium on Aug 8, 2014
mikehearn commented at 10:08 AM on August 11, 2014: contributor

Thanks for working on this! Sounds like a good step towards getaddr results matching the quality of DNS seed results (so becoming more usable for spv clients).

It's hard to test this ... it could return garbage and a test node would seem to operate properly. It'd be nice if we had a tool to diff the results of getaddr. Maybe @TheBlueMatt feels up for it, as he did some work on addr[man] in bitcoinj.
addrman: Do not propagate obviously poor addresses onto the network 3a56de7fc3
in src/addrman.cpp:None in 50dedd7625 outdated
```
 497 |      if (nNodes > ADDRMAN_GETADDR_MAX)
 498 |          nNodes = ADDRMAN_GETADDR_MAX;
 499 |  
 500 |      // perform a random shuffle over the first nNodes elements of vRandom (selecting from all)
 501 | -    for (int n = 0; n<nNodes; n++)
 502 | +    for (int n = 0; n < (int)vRandom.size(); n++)
```
laanwj commented at 12:23 PM on August 15, 2014:

You've changed the type of nNodes to unsigned already. Maybe use an unsigned int or size_t counter here as well instead of an int - no need for the (int) cast in that case, which could theoretically be a truncation.

jgarzik commented at 8:44 PM on August 18, 2014:

Updated thusly.

in src/addrman.cpp:None in de2a68d84e outdated

 491 | @@ -492,17 +492,23 @@ int CAddrMan::Check_()
 492 |  
 493 |  void CAddrMan::GetAddr_(std::vector<CAddress> &vAddr)
 494 |  {
 495 | -    int nNodes = ADDRMAN_GETADDR_MAX_PCT*vRandom.size()/100;
 496 | +    unsigned int nNodes = ADDRMAN_GETADDR_MAX_PCT * vRandom.size() / 100;
 497 |      if (nNodes > ADDRMAN_GETADDR_MAX)
 498 |          nNodes = ADDRMAN_GETADDR_MAX;
 499 |  
 500 |      // perform a random shuffle over the first nNodes elements of vRandom (selecting from all)

sipa commented at 8:47 PM on August 18, 2014:

Update the comment too, please.

BitcoinPullTester commented at 10:43 PM on August 18, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4632_3a56de7fc318a45a7096cd318e2f2d7f8124ce24/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
laanwj commented at 11:18 AM on August 21, 2014: member

I've been testing this on my node for some time, and can confirm that it remains stable.

I also ran with added logging for a while, logging which IPs are dropped as terrible, and how many peers are sent. For every getaddr about 20-30 terrible peers are avoided to be sent. I've examined a sample of these, and they were indeed all unconnectable. In total it does send 2500 addresses per query (ADDRMAN_GETADDR_MAX), as expected.

I've also sampled part of the addresses that were returned - a part of these were connectable. So all in all, this improves the quality of returned addresses

ACK.
sipa commented at 10:43 AM on August 23, 2014: member

Untested ACK
jgarzik merged this on Aug 23, 2014
jgarzik closed this on Aug 23, 2014
jgarzik referenced this in commit 57fe1eaadc on Aug 23, 2014
jgarzik deleted the branch on Aug 24, 2014
rebroad commented at 2:30 AM on September 6, 2014: contributor

Already noticing the improvements from this!
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels