net: don't discourage private broadcast peers #36312

andrewtoth commented at 1:30 AM on September 22, 2026: contributor

Discouraging a private broadcast connection may be observable. Prevent this by keeping private broadcast connections outside normal discouragement handling. Misbehaving private broadcast peers are still disconnected.

DrahtBot added the label P2P on Sep 22, 2026

DrahtBot commented at 1:30 AM on September 22, 2026: 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/36312.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	instagibbs, ViniciusCestarii, vasild, mzumsande, danielabrozzoni
Concept ACK	optout21

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

ViniciusCestarii commented at 2:20 PM on September 22, 2026: contributor

Could the motivation go in the commit message? If I'm reading it right, the concern is that discouragement is observable?

vasild approved

vasild commented at 5:29 PM on September 22, 2026: contributor

ACK b1c8ce8d3d0cc5331497ac5fb8e94888530768ab

If I'm reading it right, the concern is that discouragement is observable?

Yes, and it shouldn't be. In general, whatever happens in a private broadcast connection shouldn't alter the state of the node in such a way that this change can be observed from outside, via the P2P interface.

<details> <summary>Show Signature</summary>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK b1c8ce8d3d0cc5331497ac5fb8e94888530768ab

> If I'm reading it right, the concern is that discouragement is observable?

Yes, and it shouldn't be. In general, whatever happens in a private broadcast connection shouldn't alter the state of the node in such a way that this change can be observed from outside, via the P2P interface.
-----BEGIN PGP SIGNATURE-----

iQRPBAEBCAA5FiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmqyuusbFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMiwzAAoJEFTfBvZLVcu/D10f/RTBlc1KHbqAXufDEzBk
kPNmc6ZDMVOlSdC+fVeNhWm3MFvaVUXCSFot1whpWCG5jAGO6dLrK5UeN1ttDIhk
8gqDc39EX8uf5OwRB3veXdMPfvLHa8iTShOKX2Uo6AruBnC2vm9u81IsBNVhDdPy
p7qhwupSW5UsVNHYadlQKBSDV11dSI5RxcKm0dzu1ITEJ/2pAIOGdd/xSnFG5ARg
vgnursXyBHiepW6FXqE9MDooKVHSfUcd4TxNxMqBsZkTZUSlZLftgRx/X9F71rPh
aGhqi1baSFnBEi1aKPid1gSPx8odWTAZBtfedBTg/B9pXNT2eWKA6iKkRLWoK1qx
TOKdvqCPb+ooCwqwf3bj5166nJaWrUIoO220GdpeC+hIUghTzad8kQ9rtMNUPL9u
DjtZXcacbbYLGwdmg87UnBlBkClwFaeS29VpDQdSI+ijri9S/peCwGFBh28bIKlF
4ySUS349U4i1KDW1kVBE9jnLV6GG4w0FYNMEffneECxth5pAJ89KcQl1LPi1hWE9
wbDSbMOO1akTqD6D2sTTiInRoOL9dk3E7pz6zbGjYq994wLiVRXC6KdMz9AERGHt
XTGIxjDGWBas3ueRtWzf9baP8yB4NY+7DXygXu6k7UvHfjkRZ1gO28uCy2sKc6iN
arCZZOd14t06sleSgjMsIKwC1MrTUQRWGYnkwtEVdulLP6/OY7ft9EgpD2XIwoEk
R6Bw9sowachCvuM7x1H9PMO2EMj4H7WWTsahk7GrIlrFd53lJ4Hd+qKhUgd9fpS/
sex8KZI27wODzpO8GHIXREJctgEGSw+fkuZBt0ShszZ0ZqwOwlEvgRaZKOoGhoOp
R0RRtlE3cgHHXTGFWNT1T2N/Dt0O6oDaK73rgJME5U5jtkfiLiK7t7YfMgFiDv2w
MhKsPOy6ElTCdyR16+/y+e4JsZKlXkMOGwv2XsJDpo+K8Z/xRapHhlB4spii6jr+
xWwa6yd9OQPYty/LtBj/9rXwsb9nPaqADoPTBBxiKaM8/3CW+PGhYkwoS6iMXqVa
BUX/gxZCiLPQ5B4KqMA6QclomlPrMNV/SQ86E9fZ6SVkeQ377xy2D6KPXVBpqDbW
7c1LLrppukdCwiByI+B4Qwn/mBXOdb/YCmNL5ZjpOwLd/2NsmQBnIWjV9/aIfr8+
zGTz4LXHrLWHrNEuZ3jb+UAVZ9tBgbgCSWDj7DLK4A9ulkBcmAryMiV60ZoI6lSg
ksT+GpvInzBjyYD/BuXo2RpTmrwQdmEeqS5kunGaeoH2xZdw9K9be9HiwTJvbtK9
R2j0pknW8HgVM2IgStjKP73ICZ+j34iSaS0Oz2gBxTIbmhafMR6H80BRjnR8Pg5r
Ao0=
=Fgtc
-----END PGP SIGNATURE-----

