net_processing: don't modify addrman for private broadcast connections

vasild commented at 12:57 pm on April 8, 2026: contributor

It is best if the internal addrman database is not modified with information coming from private broadcast connections because that information can potentially later be sent via other connections.

instagibbs suggested this change, thank you!

DrahtBot commented at 12:57 pm on April 8, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, instagibbs, andrewtoth, danielabrozzoni, achow101
Stale ACK	optout21

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35027 (net: use -bind address for outgoing connections by 8144225309)
#34538 (net: advertise -externalip addresses by willcl-ark)

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.

instagibbs commented at 1:10 pm on April 8, 2026: member

Note that we already guard against this in FinalizeNode https://github.com/bitcoin/bitcoin/blob/d2844c6a4ffa39ec365009777cd87caf1fbab867/src/net_processing.cpp#L1732 which is how this was found

instagibbs commented at 1:12 pm on April 8, 2026: member

self ACK 687bbde1f87ddf10521eea096bf539ddcce9b84d anyways

vasild commented at 4:18 am on April 9, 2026: contributor

@andrewtoth, @l0rinc, @optout21, you might be interested in reviewing this. @achow101, @fanquake, @sedited, IMO this should be in 31.0.

andrewtoth commented at 4:21 am on April 9, 2026: contributor

ACK 687bbde1f87ddf10521eea096bf539ddcce9b84d

Sorry meant to ACK earlier.

achow101 commented at 5:37 am on April 9, 2026: member

Unconvinced that this is necessary for 31.0, and we’d have to do another RC. Adding it for 31.1.

achow101 added this to the milestone 31.1 on Apr 9, 2026

optout21 commented at 9:02 am on April 9, 2026: contributor

crACK 687bbde1f87ddf10521eea096bf539ddcce9b84d

Change assessment: This is a small change related to private broadcast connections: in PeerManagerImpl::ProcessMessage(), for a VERSION message received from a peer, whom we connected to as private broadcast connection, addrman is not updated with the services of the peer. The reason is information hygiene: not to store this information about private connections, as this information may get sent to other peers later. The change is limited to this one condition, and it looks correct.

Some observations:

Is it possible to easily test the effect of this change, in a unit test? A test would be desirable, to safeguard against later degradation.

Could there be other places where the same logic could be applied? I have identified one location, but I’m not sure whether the state saved there is relevant, and whether it’s striclty needed for correct functioning.

https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.cpp#L3786

0        if (!pfrom.IsInboundConn()) {
1            // For non-inbound connections, we update the addrman to record
2            // ...
3            m_addrman.Good(pfrom.addr);
4        }

Note: The changed location is the only place where SetServices() gets called.

vasild commented at 10:08 am on April 9, 2026: contributor

I have identified one location …

This will not be reached for private broadcast connections because we have stopped the processing for those connections earlier:

https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.cpp#L3708

l0rinc commented at 10:26 am on April 9, 2026: contributor

Is it possible to easily test the effect of this change, in a unit test? A test would be desirable, to safeguard against later degradation.

I had the same concern, we have identified a missing coverage, maybe we could add a dedicated test - or extend an existing one with a new assert.

 0diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
 1--- a/src/test/net_tests.cpp	(revision d2844c6a4ffa39ec365009777cd87caf1fbab867)
 2+++ b/src/test/net_tests.cpp	(date 1775729451959)
 3@@ -2,6 +2,7 @@
 4 // Distributed under the MIT software license, see the accompanying
 5 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 6 
 7+#include <addrman.h>
 8 #include <chainparams.h>
 9 #include <clientversion.h>
10 #include <common/args.h>
11@@ -1006,6 +1007,21 @@
12     RemoveLocal(addr_cjdns);
13 }
14 
15+BOOST_AUTO_TEST_CASE(private_broadcast_version_does_not_update_addrman_services)
16+{
17+    LOCK(NetEventsInterface::g_msgproc_mutex);
18+
19+    CAddress addr{Lookup("1.2.3.4", 8333, false).value(), NODE_NONE, Now<NodeSeconds>()};
20+    BOOST_REQUIRE(m_node.addrman->Add({addr}, Lookup("2.3.4.5", 8333, false).value()));
21+    CNode peer{0, nullptr, addr, 0, 0, {}, "", ConnectionType::PRIVATE_BROADCAST, false, 0};
22+
23+    auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
24+    connman.Handshake(peer, false, ServiceFlags(NODE_NETWORK | NODE_WITNESS), ServiceFlags(NODE_NETWORK | NODE_WITNESS), PROTOCOL_VERSION, true);
25+
26+    BOOST_CHECK_EQUAL(m_node.addrman->Select().first.nServices, NODE_NONE);
27+    m_node.peerman->FinalizeNode(peer);
28+}
29+
30 namespace {
31 
32 CKey GenerateRandomTestKey(FastRandomContext& rng) noexcept

optout21 commented at 10:27 am on April 9, 2026: contributor

This will not be reached for private broadcast connections

Correct. Thanks for looking into it.

net_processing: don't modify addrman for private broadcast connections

It is best if the internal addrman database is not modified with
information coming from private broadcast connections because that
information can potentially later be sent via other connections.

Co-authored-by: Greg Sanders <gsanders87@gmail.com>
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

1ed1a12402

vasild force-pushed on Apr 9, 2026

vasild commented at 12:58 pm on April 9, 2026: contributor

687bbde1f87ddf10521eea096bf539ddcce9b84d...1ed1a124028aa6783ecdd8c82083a3d7a6a16e53: added the test from above, thanks!

l0rinc commented at 1:03 pm on April 9, 2026: contributor

code review ACK 1ed1a124028aa6783ecdd8c82083a3d7a6a16e53

DrahtBot requested review from andrewtoth on Apr 9, 2026

DrahtBot requested review from instagibbs on Apr 9, 2026

DrahtBot requested review from optout21 on Apr 9, 2026

instagibbs commented at 1:11 pm on April 9, 2026: member

ACK 1ed1a124028aa6783ecdd8c82083a3d7a6a16e53

test fails as expected without fix

andrewtoth approved

andrewtoth commented at 1:24 pm on April 9, 2026: contributor

ACK 1ed1a124028aa6783ecdd8c82083a3d7a6a16e53

danielabrozzoni approved

danielabrozzoni commented at 3:33 pm on April 9, 2026: member

tACK 1ed1a124028aa6783ecdd8c82083a3d7a6a16e53

I checked if we modified addrman in any other way during private broadcast. I think it only happens in one other place: in CConnman::ConnectNode we might call Attempt, but this is not problematic as we never gossip any of m_last_try, m_last_count_attempt, nAttempts.

https://github.com/bitcoin/bitcoin/blob/141fbe4d530b51345e62dee1348e82d8a0406ffc/src/net.cpp#L493-L497

achow101 commented at 8:19 pm on April 9, 2026: member

ACK 1ed1a124028aa6783ecdd8c82083a3d7a6a16e53

achow101 merged this on Apr 9, 2026

achow101 closed this on Apr 9, 2026

fanquake added the label Needs Backport (31.x) on Apr 10, 2026

fanquake referenced this in commit 6956cdb9ca on Apr 10, 2026

fanquake removed the label Needs Backport (31.x) on Apr 10, 2026

fanquake commented at 8:50 am on April 10, 2026: member

Backported for 31.x in #35046.

net_processing: don’t modify addrman for private broadcast connections #35032

Reviews

Conflicts