p2p: disconnect peers that send filterclear + update existing filter msg disconnect logic #19260

glozow · 2020-06-12T17:59:18Z

Nodes that don't have bloomfilters turned on (i.e. no NODE_BLOOM service) should disconnect peers that send them filterclear P2P messages.

Non-bloomfilter nodes already disconnect peers for filteradd and filterload, but #8709 removed filterclear so it could be used to reset tx relay. This isn't needed now because using feefilter message is much better for this purpose (See #19204).

Also refactors existing disconnect logic for filteradd and filterload into respective message handlers and removes banning for them.

maflcko · 2020-06-12T18:25:26Z

Concept ACK. There shouldn't be a reason for a node to send filterclear when we haven't offered them bloom services to them.

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: wait for disconnect in disconnect_p2ps + bloomfilter test followups #19252 (test: wait for disconnect in disconnect_p2ps + bloomfilter test followups by gzhao408)

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.

jnewbery · 2020-06-14T00:42:40Z

Code review ACK 18a2e0a3f7071b8df6b20ecc1b34b3f2abb13cfc

Great tidy-up. Thanks Gloria!

naumenkogs · 2020-06-14T07:35:55Z

utACK 18a2e0a

I was somewhat confused by the last commit, because the description of the PR is about filterclear, but the last commit has absolutely nothing to do with filterclear.
Not sure if we want to do anything about it :)

maflcko

ACK 18a2e0a3f7071b8df6b20ecc1b34b3f2abb13cfc nice cleanup 🌫

Show signature and timestamp

Signature:

Timestamp of file with hash 76972080cdb7d6db860684d9cc7ddc281674c2c67368d5f83cad36c0fa0849c0 -

src/net_processing.cpp

-nodes not serving bloomfilters should disconnect peers that send filterclear, just like filteradd and filterload -nodes that want to enable/disable txrelay should use feefilter

-Increasing the banscore and/or banning is too harsh, just disconnecting is enough. -Return true from ProcessMessage because we already log receipt of filterclear and disconnect.

maflcko · 2020-06-15T10:59:19Z

re-ACK 3a10d93 only change is replacing false with true 🚝

Show signature and timestamp

Signature:

Timestamp of file with hash d49fb833782cb07ba218eb5ea866b2c2f12e58a1ac6670b3a225942fdd3a8e52 -

jnewbery · 2020-06-15T13:21:25Z

Code review ACK 3a10d93

gillichu · 2020-06-15T17:16:36Z

Concept ACK, I just learned a lot about how bloom filters are used in Bitcoin.

gillichu · 2020-06-15T18:46:58Z

tested ACK: quick test_runner on macOS 3a10d93

glozow · 2020-06-15T18:49:25Z

😂 Thanks @gillichu, I really appreciate the effort 🙏

It's [`commit`](link)

jnewbery · 2020-06-15T20:53:32Z

It's [`commit`](link)

No need to use markdown for links to the commit hash. Just paste the full hash and github will automatically convert it to a link to commit.

glozow · 2020-06-15T21:36:03Z

Just paste the full hash and github will automatically convert it to a link to commit.

WHAT I've been using markdown this whole time 😭

naumenkogs · 2020-06-16T06:39:42Z

utACK 3a10d93

rebroad · 2021-05-03T07:24:11Z

Concept ACK. There shouldn't be a reason for a node to send filterclear when we haven't offered them bloom services to them.

See also BIP 111:

If a node does not support bloom filters but receives a "filterload", "filteradd", or "filterclear" message from a peer the node should disconnect that peer immediately

https://github.com/bitcoin/bips/blob/master/bip-0111.mediawiki

nit: I think this is a good opportunity to move the disconnect handling of NetMsgType::FILTERLOAD and NetMsgType::FILTERADD into the respective scope of the switch statement instead of having it at the top of the function, disconnected from the actual filter message handling.

filterclear was also available to request a node sends TXs after IBD had completed, and using blocksonly at connection handshake - this worked better than using feefilter as it was part of the connection handshake, unlike feefilter, which requires a message later.

Anyway, not a big issue, but seems like a lot of work to change something that already worked.

Also, bloom filters were introduced before feefilter, so older nodes are now being disrespected by not having TXs sent to them when they ask for them. We shouldn't really be breaking backwards compatibility, IMHO, unless we actually want to on purpose - e.g. is there a reason not to send TXs to old nodes?