vasild's public key is on openpgp.org

</details>

andrewtoth commented at 2:03 AM on September 23, 2026: contributor

@ViniciusCestarii I updated the PR description with the motivation.

in src/net_processing.cpp:5410 in b1c8ce8d3d outdated

5406 | @@ -5401,7 +5407,7 @@ bool PeerManagerImpl::MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer)
5407 |      // Normal case: Disconnect the peer and discourage all nodes sharing the address
5408 |      LogDebug(BCLog::NET, "Disconnecting and discouraging peer %d!\n", peer.m_id);
5409 |      if (m_banman) m_banman->Discourage(pnode.addr);
5410 | -    m_connman.DisconnectNode(pnode.addr);
5411 | +    m_connman.DisconnectNode(CSubNet{pnode.addr}, /*disconnect_private_broadcast=*/false);

ViniciusCestarii commented at 12:49 PM on September 23, 2026:

In "net: don't discourage private broadcast peers" b1c8ce8d3d0cc5331497ac5fb8e94888530768ab

The added test don't prevent the following regression:

- m_connman.DisconnectNode(CSubNet{pnode.addr}, /*disconnect_private_broadcast=*/false);
+ m_connman.DisconnectNode(CSubNet{pnode.addr});

A case where a non-private-broadcast peer and a private broadcast peer share an address and the non-private-broadcast one misbehaves should cover it.

andrewtoth commented at 4:01 PM on September 23, 2026:

Done.

ViniciusCestarii commented at 12:59 PM on September 23, 2026: contributor

Approach ACK b1c8ce8d3d0cc5331497ac5fb8e94888530768ab

Commented a regression case that the added test don't cover.

instagibbs commented at 2:16 PM on September 23, 2026: member

is this intended for backport?

andrewtoth commented at 2:19 PM on September 23, 2026: contributor

is this intended for backport? @instagibbs yes, it should be backported.

sedited added the label Needs Backport (32.x) on Sep 23, 2026

instagibbs commented at 3:05 PM on September 23, 2026: member

conditional ACK b1c8ce8d3d0cc5331497ac5fb8e94888530768ab

Conditional on this being backported. I added my own tests to cover #36312 (review) locally and think it would be good to extend those in the PR

A targeted change that cleanly applies on 32.x

DrahtBot requested review from ViniciusCestarii on Sep 23, 2026

net: don't discourage private broadcast peers

Keep private broadcast connections outside normal discouragement handling.
Misbehaving private broadcast peers are still disconnected.

eab6630add

andrewtoth force-pushed on Sep 23, 2026

andrewtoth commented at 4:01 PM on September 23, 2026: contributor

Thank you for your reviews @instagibbs @vasild @ViniciusCestarii.

I added the regression test.

I think this should also be backported to v31.

git diff b1c8ce8d3d0cc5331497ac5fb8e94888530768ab..eab6630addc385d013db5fe6779679228e445d09

instagibbs commented at 4:09 PM on September 23, 2026: member

reACK https://github.com/bitcoin/bitcoin/pull/36312/commits/eab6630addc385d013db5fe6779679228e445d09 with same caveat

only added reverse of the first test

DrahtBot requested review from vasild on Sep 23, 2026

ViniciusCestarii commented at 6:02 PM on September 23, 2026: contributor

ACK eab6630addc385d013db5fe6779679228e445d09 confirmed the added test captures the regression

fanquake commented at 10:41 AM on September 24, 2026: member

cc @mzumsande

vasild approved

vasild commented at 10:42 AM on September 24, 2026: contributor

ACK eab6630addc385d013db5fe6779679228e445d09

IMO this belongs to 32.0 and 31.x

<details> <summary>Show Signature</summary>

vasild's public key is on openpgp.org

</details>

mzumsande commented at 12:40 PM on September 24, 2026: contributor

Code Review ACK eab6630addc385d013db5fe6779679228e445d09

in src/net.h:1388 in eab6630add

1384 | @@ -1385,7 +1385,7 @@ class CConnman
1385 |      uint32_t GetMappedAS(const CNetAddr& addr) const;
1386 |      void GetNodeStats(std::vector<CNodeStats>& vstats) const EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
1387 |      bool DisconnectNode(std::string_view node) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
1388 | -    bool DisconnectNode(const CSubNet& subnet) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);
1389 | +    bool DisconnectNode(const CSubNet& subnet, bool disconnect_private_broadcast = true) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex);

optout21 commented at 3:13 PM on September 24, 2026:

eab6630 net: don't discourage private broadcast peers:

Why introduce with the default value (true) that is not the behavior intended here? Wouldn't it be possible to not use a default value, but specify at each call site explicitly (there are not that many)?

andrewtoth commented at 4:32 PM on September 24, 2026:

Why introduce with the default value (true) that is not the behavior intended here?

