Don't process blocktxns when we have the block already. #9030

pull rebroad wants to merge 1 commits into bitcoin:master from rebroad:BlocktxnExits changing 1 files +12 −0

rebroad commented at 10:39 AM on October 27, 2016: contributor

Two checks added. Without this a debug is shown saying the block was "unexpected" but what was actually happening is that another peer is providing the full block before the compact block sequence completes.

Fixes #9031
rebroad force-pushed on Oct 27, 2016
laanwj added the label P2P on Oct 27, 2016

in src/main.cpp:None in 0a7fc7fa4c outdated

5831 |          vRecv >> resp;
5832 |  
5833 | +        BlockMap::iterator mi = mapBlockIndex.find(resp.blockhash);
5834 | +        if (mi == mapBlockIndex.end()) {
5835 | +            LogPrint("block", "recv blocktxn %s size=%d not recognised peer=%d\n", resp.blockhash.ToString(), nSize, pfrom->id);
5836 | +            return true;

TheBlueMatt commented at 1:21 PM on October 27, 2016:

This will entirely break downloading of blocks via compact blocks.

rebroad commented at 3:45 AM on October 29, 2016:

How so?

in src/main.cpp:None in 0a7fc7fa4c outdated

5826 | @@ -5827,8 +5827,20 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5827 |      else if (strCommand == NetMsgType::BLOCKTXN && !fImporting && !fReindex) // Ignore blocks received while importing
5828 |      {
5829 |          BlockTransactions resp;
5830 | +        int nSize = vRecv.size();
5831 |          vRecv >> resp;
5832 |  
5833 | +        BlockMap::iterator mi = mapBlockIndex.find(resp.blockhash);

TheBlueMatt commented at 1:21 PM on October 27, 2016:

This needs cs_main

rebroad commented at 3:46 AM on October 29, 2016:

Do you mean I need to add a LOCK(cs_main) before this line? I'm still trying to get my head around locks... what is the risk without obtaining a lock first please?

TheBlueMatt commented at 4:04 AM on October 30, 2016: member
Oops, misread the line.

On October 28, 2016 11:45:10 PM EDT, R E Broadley notifications@github.com wrote:
rebroad commented on this pull request.

vRecv >> resp;

BlockMap::iterator mi = mapBlockIndex.find(resp.blockhash);

if (mi == mapBlockIndex.end()) {

LogPrint("block", "recv blocktxn %s size=%d not recognised
peer=%d\n", resp.blockhash.ToString(), nSize, pfrom->id);

return true;

How so?

You are receiving this because you commented. Reply to this email directly or view it on GitHub: #9030
TheBlueMatt commented at 4:05 AM on October 30, 2016: member
Yea, you need a LOCK(cs_main).

See https://en.m.wikipedia.org/wiki/Lock_(computer_science) for more.

On October 28, 2016 11:46:17 PM EDT, R E Broadley notifications@github.com wrote:
rebroad commented on this pull request.

vRecv >> resp;

BlockMap::iterator mi = mapBlockIndex.find(resp.blockhash);

Do you mean I need to add a LOCK(cs_main) before this line? I'm still trying to get my head around locks... what is the risk without obtaining a lock first please?

You are receiving this because you commented. Reply to this email directly or view it on GitHub: #9030
rebroad force-pushed on Oct 31, 2016
rebroad commented at 7:18 AM on October 31, 2016: contributor

@TheBlueMatt Thanks. Now moved additions to within the LOCK.

Don't process blocktxns when we have the block already.

Also, not when we don't recognise the block. (Saves further checks).

08aabab3ca

rebroad force-pushed on Nov 1, 2016
sdaftuar commented at 4:02 PM on November 1, 2016: member

I don't think there's a situation where this code change would improve the behavior of the node. It's already the case that if we're not expecting a BLOCKTXN, then we abort processing (in the code just below the code you're adding here).

This change also further complicates the logic, as if somehow we are in the (unlikely) situation where we received a block, pruned it, and then somehow re-requested it via CMPCTBLOCK/GETBLOCKTXN, then this code which returns early if nTx > 0 without marking the block as received would cause the block download to stall and the peer to be disconnected.

Unless there's some case you have in mind where this patch improves node behavior, I'd say we should leave this alone.

EDIT: perhaps just change the LogPrint to indicate that either the message was unexpected, or the block was already received?
rebroad closed this on Dec 25, 2016
rebroad deleted the branch on Dec 25, 2016
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#9031 Many "peer sent us block transactions for a block we weren't expecting" errors