Introduce preferred download peers #5157

pull sipa wants to merge 1 commits into bitcoin:master from sipa:prefer changing 1 files +19 −1

sipa commented at 4:34 PM on October 28, 2014: member

Keep track of whether we have peers that are preferred candidates for downloading blocks from, but if there are none, also use others.
gavinandresen commented at 5:38 PM on October 28, 2014: contributor

ACK

Tested, fixes #5138 and #5113.

in src/main.cpp:None in aa0ad216e3 outdated

 259 | @@ -255,6 +260,11 @@ int GetHeight()
 260 |      return chainActive.Height();
 261 |  }
 262 |  
 263 | +bool IsPreferredDownload(CNode* node)
 264 | +{
 265 | +    return (node->fInbound || node->fWhitelisted) && !node->fOneShot && !node->fClient;

cozz commented at 6:55 PM on October 28, 2014:

Is this correct, to prefer inbound?

sipa commented at 9:37 PM on October 28, 2014:

Oops, fixed.

sipa force-pushed on Oct 28, 2014
sipa commented at 9:38 PM on October 28, 2014: member

@gavinandresen please test again; it was preferring inbound now, as noticed by @cozz.
laanwj added the label Bug on Oct 29, 2014
in src/main.cpp:None in 47bbf05f58 outdated
```
 259 | @@ -255,6 +260,11 @@ int GetHeight()
 260 |      return chainActive.Height();
 261 |  }
 262 |  
 263 | +bool IsPreferredDownload(CNode* node)
 264 | +{
 265 | +    return (!node->fInbound || node->fWhitelisted) && !node->fOneShot && !node->fClient;
```
laanwj commented at 1:35 PM on October 29, 2014:

IsPreferredDownload doesn't look at node->fPreferredDownload, is this intentional? (if so, the naming may be a bit confusing)

sipa commented at 2:33 PM on October 29, 2014:

Yes. IsPreferredDownload() checks whether a node should be preferred. fPreferredDownload remembers whether we've already counted it as a preferred downloader. Better naming suggestions are welcome :)
laanwj commented at 1:39 PM on October 29, 2014: member

Going to test this
Introduce preferred download peers b4ee0bddad
sipa force-pushed on Oct 29, 2014
sipa commented at 3:08 PM on October 29, 2014: member

Reworked the functions/names a bit; maybe it's less confusing now.

gavinandresen commented at 6:07 PM on October 29, 2014: contributor

Still issues with sync never starting, here's a test case (put in qa/rpc-tests, run with --tracerpc to see behavior):

#!/usr/bin/env python

from test_framework import BitcoinTestFramework
from bitcoinrpc.authproxy import AuthServiceProxy, JSONRPCException
from util import *
import time

class SyncDir(BitcoinTestFramework):
    def setup_network(self):
        self.nodes = []
        self.nodes.append(start_node(0, self.options.tmpdir, ["-debug=net"]))
        self.nodes.append(start_node(1, self.options.tmpdir, ["-debug=net"]))
        connect_nodes(self.nodes[0], 1)
        print("Nodes connected")
        self.is_network_split = False
        self.sync_all()
        random_transaction(self.nodes, Decimal("0.001"), Decimal("0.0001"), 0, 0)
        print ("One tx sent")
        self.nodes[0].setgenerate(True,1)
        print ("Block mined")
        self.sync_all()
        print ("Chains synced")
        stop_node(self.nodes[0],0)
        print ("Node stopped")
        self.nodes[0] = start_node(3, self.options.tmpdir,["-debug=net"])
        print ("New node created")
        connect_nodes(self.nodes[1], 0)  #connect the new node
        #connect_nodes(self.nodes[1], 3)  #connect the new node
        print ("New node connected....")
        #self.sync_all()
        sync_blocks(self.nodes)
        print ("New node synced")

    def run_test(self):
        print ("Running test")

if __name__ == '__main__':
    SyncDir().main()

Connecting node 0 to node 1 (instead of 1 to 0) and all is well.

sipa commented at 9:03 PM on October 29, 2014: member

@gavinandresen did the previous version of the PR work?
gavinandresen commented at 9:08 PM on October 29, 2014: contributor

I don't know if previous version worked-- I reversed connection direction to better test for this version, didn't test previous that way.

On Oct 29, 2014, at 5:04 PM, Pieter Wuille notifications@github.com wrote:

@gavinandresen did the previous version of the PR work?

— Reply to this email directly or view it on GitHub.
rebroad commented at 7:37 AM on October 30, 2014: contributor

Can those looking at this also look at #5099 please? I've been developing and testing functionality to do with preferred downloads and optimising the IBD since June, and it's almost near perfect now (although I'm not so sure about my C++ coding style!).
rebroad commented at 5:35 AM on November 1, 2014: contributor

I'd like to code a pull request to make download of headers faster, but given that #5099 hasn't received any attention, would I be wasting my time and effort?
sipa commented at 8:40 AM on November 1, 2014: member

@rebroad This is not about making it faster, but fixing the bug of it not starting downloads at all in some situations. The problem is the logic that tries to avoid downloading from incoming peers (because of the current expectation that you don't get much bandwidth usage from making just outgoing connections). It seems overeager and sometimes causes no download at all to start.

I'll get to looking at your pull request, but it's a lot of code, and getting the bugs out has higher priority now.
laanwj commented at 3:37 PM on November 3, 2014: member

Tested ACK
laanwj merged this on Nov 3, 2014
laanwj closed this on Nov 3, 2014
laanwj referenced this in commit 7f7fede0eb on Nov 3, 2014
laanwj referenced this in commit cd737214ce on Sep 16, 2019
sidhujag referenced this in commit f9a3ff0065 on Sep 16, 2019
MarcoFalke referenced this in commit 59c138d2f1 on Sep 18, 2019
random-zebra referenced this in commit 46585f7b62 on Mar 31, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#16737 test: Establish only one connection between nodes in rpc_invalidateblock #16898 test: Remove connect_nodes_bi