Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 10, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,8 +855,8 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Don't send reject message with code 0 or an internal reject code.
if (it != mapBlockSource.end() && State(it->second.first) && state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) {
CBlockReject reject = {(unsigned char)state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), hash};
State(it->second.first)->rejects.push_back(reject);
if (nDoS > 0 && it->second.second)
Expand Down Expand Up @@ -1942,7 +1942,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
LogPrint("mempoolrej", "%s from peer=%d was not accepted: %s\n", tx.GetHash().ToString(),
pfrom->id,
FormatStateMessage(state));
if (state.GetRejectCode() < REJECT_INTERNAL) // Never send AcceptToMemoryPool's internal codes over P2P
if (state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) // Never send AcceptToMemoryPool's internal codes over P2P
connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::REJECT, strCommand, (unsigned char)state.GetRejectCode(),
state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), inv.hash));
if (nDoS > 0) {
Expand Down
10 changes: 6 additions & 4 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1914,7 +1914,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
REJECT_INVALID, "bad-cb-amount");

if (!control.Wait())
return state.DoS(100, false);
return state.DoS(100, error("%s: CheckQueue failed", __func__), REJECT_INVALID, "block-validation-failed");
int64_t nTime4 = GetTimeMicros(); nTimeVerify += nTime4 - nTime2;
LogPrint("bench", " - Verify %u txins: %.2fms (%.3fms/txin) [%.2fs]\n", nInputs - 1, 0.001 * (nTime4 - nTime2), nInputs <= 1 ? 0 : 0.001 * (nTime4 - nTime2) / (nInputs-1), nTimeVerify * 0.000001);

Expand Down Expand Up @@ -2881,10 +2881,12 @@ static bool CheckIndexAgainstCheckpoint(const CBlockIndex* pindexPrev, CValidati
return true;

int nHeight = pindexPrev->nHeight+1;
// Don't accept any forks from the main chain prior to last checkpoint
// Don't accept any forks from the main chain prior to last checkpoint.
// GetLastCheckpoint finds the last checkpoint in MapCheckpoints that's in our
// MapBlockIndex.
CBlockIndex* pcheckpoint = Checkpoints::GetLastCheckpoint(chainparams.Checkpoints());
if (pcheckpoint && nHeight < pcheckpoint->nHeight)
return state.DoS(100, error("%s: forked chain older than last checkpoint (height %d)", __func__, nHeight));
return state.DoS(100, error("%s: forked chain older than last checkpoint (height %d)", __func__, nHeight), REJECT_CHECKPOINT, "bad-fork-prior-to-checkpoint");

return true;
}
Expand Down Expand Up @@ -3083,7 +3085,7 @@ static bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state
CBlockIndex* pindexPrev = NULL;
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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pindexPrev = (*mi).second;
if (pindexPrev->nStatus & BLOCK_FAILED_MASK)
return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk");
Expand Down
4 changes: 2 additions & 2 deletions test/functional/p2p-fullblocktest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ def update_block(block_number, new_transactions):

# Extend the b26 chain to make sure bitcoind isn't accepting b26
b27 = block(27, spend=out[7])
yield rejected(RejectResult(0, b'bad-prevblk'))
yield rejected(False)

# Now try a too-large-coinbase script
tip(15)
Expand All @@ -410,7 +410,7 @@ def update_block(block_number, new_transactions):

# Extend the b28 chain to make sure bitcoind isn't accepting b28
b29 = block(29, spend=out[7])
yield rejected(RejectResult(0, b'bad-prevblk'))
yield rejected(False)

# b30 has a max-sized coinbase scriptSig.
tip(23)
Expand Down