mapBlockSource
#29410
Opening an issue per #28235 (comment)
If we receive a valid block that passes the anti-DoS threshold but doesn’t advance the tip, the BlockChecked callback won’t be called. This is because ActivateBestChain will return early since pindexMostWork is equal to m_chain.Tip(). Since the BlockChecked callback isn’t called, mapBlockSource won’t be removed from.
I think removing entries from mapBlockSource once the corresponding peers disconnect would make the most sense because we want to be able to punish the source in case the block turns out to be invalid. If we expire or otherwise prematurely remove from the map we lose that ability.
Something like the following in FinalizeNode:
0 for (auto it = mapBlockSource.begin(); it != mapBlockSource.end();) {
1 auto& [hash, entry] = *it;
2 if (entry.first == node.GetId()) {
3 it = mapBlockSource.erase(it);
4 } else {
5 ++it;
6 }
7 }
I left some details out of both the original PR and this issue and I think the missing information is needed to come to a solution:
h and receives a BLOCK message from an honest peer B for a stale block at height h. There are a couple ways that a BLOCK message could be sent instead of a CMPCTBLOCK message. For example, the compact-block slots could all be taken by an attacker or maybe there was a collision for short ids. This will leave an entry in mapBlockSource where the punishment boolean is set to true.BLOCK message to node A for the stale block. When execution hits this line, the emplace call won’t actually add a new entry to mapBlockSource since one already exists. See https://en.cppreference.com/w/cpp/container/map/emplace.BlockChecked callback to be called. This will cause node A to punish peer B and disconnect them.Since there is a disconnect issue, I don’t think the map can be cleaned up once the peer disconnects and instead has to be done earlier. I don’t think the potential for harm here is very high, since somebody would need to get lucky or be able to generate stale blocks. It might be possible to abuse this during IBD if one knew historical stale blocks and had saved them, but I think that would also require some luck.
I think we should either: 1) document all of mapBlockSource quirks with comments and merge tests that improve the code coverage in this area, or 2) stop usage of mapBlockSource in the BlockChecked callback and instead only have punishment / reward happen when blocks are received in-order. I have done a bit of option 1) and will summarize current behavior below:
Punishment:
BlockChecked callback is usually called immediately if failure occurs in CheckBlock / AcceptBlock.ConnectTip. Note that out-of-order validation is likely to occur in IBD and not likely to occur outside of IBD.ActivateBestChain won’t call ConnectTip.Reward:
BlockChecked callback is usually called immediately in the case of success. It may also be called out-of-order.ActivateBestChain won’t call ConnectTip.Problems with out-of-order blocks
The in-order blocks case is easy to understand and punishment/reward happens normally. The out-of-order blocks case leads to two minor issues with mapBlockSource:
h+1, which the target node then requests.h+2, which the target node then requests.h+2. Since h+1 hasn’t been received yet, the target node’s tip stays the same.h+2. This calls mapBlockSource.emplace, but there is already an entry for the hash and so this is a no-op. Peer 2 & 3 would only send this invalid block if they were in some way malicious as fast-announcing a possibly-invalid block via CMPCTBLOCK doesn’t seem to be implemented.
ProcessBlock is then called and since Peer 3 hasn’t sent over a new block, the mapBlockSource.erase is hit, removing Peer 2’s entry from the map for h+2.h+1. This will cause the BlockChecked callback to be invoked for h+2.
mapBlockSource.find will not find the previous entry since it was deleted.h+2 is valid and we must not be in IBD.One of the root issues here is that mapBlockSource is not a std::multimap, so further calls to mapBlockSource.emplace are a no-op and calls to mapBlockSource.erase are premature. It was at one point suggested that it become a multimap (see: IRC) because the original intent behind the map seems to be parallel block validation in combination with allowing nodes to announce blocks via compact-block relay before fully validating them. These out-of-order issues are alluded to here and tie-in with the above linked IRC discussion.
Problems with stale blocks
BlockChecked callback is never called, leading to lingering entries in mapBlockSource. This is the issue outlined in the OP.Zooming out
Taking all the above, it doesn’t seem that out-of-order reward or punishment is useful. Out-of-order reward only occurs if we are out of IBD, but most out-of-order validation is going to occur during IBD. Out-of-order punishment only occurs if the peer is sending us invalid blocks that pass AcceptBlock – this does not seem particularly useful as pointed out in this comment. I think @mzumsande suggestion to punish / reward immediately makes the most sense.
I have written a functional test that demonstrates the three issues outlined above and can be merged if option 1) is the best bet: https://github.com/Crypt-iQ/bitcoin/commit/c3a5bbb779e5156f623072d25dae733cda8a7d79
cc @mzumsande @instagibbs @luke-jr since y’all commented on the original PR I made.
Labels
Validation
Net