why not make this < instead of <= ? why do we avoid requesting compact blocks for competing best blocks?

@morcos I couldn't see this added line in any of the 3 PRs you mentioned.

What is the deal with block height 165?

Very much approve of the variable namings chosen in various places.

Why not make this <=2 rather than ==1 (for mapBlocksInFlight.size())? this way if two blocks get announced in close proximity we can request compact blocks for both of them (rather than compact block for the oldest, and full block for the most recent).

"Only ask for the full block from the first peer we requested from"?

could just do the -= itInFlight->second.second->fValidatedHeaders as above to match.

since C++11 can also just capture the return value and set as range.first instead of worrying about iterator invalidation.

I think indexing might by NodeId would be better than using a multimap, because it would ensure that there couldn't be multiple entries for a block from the same node. I'd suggest:

typedef map<pair<uint256, NodeId>, list<QueuedBlock>::iterator> BlockDownloadMap;

This also would let you replace some of the while loops added here with direct lookups. I thought some of these (especially the while loop with the break statement setting fExpectedBLOCKTXN) were kind of confusing.

The downside of using a map is that you wouldn't have an equal_range method to call in the places that do require a loop. But you could replace those equal_range calls with calls to an equivalent helper function:

pair<BlockDownloadMap::iterator, BlockDownloadMap::iterator>
GetBlockDownloadRange(BlockDownloadMap& blocks, const uint256& hash) {
    return {blocks.lower_bound({hash, numeric_limits<NodeId>::min()}),
            blocks.upper_bound({hash, numeric_limits<NodeId>::max()})};
}

I'm unsure about this change. In particular there are a lot of mmapBlocksInFlight.count(hash) calls that would get a bit less clear... I'll think about it some more

Yeah, I noticed that in the later commits. You could have a count helper function returning std::distance(range.first, range.second). I do think a map is a better way to represent the data, but the c++ map implementation does makes it a little awkward. Anyway, it's just something to consider.

Could you expand this comment a little bit to describe the condition being checked? In particular I don't understand how the IsWitnessEnabled and fHaveWitness parts relate to making the request.

Also, maybe consider just pulling the MSG_CMPCT_BLOCK setting a bit later down up to here and just requesting the block in this if statement (possibly before the while loop above).

Is this change ("Only request full blocks from the peer we thought had the block in-flight") a change in behavior? Or is it just a cleanup after the previous multimap commit? It seems like this commit should be merged into the preceding or following one, or the commit message should be extended to say what the effect is, what motivates it.

Can you AssertLockHeld?

Can you AssertLockHeld?

Also, maybe consider just pulling the MSG_CMPCT_BLOCK setting a bit later down up to here and just requesting the block in this if statement (possibly before the while loop above).

In the strange case that a peer does not set the fHaveWItness service bit, but does announce compact blocks v2, I believe this line would result in a full block request. More generally, because the two if statements always have to be in sync to avoid this, I really prefer we pull the actual request logic into this if statement.

The way I read this, the use of countPartialBlocksStarted, instead of a countBlocksStarted, means that we will request up to two compact blocks at a time, even if we are already requesting the full block from a peer. This seems strange to me, why not just max 2 in-flights at the same time for a given block, with the second never being a full block?

Also, why drop the fInFlightFromSamePeer option? It looks like we'll never getblocktxn from two peers simultaneously?

Further, simply adding the entry to vToFetch here may result in a null pointer dereference, I believe. If a peer announces two headers messages back-to-back, the first time we will MarkBlockAsInFlight to them, and the second time we'll hit this condition and add the entry to vToFetch again (which should also be fixed). Further down, we'll MarkBlockAsInFlight to them again, but MarkBlockAsInFlight requires that, if the block is already in-flight to the same peer, pit be something non-NULL as it will be dereferenced, but it is NULL in the call below.

I believe the above needs fixing in three ways - MarkBlockAsInFlight needs to be more robust against NULL pit, the request needs to move into this if statement and skip the remainder of this block of code, and we shouldn't double-request from the same peer.

OK, so why not just check fPeerWasFirstRequest and MarkBlockAsNotInFlight otherwise?

Should this be configurable? Might make sense for miners to have this higher.

Likely not, asking all your peers for a copy of the block poses the same network-DoS risks as connecting to hundreds of nodes, which people like to do because they believe it will help (though it usually actually hurts) them get their blocks out faster.

More importantly, if you're a miner and have good peers, I think it'd be somewhat rare for you to receive a third compact block announce before the first can respond to your blocktxn request, at least it will be once we get proper multi-threaded ProcessMessages implemented to respond to blocktxn requests for the latest block in the background (see #10652 for the beginnings of the steps to do so).

What do you think the upper limit is before it would generally cause a negative impact? Maybe just have an upper limit like we do for max outbound connections.

Probably around 2 :p. Its really only useful if your peer got stuck doing something and wasn't able to respond or is being actively malicious. Once we've fixed the block-on-block-validation-before-responding-to-blocktxn-requests issue, it should be somewhat rare for this to help more than a very small amount.