Introduce preferred download peers #5157

sipa · 2014-10-28T16:34:40Z

Keep track of whether we have peers that are preferred candidates for downloading blocks from, but if there are none, also use others.

gavinandresen · 2014-10-28T17:38:07Z

ACK

Tested, fixes #5138 and #5113.

cozz · 2014-10-28T18:55:29Z

src/main.cpp

Is this correct, to prefer inbound?

Oops, fixed.

sipa · 2014-10-28T21:38:20Z

@gavinandresen please test again; it was preferring inbound now, as noticed by @cozz.

laanwj · 2014-10-29T13:35:42Z

src/main.cpp

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

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 · 2014-10-29T13:39:51Z

Going to test this

sipa · 2014-10-29T15:08:59Z

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

gavinandresen · 2014-10-29T18:07:38Z

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 · 2014-10-29T21:03:57Z

@gavinandresen did the previous version of the PR work?

gavinandresen · 2014-10-29T21:08:18Z

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 [email protected] wrote:

@gavinandresen did the previous version of the PR work?

—
Reply to this email directly or view it on GitHub.

rebroad · 2014-10-30T07:37:13Z

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 · 2014-11-01T05:35:45Z

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 · 2014-11-01T08:40:08Z

@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 · 2014-11-03T15:37:09Z

Tested ACK

b4ee0bd Introduce preferred download peers (Pieter Wuille)

…c_invalidateblock fae961d test: Establish only one connection between nodes in rpc_invalidateblock (MarcoFalke) Pull request description: Headers and block sync should eventually converge to the same result, regardless of whether the peers treat each other as "inbound" or "outbound". `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug #5113 and #5138, which has long been fixed in #5157 and #5662. Thus remove the `connect_nodes_bi` workaround from the rpc_invalidateblock test. Conveniently, this also closes #16453. See #16444 (comment) for rationale ACKs for top commit: laanwj: ACK fae961d Tree-SHA512: b3614c66a205823df73f64d19cacfbec269beb5db52ff79004d746e17d7c0dfb43ab9785fdddc97e2a76fe76286c8c605b34df3dda4a2bf5be035f01169ae89a

…s in rpc_invalidateblock fae961d test: Establish only one connection between nodes in rpc_invalidateblock (MarcoFalke) Pull request description: Headers and block sync should eventually converge to the same result, regardless of whether the peers treat each other as "inbound" or "outbound". `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug bitcoin#5113 and bitcoin#5138, which has long been fixed in bitcoin#5157 and bitcoin#5662. Thus remove the `connect_nodes_bi` workaround from the rpc_invalidateblock test. Conveniently, this also closes bitcoin#16453. See bitcoin#16444 (comment) for rationale ACKs for top commit: laanwj: ACK fae961d Tree-SHA512: b3614c66a205823df73f64d19cacfbec269beb5db52ff79004d746e17d7c0dfb43ab9785fdddc97e2a76fe76286c8c605b34df3dda4a2bf5be035f01169ae89a

fadfd84 test: Remove unused connect_nodes_bi (MarcoFalke) fa3b9ee scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke) faaee1e test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke) 1111bb9 test: Reformat python imports to aid scripted diff (MarcoFalke) Pull request description: By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side). This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times. Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests. Historic background: `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug #5113 and #5138, which has long been fixed in #5157 and #5662. ACKs for top commit: laanwj: ACK fadfd84 jonasschnelli: utACK fadfd84 - more of less a cleanup PR. promag: Tested ACK fadfd84, ran extended tests. Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24

3e9081c [Tests] Fix tests duration and sorting in test_runner.py (random-zebra) 57dd5e7 [Tests][Refactor] Move connect_nodes_bi to p2p_time_offset (random-zebra) 9b36468 test: Replace connect_nodes_bi with connect_nodes (random-zebra) c2a701e test: Use connect_nodes when connecting nodes in the test_framework (random-zebra) 5d3f2a1 test: Reformat python imports to aid scripted diff (random-zebra) Pull request description: Mostly from upstream bitcoin#16898 > By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side). > > This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to sync_ may be racy and hard to reproduce. On top of that, the test debug.logs are hard to read because txs and block invs may be relayed on the same connection multiple times. > > Fix this by inlining connect_nodes_bi in the two tests that need it, and then replace it with a single connect_nodes in all other tests. > > Historic background: > > connect_nodes_bi has been introduced as a (temporary?) workaround for bug dashpay#5113 and dashpay#5138, which has long been fixed in dashpay#5157 and dashpay#5662. ACKs for top commit: furszy: Tested ACK 3e9081c . Tree-SHA512: 4af3223c7e5a4b130855d9364b7ec3df5e141151fd1e108c55ef80dd523522780b6130e69cacd467906215c15da9a50496f12298246eb67876fcaa181463cae9

cozz reviewed Oct 28, 2014
View reviewed changes

sipa force-pushed the prefer branch from aa0ad21 to 47bbf05 Compare October 28, 2014 21:37

laanwj added the Bug label Oct 29, 2014

laanwj reviewed Oct 29, 2014
View reviewed changes

Introduce preferred download peers

b4ee0bd

sipa force-pushed the prefer branch from 47bbf05 to b4ee0bd Compare October 29, 2014 15:08

laanwj merged commit b4ee0bd into bitcoin:master Nov 3, 2014

laanwj added a commit that referenced this pull request Nov 3, 2014

Merge pull request #5157

7f7fede

b4ee0bd Introduce preferred download peers (Pieter Wuille)

This was referenced Jan 8, 2015

Syncing of chains with different height depends on "connection direction" #5113

Closed

Headers-first sync never gets going #5138

Closed

rebroad mentioned this pull request Aug 26, 2016

Provide bloom services to whitelisted nodes. #8587

Closed

maflcko mentioned this pull request Aug 27, 2019

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

Merged

maflcko mentioned this pull request Sep 17, 2019

test: Remove connect_nodes_bi #16898

Merged

random-zebra mentioned this pull request Mar 30, 2020

[Tests][Refactor] Remove connect_nodes_bi PIVX-Project/PIVX#1472

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Introduce preferred download peers #5157

Introduce preferred download peers #5157

Uh oh!

Conversation

sipa commented Oct 28, 2014

Uh oh!

gavinandresen commented Oct 28, 2014

Uh oh!

cozz Oct 28, 2014

Choose a reason for hiding this comment

Uh oh!

sipa Oct 28, 2014

Choose a reason for hiding this comment

Uh oh!

sipa commented Oct 28, 2014

Uh oh!

laanwj Oct 29, 2014

Choose a reason for hiding this comment

Uh oh!

sipa Oct 29, 2014

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 29, 2014

Uh oh!

sipa commented Oct 29, 2014

Uh oh!

gavinandresen commented Oct 29, 2014

Uh oh!

sipa commented Oct 29, 2014

Uh oh!

gavinandresen commented Oct 29, 2014

Uh oh!

rebroad commented Oct 30, 2014

Uh oh!

rebroad commented Nov 1, 2014

Uh oh!

sipa commented Nov 1, 2014

Uh oh!

laanwj commented Nov 3, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants