net: Favor peers from addrman over fetching seednodes

sr-gi commented at 9:42 pm on March 8, 2024: member

This is a follow-up of #28016 motivated by #28016#pullrequestreview-1913140932 and #28016 (comment).

The current behavior of seednode fetching is pretty eager: we do it as the first step under ThreadOpenNetworkConnections even if some peers may be queryable from our addrman. This poses two potential issues:

First, if permanently set (e.g. running with seednode in a config file) we’d be signaling such seed every time we restart our node
Second, we will be giving the seed node way too much influence over our addrman, populating the latter with data from the former even when unnecessary

This changes the behavior to only add seednodes to m_addr_fetch if our addrman is empty, or little by little after we’ve spent some time trying addresses from our addrman. Also, seednodes are added to m_addr_fetch in random order, to avoid signaling the same node in case more than one seed is added and we happen to try them over multiple restarts

DrahtBot commented at 9:42 pm on March 8, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	cbergqvist, itornaza, tdb3, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29641 (scripted-diff: Use LogInfo over LogPrintf by maflcko)
#26114 (net: Make AddrFetch connections to fixed seeds by mzumsande)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

sr-gi renamed this:
~~2024 04 07 seednode addrman~~
net: give seednode priority over dnsseed if both are provided
on Mar 8, 2024

DrahtBot added the label P2P on Mar 8, 2024

sr-gi renamed this:
~~net: give seednode priority over dnsseed if both are provided~~
net: Favor peers from addrman over fetching seednodes
on Mar 8, 2024

DrahtBot renamed this:
~~net: Favor peers from addrman over fetching seednodes~~
net: Favor peers from addrman over fetching seednodes
on Mar 8, 2024

sr-gi force-pushed on Mar 8, 2024

sr-gi commented at 9:45 pm on March 8, 2024: member

This is currently dependent on #28016, given it redefines a local constexpr into a global one, but it could be reworked to add that functionality here if the previous PR doesn’t get merged

sr-gi force-pushed on Mar 8, 2024

DrahtBot added the label CI failed on Mar 8, 2024

DrahtBot commented at 9:49 pm on March 8, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/22454758070

sr-gi force-pushed on Mar 8, 2024

DrahtBot removed the label CI failed on Mar 8, 2024

in test/functional/p2p_seednode.py:19 in e45522cac9 outdated

14+from test_framework.p2p import (
15+    P2PInterface,
16+    P2P_SERVICES,
17+)
18+
19+def setup_addr_msg(num, time):

brunoerg commented at 10:22 pm on March 9, 2024:

In e45522cac97454eefe9f30af9f5e9df6b5bc3d65: setup_addr_msg seems weird. It’s a for loop based on the first parameter but this function is called just once and with num=1.

sr-gi commented at 2:03 pm on March 11, 2024:

That is mostly borrowed from p2p_addr_relay.py, but you’re right, it can be called with a bigger num so the node gets more addresses to pick up from. I’ll address it.

brunoerg commented at 8:18 pm on March 25, 2024:

In fact, I realized that it has been used to fill addrman, right? So, wouldn’t it be better to use addpeeraddress?

sr-gi commented at 1:11 pm on March 28, 2024:

Yep, that works and it’s way simpler :D

sr-gi commented at 1:12 pm on March 28, 2024:

Addressed in a178d28

sr-gi force-pushed on Mar 11, 2024

sr-gi commented at 2:43 pm on March 11, 2024: member

Addressed @brunoerg comment, plus slightly reworked the approach to make sure seednodes are not queried if we reach an acceptable number of peers.

Before this, we would have queried them after the timer went off anyway

sr-gi force-pushed on Mar 11, 2024

sr-gi force-pushed on Mar 15, 2024

sr-gi commented at 9:14 pm on March 15, 2024: member

Rebased to fix CI due to https://github.com/bitcoin/bitcoin/pull/29610

in src/net.h:1427 in f809817391 outdated

