[p2p] Send the correct error code in reject messages #10135
Conversation
|
Extended test run succeeds for me locally. |
| int nDoS = 0; | ||
| if (state.IsInvalid(nDoS)) { | ||
| if (it != mapBlockSource.end() && State(it->second.first)) { | ||
| assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes |
There was a problem hiding this comment.
nit: I don't feel strongly but we could leave this assert in, no?
There was a problem hiding this comment.
The if statement has been modified to check that state.GetRejectCode() < REJECT_INTERNAL (so this assert can now never fail)
| BlockMap::iterator mi = mapBlockIndex.find(block.hashPrevBlock); | ||
| if (mi == mapBlockIndex.end()) | ||
| return state.DoS(10, error("%s: prev block not found", __func__), 0, "bad-prevblk"); | ||
| return state.DoS(10, error("%s: prev block not found", __func__), 0, "prev-blk-not-found"); |
There was a problem hiding this comment.
Sorry I didn't think of this before, but perhaps we were sending a reject message here specifically because we're assigning DoS points. Suppressing the reject message will just cause the peer to have less information about why they're being disconnected.
I'm not sure I think BIP 61 is a great idea (in particular, the lack of versioning makes things difficult) but the motivation of it was in part to let peers know why they're being banned. So maybe it makes more sense to use a REJECT_INVALID message here, rather than omit the reject message altogether, even though the block might not actually be invalid if its missing parent was provided?
Really, though, I don't care what we do here, just raising the point in case others have an opinion. Your change here is fine with me.
There was a problem hiding this comment.
I'd rather extend BIP61 than deliberately send a reject message that's incorrect (the block here isn't INVALID, it just doesn't connect with our chain).
If you prefer that we send a reject message, I think we should at least change the error code to a 0x4* value and change the message to 'prev-blk-not-found'.
|
I don't really have any objection to making the behavior more correct, but I would rather more or less stop sending reject messages. There is only one or two places where I'm aware of any implementation doing anything remotely useful with them (some bitcoinj users use transaction rejection to figure out that their txn hasn't relayed at all, which isn't reliable but is arguably better than nothing). They've been a source of serious bugs several times, add to code complexity, and like other error handling are fairly hard to get precise behavior from without more or less setting in stone algorithms that don't need to be set in stone. Plus they waste bandwidth... I've personally never found them useful in troubleshooting an issue. I can't help but think that any productive use for them could be better accomplished by some other means. |
|
@gmaxwell - I've heard that argument from several people, and I'm mostly on board with it. Thanks for documenting it here. reject messages are somewhat useful in black box testing. We can send in invalid messages and assert that bitcoin rejected them for the right reason. The old style 'comparison' test cases use this a lot. That's not a justification for spending a heap of time on improving them, but I think there's no harm in small fixes like this. And if no-one's using them in live deployments, then it should be an uncontentious change, right? :) Definitely agree with improving error-handling in general. I don't know exactly what that would look like, but it's beyond the scope of this PR. For my own education, do you have references to the serious bugs that reject messages have caused? |
PR #4903 was mostly what I was thinking of there, also the original implementation of reject messages themselves printed unsanitized bytes to the logs and IIRC also didn't limit the size of strings, though I believe both of these were fixed before merge. We have also had many PRs for where we broke sending reject messages (or sent the 'wrong' one)-- (PR #7179 comes to mind.) For tests, we should eventually consider something more targeted. (I have thoughts there but it's too far a tangent for this discussion.-- My comment here was in vague support of your changes but also advising that I think we should be thinking about alternatives going forward.) |
|
utACK 5d08c9c
Also for things that do custom transaction broadcasting (for example bitcoin-submittx ) it's marginally useful to see if transactions have been accepted or not, and why. I wouldn't care deeply if they were removed, though I don't see any urgency in doing so. There were indeed some bugs in initial deployment of this feature (including, as I remember, one that could cause a reject message to be sent for a reject message), but I'd say those have been ironed out by now. Attack surface is mostly introduced by parsing/accepting messages not by emitting them, and we don't do anything with them besides optional logging (which could be in a separate logging category from NET). |
5d08c9c Send the correct error code in reject messages (John Newbery) Tree-SHA512: 0cd3ef3ae202584b138cc0bbfba4125635822e0c5a755fb9276a604b39286959ab22dabc3104aa5d7e71358cd69d965de2a333ff04bf3e8ed43cf0296ac01264
…ages 5d08c9c Send the correct error code in reject messages (John Newbery) Tree-SHA512: 0cd3ef3ae202584b138cc0bbfba4125635822e0c5a755fb9276a604b39286959ab22dabc3104aa5d7e71358cd69d965de2a333ff04bf3e8ed43cf0296ac01264
This PR corrects a few places were incorrect error codes are send in reject messages, or where reject messages are sent when they shouldn't be:
@sdaftuar we discussed this. Let me know if you have any questions.