net: Feeler connections to increase online addrs in the tried table. #8282

pull EthanHeilman wants to merge 1 commits into bitcoin:master from EthanHeilman:feelers3 changing 4 files +83 −9
  1. EthanHeilman commented at 5:19 pm on June 28, 2016: contributor

    These changes implement countermeasures 3 (feeler connections) suggested in our paper: “Eclipse Attacks on Bitcoin’s Peer-to-Peer Network”.

    Design:

    We observe that a node’s resistance to eclipse attacks grows as the number of online addresses in the tried table grows. To increase the number of online addresses in the tried table the following logic is implemented in net.cpp’s ThreadOpenConnections:

    1. Every 2 minutes a short lived feeler connection is made to a randomly selected address in new.
    2. If the tested address is online and running bitcoind, this address is moved to the tried table.
    3. The feeler connection to the tested address is closed.

    Only one feeler connection is attempted at any one time and feeler connections are only attempted after all outgoing connections slots of filled.

    Advantages:

    • In our paper we sample several peer lists. We found that a large percentage of addresses in tried tables are stale IP addresses (the lowest was 72 percent stale, the highest was 95 percent stale). This large number of stale IP addresses increases the risk of eclipse attacks. This change remedies this by ensuring that the tried table grows quickly and contains many recently online addresses.
    • Not only do feeler connections provide a direct benefit to the node doing the testing but the tested node, if online, learns about our node and adds it to its tried table (conferring herd immunity even under partial deployment).
    • Countermeasure 3 (test-before-evict) builds on the feeler connections introduced in this change. This is the first step to deploying countermeasure 3.

    Risk mitigation:

    • To limit the network impact of the feeler connections we only make one new connection every 2 minutes. Compared with other networking tasks that bitcoind performs the bandwidth increase is very slight.
    • To avoid issues of synchronization we introduce a random sleep of between 0 and 1000 milliseconds prior to making a feeler connection.
    • To avoid threading issues the feeler connections are made in the same thread as non-feeler connections.

    Test plan:

    1. I’m currently running two EC2 nodes with public IP addresses. One is running this pull request and the other is running standard bitcoind. Both are running with arg -debug=1. At the end of the week I will post results from the logs looking at differences in the size of the tried table and examining the logs for any errors. I will also use the peer.dat files generated from this test to evaluate each nodes resistance to eclipse attacks.
    2. I will also use packet captures to ensure behavior and bandwidth behaves as expected.
    3. I invite anyone would like to help to run standard and feeler connection nodes and email me (Ethan.R.Heilman@gmail.com) the pcaps, logs and peers.dat files. I will post the results of this analysis here.

    This change was suggested as Countermeasure 4 in Eclipse Attacks on Bitcoin’s Peer-to-Peer Network, Ethan Heilman, Alison Kendler, Aviv Zohar, Sharon Goldberg. ePrint Archive Report 2015/263. March 2015.

  2. MarcoFalke added the label P2P on Jun 28, 2016
  3. EthanHeilman commented at 8:26 pm on June 28, 2016: contributor
    This pull request implements the feeler functionality in #6355.
  4. paveljanik commented at 12:28 pm on June 30, 2016: contributor
    tests need some love…
  5. EthanHeilman commented at 4:39 pm on June 30, 2016: contributor

    @paveljanik I am currently working on fixes these days, however I am running into issues with the rpc-tests. When I run rpc-tests locally both with this pull request and against bitcoin/master I get intermittent failures.

    I run against bitcoin/master

    0python3 qa/pull-tester/rpc-tests.py

    tests fail

     0TEST                           | PASSED | DURATION
 1bip68-112-113-p2p.py           | True   | 52 s
 2listtransactions.py            | True   | 177 s
 3wallet.py                      | True   | 287 s
 4mempool_resurrect_test.py      | True   | 20 s
 5receivedby.py                  | True   | 93 s
 6txn_doublespend.py --mineblock | True   | 50 s
 7txn_clone.py                   | True   | 40 s
 8getchaintips.py                | True   | 43 s
 9p2p-fullblocktest.py           | False  | 447 s
