p2p: Stop treating message size as misbehavior #25321

1. 
   MarcoFalke commented at 2:08 PM on June 9, 2022: member

   Currently this is treated as misbehaviour with a score of 20 (out of 100). Practically, on my node no peer that hit a score of 20 hit the threshold of 100, so it seems fine to skip.

   Effectively, large "vector" messages will be dropped, making them a bandwidth DoS if sent repeatedly. However, any peer can do a bandwidth DoS by sending a "vector" messages of smaller size, or messages of type notfound (which don't have any misbehaviour score for size), or a large block or tx message.

   This change skips over a headers, getdata, inv, addr, and addrv2 message if it is too large.

   For reference, there is no BIP that prescribes behaviour on oversized messages of those types. BIP 155 is the only BIP that mentions a size limit about the message types:

   One message can contain up to 1,000 addresses. Clients SHOULD reject messages with more addresses. https://en.bitcoin.it/wiki/BIP_0155#Specification

   For reference, getblocks and getheaders since commit e254ff5d53b79bee29203b965fca572f218bff54 will unconditionally disconnect on size. It will even disconnect noban peers and manual connections, but this seems fine to keep.

2. fanquake added the label P2P on Jun 9, 2022
3. 
   MarcoFalke commented at 2:16 PM on June 9, 2022: member

   Data on a node:

   • This is only hit once per connection, going from score 0 to 20.

   $ grep ' message size = ' ~/.bitcoin/debug.log 
   2020-12-28T17:20:47Z [msghand] Misbehaving: peer=380051 (0 -> 20): headers message size = 48166
   2021-01-04T17:58:47Z [msghand] Misbehaving: peer=567963 (0 -> 20): headers message size = 37000
   2021-03-18T20:00:39Z [msghand] Misbehaving: peer=2442599 (0 -> 20): headers message size = 29794
   2021-03-18T21:55:05Z [msghand] Misbehaving: peer=2445041 (0 -> 20): headers message size = 40538
   2021-03-23T09:05:43Z [msghand] Misbehaving: peer=2563428 (0 -> 20): headers message size = 43132
   2021-04-25T10:54:19Z [msghand] Misbehaving: peer=3388661 (0 -> 20): addrv2 message size = 1998
   2021-05-08T14:39:59Z [msghand] Misbehaving: peer=3713735 (0 -> 20): headers message size = 30507
   2021-05-23T21:06:54Z [msghand] Misbehaving: peer=4092238 (0 -> 20): headers message size = 41553
   2021-06-12T01:44:57Z [msghand] Misbehaving: peer=4525871 (0 -> 20): headers message size = 2309
   

   • This is only hit on peers that have a bogus UA (except for the addrv2 violation, which was a debug version of Bitcoin Core)

   $ grep 'receive version message' ~/.bitcoin/debug.log | grep --extended-regex '\<(380051|567963|2442599|2445041|2563428|3388661|3713735|4092238|4525871)\>' 
   2020-12-28T17:20:23Z [msghand] receive version message: : version 50001, blocks=187250, us=34.82.170.246:8333, peer=380051
   2021-01-04T17:58:42Z [msghand] receive version message: /Satoshi:0.6.0.6/: version 60000, blocks=36914, us=34.82.170.246:8333, peer=567963
   2021-03-18T20:00:35Z [msghand] receive version message: : version 32400, blocks=180700, us=34.82.170.246:8333, peer=2442599
   2021-03-18T21:54:28Z [msghand] receive version message: /Satoshi:0.6.0.6/: version 60000, blocks=191436, us=34.82.170.246:8333, peer=2445041
   2021-03-23T09:05:42Z [msghand] receive version message: : version 32100, blocks=194744, us=34.82.170.246:8333, peer=2563428
   2021-04-25T10:47:48Z [msghand] receive version message: /Satoshi:21.99.0/: version 70016, blocks=680547, us=34.82.170.246:43860, peer=3388661
   2021-05-08T14:38:27Z [msghand] receive version message: : version 32400, blocks=188779, us=34.82.170.246:8333, peer=3713735
   2021-05-23T21:06:32Z [msghand] receive version message: : version 32400, blocks=201925, us=34.82.170.246:8333, peer=4092238
   2021-06-12T01:44:56Z [msghand] receive version message: : version 32400, blocks=165237, us=34.82.170.246:8333, peer=4525871
   

   • The peers got disconnected anyway

   2020-12-28T17:28:29Z [net] socket closed for peer=380051
   2020-12-28T17:28:29Z [net] disconnecting peer=380051
   2020-12-28T17:28:29Z [net] Cleared nodestate for peer=380051
   
   2021-01-04T18:03:18Z [net] socket closed for peer=567963
   2021-01-04T18:03:18Z [net] disconnecting peer=567963
   2021-01-04T18:03:18Z [net] Cleared nodestate for peer=567963
   
   2021-03-18T20:12:52Z [net] socket recv error for peer=2442599: Connection reset by peer (104)
   2021-03-18T20:12:52Z [net] disconnecting peer=2442599
   2021-03-18T20:12:52Z [net] Cleared nodestate for peer=2442599
   
   2021-03-18T21:58:23Z [net] socket recv error for peer=2445041: Connection reset by peer (104)
   2021-03-18T21:58:23Z [net] disconnecting peer=2445041
   2021-03-18T21:58:23Z [net] Cleared nodestate for peer=2445041
   
   2021-03-23T09:25:00Z [net] socket recv error for peer=2563428: Connection reset by peer (104)
   2021-03-23T09:25:00Z [net] disconnecting peer=2563428
   2021-03-23T09:25:00Z [net] Cleared nodestate for peer=2563428
   
   2021-04-26T00:12:56Z [net] ping timeout: 1207.998686s
   2021-04-26T00:12:56Z [net] disconnecting peer=3388661
   2021-04-26T00:13:07Z [net] Cleared nodestate for peer=3388661
   
   2021-05-08T16:08:04Z [net] socket recv error for peer=3713735: Connection reset by peer (104)
   2021-05-08T16:08:04Z [net] disconnecting peer=3713735
   2021-05-08T16:08:04Z [net] Cleared nodestate for peer=3713735
   
   2021-05-24T00:12:31Z [net] socket recv error for peer=4092238: Connection reset by peer (104)
   2021-05-24T00:12:31Z [net] disconnecting peer=4092238
   2021-05-24T00:12:31Z [net] Cleared nodestate for peer=4092238
   
   2021-06-12T02:12:09Z [net] socket recv error for peer=4525871: Connection timed out (110)
   2021-06-12T02:12:09Z [net] disconnecting peer=4525871
   2021-06-12T02:12:09Z [net] Cleared nodestate for peer=4525871
   

4. MarcoFalke force-pushed on Jun 9, 2022
5. MarcoFalke force-pushed on Jun 9, 2022
6. 
   DrahtBot commented at 6:49 PM on June 9, 2022: member

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   No conflicts as of last run.

7. in src/net_processing.cpp:3002 in fac0bb6947 outdated

   2996 | @@ -2997,9 +2997,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
   2997 |              return;
   2998 |          }
   2999 |  
   3000 | -        if (vAddr.size() > MAX_ADDR_TO_SEND)
   3001 | -        {
   3002 | -            Misbehaving(pfrom.GetId(), 20, strprintf("%s message size = %u", msg_type, vAddr.size()));
   

   ---

   ---

   

   Riahiamirreza commented at 8:06 PM on June 9, 2022:

   Why did the Misbehaving was replaced by LogPrint ? I took a look at Misbehaving and saw that it calls the LogPrint inside of itself. So it seems to be a logging function with some other stuff. Would explain the difference between LogPrint and Misbehaving?

   ---

   

   MarcoFalke commented at 5:05 AM on June 10, 2022:

   This is explained in the pull request description. Practically, I couldn't find a difference between Misbehaving and LogPrint on one of my nodes. In theory there is a difference, but I explain why it shouldn't matter. (The difference is that Misbehaving may disconnect and discourage the peer, unless it is a manual or noban connection).

   ---

   

   Riahiamirreza commented at 5:42 AM on June 10, 2022:

   Thank you, @MarcoFalke I'm new to bitcoin source code and have lots of questions, posting my basic questions as comment on PRs good way to ask my questions? Or there are better places? such as IRC or bitcoin-talk? I'm aware that this question has nothing to do with the PR but I couldn't find a way to send direct message to you.

   ---

   

   MarcoFalke commented at 6:03 AM on June 10, 2022:

   You can also ask on the dev IRC (https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#communication-channels), the review IRC (https://bitcoincore.reviews/), or on Bitcoin StackExchange.

8. MarcoFalke deleted a comment on Jun 10, 2022
9. DrahtBot added the label Needs rebase on Jun 27, 2022
10. p2p: Stop treating message size as misbehavior faabc9f389
11. MarcoFalke force-pushed on Jun 27, 2022
12. fanquake requested review from ajtowns on Jun 27, 2022
13. fanquake requested review from sdaftuar on Jun 27, 2022
14. DrahtBot removed the label Needs rebase on Jun 27, 2022
15. fanquake requested review from mzumsande on Jun 27, 2022
16. 
    sdaftuar commented at 9:19 PM on June 27, 2022: member

    I think it would be helpful to have more context and understanding of how to think about peer selection and eviction in order to review making changes to these peer misbehavior scores. I agree that what we have is arbitrary and not consistent, but I also think that we could go in two different directions here -- to either be more or less permissive -- and it's not obvious to me which is better.

    The DoS reasoning you provide, which is that large messages of these types are no worse than large notfound messages, seems pretty convincing to me that we should not bother assigning DoS-points to inbound peers that send us these messages. But if an outbound peer is sending us messages like this, then perhaps we do want to disconnect and find another outbound peer? I wonder if it might make sense to differentiate behavior based on inbound/outbound-ness?

    Really though I don't have a strong feeling either way about this change; it's probably not harmful, so if people think this is a good change I don't object in principle, but hard for me to say what approach makes the most sense.

17. MarcoFalke closed this on Jul 5, 2022

    ---

18. MarcoFalke deleted the branch on Jul 5, 2022
19. DrahtBot locked this on Jul 5, 2023

Contributors

MarcoFalkeDrahtBotRiahiamirrezasdaftuar

Review Requested
ajtownssdaftuarmzumsande