net: disconnect spam node peers when no longer useful by balayanv · Pull Request #412 · bitcoinknots/bitcoin

balayanv · 2026-09-13T13:59:11Z

This is most definitely not "appropriate" for bitcoin core.

The motivation is simple, i do not want my node to accept/allow or make connections to spam coin nodes (with few exceptions).
The code is mostly self describing and compact (one function 2 call sites).

weArePastSharedHistory && peerLacksBlakeBit && peerIsNotManuallyAdded;

There are 2 places where this is being checked/enforced.

During the handshake when we get the version from the peer and inside a polling loop which i believe runs per pear 10 times a second.

Personally i think the handshake check is enough, and second one can be dropped.

Second one is only there because during IBD peers that are from spam chain are helping us sync, their handshake check is passed and they are in. If we want to disconnect them later second check does it. Or we just wait, connection is not going to be there forever i think...

Note : i believe that currently even if you're connected blocks do not sync with spam nodes anyway, but i consider that separate issue that should be fixed separately.

kwsantiago · 2026-09-13T14:42:30Z

            return;
        }

+        if (CanDisconnectSpamCoinPeer(pfrom, nServices)) {


This returns before the stale-outbound handling below, so post-fork DemoteToStaleOutbound and -maxstaleoutbound never run.

ok but whats the point of -maxstaleoutbound why 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?

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 without NODE_BLAKE2B relayed a SIGHASH_ALL tx we accepted and mined. If you disagree, delete -maxstaleoutbound in this PR rather than leave it inert.

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.

kwsantiago · 2026-09-13T14:42:31Z

    }