10rawtransactions.py             | True   | 83 s
11mempool_spendcoinbase.py       | True   | 10 s
12mempool_reorg.py               | True   | 22 s
13rest.py                        | True   | 71 s
14multi_rpc.py                   | True   | 5 s
15httpbasics.py                  | True   | 11 s
16mempool_limit.py               | True   | 24 s
17proxy_test.py                  | True   | 15 s
18merkle_blocks.py               | True   | 41 s
19signrawtransactions.py         | True   | 5 s
20zapwallettxes.py               | True   | 49 s
21nodehandling.py                | True   | 24 s
22decodescript.py                | True   | 5 s
23reindex.py                     | True   | 31 s
24blockchain.py                  | True   | 6 s
25disablewallet.py               | True   | 5 s
26keypool.py                     | True   | 18 s
27prioritise_transaction.py      | True   | 25 s
28invalidblockrequest.py         | True   | 11 s
29sendheaders.py                 | True   | 60 s
30invalidtxrequest.py            | True   | 12 s
31walletbackup.py                | True   | 647 s
32p2p-versionbits-warning.py     | True   | 24 s
33abandonconflict.py             | True   | 39 s
34fundrawtransaction.py          | False  | 201 s
35importprunedfunds.py           | True   | 37 s
36signmessages.py                | True   | 6 s
37segwit.py                      | True   | 51 s
38zmq_test.py                    | True   | 17 s
39p2p-segwit.py                  | True   | 88 s
40ALL                            | False  | 2852 s (accumulated)

    I rerun it and sometimes different tests fail

     0TEST                           | PASSED | DURATION
 1
 2p2p-fullblocktest.py           | False  | 33 s
 3bip68-112-113-p2p.py           | True   | 52 s
 4listtransactions.py            | True   | 64 s
 5receivedby.py                  | True   | 46 s
 6walletbackup.py                | False  | 107 s
 7mempool_resurrect_test.py      | True   | 21 s
 8txn_clone.py                   | True   | 58 s
 9txn_doublespend.py --mineblock | True   | 67 s
10getchaintips.py                | True   | 71 s
11wallet.py                      | True   | 193 s
12mempool_spendcoinbase.py       | True   | 8 s
13mempool_reorg.py               | True   | 18 s
14mempool_limit.py               | True   | 20 s
15httpbasics.py                  | True   | 12 s
16rest.py                        | True   | 57 s
17multi_rpc.py                   | True   | 6 s
18rawtransactions.py             | True   | 67 s
19proxy_test.py                  | True   | 16 s
20signrawtransactions.py         | True   | 7 s
21merkle_blocks.py               | True   | 32 s
22zapwallettxes.py               | True   | 46 s
23nodehandling.py                | True   | 23 s
24decodescript.py                | True   | 7 s
25blockchain.py                  | True   | 7 s
26disablewallet.py               | True   | 4 s
27reindex.py                     | True   | 25 s
28keypool.py                     | True   | 18 s
29prioritise_transaction.py      | True   | 28 s
30invalidblockrequest.py         | True   | 13 s
31invalidtxrequest.py            | True   | 10 s
32sendheaders.py                 | True   | 62 s
33p2p-versionbits-warning.py     | True   | 26 s
34abandonconflict.py             | True   | 38 s
35importprunedfunds.py           | True   | 27 s
36signmessages.py                | True   | 5 s
37segwit.py                      | True   | 43 s
38zmq_test.py                    | True   | 24 s
39fundrawtransaction.py          | True   | 184 s
40p2p-segwit.py                  | True   | 90 s
41
42ALL                            | False  | 1635 s (accumulated)

    Also sometimes it just crashes.

     0...........................................Traceback (most recent call last):
 1  File "qa/pull-tester/rpc-tests.py", line 340, in <module>
 2    runtests()
 3  File "qa/pull-tester/rpc-tests.py", line 209, in runtests
 4    (name, stdout, stderr, passed, duration) = job_queue.get_next()
 5  File "qa/pull-tester/rpc-tests.py", line 264, in get_next
 6    (stdout, stderr) = proc.communicate(timeout=3)
 7  File "/usr/lib/python3.4/subprocess.py", line 960, in communicate
 8    stdout, stderr = self._communicate(input, endtime, timeout)
 9  File "/usr/lib/python3.4/subprocess.py", line 1618, in _communicate