1424@@ -1423,6 +1425,8 @@ class CConnman
1425     mutable RecursiveMutex m_nodes_mutex;
1426     std::atomic<NodeId> nLastNodeId{0};
1427     unsigned int nPrevNodeCount{0};
1428+    // Collection of seed nodes, populated by connOptions.vSeedNodes
1429+    std::vector<std::string> m_seed_nodes;

cbergqvist commented at 11:35 pm on March 19, 2024:

This is fairly straight-forward to avoid having as a member with state that lives on in CConnman beyond startup, see my attempt in ~~2204c4666f2b86895c03523c2285450d41e46aa3~~ f9bfc588f2e81a7febe233f591b75e41a52db8b4.

If it is kept as a member, I would prefer a shorter comment.

0    // Populated from connOptions.vSeedNodes
1    std::vector<std::string> m_seed_nodes;

sr-gi commented at 12:55 pm on March 28, 2024:

I don’t really have a preference here. Maybe others that have been working on the P2P codebase longer than me can give their opinion on this

sr-gi commented at 12:56 pm on March 28, 2024:

@vasild @mzumsande @brunoerg @naumenkogs

cbergqvist commented at 9:25 pm on May 1, 2024:

Switched to using Span for a slightly smaller change suggestion in f9bfc588f2e81a7febe233f591b75e41a52db8b4.

in src/net.cpp:2497 in f809817391 outdated

2473@@ -2435,16 +2474,33 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
2474     bool add_fixed_seeds = gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS);
2475     const bool use_seednodes{gArgs.IsArgSet("-seednode")};
2476 
2477+    auto seed_node_timer = NodeClock::now();

cbergqvist commented at 12:51 pm on March 20, 2024:

It’s a bit messy that we are mixing GetTime and NodeClock::now() overlapping with each other. I made an attempt at making them compatible in 934449f439f5a869177f5865436f1114da5da6bc but needs more consideration.

sr-gi commented at 12:53 pm on March 28, 2024:

I agree, but I don’t think this is something that should be addressed in this PR. This is the case in other places of the codebase, a PR cleaning this up would be appreciated IMO

maflcko commented at 3:37 pm on March 28, 2024:

@cbergqvist Have you seen rand_expo_duration from https://github.com/bitcoin/bitcoin/pull/29625/files ? I think this answers your in-code question?

cbergqvist commented at 10:48 pm on March 28, 2024:

@maflcko Had not seen that, good that things are happening in this area. @sr-gi Given current master branch, I agree with you in deferring this kind of change.

cbergqvist commented at 12:56 pm on March 20, 2024: contributor

Concept ACK f80981739173c4c2b6afa5b7f91553082fb2da4b.

Glad you agreed something like this was worth doing!

sr-gi force-pushed on Mar 28, 2024

DrahtBot added the label CI failed on Apr 19, 2024

DrahtBot added the label Needs rebase on Apr 22, 2024

sr-gi force-pushed on Apr 22, 2024

sr-gi commented at 6:10 pm on April 22, 2024: member

Rebased to fix merge conflicts

DrahtBot removed the label Needs rebase on Apr 22, 2024

DrahtBot removed the label CI failed on Apr 22, 2024

sr-gi commented at 8:13 pm on April 25, 2024: member

Moving this to draft given it is dependant on https://github.com/bitcoin/bitcoin/pull/28016

sr-gi marked this as a draft on Apr 25, 2024

DrahtBot added the label Needs rebase on May 1, 2024

sr-gi force-pushed on May 1, 2024

DrahtBot removed the label Needs rebase on May 1, 2024

sr-gi marked this as ready for review on May 1, 2024

sr-gi commented at 3:31 am on May 1, 2024: member

Moving out from drat given #28016 was recently merged.

This comment is still outstanding, and I’d love some feedback on it: #29605 (review)

sr-gi force-pushed on May 1, 2024

DrahtBot added the label CI failed on May 1, 2024

