Repository navigation
net: disconnect spam node peers when no longer useful #412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 29.x-knots
Are you sure you want to change the base?
Changes from all commits
feb635f
3d8eb29
533c363
363d443
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -564,6 +564,14 @@ class PeerManagerImpl final : public PeerManager | |
| */ | ||
| bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer); | ||
|
|
||
| /** Check if we are good to disconnect SpamCoin peers. Pure check, callers log and disconnect. | ||
| * | ||
| * @param[in] node The node to check. | ||
| * @param[in] services The services the peer advertised in its version message. | ||
| * @return True if peer lacks NODE_BLAKE2B bit AND pre-fork history is complete AND peer is not manually added or NoBan AND peer is not addr-fetch or feeler. | ||
| */ | ||
| bool CanDisconnectSpamCoinPeer(const CNode& node, ServiceFlags services) const; | ||
|
|
||
| /** Handle a transaction whose result was not MempoolAcceptResult::ResultType::VALID. | ||
| * @param[in] first_time_failure Whether we should consider inserting into vExtraTxnForCompact, adding | ||
| * a new orphan to resolve, or looking for a package to submit. | ||
|
|
@@ -3470,6 +3478,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, | |
| return; | ||
| } | ||
|
|
||
| if (CanDisconnectSpamCoinPeer(pfrom, nServices)) { | ||
| LogDebug(BCLog::NET, "node lacks NODE_BLAKE2B, %s\n", pfrom.DisconnectMsg(fLogIPs)); | ||
|
balayanv marked this conversation as resolved.
|
||
| pfrom.fDisconnect = true; | ||
|
balayanv marked this conversation as resolved.
|
||
| return; | ||
| } | ||
|
|
||
| if (nVersion < MIN_PEER_PROTO_VERSION) { | ||
| // disconnect from peers older than this proto version | ||
| LogDebug(BCLog::NET, "peer using obsolete version %i, %s\n", nVersion, pfrom.DisconnectMsg(fLogIPs)); | ||
|
|
@@ -4954,6 +4968,19 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, | |
| return; | ||
| } | ||
|
|
||
| bool PeerManagerImpl::CanDisconnectSpamCoinPeer(const CNode& node, ServiceFlags services) const | ||
| { | ||
| // Past the shared pre-fork history, a peer lacking NODE_BLAKE2B is no longer useful | ||
| bool weArePastSharedHistory = m_chainparams.GetConsensus().IsBlake2bHeight(m_best_height); | ||
| bool peerLacksBlakeBit = !(services & NODE_BLAKE2B); | ||
| bool peerIsNotManuallyAdded = !node.IsManualConn(); | ||
| bool peerIsNotNoBan = !node.HasPermission(NetPermissionFlags::NoBan); | ||
| // Addr-fetch and feeler connections are short-lived and close on their own. | ||
| bool peerIsNotShortLived = !node.IsAddrFetchConn() && !node.IsFeelerConn(); | ||
|
|
||
| return weArePastSharedHistory && peerLacksBlakeBit && peerIsNotManuallyAdded && peerIsNotNoBan && peerIsNotShortLived; | ||
| } | ||
|
|
||
| bool PeerManagerImpl::MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer) | ||
| { | ||
| { | ||
|
|
@@ -5520,6 +5547,13 @@ bool PeerManagerImpl::SendMessages(CNode* pto) | |
| return true; | ||
| } | ||
|
|
||
| // Peers that connected before shared history was completed are dropped once shared history is synced. | ||
| if (CanDisconnectSpamCoinPeer(*pto, peer->m_their_services)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For inbound, protecting NODE_BLAKE2B peers in
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i do not get this one, inbound connections are for full nodes, once you're a full blake node, why would you ever allow or want core node to connect? Why there is any protecting... there should be only blake nodes that can connect and thats donzo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Eviction gives BLAKE2b peers the slot when inbound is full and costs nothing while there is room. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because it carries valid Bitcoin transactions to us. A legacy node relays whatever passes its own policy, and some of that is data spam we reject at our own door, but the rest is ordinary payments signed SIGHASH_ALL, valid on both chains, that our miners want in their blocks. We are Bitcoin, and Bitcoin processes valid Bitcoin transactions wherever they arrive from. The peer costs us nothing a BLAKE2b peer was using: it cannot serve blocks past the fork, it never holds an outbound slot since it is demoted to a stale outbound, and eviction can prefer BLAKE2b peers inbound when the slots are full. The test in my comment above shows that relay working on 29.4.1 and gone on this branch.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for clarification, i'll explore more later today |
||
| LogDebug(BCLog::NET, "existing node lacking NODE_BLAKE2B is no longer useful, %s\n", pto->DisconnectMsg(fLogIPs)); | ||
| pto->fDisconnect = true; | ||
| return true; | ||
| } | ||
|
|
||
| MaybeSendPing(*pto, *peer, current_time); | ||
|
|
||
| // MaybeSendPing may have marked peer for disconnection | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| #!/usr/bin/env python3 | ||
| # Copyright (c) 2026 The Bitcoin Knots developers | ||
| # Distributed under the MIT software license, see the accompanying | ||
| # file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
| """Test refusal of peers lacking NODE_BLAKE2B once shared pre-fork history is complete. | ||
|
|
||
| While the tip is below the fork height, peers without NODE_BLAKE2B are | ||
| accepted. From then on they are disconnected: new ones at their version | ||
| message, connected ones once the tip crosses into fork history. | ||
| """ | ||
|
|
||
| from test_framework.messages import ( | ||
| NODE_BLAKE2B, | ||
| NODE_NETWORK, | ||
| NODE_P2P_V2, | ||
| NODE_WITNESS, | ||
| ) | ||
| from test_framework.p2p import P2PInterface | ||
| from test_framework.test_framework import BitcoinTestFramework | ||
| from test_framework.util import assert_equal | ||
|
|
||
| FORK_HEIGHT = 20 | ||
| HANDSHAKE_DROPPED = "node lacks NODE_BLAKE2B, disconnecting" | ||
| NO_LONGER_VALID = "existing node lacking NODE_BLAKE2B is no longer useful, disconnecting" | ||
|
|
||
|
|
||
| class Blake2bPostforkFilterTest(BitcoinTestFramework): | ||
| def set_test_params(self): | ||
| self.setup_clean_chain = True | ||
| self.num_nodes = 1 | ||
| self.extra_args = [[ | ||
| f"-testactivationheight=blake2b@{FORK_HEIGHT}", | ||
| "-blake2b_headline=BLAKE2b functional test headline", | ||
| "-maxconnections=30", | ||
| ]] | ||
|
|
||
| def services(self, *, blake2b): | ||
| services = NODE_NETWORK | NODE_WITNESS | ||
| if blake2b: | ||
| services |= NODE_BLAKE2B | ||
| if self.options.v2transport: | ||
| services |= NODE_P2P_V2 | ||
| return services | ||
|
|
||
| def inbound(self, *, blake2b, **kwargs): | ||
| return self.nodes[0].add_p2p_connection( | ||
| P2PInterface(), services=self.services(blake2b=blake2b), **kwargs) | ||
|
|
||
| def outbound(self, p2p_idx, *, blake2b, **kwargs): | ||
| return self.nodes[0].add_outbound_p2p_connection( | ||
| P2PInterface(), p2p_idx=p2p_idx, connection_type="outbound-full-relay", | ||
| services=self.services(blake2b=blake2b), | ||
| supports_v2_p2p=self.options.v2transport, | ||
| advertise_v2_p2p=self.options.v2transport, **kwargs) | ||
|
|
||
| def run_test(self): | ||
| node = self.nodes[0] | ||
| addr = node.get_deterministic_priv_key().address | ||
|
|
||
| self.log.info("Below the fork height, peers lacking NODE_BLAKE2B are accepted") | ||
| self.generatetoaddress(node, FORK_HEIGHT - 1, addr) | ||
| legacy_in = self.inbound(blake2b=False) | ||
| fork_in = self.inbound(blake2b=True) | ||
| legacy_out = self.outbound(0, blake2b=False) | ||
| fork_out = self.outbound(1, blake2b=True) | ||
| for peer in (legacy_in, fork_in, legacy_out, fork_out): | ||
| peer.sync_with_ping() | ||
| assert_equal(len(node.getpeerinfo()), 4) | ||
|
|
||
| self.log.info("Completing shared history disconnects connected peers lacking NODE_BLAKE2B") | ||
| with node.assert_debug_log([NO_LONGER_VALID], unexpected_msgs=[HANDSHAKE_DROPPED]): | ||
| self.generatetoaddress(node, 1, addr) | ||
| legacy_in.wait_for_disconnect() | ||
| legacy_out.wait_for_disconnect() | ||
| fork_in.sync_with_ping() | ||
| fork_out.sync_with_ping() | ||
| self.wait_until(lambda: len(node.getpeerinfo()) == 2) | ||
|
|
||
| self.log.info("New inbound peers lacking NODE_BLAKE2B are refused at their version message") | ||
| with node.assert_debug_log([HANDSHAKE_DROPPED], unexpected_msgs=[NO_LONGER_VALID]): | ||
| refused = self.inbound(blake2b=False, expect_success=False) | ||
| refused.wait_for_disconnect() | ||
|
|
||
| self.log.info("New outbound peers lacking NODE_BLAKE2B are refused at their version message") | ||
| with node.assert_debug_log([HANDSHAKE_DROPPED], unexpected_msgs=[NO_LONGER_VALID]): | ||
| self.outbound(2, blake2b=False, wait_for_disconnect=True) | ||
| self.wait_until(lambda: len(node.getpeerinfo()) == 2) | ||
|
|
||
| self.log.info("Peers advertising NODE_BLAKE2B still connect") | ||
| self.inbound(blake2b=True).sync_with_ping() | ||
| self.outbound(3, blake2b=True).sync_with_ping() | ||
| self.wait_until(lambda: len(node.getpeerinfo()) == 4) | ||
|
|
||
| self.log.info("Fork peers stay connected across the next BLAKE2b block") | ||
| self.generatetoaddress(node, 1, addr) | ||
| assert_equal(node.getblockcount(), FORK_HEIGHT + 1) | ||
| for peer in node.p2ps: | ||
| if peer.is_connected: | ||
| peer.sync_with_ping() | ||
| assert_equal(len(node.getpeerinfo()), 4) | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| Blake2bPostforkFilterTest(__file__).main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This returns before the stale-outbound handling below, so post-fork
DemoteToStaleOutboundand-maxstaleoutboundnever run.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok but whats the point of
-maxstaleoutboundwhy do we want to connect and keep a stale connection to spam node?If IBD is done and we are on XBT chain i see 0 reasons to even accept or connect to those nodes...
maybe there is some other reason i dont know about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replay protection is opt-in (
SIGHASH_UNIFIED), so legacy wallet tx are still valid for us. Locally measured at tip 130 with the fork at 20, a peer withoutNODE_BLAKE2Brelayed aSIGHASH_ALLtx we accepted and mined. If you disagree, delete-maxstaleoutboundin this PR rather than leave it inert.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok makes sense, i think what i'll do is make this more configurable and cooperative for this usecase
but for a normal node that just keep strack of my things not used for mining, i'd completely ban all interaction with spam nodes... for mining node i get it and i'll make it configurable.