10    self._check_timeout(endtime, orig_timeout)
11  File "/usr/lib/python3.4/subprocess.py", line 986, in _check_timeout
12    raise TimeoutExpired(self.args, orig_timeout)
13subprocess.TimeoutExpired: Command '['/home/e0/work/bitcoin-standard/qa/rpc-tests/walletbackup.py', '--srcdir=/home/e0/work/bitcoin-standard/src', '--portseed=37']' timed out after 3 seconds
  6. MarcoFalke commented at 4:46 pm on June 30, 2016: member

    If you get a timeout, you may try to run the test directly qa/rpc-tests/test.py and see if it still fails. Also, you may want to check if there are any zombie bitcoin processes which you want to kill. And finally, the test_framework will no longer clean up after a failure, so you need to clean your temp dir manually in case it has limited space available.

    Usually none of this should be a problem on travis, as a fresh vm is used every time, so you may want to look into the failure on travis.

  7. MarcoFalke commented at 4:53 pm on June 30, 2016: member
    You should assert that nodes in the existing tests are not marked a feel connection and thus get disconnected.
  8. sdaftuar commented at 4:55 pm on June 30, 2016: member
    From my first glance at a failing travis test, it seems that perhaps pnode->fFeeler can be set to true on an incoming connection, causing an immediate disconnect? That would result in intermittent test failures for sure.
  9. EthanHeilman commented at 7:45 pm on July 1, 2016: contributor
    @sdaftuar @MarcoFalke I’m working on a simple unittest to make sure that fFeelers is false by default and never happens when fIncoming = true. Should be pushed by EOD.
  10. EthanHeilman commented at 9:08 pm on July 1, 2016: contributor
    I added a simple test and an assert to ensure that that fFeelers are set false by default and don’t get assigned to incoming connection.
  11. EthanHeilman commented at 2:36 pm on August 3, 2016: contributor

    Summary: To test feeler connections I ran two nodes on EC2 for ~one month. One node ran this pull request, the second node ran default Bitcoin. According to this test feeler connections (this pull request) increase the size of the tried table by about an order of magnitude and consequently increase the eclipse resistance of a node by an order of magnitude. I also measured the impact of a future pull request test-before-evict where IPs are not evicted from the tried table if they are online.

    Tried Table

    Comparing the size of the tried table over time

    One node ran this pull request for 32 days (July 1st to August 1st), the second node ran default Bitcoin for 35 Days (June 28 to August 1st).

    On August 1st I disconnected the nodes and graphed the results. I tested which nodes in the tried table were online on August 1st.

    • The Feeler connections node had 1309 IPs in the tried table of which 38.8% or 590 IPs were online.
    • The default Bitcoin node had 195 IPs in the tried table of which 28.2% or 55 IPs were online.

    Security Benefit

    To determine the security benefit of these larger numbers of IPs in the tried table I modeled the attack presented in Eclipse Attacks on Bitcoin’s Peer-to-Peer Network. Full details on how this was carried out are given at bottom of this comment.

    attackergraph40000-10-1000short-line

    Default node: 595 attacker IPs for ~50% attack success. Default node + test-before-evict: 620 attacker IPs for ~50% attack success. Feeler node: 5540 attacker IPs for ~50% attack success. Feeler node + test-before-evict: 8600 attacker IPs for ~50% attack success.

    The node running feeler connections has 10 times as many online IP addresses in its tried table making an attack 10 times harder (i.e. requiring the an attacker require 10 times as many IP addresses in different /16s). Adding test-before-evict increases resistance of the node by an additional 3000 attacker IP addresses.

    Below I graph the attack over even greater attacker resources (i.e. more attacker controled IP addresses). Note that test-before-evict maintains some security far longer even against an attacker with 50,000 IPs. If this node had a larger tried table test-before-evict could greatly boost a nodes resistance to eclipse attacks.

    attacker graph long view

    Modeling an Eclipse Attack

    Using the peers.dat file from the default node and the feeler connections node I reconstructed their tried tables (I ignore the new table in our attacks as it is trivial to fill it with trash IP addresses). The attack test was run as follows:

    1. Attempt to insert X attacker IP addresses each from distinct \16s into a model of a node’s tried table.
    2. Select nodes from each nodes tried table based on the logic in net.cpp (each outgoing connection must be from a different \16, only connect to online nodes).
    3. If the attacker’s IPs are selected for all 8 outgoing connections the attacker wins as the attacker has partitioned the node from the rest of the network otherwise the attacker loses. For realism I only allowed the node to connect to other nodes which were online.

    For each value X of number of attacker IPs I ran the attack 1000 times. For each of these 1000 runs I calculated the attack success probability by performing the selection of 8 outgoing connections 10 times. I used the peers.dat file from the feelers connection node and the default node to build the model of the tried table for the attack. I simulated the impact of enabling test-before-evict would have if turned on just prior to the attack (this should not be different than if the node has been running test-before-evict all along) by preventing the attacker from evicting an IP in the tried table if that node was online.

  12. in src/net.cpp: in 6ded229803 outdated 
    1613@@ -1609,6 +1614,7 @@ void ThreadOpenConnections()
