net: Decouple `CConnman::GetAddresses` from `CNode` #33900

pull waketraindev wants to merge 1 commits into bitcoin:master from waketraindev:prs/2025-11-net-getaddresses-decouple changing 4 files +6 −7

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

Decouple CConnman::GetAddresses from CNode which was used only for getting network_key
net: Decouple `CConnman::GetAddresses` from `CNode` 8325aed843
DrahtBot added the label P2P on Nov 18, 2025
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.
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.
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.
```
std::vector<CAddress> CConnman::GetAddresses(NodeId id, size_t max_addresses, size_t max_pct)
{
    std::shared_ptr<CNode> requestor;
    {
    LOCK(m_nodes_mutex);
        auto it = std::find_if(m_nodes.begin(), m_nodes.end(), [&id](const auto& node) { return node->GetId() == id; });
        requestor = *it;
    }
    return GetAddresses(*requestor, max_addresses, max_pct);
}

std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addresses, size_t max_pct)
{
```
waketraindev commented at 3:15 PM on November 18, 2025: contributor

@theuni is this refactor okay with you?

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.

     * [@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.

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.
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.
waketraindev closed this on Nov 19, 2025
waketraindev deleted the branch on Nov 19, 2025

Contributors

Labels

net: Decouple `CConnman::GetAddresses` from `CNode` #33900

Code Coverage & Benchmarks

Reviews