net: Decouple CConnman::GetAddresses from CNode #33900

1. 
   waketraindev commented at 1:40 pm on November 18, 2025: contributor

   Decouple CConnman::GetAddresses from CNode which was used only for getting network_key
2. net: Decouple `CConnman::GetAddresses` from `CNode` 8325aed843
3. DrahtBot added the label P2P on Nov 18, 2025
4. 
   DrahtBot commented at 1:40 pm on November 18, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33900.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

5. 
   maflcko commented at 2:13 pm on November 18, 2025: member

   it probably makes sense to check if those refactors are in line with the Net Split WG (ref https://achow101.com/ircmeetings/2025/bitcoin-core-dev.2025-11-13_16_00.log.html and the coredev meeting notes).

   Otherwise, this may be touched again soon after.

6. 
   waketraindev commented at 3:10 pm on November 18, 2025: contributor

   it probably makes sense to check if those refactors are in line with the Net Split WG (ref https://achow101.com/ircmeetings/2025/bitcoin-core-dev.2025-11-13_16_00.log.html and the coredev meeting notes).

   Otherwise, this may be touched again soon after.

   it probably makes sense to check if those refactors are in line with the Net Split WG (ref https://achow101.com/ircmeetings/2025/bitcoin-core-dev.2025-11-13_16_00.log.html and the coredev meeting notes).

   Otherwise, this may be touched again soon after.

   They should be “in line” looking over @theuni’s net_processing_no_cnode* branches nothing seems to conflict.

   If there are any concerns or things I might’ve missed, let me know.

    0std::vector<CAddress> CConnman::GetAddresses(NodeId id, size_t max_addresses, size_t max_pct)
    1{
    2    std::shared_ptr<CNode> requestor;
    3    {
    4    LOCK(m_nodes_mutex);
    5        auto it = std::find_if(m_nodes.begin(), m_nodes.end(), [&id](const auto& node) { return node->GetId() == id; });
    6        requestor = *it;
    7    }
    8    return GetAddresses(*requestor, max_addresses, max_pct);
    9}
   10
   11std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addresses, size_t max_pct)
   12{
   

7. 
   waketraindev commented at 3:15 pm on November 18, 2025: contributor

   @theuni is this refactor okay with you?
8. in src/net.h:1223 in 8325aed843

   1219@@ -1220,14 +1220,14 @@ class CConnman
   1220      * A trusted caller (e.g. from RPC or a peer with addr permission) can use
   1221      * @ref GetAddressesUnsafe to avoid using the cache.
   1222      *
   1223-     * @param[in] requestor      The requesting peer. Used to key the cache to prevent privacy leaks.
   1224+     * @param[in] network_key    Per peer network key. Used to key the cache to prevent privacy leaks.
   

   ---

   ---

   

   fjahr commented at 7:03 pm on November 19, 2025:

   I think the comment is slightly less helpful with the current version and the renaming of the variable otherwise.

   0     * [@param](/bitcoin-bitcoin/contributor/param/)[in] network_key    Network key of the requesting peer. Used to key the cache to prevent privacy leaks.
   

   ---

   

   fjahr commented at 7:04 pm on November 19, 2025:

   Alternatively the variable could be named requestor_key or something similar to transport this information.
9. 
   fjahr commented at 10:03 pm on November 19, 2025: contributor

   I’m neutral on this change. I don’t think the decoupling is very effective because conceptually the function is still tied to a “requesting peer”/user/attacker looking at the comments around that function. And pushing the access of the m_network_key to the call site doesn’t really make the API nicer in my view. It would be different if the key was aready available at the call sites alone but that doesn’t seem to be the case.
10. 
    waketraindev commented at 10:43 pm on November 19, 2025: contributor

    Closed. Going to let Net Split WG handle this one :) thanks for the comments.
11. waketraindev closed this on Nov 19, 2025

    ---

12. waketraindev deleted the branch on Nov 19, 2025

Contributors
waketraindev DrahtBot maflcko fjahr

Labels
P2P