maflcko · 2021-05-03T07:52:48Z

@rebroad Please, there is no reason to leave comments on three pull requests to ask questions. The thread you created in #21839 is sufficient.

… existing filter msg disconnect logic Summary: 3a10d935ac8ebabdfd336569d943f042ff84b13e [p2p/refactor] move disconnect logic and remove misbehaving (gzhao408) ff8c430c6589ea72b9e169455cf6437c8623cc52 [test] test disconnect for filterclear (gzhao408) 1c6b787e0319c44f0e0bede3f4a77ac7c2089db2 [netprocessing] disconnect node that sends filterclear (gzhao408) Pull request description: Nodes that don't have bloomfilters turned on (i.e. no `NODE_BLOOM` service) should disconnect peers that send them `filterclear` P2P messages. Non-bloomfilter nodes already disconnect peers for [`filteradd` and `filterload`](https://github.com/bitcoin/bitcoin/blob/19e919217e6d62e3640525e4149de1a4ae04e74f/src/net_processing.cpp#L2218), but #8709 removed `filterclear` so it could be used to reset tx relay. This isn't needed now because using `feefilter` message is much better for this purpose (See #19204). Also refactors existing disconnect logic for `filteradd` and `filterload` into respective message handlers and removes banning for them. --- Backport of [[bitcoin/bitcoin#19260 | core#19260]] Depends on D9520 Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9512

DrahtBot added P2P Tests labels Jun 12, 2020

maflcko removed the Tests label Jun 12, 2020

glozow force-pushed the filterclear-disconnect branch from dbc7d30 to 8207e3b Compare June 12, 2020 21:06

glozow commented Jun 12, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jun 13, 2020

test: wait for disconnect in disconnect_p2ps + bloomfilter test followups #19252

Merged

glozow force-pushed the filterclear-disconnect branch from 8207e3b to 18a2e0a Compare June 13, 2020 18:59

maflcko approved these changes Jun 14, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

glozow changed the title ~~p2p: disconnect peers that send filterclear~~ p2p: disconnect peers that send filterclear + update existing filter msg disconnect logic Jun 14, 2020

glozow added 3 commits June 14, 2020 11:47

[netprocessing] disconnect node that sends filterclear

1c6b787

-nodes not serving bloomfilters should disconnect peers that send filterclear, just like filteradd and filterload -nodes that want to enable/disable txrelay should use feefilter

[test] test disconnect for filterclear

ff8c430

[p2p/refactor] move disconnect logic and remove misbehaving

3a10d93

-Increasing the banscore and/or banning is too harsh, just disconnecting is enough. -Return true from ProcessMessage because we already log receipt of filterclear and disconnect.

glozow force-pushed the filterclear-disconnect branch from 18a2e0a to 3a10d93 Compare June 14, 2020 18:55

fanquake merged commit 28ce05d into bitcoin:master Jun 16, 2020

glozow deleted the filterclear-disconnect branch July 25, 2020 14:47

maflcko mentioned this pull request Aug 20, 2020

p2p: Reduce inv traffic during IBD #19204

Merged

rebroad mentioned this pull request May 3, 2021

revert protocol change (feefilter now ignored) #21839

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

p2p: disconnect peers that send filterclear + update existing filter msg disconnect logic #19260

p2p: disconnect peers that send filterclear + update existing filter msg disconnect logic #19260

Uh oh!

Conversation

glozow commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 12, 2020

Uh oh!

Uh oh!

DrahtBot commented Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Jun 14, 2020

Uh oh!

naumenkogs commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jun 15, 2020

Uh oh!

jnewbery commented Jun 15, 2020

Uh oh!

gillichu commented Jun 15, 2020

Uh oh!

gillichu commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Jun 15, 2020

Uh oh!

jnewbery commented Jun 15, 2020

Uh oh!

glozow commented Jun 15, 2020

Uh oh!

naumenkogs commented Jun 16, 2020

Uh oh!

rebroad commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

glozow commented Jun 12, 2020 •

edited

Loading

DrahtBot commented Jun 13, 2020 •

edited

Loading

naumenkogs commented Jun 14, 2020 •

edited

Loading

gillichu commented Jun 15, 2020 •

edited

Loading

rebroad commented May 3, 2021 •

edited

Loading