+    // Peers that connected before shared history was completed are dropped once shared history is synced.
+    if (CanDisconnectSpamCoinPeer(*pto, peer->m_their_services)) {


For inbound, protecting NODE_BLAKE2B peers in AttemptToEvictConnection gets the same slots without disconnecting anyone.

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.

Eviction gives BLAKE2b peers the slot when inbound is full and costs nothing while there is room.

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.

thanks for clarification, i'll explore more later today

…BLAKE2B

…k height

jasonsopko · 2026-09-13T18:10:53Z

Concept NACK.

A peer without NODE_BLAKE2B cannot give us blocks past the fork, and 29.4.1 already accounts for that. What it can still give us is transactions, and this PR throws those away.

Transaction ids, signatures and wallets are unchanged across the split, and there is no replay protection, so a transaction broadcast to a legacy node is valid here unless it spends an output that only exists on the other chain. Wallets, exchanges and Electrum servers that have not updated still broadcast through legacy nodes. Those are fee-paying transactions our miners want, and they reach us through peers that talk to both networks. Every node that drops legacy peers pushes the two networks toward a partition at the P2P layer, where a transaction has to be broadcast twice on purpose to reach both. That is a cost with nothing on the other side of the ledger.

The slot argument is already handled. Outbound dialing prefers NODE_BLAKE2B (net.cpp:2989), a legacy outbound is demoted to a stale outbound that draws on the inbound budget rather than the outbound target (DemoteToStaleOutbound, at most -maxstaleoutbound, default 8), and the DNS-seed cadence is driven by how many NODE_BLAKE2B addresses addrman holds rather than its total size (net.cpp:2345). #398 tightens the dialing further. A legacy peer does not hold a slot a BLAKE2b peer wants; what it costs us is the bandwidth to relay transactions, which is the thing we keep it for.

I wrote a functional test for that, p2p_blake2b_legacy_tx_relay.py in the gist (https://gist.github.com/jasonsopko/62c6c7ff9ca368c34e0fdb117f4ca96e). On 29.4.1 (8c85b15) with the tip at the fork height: an inbound peer without NODE_BLAKE2B connects and relays a transaction that we accept, an outbound peer without the bit is demoted (connected to stale outbound peer (1/8)) and relays a second one, and both mine in the next BLAKE2b block. Passes with and without --v2transport. On this branch (363d443):

2026-09-13T18:02:26.761000Z TestFramework (INFO): Inbound peer without NODE_BLAKE2B, after the fork height
AssertionError                                   # wait_for_verack: the peer is gone
debug.log: [net] node lacks NODE_BLAKE2B, disconnecting peer=0

Your own p2p_blake2b_postfork_filter.py passes on this head through python3, with and without --v2transport, so the branch does what it says. As committed the file is mode 100644: running it directly fails with Permission denied, and lint-files.py flags a shebang without the executable bit. test_runner.py goes through the interpreter, so it does not notice. p2p_stale_outbound.py, p2p_blake2b_outbound_preference.py and p2p_blake2b_dns_immediate.py still pass, but note that the stale-outbound test never crosses the fork height, so it does not see this change at all.

Code notes on the current head, in case the approach survives:

The handshake check returns before the stale-outbound demotion at net_processing.cpp:3554, so past the fork -maxstaleoutbound never runs and the init.cpp help text for it ("Connections to full nodes will still be sought and preferred over stale ones") describes behavior that no longer exists.
The SendMessages check runs on every pass for every peer and drops inbound legacy peers that were accepted before the tip crossed the fork, including ones relaying to us at that moment.
AttemptToEvictConnection (net.cpp:1712) has no NODE_BLAKE2B term today. @kwsantiago's suggestion to protect NODE_BLAKE2B peers there is the version of this I would ACK: BLAKE2b peers win when inbound is full, and nobody is refused while there is room.

To the question of why we would want a node on the other chain connected at all: because it carries transactions for us, and it costs us nothing we were using.

jasonsopko · 2026-09-13T19:38:41Z

Correction to my comment above: replay protection does exist, as an opt-in. SIGHASH_UNIFIED shipped in 29.4.1, and a transaction signed with it is valid on this chain only. I wrote that there was none, which was wrong. The point stands: a wallet that has not opted in signs SIGHASH_ALL, which is valid on both chains, and that is what legacy peers carry to us. @kwsantiago measured the same thing at tip 130 with the fork at 20.

walkjivefly · 2026-09-16T12:49:59Z

Concept NACK.

We want (some of) the transactions those nodes can give us. And exclusion based on the absence of the BLAKE2B bit will disconnect not only Core nodes but also other peers like NBitcoin, bitcoinj, breadwallet, bitcore (none of which implement Core v30 excess-permissiveness) unnecessarily limiting fee-paying transactions our nodes can benefit from including in blocks.

balayanv · 2026-09-16T14:36:01Z

Concept NACK.

We want (some of) the transactions those nodes can give us. And exclusion based on the absence of the BLAKE2B bit will disconnect not only Core nodes but also other peers like NBitcoin, bitcoinj, breadwallet, bitcore (none of which implement Core v30 excess-permissiveness) unnecessarily limiting fee-paying transactions our nodes can benefit from including in blocks.

I might be wrong but from what i know there are 2 types of node runners, those who use it for mining (in which case what you describe makes sense), and those of us who simply run a node for our own wallets/tools/infra + helping people IBD and relay transactions.

I'm the second category (for now), but for mining i'm going to have dedicated separate node+datum.
For cases where you don't care about mining, is there any reason why you would want to connect to non blake node? Because if there is then imo this pr is redundant completely.

NBitcoin, bitcoinj, breadwallet, bitcore if i understand correctly those are still on spamchain, so mining nodes would want to talk to those.

Personally i like that all my peers are blake2b nodes and that i'm helping them with IBD instead of the spammers.

net: disconnect spam node peers when no longer useful

feb635f

kwsantiago reviewed Sep 13, 2026

View reviewed changes

balayanv added 3 commits September 13, 2026 23:10

net: don't disconnect addr-fetch and feeler connections lacking NODE_…

3d8eb29

…BLAKE2B

net: don't disconnect NoBan peers lacking NODE_BLAKE2B

533c363

net: disconnect peers lacking NODE_BLAKE2B once the tip is at the for…

363d443

…k height

LionThunderFingers mentioned this pull request Sep 13, 2026

net: never start header sync with a pre-fork peer while a fork-aware one is available #418

Closed

Conversation

balayanv commented Sep 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonsopko commented Sep 13, 2026

Uh oh!

jasonsopko commented Sep 13, 2026

Uh oh!

walkjivefly commented Sep 16, 2026

Uh oh!

balayanv commented Sep 16, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

balayanv commented Sep 13, 2026 •

edited

Loading