p2p: rename GetAddresses -> GetAddressesUnsafe #32994

pull danielabrozzoni wants to merge 2 commits into bitcoin:master from danielabrozzoni:docs/getaddresses changing 5 files +24 −13
  1. danielabrozzoni commented at 1:43 pm on July 16, 2025: contributor

    Rename GetAddresses to GetAddressesUnsafe to make it clearer that this function should only be used in trusted contexts. This helps avoid accidental privacy leaks by preventing the uncached version from being used in non-trusted scenarios, like P2P.

    Additionally, better reflect in the documentation that the two methods should be used in different contexts. Also update the outdated “call the function without a parameter” phrasing in the cached version. This wording was accurate when the cache was introduced in #18991, but became outdated after later commits (f26502e9fc8a669b30717525597e3f468eaecf79, 81b00f87800f40cb14f2131ff27668bd2bb9e551) added parameters to each function, and the previous commit changed the function naming completely.

  2. DrahtBot added the label Docs on Jul 16, 2025
  3. DrahtBot commented at 1:43 pm on July 16, 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/32994.

    Reviews

    See the guideline for information on the review process.

    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:

    • #31650 (refactor: Avoid copies by using const references or by move-construction by maflcko)

    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.

  4. danielabrozzoni force-pushed on Jul 16, 2025
  5. danielabrozzoni renamed this:
    doc: clarify AddrMan::GetAddresses documentation
    doc: clarify GetAddresses documentation
    on Jul 16, 2025
  6. stickies-v commented at 3:09 pm on July 16, 2025: contributor
    Concept ACK. I would strongly prefer to go further and rename the trusted function to GetAddressesUncached. Overloads that behave vastly differently and can lead to dangerous behaviour is a big red flag for me.
  7. brunoerg commented at 7:50 pm on July 17, 2025: contributor
    I agree with @stickies-v. It’s weird to have two functions with the same name but with a relevant behavior difference, renaming one of the functions seems appropriate. In any case, Concept ACK about improving the documentation.
  8. danielabrozzoni force-pushed on Jul 18, 2025
  9. danielabrozzoni force-pushed on Jul 18, 2025
  10. danielabrozzoni renamed this:
    doc: clarify GetAddresses documentation
    p2p: rename GetAddresses -> GetAddressesUncached
    on Jul 18, 2025
  11. danielabrozzoni commented at 2:12 pm on July 18, 2025: contributor

    @stickies-v @brunoerg thank you for the suggestion!

    I agree, I think renaming makes the code clearer and reduces the chances of accidentally using the uncached version and leaking addrman addresses :)

    I updated the PR title/description and pushed a commit to rename GetAddresses to GetAddressesUncached!

  12. in src/net.h:1173 in 7c7ab17c8d outdated
    1169@@ -1170,18 +1170,22 @@ class CConnman
    1170     // Addrman functions
    1171     /**
    1172      * Return all or many randomly selected addresses, optionally by network.
    1173+     * This function does not use an address response cache and should only be
    


    l0rinc commented at 5:00 pm on July 20, 2025:
    Even though the PR is quite simple, I’d separate the comment-changing-commits from the refactoring commits. I guess it may not make a lot of sense doing the rename with a scripted diff since it might confuse the two similarly named methods.

    stickies-v commented at 3:10 pm on July 21, 2025:

    nit: documentation could be made more consistent, as per e.g. the below diff

     0diff --git a/src/net.h b/src/net.h
     1index 61b25bd47d..eb08d9de05 100644
     2--- a/src/net.h
     3+++ b/src/net.h
     4@@ -1169,9 +1169,10 @@ public:
     5 
     6     // Addrman functions
     7     /**
     8-     * Return all or many randomly selected addresses, optionally by network.
     9-     * This function does not use an address response cache and should only be
    10-     * used in trusted contexts.
    11+     * Return randomly selected addresses. This function does not use the address response cache and
    12+     * should only be used in trusted contexts.
    13+     *
    14+     * An untrusted caller (e.g. from p2p) should instead use [@ref](/bitcoin-bitcoin/contributor/ref/) GetAddresses to use the cache.
    15      *
    16      * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_addresses  Maximum number of addresses to return (0 = all).
    17      * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_pct        Maximum percentage of addresses to return (0 = all). Value must be from 0 to 100.
    18@@ -1180,12 +1181,18 @@ public:
    19      */
    20     std::vector<CAddress> GetAddressesUncached(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const;
    21     /**
    22-     * Return all or many addresses, using the address response cache.
    23-     * Cache is used to minimize topology leaks, so it should be used for all
    24-     * non-trusted calls, for example, p2p.
    25+     * Return addresses from the per-requestor cache. If no cache entry exists, it is populated with
    26+     * randomly selected addresses. This function can be used in untrusted contexts.
    27+     *
    28+     * A trusted caller (e.g. from RPC or a peer with addr permission) can use
    29+     * [@ref](/bitcoin-bitcoin/contributor/ref/) GetAddressesUncached to avoid using the cache.
    30      *
    31-     * A non-malicious call (from RPC or a peer with addr permission) should
    32-     * instead use GetAddressesUncached to avoid using the cache.
    33+     * [@param](/bitcoin-bitcoin/contributor/param/)[in] requestor      The requesting peer. Used to key the cache to prevent privacy leaks.
    34+     * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_addresses  Maximum number of addresses to return (0 = all). Ignored when cache
    35+     *                           already contains an entry for requestor.
    36+     * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_pct        Maximum percentage of addresses to return (0 = all). Value must be
    37+     *                           from 0 to 100. Ignored when cache already contains an entry for
    38+     *                           requestor.
    39      */
    40     std::vector<CAddress> GetAddresses(CNode& requestor, size_t max_addresses, size_t max_pct);
    41 
    

    danielabrozzoni commented at 12:22 pm on July 22, 2025:
    This is nice, thank you! I have included it and added you as a co-author of the last commit
  13. in src/net.cpp:3499 in 7c7ab17c8d outdated
    3495@@ -3496,7 +3496,7 @@ CConnman::~CConnman()
    3496     Stop();
    3497 }
    3498 
    3499-std::vector<CAddress> CConnman::GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const
    3500+std::vector<CAddress> CConnman::GetAddressesUncached(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const
    


    l0rinc commented at 5:03 pm on July 20, 2025:
    I’m wondering if it makes more sense to signal the lack of something here (which is only relevant because another similar method exists) instead of the additional functionality in the pair - i.e. leave this and rename the other to GetAddressesCached, since that’s the one that has an additional functionality, not this one.

    stickies-v commented at 10:13 am on July 21, 2025:

    I’m wondering if it makes more sense to signal the lack of something … instead of the additional functionality in the pair

    I considered that too, but I don’t think that’s better. The “Unchached” one is unsafe, and for that it should stand out. My initial thought was to name it GetAddressesUnsafe or GetAddressesInternal, but those names carry less meaning, so I eventually landed on just describing what it does, which I think at least gives people somewhat familiar with the code a good enough clue if something is wrong. Using caching is the normal modus operandus, so I don’t think that needs to be especially reflected in the name.


    danielabrozzoni commented at 12:41 pm on July 21, 2025:

    I also don’t love the GetAddresses/GetAddressesUncached naming either, but I agree with stickies that it’s important that the unsafe version stands out.

    What if we instead renamed them to GetAddressesCached / GetAddressesUncached? That way, there’s no ambiguity about what each function does, and the naming feels a bit more consistent.


    stickies-v commented at 3:14 pm on July 21, 2025:
    GetAddressesForPeer might also make sense. I don’t really care about it too much, I think all the above options are all fine and the most important thing for me is that they’re not the same, which is addressed, so would personally prefer to pick one thing and then move on.

    l0rinc commented at 4:49 pm on July 21, 2025:
    Actually, first seeing Uncached in a name implied to me that it’s safer, since it doesn’t have side-effects, doesn’t pollute the context, it cannot blow up, etc - I don’t think it achieves scaring people away. In other cases the suffix was “Unsafe” if it’s important to signal that the user should pay extra attention.

    danielabrozzoni commented at 12:11 pm on July 22, 2025:

    Fair enough, I think Uncached scares whoever knows about the getaddr cache, but if you don’t have that context it’s not immediately obvious.

    I changed it to GetAddresses/GetAddressesUnsafe :)

  14. l0rinc approved
  15. l0rinc commented at 5:06 pm on July 20, 2025: contributor
    Concept ACK, please consider splitting different change types to separate commits and adding extra qualifications to the methods that does the extra work.
  16. in src/net.cpp:3968 in 7c7ab17c8d outdated
    3963@@ -3964,8 +3964,8 @@ void CConnman::PerformReconnections()
    3964 
    3965 void CConnman::ASMapHealthCheck()
    3966 {
    3967-    const std::vector<CAddress> v4_addrs{GetAddresses(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV4, /*filtered=*/ false)};
    3968-    const std::vector<CAddress> v6_addrs{GetAddresses(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV6, /*filtered=*/ false)};
    3969+    const std::vector<CAddress> v4_addrs{GetAddressesUncached(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV4, /*filtered=*/ false)};
    3970+    const std::vector<CAddress> v6_addrs{GetAddressesUncached(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV6, /*filtered=*/ false)};
    


    stickies-v commented at 10:16 am on July 21, 2025:

    clang-format nit

    0    const std::vector<CAddress> v6_addrs{GetAddressesUncached(/*max_addresses=*/0, /*max_pct=*/0, Network::NET_IPV6, /*filtered=*/false)};
    
  17. danielabrozzoni force-pushed on Jul 21, 2025
  18. danielabrozzoni commented at 2:05 pm on July 21, 2025: contributor

    In my last push (df65b527f00cbf9fd6675bf22bb1285bfce24f66):

    • split changes into two commits (one for rename, one for docs)
  19. stickies-v approved
  20. stickies-v commented at 3:11 pm on July 21, 2025: contributor
    ACK df65b527f00cbf9fd6675bf22bb1285bfce24f66
  21. DrahtBot requested review from brunoerg on Jul 21, 2025
  22. DrahtBot requested review from l0rinc on Jul 21, 2025
  23. danielabrozzoni renamed this:
    p2p: rename GetAddresses -> GetAddressesUncached
    p2p: rename GetAddresses -> GetAddressesUnsafe
    on Jul 22, 2025
  24. p2p: rename GetAddresses -> GetAddressesUnsafe
    Rename GetAddresses to GetAddressesUnsafe to make it clearer that this
    function should only be used in trusted contexts. This helps avoid
    accidental privacy leaks by preventing the uncached version from being
    used in non-trusted scenarios, like P2P.
    e5a7dfd79f
  25. doc: clarify the GetAddresses/GetAddressesUnsafe documentation
    Better reflect in the documentation that the two methods should be
    used in different contexts.
    Also update the outdated "call the function without a parameter" phrasing
    in the cached version. This wording was accurate when the cache was
    introduced in #18991, but became outdated after later commits
    (f26502e9fc8a669b30717525597e3f468eaecf79,
    81b00f87800f40cb14f2131ff27668bd2bb9e551) added parameters to each
    function, and the previous commit changed the function naming completely.
    
    Co-Authored-By: stickies-v <stickies-v@protonmail.com>
    1cb2399703
  26. danielabrozzoni force-pushed on Jul 22, 2025
  27. danielabrozzoni commented at 12:31 pm on July 22, 2025: contributor

    last push (1cb23997033c395d9ecd7bf2f54787b134485f41):

    • rename GetAddressesUncached -> GetAddressesUnsafe
    • improve documentation
  28. stickies-v commented at 1:47 pm on July 22, 2025: contributor
    re-ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
  29. l0rinc commented at 5:53 pm on July 22, 2025: contributor
    ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
  30. brunoerg approved
  31. brunoerg commented at 6:30 pm on July 22, 2025: contributor

    ACK 1cb23997033c395d9ecd7bf2f54787b134485f41

    GetAddresses{Unsafe} sounds good and it’s better than simply just changing the documentation.

  32. luisschwab commented at 5:23 am on July 23, 2025: contributor
    ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
  33. theStack approved
  34. theStack commented at 11:20 pm on July 24, 2025: contributor

    Code-review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41

    Nice documentation improvements, happy to see the method rename for improved clarity (fwiw, I’m generally not a huge fan of function overloading and think we tend to overuse it; it’s often just confusing and makes it harder to find call-sites for a specific function, especially across multiple commits, e.g. for investigating past changes via git log -S).

  35. fanquake commented at 10:10 am on July 25, 2025: member
  36. mzumsande commented at 3:11 pm on July 25, 2025: contributor
    Code review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
  37. fanquake merged this on Jul 25, 2025
  38. fanquake closed this on Jul 25, 2025


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: 2025-08-02 12:13 UTC

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