p2p: rename GetAddresses -> GetAddressesUnsafe #32994

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.

DrahtBot added the label Docs on Jul 16, 2025

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.

Type	Reviewers
ACK	stickies-v, l0rinc, brunoerg, luisschwab, theStack, mzumsande

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.

danielabrozzoni force-pushed on Jul 16, 2025

danielabrozzoni renamed this:
~~doc: clarify AddrMan::GetAddresses documentation~~
doc: clarify GetAddresses documentation
on Jul 16, 2025

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.

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.

danielabrozzoni force-pushed on Jul 18, 2025

danielabrozzoni renamed this:
~~doc: clarify GetAddresses documentation~~
p2p: rename GetAddresses -> GetAddressesUncached
on Jul 18, 2025

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!

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

diff --git a/src/net.h b/src/net.h
index 61b25bd47d..eb08d9de05 100644
--- a/src/net.h
+++ b/src/net.h
@@ -1169,9 +1169,10 @@ public:
 
     // Addrman functions
     /**
-     * Return all or many randomly selected addresses, optionally by network.
-     * This function does not use an address response cache and should only be
-     * used in trusted contexts.
+     * Return randomly selected addresses. This function does not use the address response cache and
+     * should only be used in trusted contexts.
+     *
+     * An untrusted caller (e.g. from p2p) should instead use [@ref](/bitcoin-bitcoin/contributor/ref/) GetAddresses to use the cache.
      *
      * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_addresses  Maximum number of addresses to return (0 = all).
      * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_pct        Maximum percentage of addresses to return (0 = all). Value must be from 0 to 100.
@@ -1180,12 +1181,18 @@ public:
      */
     std::vector<CAddress> GetAddressesUncached(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const;
     /**
-     * Return all or many addresses, using the address response cache.
-     * Cache is used to minimize topology leaks, so it should be used for all
-     * non-trusted calls, for example, p2p.
+     * Return addresses from the per-requestor cache. If no cache entry exists, it is populated with
+     * randomly selected addresses. This function can be used in untrusted contexts.
+     *
+     * A trusted caller (e.g. from RPC or a peer with addr permission) can use
+     * [@ref](/bitcoin-bitcoin/contributor/ref/) GetAddressesUncached to avoid using the cache.
      *
-     * A non-malicious call (from RPC or a peer with addr permission) should
-     * instead use GetAddressesUncached to avoid using the cache.
+     * [@param](/bitcoin-bitcoin/contributor/param/)[in] requestor      The requesting peer. Used to key the cache to prevent privacy leaks.
+     * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_addresses  Maximum number of addresses to return (0 = all). Ignored when cache
+     *                           already contains an entry for requestor.
+     * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_pct        Maximum percentage of addresses to return (0 = all). Value must be
+     *                           from 0 to 100. Ignored when cache already contains an entry for
+     *                           requestor.
      */
     std::vector<CAddress> GetAddresses(CNode& requestor, size_t max_addresses, size_t max_pct);

</details>

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

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

l0rinc approved

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.

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

    const std::vector<CAddress> v6_addrs{GetAddressesUncached(/*max_addresses=*/0, /*max_pct=*/0, Network::NET_IPV6, /*filtered=*/false)};

danielabrozzoni force-pushed on Jul 21, 2025

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)

stickies-v approved

stickies-v commented at 3:11 PM on July 21, 2025: contributor

ACK df65b527f00cbf9fd6675bf22bb1285bfce24f66

DrahtBot requested review from brunoerg on Jul 21, 2025

DrahtBot requested review from l0rinc on Jul 21, 2025

danielabrozzoni renamed this:
~~p2p: rename GetAddresses -> GetAddressesUncached~~
p2p: rename GetAddresses -> GetAddressesUnsafe
on Jul 22, 2025

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

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

danielabrozzoni force-pushed on Jul 22, 2025

danielabrozzoni commented at 12:31 PM on July 22, 2025: contributor

last push (1cb23997033c395d9ecd7bf2f54787b134485f41):

rename GetAddressesUncached -> GetAddressesUnsafe
improve documentation

stickies-v commented at 1:47 PM on July 22, 2025: contributor

re-ACK 1cb23997033c395d9ecd7bf2f54787b134485f41

l0rinc commented at 5:53 PM on July 22, 2025: contributor

ACK 1cb23997033c395d9ecd7bf2f54787b134485f41

brunoerg approved

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.

luisschwab commented at 5:23 AM on July 23, 2025: contributor

ACK 1cb23997033c395d9ecd7bf2f54787b134485f41

theStack approved

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

fanquake commented at 10:10 AM on July 25, 2025: member

cc @mzumsande

mzumsande commented at 3:11 PM on July 25, 2025: contributor

Code review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41

fanquake merged this on Jul 25, 2025

fanquake closed this on Jul 25, 2025