[p2p] Send the correct error code in reject messages

jnewbery commented at 6:23 pm on March 31, 2017: member

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:

if a block failed to validate due to CheckQueue returning false, we should send a reject message with REJECT_INVALID
if we reject a block because it’s a fork from the chain before a checkpoint block that we’ve already processed, we should send a reject message with REJECT_CHECKPOINT
if we reject a block because we don’t have it’s parent block, we shouldn’t send a reject message (because the block is not necessarily invalid). @sdaftuar we discussed this. Let me know if you have any questions.

Send the correct error code in reject messages 5d08c9c579

jnewbery renamed this:
~~Send the correct error code in reject messages~~
[p2p] Send the correct error code in reject messages
on Mar 31, 2017

jnewbery commented at 6:26 pm on March 31, 2017: member

Extended test run succeeds for me locally.

in src/net_processing.cpp:859 in 5d08c9c579

854@@ -855,8 +855,8 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta
855 
856     int nDoS = 0;
857     if (state.IsInvalid(nDoS)) {
858-        if (it != mapBlockSource.end() && State(it->second.first)) {
859-            assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes

sdaftuar commented at 6:28 pm on March 31, 2017:

nit: I don’t feel strongly but we could leave this assert in, no?

jnewbery commented at 7:46 pm on March 31, 2017:

The if statement has been modified to check that state.GetRejectCode() < REJECT_INTERNAL (so this assert can now never fail)

in src/validation.cpp:3088 in 5d08c9c579

3084@@ -3083,7 +3085,7 @@ static bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state
3085         CBlockIndex* pindexPrev = NULL;
3086         BlockMap::iterator mi = mapBlockIndex.find(block.hashPrevBlock);
3087         if (mi == mapBlockIndex.end())
3088-            return state.DoS(10, error("%s: prev block not found", __func__), 0, "bad-prevblk");
3089+            return state.DoS(10, error("%s: prev block not found", __func__), 0, "prev-blk-not-found");

sdaftuar commented at 6:37 pm on March 31, 2017:

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.

jnewbery commented at 7:52 pm on March 31, 2017:

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

sdaftuar approved

sdaftuar commented at 6:38 pm on March 31, 2017: member

One nit you can take or leave, and one point of information. utACK 5d08c9c579ba8cc7b684105c6a08263992b08d52

fanquake added the label P2P on Mar 31, 2017

gmaxwell commented at 7:36 pm on April 1, 2017: contributor

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.

jnewbery commented at 11:59 pm on April 1, 2017: member

@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?

gmaxwell commented at 1:07 am on April 2, 2017: contributor

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

laanwj commented at 12:23 pm on April 2, 2017: member

utACK https://github.com/bitcoin/bitcoin/pull/10135/commits/5d08c9c579ba8cc7b684105c6a08263992b08d52

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.

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

laanwj merged this on Apr 10, 2017

laanwj closed this on Apr 10, 2017

laanwj referenced this in commit e19586a8a9 on Apr 10, 2017

PastaPastaPasta referenced this in commit 8774fba521 on May 10, 2019

PastaPastaPasta referenced this in commit d091bf939a on May 15, 2019

PastaPastaPasta referenced this in commit 23530a88a8 on May 20, 2019

PastaPastaPasta referenced this in commit 3286a7e2bb on May 21, 2019

barrystyle referenced this in commit e25afb12e1 on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021

[p2p] Send the correct error code in reject messages #10135