Remove CNode dependency for local address functions and introduce LocalAddressManager #35040

This is part of the net/net_processing split project.

This PR does not change any current behavior, though it does highlight some areas for improvement.

Background

All of this trouble is necessary for a single purpose: advertising our own network address to peers. This currently happens in PeerManagerImpl::MaybeSendAddr.

In order to advertise our network address, we need to know what to advertise. We make our best guess based on these criteria (ordered from worst to best):

enum
{
    LOCAL_NONE,   // unknown
    LOCAL_IF,     // address a local interface listens on
    LOCAL_BIND,   // address explicit bound to
    LOCAL_MAPPED, // address reported by PCP
    LOCAL_MANUAL, // address explicitly specified (-externalip=)

    LOCAL_MAX
};

Because we may have more than one publicly visible network address (ipv4, ipv6, tor, etc), we do our best to advertise an address to peers that they would be able to reach. This avoids (for example) disclosing our public ipv4 address to a node connected to our onion service. Accordingly, in order to determine the best address for a given peer which maybe have connected through a proxy, in addition to their address, the LocalAddressManager must also know the network through which they are connected.

We have one last source of data for what our public network address may be: each peer (as part of the version handshake) tells us what they think our address is. This is untrustworthy data, so it is not added to our list of possible addresses for consideration for other peers. We currently only consider advertising it back to a peer (in GetLocalAddrForPeer) if it is routable AND either we don't have a good guess OR randomly. The LocalAddressManager introduced in this PR is unrelated to that logic.

What's happening in this PR (and how to review)

• Because local address management is unrelated to CConnman, its reliance on them (via CNode) is removed.
• LocalAddressManager is created in order to house the local network address functionality.
• All functionality is still global in scope, though LocalAddressManager is now instantiated when possible for testing and fuzzing
• There should be no behavioral changes

Reviewers should be able to verify that there are no behavioral changes, and that the handling of local address management is now contained and more straightforward.

Next steps

I stopped at this point because the changes, while straightforward and boring, have already added up. The main obvious change that remains is to remove the CNode dependency from GetLocalAddrForPeer.

Beyond that:

• Store addrLocal in Peer rather than CNode as it is an artifact of the message layer, not the networking layer
• Move the GetLocalAddrForPeer logic to PeerMan for the same reason.
• Instantiate LocalAddressManager with options to avoid its use of globals where possible (fListen, fDiscover, fLogIPs).
• Potentially remove g_localaddressman and store an instance in NodeContext instead.
• From a quick glance, I think that fuzzing is currently broken because g_reachable_nets is not populated.

Potential behavioral improvements

• The (unchanged) logic in LocalAddressManager::Get uses a peer's IP and the network through which they're connected to decide which of our addresses to advertise. This address doesn't have much meaning when the peer is connected through an outbound proxy or is inbound via tor.
• Only advertise an address to a peer of the same network through which they're connected to us. OR codify and add tests for what the exceptions should be. The current logic in LocalAddressManager::Get and GetLocalAddrForPeer is brittle and difficult to reason about.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34806 (refactor: logging: Various API improvements by ajtowns)
• #34213 (net: preserve anchors when network is disabled by brunoerg)
• #34028 (p2p: Prevent integer overflow in LocalServiceInfo::nScore by codeabysss)
• #32394 (net: make m_nodes_mutex non-recursive by vasild)
• #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
• #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task 32 bit ARM: https://github.com/bitcoin/bitcoin/actions/runs/24201296439/job/70645354883} _{LLM reason (✨ experimental): CI failed due to a compilation error treated as fatal by -Werror: netaddress.h triggered -Werror=parentheses for return net == NET_ONION | net == NET_I2P;.}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

Typo in title: "LocalAddressManger" :)

Typo in title: "LocalAddressManger" :)

Clearly this was supposed to be LocalAddressMonger, peddler of the finest Local Addresses...

@maflcko I suspect you'll want to weigh in here on namespaces and/or file locations?

@maflcko I suspect you'll want to weigh in here on namespaces and/or file locations?

I think this one is easy: You are adding a new file to bitcoin_node lib, so it should be in the node folder, like the other p2p stuff, like node/eviction.cpp, or node/txorphanage.cpp.

The fix for the circular dependency problem here is non-trivial. Converting to draft while I investigate.

This open attake quantize !!!!