p2p: Stop treating message size as misbehavior #25321
Conversation
|
Data on a node:
|
fa373d3 to
fac0bb6
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
src/net_processing.cpp
Outdated
| @@ -2997,9 +2997,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, | |||
| return; | |||
| } | |||
|
|
|||
| if (vAddr.size() > MAX_ADDR_TO_SEND) | |||
| { | |||
| Misbehaving(pfrom.GetId(), 20, strprintf("%s message size = %u", msg_type, vAddr.size())); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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 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. |
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 largeblockortxmessage.This change skips over a
headers,getdata,inv,addr, andaddrv2message 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:
For reference,
getblocksandgetheaderssince commit e254ff5 will unconditionally disconnect on size. It will even disconnect noban peers and manual connections, but this seems fine to keep.