Getblocks efficiency #4581

pull rebroad wants to merge 2 commits into bitcoin:master from rebroad:GetblocksEfficiency changing 2 files +32 −12

rebroad commented at 3:32 AM on July 24, 2014: contributor

A lot of time and CPU is wasted dealing with superfluous getblocks requests from peers (this can be seen if just the first commit is merged). The 2nd commit makes the node ignore getblocks requests that are duplicating the previous requests (rather than loop through up to 500 blocks looking for the "end block" mentioned in the request).

This will cause some getblocks requests due to orphan blocks to be ignored, but this will be ok as the invs will already have been sent. All IBD will be unaffected.
Improve getblocks (reception) logging 886c2f1ed4
Only respond to getblocks requests that are asking for substantially newer block invs. 465a13c0ba
BitcoinPullTester commented at 3:56 AM on July 24, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4581_465a13c0bac5470cf4199fcdfe2c2b3eaec476c6/ 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 5:41 AM on July 24, 2014: member

Please first show profiling results that this is actually a problem, before we even look into fixing the 'issue'. I'm not convinced. From what I've seen in my own experiments the network code doesn't even figure into overall CPU usage.
rebroad commented at 1:55 AM on July 25, 2014: contributor

The majority of getblocks my node receives from clients is for block invs it has already sent to the requesting node. I'll paste logs soon.
rebroad commented at 2:39 AM on July 25, 2014: contributor

But anyway, surely this is simply a common sense change in that it's never been necessary to keep sending block invs already sent to the same node. Whether there are currently nodes causing this behavior or not should be irrelevant.
laanwj added the label Improvement on Aug 1, 2014
laanwj commented at 7:09 AM on August 18, 2014: member

As there is no consensus that this actually an improvement. I don't think this is worth the risk of breaking things. Closing this pull.
laanwj closed this on Aug 18, 2014
rebroad commented at 5:10 PM on August 29, 2014: contributor

@laanwj I think this is a misleading use of the word "consensus" since there's been no actual discussion regarding this pull requerst.

Please could you re-open it to at least give people a chance to see this, review, test, discuss, etc?
rebroad commented at 5:13 PM on August 29, 2014: contributor

@laanwj you're probably right regarding your CPU comment, wasted bandwidth is more of an issue I suspect.
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels