At a glance
The following diff causes no tests to fail:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index e5b4bc7772..d9b0b46733 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -4576,10 +4576,6 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
5
6 bool fProcessBLOCKTXN = false;
7
8- // If we end up treating this as a plain headers message, call that as well
9- // without cs_main.
10- bool fRevertToHeaderProcessing = false;
11-
12 // Keep a CBlock for "optimistic" compactblock reconstructions (see
13 // below)
14 std::shared_ptr<CBlock> pblock = std::make_shared<CBlock>();
15@@ -4713,18 +4709,6 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
16 fBlockReconstructed = true;
17 }
18 }
19- } else {
20- if (requested_block_from_this_peer) {
21- // We requested this block, but its far into the future, so our
22- // mempool will probably be useless - request the block normally
23- std::vector<CInv> vInv(1);
24- vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(peer), blockhash);
25- MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vInv);
26- return;
27- } else {
28- // If this was an announce-cmpctblock, we want the same treatment as a header message
29- fRevertToHeaderProcessing = true;
30- }
31 }
32 } // cs_main
33
34@@ -4734,15 +4718,6 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
35 return ProcessCompactBlockTxns(pfrom, peer, txn);
36 }
37
38- if (fRevertToHeaderProcessing) {
39- // Headers received from HB compact block peers are permitted to be
40- // relayed before full validation (see BIP 152), so we don't want to disconnect
41- // the peer if the header turns out to be for an invalid block.
42- // Note that if a peer tries to build on an invalid chain, that
43- // will be detected and the peer will be disconnected/discouraged.
44- return ProcessHeadersMessage(pfrom, peer, {cmpctblock.header}, /*via_compact_block=*/true);
45- }
46-
47 if (fBlockReconstructed) {
48 // If we got here, we were able to optimistically reconstruct a
49 // block that is in flight from some other peer.
In some circumstances, a node might receive a CMPCTBLOCK message which for one reason or another that there is no hope of being able to successfully reconstruct the block from the CMPCTBLOCK.
One case that is covered in existing code, is if a CMPCTBLOCK is received that has a height >= ActiveChain().Height() + 2:
https://github.com/bitcoin/bitcoin/blob/9e4567b17a285a266fa540b4141e0db5781cc228/src/net_processing.cpp#L4637 // [Process the block bla bla bla...]
https://github.com/bitcoin/bitcoin/blob/9e4567b17a285a266fa540b4141e0db5781cc228/src/net_processing.cpp#L4716-L4728
In this case, our mempool is not very likely to be helpful but a header exists with valid PoW, so it’s worth it for us to work hard to try to get the block from our peer, so we revert to treating it as a headers message. My assumption is that the reason for checking if we already have requested the block is to be able to break out of a loop where we request a block and get a CMPCTBLOCK response, but can’t process the CMPCTBLOCK message.
That all seems reasonable, and we should probably have functional test cases for it, but I want to also ask if it makes sense to do this at all since above in the CMPCTBLOCK messages, we have already processed the header as part of our anti-dos checks:
and we’ve updated our state for the peer to indicate that they have this block:
So what is the difference between doing those two things and calling ProcessHeadersMessage? As far as I can tell the only difference is that ProcessHeadersMessage will send a request for the block immediately. But I’m pretty sure that by learning the header, and marking the peer as knowing about the header, we’ll send a GETDATA for the block on the next tick of SendMessages for this peer:
So maybe this code can be safely deleted, since it’s mostly a no-op.
