Limited mapAlreadyAskedFor #2423

pull TheBlueMatt wants to merge 3 commits into bitcoin:master from TheBlueMatt:limitedmapalreadyaskedfor changing 4 files +113 −4

TheBlueMatt commented at 6:59 AM on March 29, 2013: member

Based on #2409 (though I dont think it needs it). Just puts a limit on how large mapAlreadyAskedFor can grow.
sipa commented at 4:51 PM on March 30, 2013: member

@TheBlueMatt Can you drop the #2409 included code, or switch to its latest version? They should combine cleanly anyway.
TheBlueMatt commented at 6:02 PM on March 30, 2013: member

No longer depends on #2409
rebroad commented at 2:05 AM on April 1, 2013: contributor

How about separating askedfor blocks and askedfor transactions? That way, one growing large won't affect downloads of the other.
TheBlueMatt commented at 2:09 PM on April 1, 2013: member

@rebroad meh, that just sounds like useless duplication...in that case, anyone will just send block invs so that you re-request blocks more often, sounds like a better way to let an attacker be more targeted.

Revert "Actually use mapAlreadyAskedFor."

This reverts commit 643160f6e7e5e8ca84bc7d2c1a0f37d9cf43a6e1.

Turns out this commit was useless after a more careful reading of
CNode::AskFor

eb59c4ca8e

Add a limitedmap class similar to mruset 5ffc299404

Move mapAlreadyAskedFor to limitedmap

This will result in re-requesting invs if we are under heavy inv
load, however as long as we get no more than 16,000 invs in two
minutes, this should have no effect on runtime behavior.

b5afda67f2

TheBlueMatt commented at 3:56 PM on April 1, 2013: member

OK, switched to MAX_INV_SZ for the map's limit...Im not a huge fan of such a big number, but since its already a protocol rule it makes more sense to use that.
gmaxwell commented at 7:08 PM on April 2, 2013: contributor

This has passed a couple days of in-valgrind production testing, but I think the new limited map needs some unit tests. OTOH, I'd like this merged ASAP for previously discussed reasons.
sipa commented at 7:27 PM on April 2, 2013: member

If my understanding of how maps are represented at runtime in STL is correct, this should mean at most ~6.2MB of memory for 50000 invs (with 8-byte pointers, with 4-byte pointers is 4.4MB).
jgarzik commented at 7:33 PM on April 2, 2013: contributor

@gmaxwell what are the previously discussed reasons? I don't see them in this pull.
sipa commented at 7:37 PM on April 2, 2013: member

ACK, but unit tests for the map would be nice indeed. Has been running for several days for me without problems (together with #2419 #2418 #2422 #2420 #2409)
BitcoinPullTester commented at 2:54 AM on April 3, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b5afda67f2846ddc6554304acc1567796733725b 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.
gavinandresen referenced this in commit aaf47eac3a on Apr 4, 2013
gavinandresen merged this on Apr 4, 2013
gavinandresen closed this on Apr 4, 2013
laudney referenced this in commit 12c2bb4b94 on Mar 19, 2014
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#2409 Network optimalizations