net: improves addnode / m_added_nodes logic #28155

pull sr-gi wants to merge 5 commits into bitcoin:master from sr-gi:202307-addednodeinfo changing 9 files +238 −36
  1. sr-gi commented at 8:47 pm on July 25, 2023: member

    Rationale

    Currently, addnode has a couple of corner cases that allow it to either connect to the same peer more than once, hence wasting outbound connection slots, or add redundant information to m_added_nodes, hence making Bitcoin iterate through useless data on a regular basis.

    Connecting to the same node more than once

    In general, connecting to the same node more than once is something we should try to prevent. Currently, this is possible via addnode in two different ways:

    1. Calling addnode more than once in a short time period, using two equivalent but distinct addresses
    2. Calling addnode add using an IP, and addnode onetry after with an address that resolved to the same IP

    For the former, the issue boils down to CConnman::ThreadOpenAddedConnections calling CConnman::GetAddedNodeInfo once, and iterating over the result to open connections (CConman::OpenNetworkConnection) on the same loop for all addresses.CConnman::ConnectNode only checks a single address, at random, when resolving from a hostname, and uses it to check whether we are already connected to it.

    An example to test this would be calling:

    0bitcoin-cli addnode "127.0.0.1:port" add
    1bitcoin-cli addnode "localhost:port" add
    

    And check how it allows us to perform both connections some times, and some times it fails.

    The latter boils down to the same issue, but takes advantage of onetry bypassing the CConnman::ThreadOpenAddedConnections logic and calling CConnman::OpenNetworkConnection straightaway. A way to test this would be:

    0bitcoin-cli addnode "127.0.0.1:port" add
    1bitcoin-cli addnode "localhost:port" onetry
    

    Adding the same peer with two different, yet equivalent, addresses

    The current implementation of addnode is pretty naive when checking what data is added to m_added_nodes. Given the collection stores strings, the checks at CConnman::AddNode() basically check wether the exact provided string is already in the collection. If so, the data is rejected, otherwise, it is accepted. However, ips can be formatted in several ways that would bypass those checks.

    Two examples would be 127.0.0.1 being equal to 127.1 and [::1] being equal to [0:0:0:0:0:0:0:1]. Adding any pair of these will be allowed by the rpc command, and both will be reported as connected by getaddednodeinfo, given they map to the same CService.

    This is less severe than the previous issue, since even tough both nodes are reported as connected by getaddednodeinfo, there is only a single connection to them (as properly reported by getpeerinfo). However, this adds redundant data to m_added_nodes, which is undesirable.

    Parametrize CConnman::GetAddedNodeInfo

    Finally, this PR also parametrizes CConnman::GetAddedNodeInfo so it returns either all added nodes info, or only info about the nodes we are not connected to. This method is used both for rpc, in getaddednodeinfo, in which we are reporting all data to the user, so the former applies, and to check what nodes we are not connected to, in CConnman::ThreadOpenAddedConnections, in which we are currently returning more data than needed and then actively filtering using CService.fConnected()

  2. DrahtBot commented at 8:47 pm on July 25, 2023: 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 jonatack, kashifs, mzumsande
    Stale ACK vasild

    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:

    • #28248 (p2p: peer connection bug fixes by jonatack)
    • #26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)

    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 commented at 8:49 pm on July 25, 2023: member
    I’ve purposely split this into three commits according to each of the fixes so they can be discussed, and considered independently. That way it’ll be easier to remove any if the change does not reach enough consensus.
  4. sr-gi force-pushed on Jul 25, 2023
  5. DrahtBot added the label CI failed on Jul 25, 2023
  6. sr-gi force-pushed on Jul 26, 2023
  7. DrahtBot removed the label CI failed on Jul 26, 2023
  8. kevkevinpal commented at 9:29 pm on July 26, 2023: contributor

    would it make sense to add a functional test like the below under

    0ip_port = "localhost:[]".format(p2p_port='add')
    1self.nodes[0].addnode(node=ip_port, command='add')
    

    in test/functional/rpc_net.py on line 212 because we shortly after assert that there is only 1 added node added

  9. sr-gi commented at 4:45 pm on July 28, 2023: member

    would it make sense to add a functional test like the below under

    0ip_port = "localhost:[]".format(p2p_port='add')
    1self.nodes[0].addnode(node=ip_port, command='add')
    

    in test/functional/rpc_net.py on line 212 because we shortly after assert that there is only 1 added node added

    I don’t think that’d work. You’ll still be able to add two nodes that resolve to the same IP using addnode as long as they belong to different namespaces. Lookups are not performed. The difference though is that both connections won’t be established. However, the connection is not being checked by the test.

    What could be added, however, is a check for 127.0.0.1 and 127.1.

    PS: This actually made me realize that the check for whether to add something to m_added_nodes was not good enough.

  10. sr-gi force-pushed on Jul 28, 2023
  11. sr-gi force-pushed on Jul 28, 2023
  12. DrahtBot added the label CI failed on Jul 28, 2023
  13. sr-gi commented at 5:32 pm on July 28, 2023: member
    Added a test to test/functional/rpc_net.py and fixed the check on CConnman::AddNode
  14. in test/functional/rpc_net.py:67 in b5763bc83e outdated
    69-        self.test_service_flags()
    70-        self.test_getnodeaddresses()
    71-        self.test_addpeeraddress()
    72+        # self.test_service_flags()
    73+        # self.test_getnodeaddresses()
    74+        # self.test_addpeeraddress()
    


    kevkevinpal commented at 6:25 pm on July 28, 2023:
    might want to uncomment these other tests

    sr-gi commented at 6:41 pm on July 28, 2023:
    Ups, my bad
  15. sr-gi force-pushed on Jul 28, 2023
  16. DrahtBot removed the label CI failed on Jul 28, 2023
  17. in src/net.cpp:467 in a6fcf17f9d outdated
    475-            if (pnode) {
    476-                LogPrintf("Failed to open new connection, already connected\n");
    477-                return nullptr;
    478+            // If the connection is made by name, it can be the case that the name resolves to more than one address.
    479+            // We don't want to connect any more of them if we are already connected to one
    480+            for (auto &r : resolved) {
    


    vasild commented at 7:53 am on August 17, 2023:

    nit, style (const and attach the & to auto):

    0            for (const auto& r : resolved) {
    
  18. in src/net.cpp:478 in a6fcf17f9d outdated
    486+                // It is possible that we already have a connection to the IP/port pszDest resolved to.
    487+                // In that case, drop the connection that was just created.
    488+                LOCK(m_nodes_mutex);
    489+                CNode* pnode = FindNode(static_cast<CService>(addrConnect));
    490+                if (pnode) {
    491+                    LogPrintf("Failed to open new connection, already connected\n");
    


    vasild commented at 7:58 am on August 17, 2023:

    This log can be improved to something like this:

    0Not opening a new connection to pool.bitcoinnodes.org, already connected to 1.2.3.4
    

    vasild commented at 3:08 pm on August 24, 2023:

    This is now:

    0LogPrintf("Not opening a connection to %d, already connected to %s\n", pnode->addr.ToStringAddrPort(), addrConnect.ToStringAddrPort());
    

    It will print two IP addresses, which are most likely equal. It would be more valuable to print the hostname: “… pool.bitcoinnodes.org, already connected to 1.2.3.4” which implies that 1.2.3.4 is the same as pool.bitcoinnodes.org.

    0LogPrintf("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort());
    

    sr-gi commented at 1:48 pm on August 25, 2023:
    Addressed in 079c56c
  19. in src/net.cpp:469 in a6fcf17f9d outdated
    461@@ -462,19 +462,22 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
    462     if (pszDest) {
    463         const std::vector<CService> resolved{Lookup(pszDest, default_port, fNameLookup && !HaveNameProxy(), 256)};
    464         if (!resolved.empty()) {
    465-            const CService& rnd{resolved[GetRand(resolved.size())]};
    


    vasild commented at 8:03 am on August 17, 2023:

    The old code would pick a random address from the list of resolved addresses. The new code would pick the last one. I think it is good to preserve the randomness. One way to achieve that would be to shuffle the result before iterating and picking the last one.

    0if (!resolved.empty()) {
    1    Shuffle(resolved.begin(), resolved.end(), FastRandomContext{});
    2    for (const auto& r : resolved) {
    

    or if the shuffle is perceived expensive for big lists, before the loop auto to_pick = GetRand(resolved.size()) and inside the loop only assign addrConnect for the to_pickth element.


    sr-gi commented at 8:04 am on August 23, 2023:
    I think Shuffle should do, given this picks on a list of resolved DNS lookups. Normally, that should be rather short, and if this is still up to date, the max length should be 4095.
  20. in src/net.cpp:2581 in a6fcf17f9d outdated
    2576@@ -2570,9 +2577,10 @@ std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addres
    2577 
    2578 bool CConnman::AddNode(const std::string& strNode)
    2579 {
    2580+    CService resolved(LookupNumeric(strNode, Params().GetDefaultPort(strNode)));
    


    vasild commented at 8:11 am on August 17, 2023:

    In the commit message of 12e601c33e rpc: Prevents adding the same ip more than once when formatted differently:

    … formatting IPs in aborts different,

  21. in src/net.cpp:2583 in a6fcf17f9d outdated
    2576@@ -2570,9 +2577,10 @@ std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addres
    2577 
    2578 bool CConnman::AddNode(const std::string& strNode)
    2579 {
    2580+    CService resolved(LookupNumeric(strNode, Params().GetDefaultPort(strNode)));
    2581     LOCK(m_added_nodes_mutex);
    2582     for (const std::string& it : m_added_nodes) {
    2583-        if (strNode == it) return false;
    2584+        if (strNode == it || (resolved != CService{} && resolved == LookupNumeric(it, Params().GetDefaultPort(it)))) return false;
    


    vasild commented at 8:44 am on August 17, 2023:
    nit: use resolved.IsValid() instead of resolved != CService{}.
  22. in src/net.cpp:1925 in a6fcf17f9d outdated
    1921@@ -1919,7 +1922,7 @@ std::vector<CAddress> CConnman::GetCurrentBlockRelayOnlyConns() const
    1922     return ret;
    1923 }
    1924 
    1925-std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo() const
    1926+std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo(bool includeConnected) const
    


    vasild commented at 8:47 am on August 17, 2023:

    style (same in the declaration):

    0std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo(bool include_connected) const
    
  23. in src/net.cpp:1991 in a6fcf17f9d outdated
    1987@@ -1979,21 +1988,19 @@ void CConnman::ThreadOpenAddedConnections()
    1988     while (true)
    1989     {
    1990         CSemaphoreGrant grant(*semAddnode);
    1991-        std::vector<AddedNodeInfo> vInfo = GetAddedNodeInfo();
    1992+        std::vector<AddedNodeInfo> vInfo = GetAddedNodeInfo(false);
    


    vasild commented at 8:50 am on August 17, 2023:
    0        std::vector<AddedNodeInfo> vInfo = GetAddedNodeInfo(/*include_connected=*/false);
    

    and in other call sites too

  24. vasild commented at 8:54 am on August 17, 2023: contributor
    Approach ACK a6fcf17f9dde56637bcfe824bb3def83e764b38c
  25. sr-gi force-pushed on Aug 23, 2023
  26. sr-gi commented at 8:40 am on August 23, 2023: member
    Addressed @vasild comments
  27. vasild approved
  28. vasild commented at 3:09 pm on August 24, 2023: contributor

    ACK d0e7e181840a6c76f2a3a5cb39ccc5653c0e62e0

    Would be nice to print the hostname here: #28155 (review)

  29. sr-gi force-pushed on Aug 25, 2023
  30. vasild approved
  31. vasild commented at 3:55 pm on August 25, 2023: contributor
    ACK fd8b1db9ca8fb5aa0678d36e79b76bd283cf8fed
  32. glozow added the label P2P on Sep 15, 2023
  33. DrahtBot added the label CI failed on Sep 15, 2023
  34. sr-gi force-pushed on Sep 15, 2023
  35. DrahtBot removed the label CI failed on Sep 15, 2023
  36. sr-gi renamed this:
    Improves addnode / m_added_nodes logic
    net: improves addnode / m_added_nodes logic
    on Sep 19, 2023
  37. DrahtBot added the label Needs rebase on Oct 3, 2023
  38. sr-gi force-pushed on Oct 3, 2023
  39. DrahtBot removed the label Needs rebase on Oct 3, 2023
  40. DrahtBot added the label CI failed on Oct 4, 2023
  41. DrahtBot removed the label CI failed on Oct 4, 2023
  42. vasild approved
  43. vasild commented at 12:10 pm on October 10, 2023: contributor
    ACK afa15ed2c741593f49d608d75565daa5e55c6353
  44. DrahtBot requested review from jonatack on Oct 10, 2023
  45. in src/net.cpp:467 in b98e20e53a outdated
    463@@ -464,21 +464,25 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
    464     const uint16_t default_port{pszDest != nullptr ? GetDefaultPort(pszDest) :
    465                                                      m_params.GetDefaultPort()};
    466     if (pszDest) {
    467-        const std::vector<CService> resolved{Lookup(pszDest, default_port, fNameLookup && !HaveNameProxy(), 256)};
    468+        std::vector<CService> resolved{Lookup(pszDest, default_port, fNameLookup && !HaveNameProxy(), 256)};
    


    mzumsande commented at 7:49 pm on October 17, 2023:

    While the first commit solves the problem of now looking up all of the possible addresses to avoid duplicate connections, I think there is a related issue that could be improved (which I ran into recently):

    We still choose one of the addresses at random to connect to (addrConnect). If that connection fails, we report failure and give up. That means if I am in a network environment where I can’t connect to IPv6 addresses, and try to connect to a valid node like xyz.xyz which resolves to both a IPv4 and IPv6 address with addnode bitcoin-cli addnode "xyz.xyz" "onetry", the call will fail to connect 50% of the time.

    Wouldn’t it be better to try to open a connection to each of the addresses pszDest resolves to until one of them succeeds? Or at least to a few of them? I don’t think that a fix for this issue needs to be included in this PR though.


    sr-gi commented at 5:24 pm on October 18, 2023:
    Agreed. I’ll add this as a follow-up if reviewers are OK with that
  46. in src/net.cpp:483 in b98e20e53a outdated
    492+                // It is possible that we already have a connection to the IP/port pszDest resolved to.
    493+                // In that case, drop the connection that was just created.
    494+                LOCK(m_nodes_mutex);
    495+                CNode* pnode = FindNode(static_cast<CService>(addrConnect));
    496+                if (pnode) {
    497+                    LogPrintf("Not opening a connection to %d, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort());
    


    mzumsande commented at 8:00 pm on October 17, 2023:
    This should be %s not %d (not that it actually matters with tinyformat)
  47. in src/test/fuzz/connman.cpp:124 in afa15ed2c7 outdated
    120@@ -121,7 +121,7 @@ FUZZ_TARGET(connman, .init = initialize_connman)
    121                 connman.SetTryNewOutboundPeer(fuzzed_data_provider.ConsumeBool());
    122             });
    123     }
    124-    (void)connman.GetAddedNodeInfo();
    125+    (void)connman.GetAddedNodeInfo(/*include_connected=*/true);
    


    mzumsande commented at 8:06 pm on October 17, 2023:
    could fuzz the added arg (fuzzed_data_provider.ConsumeBool())
  48. mzumsande commented at 8:07 pm on October 17, 2023: contributor
    Code review ACK afa15ed2c741593f49d608d75565daa5e55c6353
  49. sr-gi force-pushed on Oct 18, 2023
  50. sr-gi commented at 5:27 pm on October 18, 2023: member
    Addresses comments from @mzumsande
  51. sr-gi commented at 6:47 pm on October 18, 2023: member

    I’ve been revisiting the patch introduced by this PR to follow up based on #28155 (review), and I realized that the following piece of code (net.cpp L558-L569) may not be required anymore (and it should be removed here too if so):

     0if (addrConnect.IsValid()) {
     1    [...]
     2} else if (pszDest && GetNameProxy(proxy)) {
     3    sock = CreateSock(proxy.proxy);
     4    if (!sock) {
     5        return nullptr;
     6    }
     7    std::string host;
     8    uint16_t port{default_port};
     9    SplitHostPort(std::string(pszDest), port, host);
    10    bool proxyConnectionFailed;
    11    connected = ConnectThroughProxy(proxy, host, port, *sock, nConnectTimeout,
    12                                    proxyConnectionFailed);
    13}
    

    To my understanding, it cannot be the case that !addrConnect.IsValid() while pszDest is set, given we now check for its validity after re-assigning addrConnect in L474-L478. Therefore, any addrConnect is ensured to be valid provided pszDest is set.

    Does this make sense @vasild @mzumsande?

  52. mzumsande commented at 8:19 pm on October 20, 2023: contributor

    re-ACK 3d1854cdf31a83f4daafd06a760e3355f3bcc547

    To my understanding, it cannot be the case that !addrConnect.IsValid() while pszDest is set, given we now check for its validity after re-assigning addrConnect in L474-L478. Therefore, any addrConnect is ensured to be valid provided pszDest is set.

    Does this make sense @vasild @mzumsande?

    I don’t think that’s correct. If we are connecting through a proxy, Lookup() could return no entries, addrConnect would be default-initialized (and therefore invalid) and we might need this code.

  53. DrahtBot requested review from vasild on Oct 20, 2023
  54. sr-gi commented at 7:41 pm on October 23, 2023: member

    re-ACK 3d1854c

    To my understanding, it cannot be the case that !addrConnect.IsValid() while pszDest is set, given we now check for its validity after re-assigning addrConnect in L474-L478. Therefore, any addrConnect is ensured to be valid provided pszDest is set. Does this make sense @vasild @mzumsande?

    I don’t think that’s correct. If we are connecting through a proxy, Lookup() could return no entries, addrConnect would be default-initialized (and therefore invalid) and we might need this code.

    You’re right!

  55. jonatack commented at 3:58 am on October 24, 2023: member
    Reviewing this PR tomorrow (sorry for not getting to it earlier).
  56. jonatack commented at 5:08 am on October 25, 2023: member

    utACK 3d1854cdf31a83f4daafd06a760e3355f3bcc547 code review, still need to test these manually

    Regarding rpc: Prevents adding the same ip more than once when formatted differently, I noticed this issue with I2P and CJDNS peers addnoded both with and without a port will cause multiple entries to be returned by RPC getaddednodesinfo. It looks like this commit may fix that and I’ll test it manually. If yes, that might be a good test case to add.

    Commit net/rpc: prevents connecting to an address we are already connected to might be more accurately named Check all resolved addresses in ConnectNode rather than one, as there remain a number of open issues to solve related to peer connection detection, i.e. #28248.

  57. in src/net.cpp:3481 in 3d1854cdf3 outdated
    3474@@ -3467,9 +3475,10 @@ std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addres
    3475 
    3476 bool CConnman::AddNode(const AddedNodeParams& add)
    3477 {
    3478+    CService resolved(LookupNumeric(add.m_added_node, GetDefaultPort(add.m_added_node)));
    3479     LOCK(m_added_nodes_mutex);
    3480     for (const auto& it : m_added_node_params) {
    3481-        if (add.m_added_node == it.m_added_node) return false;
    3482+        if (add.m_added_node == it.m_added_node || (resolved.IsValid() && resolved == LookupNumeric(it.m_added_node, GetDefaultPort(it.m_added_node)))) return false;
    


    jonatack commented at 5:32 am on October 27, 2023:

    It may be good to hoist the resolved.IsValid() computation out of the loop here.

    0 bool CConnman::AddNode(const AddedNodeParams& add)
    1 {
    2-    CService resolved(LookupNumeric(add.m_added_node, GetDefaultPort(add.m_added_node)));
    3+    const CService resolved{LookupNumeric(add.m_added_node, GetDefaultPort(add.m_added_node))};
    4+    const bool resolved_is_valid{resolved.IsValid()};
    5     LOCK(m_added_nodes_mutex);
    6     for (const auto& it : m_added_node_params) {
    7-        if (add.m_added_node == it.m_added_node || (resolved.IsValid() && resolved == LookupNumeric(it.m_added_node, GetDefaultPort(it.m_added_node)))) return false;
    8+        if (add.m_added_node == it.m_added_node || (resolved_is_valid && resolved == LookupNumeric(it.m_added_node, GetDefaultPort(it.m_added_node)))) return false;
    9     }
    
  58. jonatack commented at 5:35 am on October 27, 2023: member

    As part of writing unit test coverage for #28248, I’ve written unit test coverage for all three commits of this pull. Feel free to pull in the last 2 commits in #28749. Suggest running the following to test it:

    0./src/test/test_bitcoin -t net_peer_connection_tests -l message -- -printtoconsole=1
    

    or

    0./src/test/test_bitcoin -t net_peer_connection_tests -l all -- -printtoconsole=1 
    
  59. net/rpc: Check all resolved addresses in ConnectNode rather than just one
    The current `addnode` rpc command has some edge cases in where it is possible to
    connect to the same node twice by combining ip and address requests. This can happen under two situations:
    
    The two commands are run one right after each other, in which case they will be processed
    under the same loop in `CConnman::ThreadOpenAddedConnections` without refreshing `vInfo`, so both
    will go trough. An example of this would be:
    
    ```
    bitcoin-cli addnode "localhost:port" add
    
    ```
    
    A node is added by IP using `addnode "add"` while the other is added by name using
    `addnode "onetry"` with an address that resolves to multiple IPs. In this case, we currently
    only check one of the resolved IPs (picked at random), instead of all the resolved ones, meaning
    this will only probabilistically fail/succeed. An example of this would be:
    
    ```
    bitcoin-cli addnode "127.0.0.1:port" add
    [...]
    bitcoin-cli addnode "localhost:port" onetry
    ```
    
    Both cases can be fixed by iterating over all resolved addresses in `CConnman::ConnectNode` instead
    of picking one at random
    2574b7e177
  60. rpc: Prevents adding the same ip more than once when formatted differently
    Currently it is possible to add the same node twice when formatting IPs in
    different, yet equivalent, manner. This applies to both ipv4 and ipv6, e.g:
    
    127.0.0.1 = 127.1 | [::1] = [0:0:0:0:0:0:0:1]
    
    `addnode` will accept both and display both as connected (given they translate to
    the same IP). This will not result in multiple connections to the same node, but
    will report redundant info when querying `getaddednodeinfo` and populate `m_added_nodes`
    with redundant data.
    
    This can be avoided performing comparing the contents of `m_added_addr` and the address
    to be added as `CServices` instead of as strings.
    94e8882d82
  61. net/rpc: Makes CConnman::GetAddedNodeInfo able to return only non-connected address on request
    `CConnman::GetAddedNodeInfo` is used both to get a list of addresses to manually connect to
    in `CConnman::ThreadOpenAddedConnections`, and to report about manually added connections in
    `getaddednodeinfo`. In both cases, all addresses added to `m_added_nodes` are returned, however
    the nodes we are already connected to are only relevant to the latter, in the former they are
    actively discarded.
    
    Parametrizes `CConnman::GetAddedNodeInfo` so we can ask for only addresses we are not connected to,
    to avoid passing useless information around.
    34b9ef443b
  62. Allow unit tests to access additional CConnman members
    that are otherwise private:
    - CConnman::m_nodes
    - CConnman::ConnectNodes()
    - CConnman::AlreadyConnectedToAddress()
    
    and update the #include headers per iwyu.
    4b834f6499
  63. sr-gi force-pushed on Oct 30, 2023
  64. sr-gi commented at 5:31 pm on October 30, 2023: member
    Addresses review comments and add @jonatack’s test commits
  65. jonatack commented at 8:38 pm on October 30, 2023: member

    ACK fc1e9e4780e72ed432afb5c0961511449147f1b0

    94e8882d820969ddc83f24f4cbe1515a886da4ea, and maybe also 2574b7e177ef045, may be worth backporting.

    (If any review feedback related to the unit tests, happy to address them in test coverage follow-ups.)

  66. DrahtBot requested review from mzumsande on Oct 30, 2023
  67. in src/test/net_peer_connection_tests.cpp:22 in fc1e9e4780 outdated
    17+#include <test/util/setup_common.h>
    18+#include <tinyformat.h>
    19+#include <util/chaintype.h>
    20+#include <version.h>
    21+
    22+#ifdef WIN32
    


    fanquake commented at 11:44 am on October 31, 2023:
    This should be including compat.h, rather than introducing the same networking #ifdef here?

    sr-gi commented at 2:51 pm on October 31, 2023:
    That works for me. @jonatack if you rebase it I’ll cherry-pick it again.

    jonatack commented at 5:32 pm on October 31, 2023:
    Thanks, I’ve repushed #28749 with the update.

    sr-gi commented at 5:38 pm on October 31, 2023:
    Rebased
  68. Create net_peer_connection unit tests
    for initial partial unit test coverage of these CConnman class methods:
    
    - AddNode()
    - ConnectNode()
    - GetAddedNodeInfo()
    - AlreadyConnectedToAddress()
    - ThreadOpenAddedConnections()
    
    and of the GetAddedNodeInfo() call in RPC addnode.
    0420f99f42
  69. sr-gi force-pushed on Oct 31, 2023
  70. jonatack commented at 5:59 pm on October 31, 2023: member
    re-ACK 0420f99f429ce2382057e101859067f40de47be0
  71. kashifs commented at 11:16 pm on November 2, 2023: contributor
    tACK 0420f9
  72. mzumsande commented at 5:50 pm on November 3, 2023: contributor
    Tested ACK 0420f99f429ce2382057e101859067f40de47be0
  73. sr-gi commented at 7:22 pm on November 3, 2023: member

    tACK 0420f9

    Thanks for reviewing! Would you mind sharing what did you test and how? @vasild would you mind re-acking this when you have some time?

  74. kashifs commented at 6:12 pm on November 4, 2023: contributor

    tACK 0420f9

    Thanks for reviewing! Would you mind sharing what did you test and how?

    @vasild would you mind re-acking this when you have some time?

    I pulled the branch, compiled from source on my Mac, and ran:

    ./src/test/test_bitcoin -t net_peer_connection_tests -l message -- -printtoconsole=1

    and

    ./src/test/test_bitcoin -t net_peer_connection_tests -l all -- -printtoconsole=1

    I also pored over every line of modified code in order to better understand the design.

    Is there anything else that I should do that might be helpful?

  75. sr-gi commented at 9:57 pm on November 4, 2023: member

    tACK 0420f9

    Thanks for reviewing! Would you mind sharing what did you test and how?

    @vasild would you mind re-acking this when you have some time?

    I pulled the branch, compiled from source on my Mac, and ran:

    ./src/test/test_bitcoin -t net_peer_connection_tests -l message -- -printtoconsole=1

    and

    ./src/test/test_bitcoin -t net_peer_connection_tests -l all -- -printtoconsole=1

    I also pored over every line of modified code in order to better understand the design.

    Is there anything else that I should do that might be helpful?

    Nope, that’s good. Thanks again for reviewing and testing.

  76. glozow merged this on Nov 8, 2023
  77. glozow closed this on Nov 8, 2023

  78. in src/test/util/net.cpp:114 in 0420f99f42
    109+    node->SetCommonVersion(PROTOCOL_VERSION);
    110+    peerman.InitializeNode(*node, ServiceFlags(NODE_NETWORK | NODE_WITNESS));
    111+    node->fSuccessfullyConnected = true;
    112+    AddTestNode(*node);
    113+    return node;
    114+}
    


    vasild commented at 12:40 pm on November 14, 2023:
    nit, naming: maybe a different name would have been better than ConnectNodePublic(), given that it contains extra logic on top of the private ConnectNode().

    jonatack commented at 4:53 pm on November 14, 2023:
    True. I’m touching that code for #28248, if you have a naming suggestion.

    vasild commented at 10:07 am on November 15, 2023:
    Dunno, maybe ConnectNodeAndInitialize()?
  79. vasild commented at 12:43 pm on November 14, 2023: contributor
    ACK 0420f99f429ce2382057e101859067f40de47be0
  80. achow101 referenced this in commit 2eff198f49 on Apr 25, 2024
  81. luke-jr referenced this in commit 7b636bff6f on May 13, 2024
  82. luke-jr referenced this in commit 3559fd6d08 on May 13, 2024
  83. luke-jr referenced this in commit 90300177ec on May 13, 2024
  84. luke-jr referenced this in commit 19ab1829af on May 13, 2024
  85. luke-jr referenced this in commit c50a2458ce on May 13, 2024
  86. kwvg referenced this in commit b7320e766a on Oct 26, 2024
  87. kwvg referenced this in commit 6d2d38b27a on Oct 26, 2024
  88. kwvg referenced this in commit 7ebad1f9a7 on Oct 26, 2024
  89. kwvg referenced this in commit 6cf206ca0e on Oct 26, 2024
  90. PastaPastaPasta referenced this in commit 25c3355053 on Oct 27, 2024
  91. bitcoin locked this on Nov 14, 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 03:12 UTC

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