The intended behavior is to disconnect private broadcast nodes. There is an exception only for discouragement.

Wouldn't it be possible to not use a default value, but specify at each call site explicitly (there are not that many)?

It would be possible, but that would be updating more lines. Also, this would make the change more difficult to review, since reviewers would need to check every callsite.

in src/net_processing.cpp:5410 in eab6630add

5406 | @@ -5399,7 +5407,7 @@ bool PeerManagerImpl::MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer)
5407 |      // Normal case: Disconnect the peer and discourage all nodes sharing the address
5408 |      LogDebug(BCLog::NET, "Disconnecting and discouraging peer %d!\n", peer.m_id);
5409 |      if (m_banman) m_banman->Discourage(pnode.addr);
5410 | -    m_connman.DisconnectNode(pnode.addr);
5411 | +    m_connman.DisconnectNode(CSubNet{pnode.addr}, /*disconnect_private_broadcast=*/false);

optout21 commented at 3:15 PM on September 24, 2026:

eab6630 net: don't discourage private broadcast peers:

Wouldn't it be possible to add the logic not inside DisconnectNode, but within this method, similar to the conditionals above for various exceptions?

andrewtoth commented at 4:39 PM on September 24, 2026:

Sorry, I don't see how that could be possible. Perhaps I am not seeing something?

optout21 commented at 3:18 PM on September 24, 2026: contributor

Concept ACK. Reviewed the implementation, but I can't give a based judgement. Left two concern/question comments (feel free to disregard if not helpful).

sedited added this to the milestone 32.0 on Sep 24, 2026

in src/test/denialofservice_tests.cpp:432 in eab6630add

 427 | +    connman->AddTestNode(*nodes[4]);
 428 | +
 429 | +    peerLogic->UnitTestMisbehaving(nodes[4]->GetId());
 430 | +    BOOST_CHECK(peerLogic->SendMessages(*nodes[4]));
 431 | +    BOOST_CHECK(nodes[4]->fDisconnect);
 432 | +    BOOST_CHECK(!banman->IsDiscouraged(other_addr));

danielabrozzoni commented at 5:04 PM on September 24, 2026:

nit: You could also check that if a peer shares the same address as nodes[4], it won't get disconnected when nodes[4] misbehaves:

diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index 69f9930551..4f6297238b 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -319,7 +319,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
 
     const CNetAddr other_addr{ip(0xa0b0ff01)}; // Not any of addr[].
 
-    std::array<CNode*, 5> nodes;
+    std::array<CNode*, 6> nodes;
 
     banman->ClearBanned();
     NodeId id{0};
@@ -425,10 +425,23 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
                          /*network_key=*/0};
     peerLogic->InitializeNode(*nodes[4], NODE_NETWORK);
     connman->AddTestNode(*nodes[4]);
+    nodes[5] = new CNode{/*id=*/id++,
+                         /*sock=*/nullptr,
+                         /*addrIn=*/CAddress{CService{other_addr, Params().GetDefaultPort()}, NODE_NONE},
+                         /*nKeyedNetGroupIn=*/0,
+                         /*nLocalHostNonceIn=*/0,
+                         /*addrBindIn=*/CAddress{},
+                         /*addrNameIn=*/"",
+                         /*conn_type_in=*/ConnectionType::OUTBOUND_FULL_RELAY,
+                         /*inbound_onion=*/false,
+                         /*network_key=*/0};
+    peerLogic->InitializeNode(*nodes[5], NODE_NETWORK);
+    connman->AddTestNode(*nodes[5]);
 
     peerLogic->UnitTestMisbehaving(nodes[4]->GetId());
     BOOST_CHECK(peerLogic->SendMessages(*nodes[4]));
     BOOST_CHECK(nodes[4]->fDisconnect);
+    BOOST_CHECK(!nodes[5]->fDisconnect);
     BOOST_CHECK(!banman->IsDiscouraged(other_addr));
 
     for (CNode* node : nodes) {

danielabrozzoni commented at 5:07 PM on September 24, 2026: member

tACK eab6630addc385d013db5fe6779679228e445d09

Marking misbehaving private broadcast peers as discouraged could leak our identity, because we treat discouraged peers differently, and usually disconnect all the peers that have the same address as the misbehaving one.

DrahtBot requested review from optout21 on Sep 24, 2026

fanquake merged this on Sep 25, 2026

fanquake closed this on Sep 25, 2026

fanquake referenced this in commit 8ec0fd46f4 on Sep 25, 2026

fanquake removed the label Needs Backport (32.x) on Sep 25, 2026

fanquake commented at 8:41 AM on September 25, 2026: member

Backported to 32.x in #36300.

andrewtoth deleted the branch on Sep 25, 2026

Kino1994 referenced this in commit aa31053149 on Sep 28, 2026

jonatack commented at 4:34 AM on September 28, 2026: member

Post-merge ACK