net: Favor peers from addrman over fetching seednodes #29605

pull sr-gi wants to merge 2 commits into bitcoin:master from sr-gi:2024-04-07-seednode-addrman changing 4 files +92 −9
  1. 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

  2. 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.

  3. sr-gi renamed this:
    2024 04 07 seednode addrman
    net: give seednode priority over dnsseed if both are provided
    on Mar 8, 2024
  4. DrahtBot added the label P2P on Mar 8, 2024
  5. 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
  6. DrahtBot renamed this:
    net: Favor peers from addrman over fetching seednodes
    net: Favor peers from addrman over fetching seednodes
    on Mar 8, 2024
  7. sr-gi force-pushed on Mar 8, 2024
  8. 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
  9. sr-gi force-pushed on Mar 8, 2024
  10. DrahtBot added the label CI failed on Mar 8, 2024
  11. 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

  12. sr-gi force-pushed on Mar 8, 2024
  13. sr-gi force-pushed on Mar 8, 2024
  14. DrahtBot removed the label CI failed on Mar 8, 2024
  15. 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
  16. sr-gi force-pushed on Mar 11, 2024
  17. 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

  18. sr-gi force-pushed on Mar 11, 2024
  19. sr-gi force-pushed on Mar 15, 2024
  20. sr-gi commented at 9:14 pm on March 15, 2024: member
  21. 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:

    cbergqvist commented at 9:25 pm on May 1, 2024:
    Switched to using Span for a slightly smaller change suggestion in f9bfc588f2e81a7febe233f591b75e41a52db8b4.
  22. 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.
  23. cbergqvist commented at 12:56 pm on March 20, 2024: contributor

    Concept ACK f80981739173c4c2b6afa5b7f91553082fb2da4b.

    Glad you agreed something like this was worth doing!

  24. sr-gi force-pushed on Mar 28, 2024
  25. DrahtBot added the label CI failed on Apr 19, 2024
  26. DrahtBot added the label Needs rebase on Apr 22, 2024
  27. sr-gi force-pushed on Apr 22, 2024
  28. sr-gi commented at 6:10 pm on April 22, 2024: member
    Rebased to fix merge conflicts
  29. DrahtBot removed the label Needs rebase on Apr 22, 2024
  30. DrahtBot removed the label CI failed on Apr 22, 2024
  31. 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
  32. sr-gi marked this as a draft on Apr 25, 2024
  33. DrahtBot added the label Needs rebase on May 1, 2024
  34. sr-gi force-pushed on May 1, 2024
  35. DrahtBot removed the label Needs rebase on May 1, 2024
  36. sr-gi marked this as ready for review on May 1, 2024
  37. 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)

  38. sr-gi force-pushed on May 1, 2024
  39. DrahtBot added the label CI failed on May 1, 2024
  40. 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

  41. 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
  42. sr-gi force-pushed on May 1, 2024
  43. DrahtBot removed the label CI failed on May 1, 2024
  44. 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
  45. 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.
  46. cbergqvist changes_requested
  47. cbergqvist commented at 9:31 pm on May 1, 2024: contributor
    Re-tested 33ddd1b4c1cb1165b5068fbf7a9461e295f6cef1. Bumping of timeout seems blocking.
  48. sr-gi force-pushed on May 6, 2024
  49. sr-gi force-pushed on May 8, 2024
  50. 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
  51. sr-gi force-pushed on May 8, 2024
  52. DrahtBot added the label CI failed on May 8, 2024
  53. DrahtBot removed the label CI failed on May 9, 2024
  54. sr-gi force-pushed on May 10, 2024
  55. 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.

  56. sr-gi force-pushed on May 13, 2024
  57. sr-gi commented at 2:09 pm on May 13, 2024: member
    @cbergqvist I took your suggestion from #29605 (review)
  58. cbergqvist approved
  59. cbergqvist commented at 7:10 am on May 14, 2024: contributor

    re ACK b41bbd5688f8f04b2c0e8d7f60d6df6931c17079

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

  60. DrahtBot added the label CI failed on Jul 13, 2024
  61. 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.

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

  63. sr-gi commented at 0:33 am on July 14, 2024: member
    @cbergqvist I’ll squash for simplicity if you’re fine with that
  64. sr-gi force-pushed on Jul 14, 2024
  65. DrahtBot removed the label CI failed on Jul 14, 2024
  66. cbergqvist approved
  67. 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.

  68. DrahtBot added the label CI failed on Jul 23, 2024
  69. 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.

  70. 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
  71. test: adds seednode functional tests
    Adds functional tests to test the interaction between seednode and the AddrMan
    6eeb188d40
  72. sr-gi force-pushed on Jul 24, 2024
  73. 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

  74. DrahtBot removed the label CI failed on Jul 24, 2024
  75. cbergqvist approved
  76. 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.

  77. itornaza approved
  78. 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.

  79. 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.

  80. 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.
  81. tdb3 approved
  82. tdb3 commented at 0:16 am on August 2, 2024: contributor
    ACK 6eeb188d40fe8f6c3b0a3be6dca519ea9a7b2358
  83. 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
  84. achow101 commented at 3:41 pm on September 4, 2024: member
    ACK 6eeb188d40fe8f6c3b0a3be6dca519ea9a7b2358
  85. achow101 merged this on Sep 4, 2024
  86. achow101 closed this on Sep 4, 2024


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-12-11 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me