banman: compact-filter sync can ban peers for network failures, not just bad data

[starius]

Compact-filter sync currently has code paths that ban peers when they fail to answer a follow-up query, even though the code has not proved that they served invalid data. The implementation is conflating "peer sent bad compact-filter data" with "peer did not respond / disconnected / local node was offline during the follow-up exchange".

This matters because a ban is a strong classification. A peer that times out or disappears during a follow-up compact-filter query may be experiencing an ordinary network failure, or the local node may be offline, but the code can still treat that as misbehavior.

There are two concrete places where this happens. In resolveConflict, peers that do not appear in the follow-up cfheaders response map are banned with InvalidFilterHeaderCheckpoint. In detectBadPeers, a peer that does not answer the follow-up cfilter request is marked bad and then banned with InvalidFilterHeader during checkpoint conflict handling and during uncheckpointed filter-header mismatch handling.

The shared queryAllPeers helper only works with a snapshot of currently connected peers and waits for replies, while its callers only add cfheaders responders to the result map, only add cfilter responders to the result map, and only add checkpoint responders to the result map. A timeout, disconnect, or other network failure therefore just means the peer is missing from the result map, and higher layers then interpret that absence as evidence of bad data in the paths above.

Expected behavior is to distinguish between "peer responded with data that can be verified as invalid" and "peer did not respond / request could not be completed". Only the first case should lead to InvalidFilterHeader or InvalidFilterHeaderCheckpoint. The second case may justify retrying, deprioritizing, or disconnecting, but not banning.

This can cause honest IPv4/IPv6 peers to be banned for 24 hours (the BanDuration constant in neutrino.go) during periods where the local node is offline or connectivity is unstable. PR #348 teaches banman to also recognize onion peers (Tor V3). Currently onion peers are not banned at all.

See also #4 and #253

[Vandit1604]

I'd like to take this one.

From reading the code, the two spots that ban on non-response look like:

• resolveConflict: peers that are missing from the cfheaders response map get banned with InvalidFilterHeaderCheckpoint, even though missing just means they didn't reply.
• detectBadPeers: a peer that doesn't answer the follow-up cfilter request gets added to the bad list and then banned with InvalidFilterHeader.

My plan is to only ban when we have actually verified the peer served invalid data, and to just drop non-responders from the result set instead of banning them. I'll add tests that check a non-responding peer is not banned while a peer serving an invalid filter still is.

Let me know if that direction sounds right before I open a PR.

[Vandit1604]

Quick update on the approach now that I've gone through the code.

The tricky part is that there is an existing test that wants header-only peers to be kicked, so they cannot stall the sync by answering header queries but never the follow-up filter query (see

neutrino/blockmanager_test.go

Lines 763 to 767 in 8094357

	// We let the "bad" peers not respond to the filter
	// query. They should be marked bad because they are
	// unresponsive. We do this to ensure peers cannot
	// only respond to us with headers, and stall our sync
	// by not responding to filter requests.

). So I did not want to just drop non-responders and leave them connected.

So I split the handling into two cases:

• Peers we can verify served invalid data (filter does not match its hashes, or loses the block-based reconciliation) still get banned, same as before.
• Peers that simply did not respond now get disconnected without a ban. They are still dropped from the round, so the stalling case is still handled, they just do not get a 24 hour ban for what might be a network issue on either side.

Concretely:

• detectBadPeers now returns the invalid peers and the unresponsive peers separately.
• Added a DisconnectPeer that drops the connection but does not write a ban record.
• The checkpoint path in resolveConflict that used to ban peers missing from the response now disconnects them instead.
• Updated the existing test so the non-responding peers are expected to be disconnected, not banned.

I have this working locally with tests passing. I will open the PR once this direction looks good to you.

[starius]

Thanks for working on this!

Yes, I think this direction is worth turning into a PR.

The split between verified invalid data and non-response matches BIP157 better: peers that send invalid filter headers or filters should still be banned, while peers that do not answer a follow-up query should be dropped from the round and disconnected without writing a ban record.

Things to look for in the PR:

• detectBadPeers clearly separates invalid peers from unresponsive peers.
• Invalid peers are still banned with InvalidFilterHeader or InvalidFilterHeaderCheckpoint.
• Unresponsive peers are disconnected and removed from the current round, but not banned.
• The checkpoint conflict path in resolveConflict gets the same treatment as the filter follow-up path.
• The disconnect path is wired cleanly, ideally as a narrow DisconnectPeer callback similar to BanPeer.
• A disconnect failure for an already-gone peer does not fail sync.
• Tests cover both cases: non-responders are disconnected but not banned, while peers that serve provably invalid data are still banned.
• The existing anti-stall behavior remains: peers cannot keep answering headers/checkpoints while refusing the follow-up filter query and stay in the active round.

[Vandit1604]

Opened the PR for this: #365.

It follows the direction from above: invalid peers are still banned, non-responders are disconnected and dropped from the round without a ban, and the existing anti-stall behavior is kept. Tests cover both cases.