1614 
1615     // Initiate network connections
1616     int64_t nStart = GetTime();
1617+    int64_t nLastFeeler = GetTime();
    sipa commented at 5:53 pm on August 3, 2016:
    No need to call GetTime() again.
  13. in src/net.cpp: in 6ded229803 outdated 
    1726-            OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant);
1727+        if (addrConnect.IsValid()) {
1728+
1729+            if (fFeeler) {
1730+                // Add small amount of random noise before connection to avoid syncronization.
1731+                int randsleep = GetRandInt(FEELER_SLEEP_WINDOW * 1000);
    sipa commented at 5:57 pm on August 3, 2016:

    Would it make sense to use PoissonNextSend here, resulting in maximally unpredictable times?

    EDIT: This comment was meant to appy to FEELER_INTERVAL instead.

    EthanHeilman commented at 8:11 pm on August 4, 2016:
    Oh, I didn’t know about PoissonNextSend, very cool. I moved FEELER_INTERVAL to use PoissonNextSend.
  14. in src/net.cpp: in 6ded229803 outdated 
    1668+        //  * Only make a feeler connection once every few minutes.
1669+        //
1670+        bool fFeeler = false;
1671+        if (nOutbound >= MAX_OUTBOUND_CONNECTIONS){
1672+            if (GetTime() - nLastFeeler > FEELER_INTERVAL) {
1673+                nLastFeeler = GetTime();
    sipa commented at 6:00 pm on August 3, 2016:
    GetTime() is relatively expensive; I think you can introduce an nTime = GetTime() above and then reuse that value 4 times. It probably doesn’t matter, but gettimeofday calls appear so many times in traces already :)
    EthanHeilman commented at 8:23 pm on August 4, 2016:

    I managed to get rid of one call to GetTime(). PoissonNextSend uses GetTimeMicros() so replacing additional GetTime() calls would require nNow_(1000_1000) which looks a little ugly.

    In a local branch I have some non-ugly changes which can reduce the number of GetTime() and GetTimeMicros() calls in ThreadOpenConnections but it touches code which is out of scope for this pull request. Would it be worthwhile to create a separate pull request for this?

  15. in src/net.cpp: in 6ded229803 outdated 
    1686+            CAddrInfo addr;
1687+            if (fFeeler) {
1688+                addr = addrman.Select(true);
1689+            } else {
1690+                addr = addrman.Select();
1691+            }
    paveljanik commented at 8:26 pm on August 3, 2016:

    Can’t this be simply changed to

    0CAddrInfo addr = addrman.Select(fFeeler);
  16. in src/net.cpp: in 6ded229803 outdated 
    1721@@ -1688,8 +1722,17 @@ void ThreadOpenConnections()
1722             break;
1723         }
1724 
1725-        if (addrConnect.IsValid())
1726-            OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant);
1727+        if (addrConnect.IsValid()) {
1728+
1729+            if (fFeeler) {
1730+                // Add small amount of random noise before connection to avoid syncronization.
    paveljanik commented at 8:27 pm on August 3, 2016:
    syncronization -> synchronization
  17. sipa commented at 8:36 pm on August 3, 2016: member
    Thanks a lot for the thorough analysis! Concept ACK with a few nits. I’m surprised by how little code was needed.
  18. paveljanik commented at 8:41 pm on August 3, 2016: contributor
    How will feeler endpoints view us when we disconnect just after their version message?
  19. sipa commented at 8:42 pm on August 3, 2016: member
    @paveljanik Similar to oneshot connections that are used for DNS seeding over tor.
  20. in src/net.cpp: in 6ded229803 outdated 
    1728+
1729+            if (fFeeler) {
1730+                // Add small amount of random noise before connection to avoid syncronization.
1731+                int randsleep = GetRandInt(FEELER_SLEEP_WINDOW * 1000);
1732+                MilliSleep(randsleep);
1733+                LogPrint("net", "Making feeler connection to %s\n",addrConnect.ToString());
    paveljanik commented at 8:45 pm on August 3, 2016:
    space after , please.
  21. paveljanik commented at 11:32 am on August 4, 2016: contributor
    Concept ACK. Nice work and interesting reading!
  22. in src/test/net_tests.cpp: in 6ded229803 outdated 
    149+    CAddress addr = CAddress(CService("252.1.1.1", 7777), NODE_NETWORK);
150+    std::string pszDest = "";
151+    bool fInboundIn = false;
152+
153+    // Test that fFeeler is false by default.
154+    CNode* pnode1 = new CNode(hSocket, addr, pszDest, fInboundIn);
    paveljanik commented at 11:43 am on August 4, 2016:

    New compile warning here:

    0+test/net_tests.cpp:154:31: warning: variable 'hSocket' is uninitialized when used here [-Wuninitialized]
1+    CNode* pnode1 = new CNode(hSocket, addr, pszDest, fInboundIn);
2+                              ^~~~~~~
3+test/net_tests.cpp:148:19: note: initialize the variable 'hSocket' to silence this warning
4+    SOCKET hSocket;
5+                  ^
6+                   = 0
    EthanHeilman commented at 6:34 pm on August 8, 2016:
    This compile warning has been fixed.
  23. in src/net.cpp: in 4b4517ce03 outdated 
    1718@@ -1688,8 +1719,17 @@ void ThreadOpenConnections()
1719             break;
1720         }
1721 
1722-        if (addrConnect.IsValid())
1723-            OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant);
1724+        if (addrConnect.IsValid()) {
1725+
1726+            if (fFeeler) {
1727+                // Add small amount of random noise before connection to avoid synchronization.
    sipa commented at 8:35 pm on August 4, 2016:
    Is this still needed with interval randomization?
    EthanHeilman commented at 9:15 pm on August 4, 2016:

    Yes. If we remove the random sleep synchronization becomes possible.

    Consider a network event which freezes ThreadOpenConnections for 120+ seconds. When that event unfreezes the thread all nodes will connect to an address in the new table all at once.

    This anti-synchronization mechanism becomes more important when test-before-evict is added. Under test-before-evict a connection to a node can trigger the connected-to node to make another connection. My thinking is to get this protection in earlier rather than later since it does provide a small benefit to feeler connections.

  24. in src/net.cpp: in 4b4517ce03 outdated 
    62@@ -60,6 +63,7 @@
63 
64 namespace {
65     const int MAX_OUTBOUND_CONNECTIONS = 8;
66+    const int MAX_FEELER_CONNECTIONS = 1;
    rebroad commented at 4:08 pm on August 6, 2016:
    This constant seems not to be used other than to effectively increase MAX_OUTBOUND_CONNECTIONS.
    rebroad commented at 2:06 am on August 7, 2016:
    I’m perhaps reading the code incorrectly, but it seems that it will only ever make one feeler connection at a time, regardless of the value of this constant. I.e., increasing this value does not cause it to make concurrent feeler connections.
    EthanHeilman commented at 6:33 pm on August 8, 2016:

    I reread the pull request and my understanding is still that MAX_FEELER_CONNECTIONS > 1 allows concurrent feeler connections. I have run tests in response to your comment which appear to show multiple concurrent feeler connections.

    Would you be willing to write out your thought process which led you to the conclusion above in case there is something I am missing?

  25. rebroad commented at 4:01 am on August 7, 2016: contributor
    One potential niggle with feeler connections is that they cause the node_id to go up rather quickly. I think for feeler connections it might be worthwhile not incrementing the node_id since the connection is so short-lived. Perhaps the feeler connections can have a node_id of zero, or something like this?
  26. in src/net.h: in 4b4517ce03 outdated 
    40@@ -41,6 +41,8 @@ namespace boost {
41 static const int PING_INTERVAL = 2 * 60;
42 /** Time after which to disconnect, after waiting for a ping response (or inactivity). */
43 static const int TIMEOUT_INTERVAL = 20 * 60;
44+/** Run the feeler connection loop once every 2 minutes or 120 seconds. **/
45+static const int FEELER_INTERVAL = 120;
    rebroad commented at 4:13 am on August 7, 2016:
    Why is this in net.h rather than in net.cpp? net.cpp would cause less impact on compile times.
    EthanHeilman commented at 2:31 pm on August 8, 2016:
    I put FEELER_INTERVAL in net.h since that is where all the other net.* timing intervals static const variables are and because putting static const variables in *.h appears to be the convention in the Bitcoind networking code.
    rebroad commented at 10:09 am on August 24, 2016:
    @sipa @laanwj Given this current apparent convention causes a waste of CPU compiling code that doesn’t need compiling, can we move some of the net.h code to net.cpp (or other applicable) places in a future pull request?
  27. in src/net.cpp: in 4b4517ce03 outdated 
    1723-            OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant);
1724+        if (addrConnect.IsValid()) {
1725+
1726+            if (fFeeler) {
1727+                // Add small amount of random noise before connection to avoid synchronization.
1728+                int randsleep = GetRandInt(FEELER_SLEEP_WINDOW * 1000);
    rebroad commented at 4:20 am on August 7, 2016:
    Why not make the feeler interval random also, so instead of 120 seconds fixed, it will occur anytime between 0 and 240 seconds? (thereby 120seconds on average).
    EthanHeilman commented at 2:24 pm on August 8, 2016:
    Randomizing the interval was suggested by @sipa and it is an excellent idea! This pull request already does exactly what you describe using PoissonNextSend.
  28. EthanHeilman commented at 1:32 pm on August 15, 2016: contributor

    Bitcoin’s code review process has consistently been the most thoughtful and thorough code review process I’ve encountered. I want to thank all the reviewers for spending their time to read my code and help improve Bitcoin. My pull requests are better for it.

    I realize reviewers might want to take more time to read it over carefully, but in terms of allocating my time for this week is there any particular issue that I should resolve in the next few days?

  29. MarcoFalke commented at 7:55 pm on August 15, 2016: member
    utACK 4b4517ce0340c813553f17d1e7710129542d58bd
  30. paveljanik commented at 8:10 pm on August 15, 2016: contributor

    @EthanHeilman Can you please rebase to the current master with #8128 in? See #8128 (comment) for more information…

    If I do so manually locally:

    0/Applications/Xcode.app/Contents/Developer/usr/bin/make -C .. bitcoin_test
1  CXX      test/test_test_bitcoin-net_tests.o
2test/net_tests.cpp:157:30: error: no matching constructor for initialization of 'CService'
3    CAddress addr = CAddress(CService("252.1.1.1", 7777), NODE_NETWORK);
4                             ^        ~~~~~~~~~~~~~~~~~
  31. in src/net.cpp: in 4b4517ce03 outdated 
    1666+        //  * Choose a random address from new and attempt to connect to it if we can connect 
1667+        //    successfully it is added to tried.
1668+        //  * Start attempting feeler connections only after node finishes making outbound 
1669+        //    connections.
1670+        //  * Only make a feeler connection once every few minutes.
1671+        //
    sipa commented at 9:10 pm on August 15, 2016:
    Perhaps include a link to the Eclipse Attacks paper here? Always good to know that the design behind some code has extensive analysis backing it.
  32. sipa commented at 10:09 pm on August 15, 2016: member
    Testing this on bitcoin.sipa.be (together with #8223 #8453 #8452 #8393 #8515).
  33. MarcoFalke commented at 5:14 pm on August 18, 2016: member
    Can confirm that 4b4517ce0340c813553f17d1e7710129542d58bd improves the problems raised in #8470
  34. MarcoFalke commented at 5:28 pm on August 18, 2016: member
    I think we should move forward with this. @EthanHeilman Can you try to rebase this (there is a silent merge conflict) and fix any issues as they arise?
  35. EthanHeilman commented at 1:25 am on August 19, 2016: contributor

    @MarcoFalke https://github.com/bitcoin/bitcoin/commit/4b4517ce0340c813553f17d1e7710129542d58bd was not designed to fix #8470 but it should have help for three reasons:

    1. By moving reachable addresses from the new table to the tried table it protects those reachable addresses from being evicted by unreachable addresses.
    2. By testing addresses in the new table via feeler connections and marking those addresses in the new table which are offline as failed it should help to slowly remove unreachable nodes from the new table increasing the percentage of reachable nodes in the new table.
    3. By making feeler connections a node helps advertise itself increasing the number of reachable nodes in other nodes tried table.

    These changes will increase the health of the tried and new table and should by extension increase the reachable nodes in address advertisements.

    I have rebased.

  36. EthanHeilman commented at 1:28 am on August 19, 2016: contributor
    I’m going to be away from my computer for a few days. I will fix the newly broken tests when I get back.
  37. Added feeler connections increasing good addrs in the tried table. 
    Tests if addresses are online or offline by briefly connecting to them. These short lived connections are referred to as feeler connections. Feeler connections are designed to increase the number of fresh online addresses in tried by selecting and connecting to addresses in new. One feeler connection is attempted on average once every two minutes.

This change was suggested as Countermeasure 4 in
Eclipse Attacks on Bitcoin’s Peer-to-Peer Network, Ethan Heilman,
Alison Kendler, Aviv Zohar, Sharon Goldberg. ePrint Archive Report
2015/263. March 2015.
    dbb1f640e6
  38. in src/main.cpp: in dbb1f640e6 
    4902@@ -4903,6 +4903,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
4903 
4904     if (strCommand == NetMsgType::VERSION)
4905     {
4906+        // Feeler connections exist only to verify if address is online.
    rebroad commented at 1:09 am on August 24, 2016:
    This would be better after the LogPrint to display version message I think, as otherwise the debug.log shows the disconnect and then after, the version message received, which is potentially confusing.
  39. in src/net.cpp: in dbb1f640e6 
    1020@@ -1017,7 +1021,8 @@ static void AcceptConnection(const ListenSocket& hListenSocket) {
1021     SOCKET hSocket = accept(hListenSocket.socket, (struct sockaddr*)&sockaddr, &len);
1022     CAddress addr;
1023     int nInbound = 0;
1024-    int nMaxInbound = nMaxConnections - MAX_OUTBOUND_CONNECTIONS;
1025+    int nMaxInbound = nMaxConnections - (MAX_OUTBOUND_CONNECTIONS + MAX_FEELER_CONNECTIONS);
1026+    assert(nMaxInbound > 0);
    rebroad commented at 1:11 am on August 24, 2016:
    Why add this assert here?
  40. laanwj commented at 8:47 am on August 25, 2016: member
    code review ACK dbb1f64
  41. laanwj added the label Needs backport on Aug 25, 2016
  42. laanwj added this to the milestone 0.13.1 on Aug 25, 2016
  43. laanwj merged this on Aug 25, 2016
  44. laanwj closed this on Aug 25, 2016
  45. laanwj referenced this in commit 026c6edac9 on Aug 25, 2016
  46. EthanHeilman deleted the branch on Aug 25, 2016
  47. jonasschnelli referenced this in commit b7b40c3b69 on Sep 1, 2016
  48. MarcoFalke removed the label Needs backport on Sep 9, 2016
  49. sickpig referenced this in commit b90cea5c48 on Apr 13, 2017
  50. sickpig referenced this in commit d024bdf39a on Apr 14, 2017
  51. sickpig referenced this in commit 40f8ffcdd7 on Apr 14, 2017
  52. laanwj referenced this in commit a36834f10b on Mar 6, 2018
  53. PastaPastaPasta referenced this in commit 84620651c8 on Jun 10, 2020
  54. PastaPastaPasta referenced this in commit 8e50809b48 on Jun 13, 2020
  55. PastaPastaPasta referenced this in commit 79b2ce9b86 on Jun 13, 2020
  56. PastaPastaPasta referenced this in commit 8b9012d292 on Jun 13, 2020
  57. PastaPastaPasta referenced this in commit f442923aee on Jun 17, 2020
  58. random-zebra referenced this in commit 8bbc0650e6 on Jul 1, 2020
  59. gades referenced this in commit 7ad9414b05 on Jun 25, 2021
  60. MarcoFalke locked this on Sep 8, 2021


EthanHeilman paveljanik MarcoFalke sdaftuar sipa rebroad laanwj

Labels
P2P

Milestone
0.13.1

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 16:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me