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

glozow commented at 5:59 pm on June 12, 2020: member

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.

MarcoFalke commented at 6:25 pm on June 12, 2020: member

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.

DrahtBot added the label P2P on Jun 12, 2020

DrahtBot added the label Tests on Jun 12, 2020

MarcoFalke removed the label Tests on Jun 12, 2020

glozow force-pushed on Jun 12, 2020

in src/net_processing.cpp:3436 in 8207e3b7e3 outdated

3432@@ -3447,6 +3433,16 @@ bool ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRec
3433     }
3434 
3435     if (msg_type == NetMsgType::FILTERLOAD) {
3436+        if (!(pfrom.GetLocalServices() & NODE_BLOOM)) {

glozow commented at 9:11 pm on June 12, 2020:

This code block is repeated 3 times and bloats the ProcessMessage function. Would a helper function like MaybePunishNodeForBloom be appropriate or is this fine?

jnewbery commented at 2:39 am on June 13, 2020:

Yes, but I think you can also just reduce the complexity here. I don’t think there’s any benefit to calling Misbehaving() on a peer that sends us a FILTER* message. Calling Misbehaving(100) results in us disconnecting the peer and then marking it discouraged (see #19219). Discouragement is a mechanism to prevent our limited connection slots being used up by incompatible or broken peers.

So I think we should just set fDisconnect for any peer that sends us a FILTER* message when we don’t have NODE_BLOOM enabled, regardless of their version number.

MarcoFalke commented at 11:46 am on June 13, 2020:

In theory it is possible to set -banscore=101 to not disconnect a 100-misbehaving peer. Though, that would only kick down the can by one more message. I can’t see a use case for that and encouraging users to change banscore to accomodate misbehaving light clients seems ill-advised. So I agree with the suggestion made by @jnewbery (simply make an fDisconnect)

jnewbery commented at 2:50 pm on June 13, 2020:

We should remove -banscore entirely. It’s not useful user configuration.

glozow commented at 8:05 pm on June 13, 2020:

Thanks for the suggestion! I’ve updated it but I think Travis timed out on one of the builds.

DrahtBot commented at 2:50 am on June 13, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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.

glozow force-pushed on Jun 13, 2020

jnewbery commented at 0:42 am on June 14, 2020: member

Code review ACK 18a2e0a3f7071b8df6b20ecc1b34b3f2abb13cfc

Great tidy-up. Thanks Gloria!

naumenkogs commented at 7:35 am on June 14, 2020: member

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 :)

in src/net_processing.cpp:3495 in 294b96772b outdated

3489@@ -3490,13 +3490,15 @@ bool ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRec
3490     }
3491 
3492     if (msg_type == NetMsgType::FILTERCLEAR) {
3493+        if (!(pfrom.GetLocalServices() & NODE_BLOOM)) {
3494+            pfrom.fDisconnect = true;
3495+            return false;

MarcoFalke commented at 1:05 pm on June 14, 2020:

in commit 294b96772b7d184a0cd6f56bd76ef118f4c78cc6:

Any reason to return false here? The only effect is that it will log a line with FAILED with the NET debug category. However, it should be pretty clear from the log already that receiving a filterclear message and then disconnecting a peer means the message was ignored.

naumenkogs commented at 1:53 pm on June 14, 2020:

I was confused here too, but it seems like this is a common thing to do. Many occurences of


 pfrom.fDisconnect = true;
 return false;

MarcoFalke commented at 2:02 pm on June 14, 2020:

The majority of disconnects is followed by a return true;. The ones that don’t should be changed too. And longer term the return value should be removed completely.

glozow commented at 6:54 pm on June 14, 2020:

Thanks! Yup, my reasoning was that’s what it looked like in other places 😅

in src/net_processing.cpp:3461 in 18a2e0a3f7 outdated

3455@@ -3466,6 +3456,10 @@ bool ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRec
3456     }
3457 
3458     if (msg_type == NetMsgType::FILTERADD) {
3459+        if (!(pfrom.GetLocalServices() & NODE_BLOOM)) {
3460+            pfrom.fDisconnect = true;
3461+            return false;

MarcoFalke commented at 1:07 pm on June 14, 2020:

in commit 18a2e0a3f7071b8df6b20ecc1b34b3f2abb13cfc:

Same

MarcoFalke approved

MarcoFalke commented at 1:09 pm on June 14, 2020: member

ACK 18a2e0a3f7071b8df6b20ecc1b34b3f2abb13cfc nice cleanup 🌫

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 18a2e0a3f7071b8df6b20ecc1b34b3f2abb13cfc nice cleanup 🌫
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhzKAv/ejpwEnqSww3TRDRR7fvVaVti9v81jtyV0QVIwfjHvYw3MD3BSifUI8KF
 8KCWYTX/MVDobco2GQR3Iw1Xnc1oGgH8416c5ehDMTiO9T7+DftefLR1hHb8nidXs
 9bPrsTfbwvh7TkPsEwVWjSu+lVL1CtYDHDcdyP/t2u8p/r/RpCq1WxEwmSQgqybTK
10rCIdWUNQ1yrNd5/gkpydpzIZPLpui19aA+lxwndwuWiME0hTc2li/qE9acsFSLSI
11PXM/gClEJ6aKDyVX2AOGfpQv1KEY5OReDsUBi2jdXRXI7n3/1D6kz9YGar4DIhVy
12nuVcQFEEMGvGEclQVeIp5ik5JHPta0bFm/i6rSdHSk1Y+b5zqgYmSeNqnUwcUxcG
135jyVXszUw0p61yVgy/7CFaFvIr8oa2xjAt8vvLogdePIjH320j2TA3UBhOWLBkfU
14bcWafRJwm13cB3VtSB00HNZ7LBb5mS8AmsV0sY4U4a9//RbgBv/HHgrTkjbFmiCQ
15gqH6/zVZ
16=Wizi
17-----END PGP SIGNATURE-----

Timestamp of file with hash 76972080cdb7d6db860684d9cc7ddc281674c2c67368d5f83cad36c0fa0849c0 -

glozow renamed this:
~~p2p: disconnect peers that send filterclear~~
p2p: disconnect peers that send filterclear + update existing filter msg disconnect logic
on Jun 14, 2020

[netprocessing] disconnect node that sends filterclear

-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

1c6b787e03

[test] test disconnect for filterclear ff8c430c65

[p2p/refactor] move disconnect logic and remove misbehaving

-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.

3a10d935ac

glozow force-pushed on Jun 14, 2020

MarcoFalke commented at 10:59 am on June 15, 2020: member

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

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 3a10d935ac only change is replacing false with true 🚝
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiicAwAuImcvIy26lnaw4VoTkzdS0iEgf2zA6xyuFKb3zUCSKpX80mPbnrdDxue
 832BIzcZfgyy5FvhWuJddItl3wEdmnyJK/1vZqRogs/Q4oljfVaDxzdIPKc3lhoG/
 9GNOut9oyn0WAhL26QQDdqWwCq94GFJ7DSClIfsozyXQcs61ylKQULEBpP039uR1R
10/PQEKZoS8t2ThDQXHgcLmC04QYARp8PX5RvSBbxFQz75Kl9vUD9Qfs3/KlCHBThU
11dKwWnLt/yQp3w0Wz4XVgjborfhgeLVVzmvkBRZboFEMXJPaC1OkinxS3nydh6MYz
127fMTVM61b95qy21pBSr0iT8T/gslbr7sj2H9OEkfU7t8s096qnPfVgd/qTtMmneR
13v5NN9WVf9SXq8nmhGZQPLL/gehW1RGC99vSL3MwrQQwspz13lU+3wAXEGV16k7Py
14uFKUrHkUlNitTxn9vFWoeTLIlWFtJ6Qc2B6dc3wMJ7k1KHfv2yI1CA1OcdO4osNv
15pRAS6UPd
16=ZuJt
17-----END PGP SIGNATURE-----

Timestamp of file with hash d49fb833782cb07ba218eb5ea866b2c2f12e58a1ac6670b3a225942fdd3a8e52 -

jnewbery commented at 1:21 pm on June 15, 2020: member

Code review ACK 3a10d935ac8ebabdfd336569d943f042ff84b13e

gillichu commented at 5:16 pm on June 15, 2020: none

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

gillichu commented at 6:46 pm on June 15, 2020: none

tested ACK: quick test_runner on macOS 3a10d93

glozow commented at 6:49 pm on June 15, 2020: member

😂 Thanks @gillichu, I really appreciate the effort 🙏

It’s [`commit`](link)

jnewbery commented at 8:53 pm on June 15, 2020: member

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 commented at 9:36 pm on June 15, 2020: member

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 commented at 6:39 am on June 16, 2020: member

utACK 3a10d93

fanquake merged this on Jun 16, 2020

fanquake closed this on Jun 16, 2020

glozow deleted the branch on Jul 25, 2020

rebroad commented at 7:24 am on May 3, 2021: contributor

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?

MarcoFalke commented at 7:52 am on May 3, 2021: member

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

deadalnix referenced this in commit a496d457ee on May 13, 2021

DrahtBot locked this on Aug 16, 2022

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

Conflicts