DrahtBot commented at 4:42 am on May 1, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/24455108062

in src/net.cpp:2515 in 4342d73cbd outdated

2510+            AddAddrFetch(seed);
2511+
2512+            if (addrman.Size() == 0) {
2513+                LogPrintf("Empty addrman, adding seednode (%s) to addrfetch\n", seed);
2514+            } else {
2515+                LogPrintf("Couldn't connect to peers from addrman after %d seconds. Adding seednode (%s) to addrfetch\n", ADD_NEXT_SEEDNODE.count(), seed);

maflcko commented at 6:42 am on May 1, 2024:

0                LogInfo("Couldn't connect to peers from addrman after %d seconds. Adding seednode (%s) to addrfetch\n", ADD_NEXT_SEEDNODE.count(), seed);

nit: For new code, it could make sense to use the clear and non-deprecated alias here.

sr-gi commented at 1:09 pm on May 1, 2024:

Done in 4342d73

sr-gi force-pushed on May 1, 2024

DrahtBot removed the label CI failed on May 1, 2024

in src/net.cpp:2522 in 33ddd1b4c1 outdated

2518+
2519         ProcessAddrFetch();
2520 
2521         if (!interruptNet.sleep_for(std::chrono::milliseconds(500)))
2522-            return;
2523+        return;

cbergqvist commented at 8:57 pm on May 1, 2024:

Accidental indentation change?

sr-gi commented at 1:50 pm on May 6, 2024:

Yep

in test/functional/p2p_seednode.py:39 in 33ddd1b4c1 outdated

34+            ip = f"{random.randrange(128,169)}.{random.randrange(1,255)}.{random.randrange(1,255)}.{random.randrange(1,255)}"
35+            port = 8333 + i
36+            self.nodes[0].addpeeraddress(ip, port)
37+
38+        # Restart the node so seednode is processed again
39+        with self.nodes[0].assert_debug_log(expected_msgs=[f"Couldn't connect to peers from addrman after 10 seconds. Adding seednode ({seed_node}) to addrfetch"], unexpected_msgs=["Empty addrman, adding seednode"], timeout=20):

cbergqvist commented at 9:29 pm on May 1, 2024:

Had to bump the timeout 20 -> 30 seconds for the test to pass. Example commit with extra logging: 46bac29032c0148ce5288faadbcdc3961d522060

sr-gi commented at 1:51 pm on May 6, 2024:

I’ve been trying to reproduce in my local setup but I’m unable. Did you manage to trigger this on your updated branch, or with this one?

cbergqvist commented at 8:45 pm on May 7, 2024:

Without any additional changes. With --enable-debug and building with Clang.

I’ve retested on ecded92737c4173a30a43e612a21c6e1f24d1605, here is the combined log for test/functional/p2p_seednode.py: 29605_timeout.combined_log.gz

It seems like I only have to bump 20 -> 23 seconds to prevent frequent timeouts. I’ve got IPv6 turned off but otherwise not much weird stuff going on AFAIK. Maybe something stands out if you compare my logs to yours.

sr-gi commented at 3:54 pm on May 8, 2024:

I haven’t been able to reproduce the issue in my local setup, but I think we may be able to improve on this (plus reduce the waiting time) by mocking the node’s time after restarting it

sr-gi commented at 3:55 pm on May 8, 2024:

Tried that (plus some minimal improvements) in 138a2e2

cbergqvist commented at 8:45 pm on May 8, 2024:

Still failing on 1ed818a35d78a5bf6eaf0f72f3730041913b4859 for me. 29605_timeout_1ed818a.combined_log.gz

Excerpts

Startup:

0test  2024-05-08T20:37:27.879000Z TestFramework (INFO): PRNG seed is: 2801220528353188963 
1test  2024-05-08T20:37:27.879000Z TestFramework (DEBUG): Setting up network thread

~8 seconds later we make the attempt to connect to the bogus IP:

0node0 2024-05-08T20:37:35.701878Z (mocktime: 2024-05-08T20:37:46Z) [opencon] [net.cpp:413] [ConnectNode] [net] trying v1 connection 140.186.182.6:8333 lastseen=0.0hrs

Within less than 5 seconds, the assert triggers:

0test  2024-05-08T20:37:40.024000Z TestFramework (ERROR): Assertion failed 
1                                   Traceback (most recent call last):
2                                     File "/home/chris/Documents/Code/bitcoin-core/test/functional/test_framework/test_framework.py", line 132, in main
3                                       self.run_test()
4                                     File "/home/chris/Documents/Code/bitcoin-core/test/functional/p2p_seednode.py", line 50, in run_test
5                                       self.test_seednode_addrman_unreachable_peers()
6                                     File "/home/chris/Documents/Code/bitcoin-core/test/functional/p2p_seednode.py", line 43, in test_seednode_addrman_unreachable_peers
7                                       with node.assert_debug_log(expected_msgs=[f"Couldn't connect to peers from addrman after {ADD_NEXT_SEEDNODE} seconds. Adding seednode ({seed_node}) to addrfetch"], unexpected_msgs=["Empty addrman, adding seednode"], timeout=ADD_NEXT_SEEDNODE):

sr-gi commented at 8:55 pm on May 8, 2024:

That’s interesting. I guess it’s still too close.

0 node0 2024-05-08T20:37:35.200946Z [opencon] [net.cpp:2501] [ThreadOpenConnections] Fixed seeds are disabled
1 node0 2024-05-08T20:37:35.254287Z [http] [httpserver.cpp:306] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:60322
2 node0 2024-05-08T20:37:35.254724Z [httpworker.0] [rpc/request.cpp:187] [parse] [rpc] ThreadRPCServer method=getblockcount user=__cookie__
3 node0 2024-05-08T20:37:35.256529Z [http] [httpserver.cpp:306] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:60322
4 node0 2024-05-08T20:37:35.256960Z [httpworker.2] [rpc/request.cpp:187] [parse] [rpc] ThreadRPCServer method=getmempoolinfo user=__cookie__
5 test  2024-05-08T20:37:35.258000Z TestFramework.node0 (DEBUG): RPC successfully started
6 node0 2024-05-08T20:37:35.259402Z [http] [httpserver.cpp:306] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:60322
7 node0 2024-05-08T20:37:35.259821Z [httpworker.1] [rpc/request.cpp:187] [parse] [rpc] ThreadRPCServer method=setmocktime user=__cookie__
8 node0 2024-05-08T20:37:35.701790Z (mocktime: 2024-05-08T20:37:46Z) [opencon] [addrman.cpp:782] [Select_] [addrman] Selected 140.186.182.6:8333 from new
9 node0 2024-05-08T20:37:35.701878Z (mocktime: 2024-05-08T20:37:46Z) [opencon] [net.cpp:413] [ConnectNode] [net] trying v1 connection 140.186.182.6:8333 lastseen=0.0hrs

There’s more than 10 secs (of mocked time) between when we enter the thread and make that last attempt. Can you increase the wait to ~15 and see if it still fails?

cbergqvist commented at 11:41 am on May 10, 2024:

Adding 5 to the timeout fixes it for me. Modifying the line with the mocktime does not (I also tried removing that line which caused the test to fail again, proving its usefulness).

0        with node.assert_debug_log(expected_msgs=[f"Couldn't connect to peers from addrman after {ADD_NEXT_SEEDNODE} seconds. Adding seednode ({seed_node}) to addrfetch"], unexpected_msgs=["Empty addrman, adding seednode"], timeout=ADD_NEXT_SEEDNODE + 5):
1            self.restart_node(0, extra_args=[f'-seednode={seed_node}'])
2            node.setmocktime(int(time.time()) + ADD_NEXT_SEEDNODE + 1)

sr-gi commented at 2:17 pm on May 10, 2024:

Yeah, the mock time seems to reduce the runtime by 5-10 secs. I’m guessing with 10 secs of wait it could be the case that the connection is still being tried, so the timeout hasn’t been considered. I’ll add 5 more.

cbergqvist changes_requested

cbergqvist commented at 9:31 pm on May 1, 2024: contributor

Re-tested 33ddd1b4c1cb1165b5068fbf7a9461e295f6cef1. Bumping of timeout seems blocking.

sr-gi force-pushed on May 6, 2024

sr-gi force-pushed on May 8, 2024

in test/functional/p2p_seednode.py:40 in 138a2e2a79 outdated

35+        node = self.nodes[0]
36+        # Fill the addrman with unreachable nodes
37+        for i in range(10):
38+            ip = f"{random.randrange(128,169)}.{random.randrange(1,255)}.{random.randrange(1,255)}.{random.randrange(1,255)}"
39+            port = 8333 + i
40+            self.nodes[0].addpeeraddress(ip, port)

cbergqvist commented at 8:19 pm on May 8, 2024:

nit: self.nodes[0] -> node

sr-gi commented at 8:32 pm on May 8, 2024:

Covered in 1ed818a

sr-gi force-pushed on May 8, 2024

DrahtBot added the label CI failed on May 8, 2024

DrahtBot removed the label CI failed on May 9, 2024

sr-gi force-pushed on May 10, 2024

cbergqvist commented at 8:22 pm on May 10, 2024: contributor

re-ACK f3c4c4ba894cf919f7dd2d997c3c0c47b320229e

Ran test/functional/p2p_seednode.py 15 times to confirm robustness against timeouts. Also ran functional tests with -extended --exclude=feature_dbcrash with no failures.

Still urge you to do something like f9bfc588f2e81a7febe233f591b75e41a52db8b4 from #29605 (review) (still cherry-picks cleanly). Narrowing state carried around to when it is actually used is cleaner and more respectful to others reading through CConnman, so they don’t have to figure out where that extra m_seed_nodes-member is accessed (+ to RAM that no longer needs to make room for unused state). Maybe it looks a bit more messy around your current change, but it reduces cognitive load/complexity for the rest of the code by reducing “live state”. It’s a bit like leaving a local variable mutable instead of const at the top of a long function even though it is never changed after initialization.

sr-gi force-pushed on May 13, 2024

sr-gi commented at 2:09 pm on May 13, 2024: member

@cbergqvist I took your suggestion from #29605 (review)

cbergqvist approved

cbergqvist commented at 7:10 am on May 14, 2024: contributor

re ACK b41bbd5688f8f04b2c0e8d7f60d6df6931c17079

Passed functional tests (with --extended --exclude=feature_dbcrash).

DrahtBot added the label CI failed on Jul 13, 2024

DrahtBot commented at 4:52 pm on July 13, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/24902524268

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

cbergqvist commented at 8:52 pm on July 13, 2024: contributor

CI failure would be fixed by.

 0diff --git a/src/net.cpp b/src/net.cpp
 1index 01522a6286..36889a6da6 100644
 2--- a/src/net.cpp
 3+++ b/src/net.cpp
 4@@ -3269,9 +3269,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
 5 
 6     // Randomize the order in which we may query seednode to potentially prevent connecting to the same one every restart (and signal that we have restarted)
 7     std::vector<std::string> seed_nodes = connOptions.vSeedNodes;
 8-    if (!seed_nodes.empty()) {
 9-        Shuffle(seed_nodes.begin(), seed_nodes.end(), FastRandomContext{});
10-    }
11+    std::shuffle(seed_nodes.begin(), seed_nodes.end(), FastRandomContext{});
12 
13     if (m_use_addrman_outgoing) {
14         // Load addresses from anchors.dat

Making all commits compile on top of the new master and preserving my existing commit a28546e4d203ec73aa734c1ff91d2b909729b1af is not possible. I’m fine with you squashing it into your 94f4a468744959cb1bc80d79b9f91b694dd831df. Or I could provide a new commit for you to cherry-pick once you’ve rewritten yours, whichever you prefer. :)

sr-gi commented at 0:33 am on July 14, 2024: member

@cbergqvist I’ll squash for simplicity if you’re fine with that

sr-gi force-pushed on Jul 14, 2024

DrahtBot removed the label CI failed on Jul 14, 2024

cbergqvist approved

cbergqvist commented at 8:28 pm on July 14, 2024: contributor

ACK 7aef6ba1cf1023a7ec34a57800f1044c667243a1

Couldn’t wrangle git range-diff so went back to:

0git diff b41bbd5688f8f04b2c0e8d7f60d6df6931c17079~3 b41bbd5688f8f04b2c0e8d7f60d6df6931c17079 src/net.cpp > old
1git diff 7aef6ba1cf1023a7ec34a57800f1044c667243a1~2 7aef6ba1cf1023a7ec34a57800f1044c667243a1 src/net.cpp > new
2meld old new

Confirmed only the shuffle-function was swapped.

test/functional/test_runner.py passed.

DrahtBot added the label CI failed on Jul 23, 2024

DrahtBot commented at 10:32 pm on July 23, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27414823329

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

net: Favor peers from addrman over fetching seednodes

The current behavior of seednode fetching is pretty eager: we do it as the first
step under `ThreadOpenNetworkConnections` even if some peers may be queryable
from our addrman. This poses two potential issues:

- First, if permanently set (e.g. running with seednode in a config file) we'd
be signaling such seed every time we restart our node
- Second, we will be giving the seed node way too much influence over our addrman,
populating the latter even with data from the former even when unnecessary

This changes the behavior to only add seednodes to `m_addr_fetch` if our addrman
is empty, or little by little after we've spent some time trying addresses from
our addrman. Also, seednodes are added to `m_addr_fetch` in random order, to avoid
signaling the same node in case more than one seed is added and we happen to try
them over multiple restarts

3270f0adad

test: adds seednode functional tests

Adds functional tests to test the interaction between seednode and the AddrMan

6eeb188d40

sr-gi force-pushed on Jul 24, 2024

sr-gi commented at 3:14 pm on July 24, 2024: member

Rebased to deal with CI failing.

6eeb188d40fe8f6c3b0a3be6dca519ea9a7b2358 has been amended to include __file__ in the P2PSeedNodes constructor, as required by any subclass of BitcoinTestFramework since #30463

DrahtBot removed the label CI failed on Jul 24, 2024

cbergqvist approved

cbergqvist commented at 2:35 pm on July 25, 2024: contributor

ACK 6eeb188d40fe8f6c3b0a3be6dca519ea9a7b2358

Confirmed only __file__ parameter was added to Python test constructor.

Passed functional tests.

itornaza approved

itornaza commented at 2:22 pm on July 26, 2024: none

Tested ACK 6eeb188d40fe8f6c3b0a3be6dca519ea9a7b2358

Reviewed the code changes in src/net.cpp and src/net.h which efficiently and effectively implement the intended behavior of this PR. Run all unit, functional and extended tests (including dbcrash) and all of them pass.

in test/functional/p2p_seednode.py:40 in 6eeb188d40

35+        node = self.nodes[0]
36+        # Fill the addrman with unreachable nodes
37+        for i in range(10):
38+            ip = f"{random.randrange(128,169)}.{random.randrange(1,255)}.{random.randrange(1,255)}.{random.randrange(1,255)}"
39+            port = 8333 + i
40+            node.addpeeraddress(ip, port)

tdb3 commented at 2:28 am on July 31, 2024:

Although unlikely to be reachable, do we have a guarantee that these addresses aren’t reachable? What was the rationale for choosing this range (vs some range within 0.0.0.0/8)?

sr-gi commented at 2:26 pm on August 1, 2024:

I got this from here: https://github.com/bitcoin/bitcoin/blob/ebd82fa9fae13d5a7a395800295dd7fd34185b58/test/functional/p2p_addr_relay.py#L120

I think the rationale was that I wanted routable yet unreachable peers, but I think you may be right that there’s nothing really preventing an IP on that range from running a node at the time the test is run (yet it may be pretty unlikely, given we also use custom ports)

tdb3 commented at 0:15 am on August 2, 2024:

Did a bit of digging. Looks like the approach may have been inherited from https://github.com/bitcoin/bitcoin/pull/22387/commits/b4ece8a1cda69cc268d39d21bba59c51fa2fb9ed (PR #22387). The risk of “collision” with a real node is extremely low (for now), so it seems like it could be ok to leave as-is (doesn’t feel ideal, but unless a suitable alternate address range is identified, it’s what we have).

Tried 0.0.1.{random 1-255}, but the test failed (i.e. with Unexpected message "Empty addrman, adding seednode" partially matches log).

Nothing pops out immediately in https://en.wikipedia.org/wiki/Reserved_IP_addresses) as both publicly routable and something we don’t disqualify already (e.g. CNetAddr::IsRoutable()).

vasild commented at 9:50 am on August 2, 2024:

Is this going to cause the test to try to open connections to somebody on the internet? I think it is highly undesirable if my ISP sees that I try to open connections to these addresses/ports and is able to deduce that I am running bitcoin tests.

Further connections to these addresses may be slow to fail, thus slowing down this test.

You can use something like -proxy=127.0.0.1:1 to have all connections to any addresses fail quickly.

tdb3 commented at 3:06 am on July 31, 2024: contributor

Approach ACK Good work. This seems to help support decentralization and privacy. Left a question for now, but will take a closer look soon.

tdb3 approved

tdb3 commented at 0:16 am on August 2, 2024: contributor

ACK 6eeb188d40fe8f6c3b0a3be6dca519ea9a7b2358

in test/functional/p2p_seednode.py:45 in 6eeb188d40

40+            node.addpeeraddress(ip, port)
41+
42+        # Restart the node so seednode is processed again
43+        with node.assert_debug_log(expected_msgs=[f"Couldn't connect to peers from addrman after {ADD_NEXT_SEEDNODE} seconds. Adding seednode ({seed_node}) to addrfetch"], unexpected_msgs=["Empty addrman, adding seednode"], timeout=ADD_NEXT_SEEDNODE * 1.5):
44+            self.restart_node(0, extra_args=[f'-seednode={seed_node}'])
45+            node.setmocktime(int(time.time()) + ADD_NEXT_SEEDNODE + 1)

vasild commented at 9:51 am on August 2, 2024:

“timeout=ADD_NEXT_SEEDNODE * 1.5” – does it mean that this test will wait for 15 seconds here?

sr-gi commented at 2:06 pm on August 2, 2024:

If the test was broken, yes. I guess we may be able to reduce that a bit (I don’t really recall why I went for ADD_NEXT_SEEDNODE * 1.5), but the node needs time to shutdown and reboot, so we cannot simply mock the time straightaway

cbergqvist commented at 8:41 pm on August 3, 2024:

The reason was that I was getting timeouts (under certain configurations at least): #29605 (review)

sr-gi commented at 6:10 pm on August 5, 2024:

I see. So long story short @vasild, I think it really depends on the system running the test. The conservative timeout is just to make sure the test has enough time to reboot the node and trigger the expected logic, which under normal conditions (and a reasonable machine) should not take that long

achow101 commented at 3:41 pm on September 4, 2024: member

ACK 6eeb188d40fe8f6c3b0a3be6dca519ea9a7b2358

achow101 merged this on Sep 4, 2024

achow101 closed this on Sep 4, 2024

net: Favor peers from addrman over fetching seednodes #29605

Code Coverage

Reviews

Conflicts

Excerpts