Get rid of MAX_GETBLOCKTXN_DEPTH constant

ryanofsky · ryanofsky · commit 36692c9b426a · 2016-10-26T17:40:07.000-04:00
Instead of allowing getblocktxn requests for the most recent 10 blocks (and ignoring requests for other blocks), allow getblocktxn requests for blocks more recent than the oldest outstanding cmpctblock message. This fixes a race condition that can happen during tests where more than 10 cmpctblock messages are sent to a peer before the first incoming getblocktxn messages can be processed, causing sync timeouts when the peer is not to be able to get in sync. This is an alternative to the fix in chaincodelabs/ccl-bitcoin@fix-8842-sync-timeouts Disadvantages of this implementation: - Requires a new state member. - Larger code change than the other fix. Advantages of this implementation: - Gets rid of the MAX_GETBLOCKTXN_DEPTH constant (which is just arbitrarily set at twice the value of MAX_CMPCTBLOCK_DEPTH). - Makes the protocol simpler and more deterministic from the perspective of receiving peers. Instead of getblocktxn responses yielding either blocktxn or block messages depending on timing and unrelated internal state of the sending node, valid getblocktxn requests will reliably trigger blocktxn responses and no other types of response. - Unlike the other fix, this doesn't require an update to BIP-152.
diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py
@@ -589,12 +589,12 @@ def test_incorrect_blocktxn_response(self, node, test_node, version):
         assert_equal(int(node.getbestblockhash(), 16), block.sha256)
 
     def test_getblocktxn_handler(self, node, test_node, version):
-        # bitcoind won't respond for blocks whose height is more than 15 blocks
+        # bitcoind won't respond for blocks whose height is more than 5 blocks
         # deep.
-        MAX_GETBLOCKTXN_DEPTH = 10
+        MAX_CMPCTBLOCK_DEPTH = 5
         chain_height = node.getblockcount()
-        current_height = chain_height
-        while (current_height >= chain_height - MAX_GETBLOCKTXN_DEPTH):
+        current_height = chain_height - MAX_CMPCTBLOCK_DEPTH + 1
+        while current_height <= chain_height:
             block_hash = node.getblockhash(current_height)
             block = FromHex(CBlock(), node.getblock(block_hash, False))
 
@@ -621,10 +621,10 @@ def test_getblocktxn_handler(self, node, test_node, version):
                         # Check that the witness matches
                         assert_equal(tx.calc_sha256(True), block.vtx[index].calc_sha256(True))
                 test_node.last_blocktxn = None
-            current_height -= 1
+            current_height += 1
 
         # Next request should be ignored, as we're past the allowed depth.
-        block_hash = node.getblockhash(current_height)
+        block_hash = node.getblockhash(chain_height)
         msg.block_txn_request = BlockTransactionsRequest(int(block_hash, 16), [0])
         test_node.send_and_ping(msg)
         with mininode_lock:
diff --git a/src/main.cpp b/src/main.cpp
@@ -308,6 +308,7 @@ struct CNodeState {
      * otherwise: whether this peer sends non-witnesses in cmpctblocks/blocktxns.
      */
     bool fSupportsDesiredCmpctVersion;
+    int nGetblocktxnMinHeight;
 
     CNodeState() {
         fCurrentlyConnected = false;
@@ -330,6 +331,7 @@ struct CNodeState {
         fHaveWitness = false;
         fWantsCmpctWitness = false;
         fSupportsDesiredCmpctVersion = false;
+        nGetblocktxnMinHeight = std::numeric_limits<int>::max();
     }
 };
 
@@ -4922,6 +4924,9 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam
                         bool fPeerWantsWitness = State(pfrom->GetId())->fWantsCmpctWitness;
                         if (CanDirectFetch(consensusParams) && mi->second->nHeight >= chainActive.Height() - MAX_CMPCTBLOCK_DEPTH) {
                             CBlockHeaderAndShortTxIDs cmpctblock(block, fPeerWantsWitness);
+                            if (State(pfrom->GetId())->nGetblocktxnMinHeight > mi->second->nHeight) {
+                                State(pfrom->GetId())->nGetblocktxnMinHeight = mi->second->nHeight;
+                            }
                             pfrom->PushMessageWithFlag(fPeerWantsWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::CMPCTBLOCK, cmpctblock);
                         } else
                             pfrom->PushMessageWithFlag(fPeerWantsWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::BLOCK, block);
@@ -5450,8 +5455,11 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
             return true;
         }
 
-        if (it->second->nHeight < chainActive.Height() - MAX_BLOCKTXN_DEPTH) {
-            LogPrint("net", "Peer %d sent us a getblocktxn for a block > %i deep", pfrom->id, MAX_BLOCKTXN_DEPTH);
+        if (it->second->nHeight < State(pfrom->GetId())->nGetblocktxnMinHeight) {
+            // If an older block is requested that this node didn't previously
+            // send a CMPCTBLOCK message for, just ignore the request instead
+            // of doing a potentially expensive block read.
+            LogPrint("net", "Peer %d sent us a getblocktxn for an out-of-bounds block.", pfrom->id);
             return true;
         }
 
@@ -5468,6 +5476,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
             resp.txn[i] = block.vtx[req.indexes[i]];
         }
         pfrom->PushMessageWithFlag(State(pfrom->GetId())->fWantsCmpctWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::BLOCKTXN, resp);
+
+        State(pfrom->GetId())->nGetblocktxnMinHeight = it->second->nHeight + 1;
     }
 
 
@@ -6636,6 +6646,9 @@ bool SendMessages(CNode* pto, CConnman& connman)
                     CBlock block;
                     assert(ReadBlockFromDisk(block, pBestIndex, consensusParams));
                     CBlockHeaderAndShortTxIDs cmpctblock(block, state.fWantsCmpctWitness);
+                    if (state.nGetblocktxnMinHeight > pBestIndex->nHeight) {
+                        state.nGetblocktxnMinHeight = pBestIndex->nHeight;
+                    }
                     pto->PushMessageWithFlag(state.fWantsCmpctWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::CMPCTBLOCK, cmpctblock);
                     state.pindexBestHeaderSent = pBestIndex;
                 } else if (state.fPreferHeaders) {
diff --git a/src/main.h b/src/main.h
@@ -93,8 +93,6 @@ static const unsigned int MAX_HEADERS_RESULTS = 2000;
 /** Maximum depth of blocks we're willing to serve as compact blocks to peers
  *  when requested. For older blocks, a regular BLOCK response will be sent. */
 static const int MAX_CMPCTBLOCK_DEPTH = 5;
-/** Maximum depth of blocks we're willing to respond to GETBLOCKTXN requests for. */
-static const int MAX_BLOCKTXN_DEPTH = 10;
 /** Size of the "block download window": how far ahead of our current height do we fetch?
  *  Larger windows tolerate larger download speed differences between peer, but increase the potential
  *  degree of disordering of blocks on disk (which make reindexing and in the future perhaps pruning
