Introduce preferred download peers

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: 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: 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):

 0#!/usr/bin/env python
 1
 2from test_framework import BitcoinTestFramework
 3from bitcoinrpc.authproxy import AuthServiceProxy, JSONRPCException
 4from util import *
 5import time
 6
 7class SyncDir(BitcoinTestFramework):
 8    def setup_network(self):
 9        self.nodes = []
10        self.nodes.append(start_node(0, self.options.tmpdir, ["-debug=net"]))
11        self.nodes.append(start_node(1, self.options.tmpdir, ["-debug=net"]))
12        connect_nodes(self.nodes[0], 1)
13        print("Nodes connected")
14        self.is_network_split = False
15        self.sync_all()
16        random_transaction(self.nodes, Decimal("0.001"), Decimal("0.0001"), 0, 0)
17        print ("One tx sent")
18        self.nodes[0].setgenerate(True,1)
19        print ("Block mined")
20        self.sync_all()
21        print ("Chains synced")
22        stop_node(self.nodes[0],0)
23        print ("Node stopped")
24        self.nodes[0] = start_node(3, self.options.tmpdir,["-debug=net"])
25        print ("New node created")
26        connect_nodes(self.nodes[1], 0)  #connect the new node
27        #connect_nodes(self.nodes[1], 3)  #connect the new node
28        print ("New node connected....")
29        #self.sync_all()
30        sync_blocks(self.nodes)
31        print ("New node synced")
32
33    def run_test(self):
34        print ("Running test")
35
36if __name__ == '__main__':
37    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

Introduce preferred download peers #5157