p2p: ensure mapBlockSource is removed from in ProcessBlock

Crypt-iQ commented at 9:20 pm on August 7, 2023: contributor

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. Fix that by always removing from mapBlockSource in ProcessBlock.

net_processing: ensure mapBlockSource is removed from in ProcessBlock

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. Fix that by always removing
from mapBlockSource in ProcessBlock.

16770d053d

DrahtBot commented at 9:20 pm on August 7, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

maflcko commented at 5:58 am on August 8, 2023: member

Looks like the tests time all out after 2 hours? Also, could use p2p instead of net_processing as the prefix, according to the docs?

fanquake renamed this:
~~net_processing: ensure mapBlockSource is removed from in ProcessBlock~~
p2p: ensure mapBlockSource is removed from in ProcessBlock
on Aug 8, 2023

DrahtBot added the label P2P on Aug 8, 2023

DrahtBot added the label CI failed on Aug 8, 2023

Crypt-iQ marked this as a draft on Aug 8, 2023

Crypt-iQ commented at 12:29 pm on August 8, 2023: contributor

I guess I forgot to run the functional tests - the issue is that this patch removes from mapBlockSource in ProcessBlock if new_block is set. I assumed that this meant it was a valid block, but that’s not always the case. With this patch, if we receive more blocks on top of this invalid block that give us more work than the tip, the BlockChecked callback will run and won’t find the peer to punish. I can’t think of a scenario where you’re sure that you can remove from mapBlockSource unless the peer disconnects.

instagibbs commented at 2:08 pm on August 8, 2023: member

Since I think this behavior is fairly long-running, I think I might try to recap the current logic:

Every time a compact block is reconstructed, or full block directly given, we put an entry in a map for one of two reasons:

reward if they’re a good high-bandwidth peer candidates because they gave us a new most-PoW block
punish if they gave us a full block and it has any fundamental issue, or if they handed us a compact block they should have known was faulty(e.g., invalid header)

Issue is that we add an entry for any full block retrieved, but only remove for those two cases, which means we miss whatever is not covered by (1) and (2), which may included cases like recent(valid enough merkle root and good enough PoW to persist to disk) stale chain tips. This should be extremely expensive to do, and more likely a growing data structure merely due to chance stale tips.

Does this match your understanding?

Crypt-iQ commented at 2:37 pm on August 8, 2023: contributor

Yup that makes sense

bitcoin deleted a comment on Aug 8, 2023

mzumsande commented at 9:44 pm on August 8, 2023: contributor

I can’t think of a scenario where you’re sure that you can remove from mapBlockSource unless the peer disconnects.

I agree. If we could immediately remove from mapBlockSource in ProcessBlock in all cases, there wouldn’t really be a need to have a map for this info in the first place. It’s purpose, I think, is to store the information who provided us with that not fully validated block, so the peer is still available for potential punishment in case we get to fully validate it at a later time - and accordingly, only when that full validation has been done, we can remove the entry from the map.

luke-jr commented at 1:29 pm on August 10, 2023: member

Is there a case where we really need to punish for this anyway? We already bypass the punishment for compact-block relaying…

mzumsande commented at 1:41 pm on August 10, 2023: contributor

Is there a case where we really need to punish for this anyway? We already bypass the punishment for compact-block relaying…

IBD I think, where blocks are typically received out of order, and fully validated later. If peers send us blocks then that turn out to be invalid, we probably want to remember these peers to disconnect them.

[Edit]: on second thought, it doesn’t seem necessary during IBD either. Maybe it would be possible to just punish immediately if AceptBlock() fails instead of having mapBlockSource / punishing in BlockChecked() callbacks, because situations where someone would create a block that doesn’t fail AcceptBlock() and only fails during full validation are so expensive that that it’s not really necessary to punish that peer?

maflcko commented at 10:20 am on September 27, 2023: member

Are you still working on this?

Crypt-iQ commented at 8:04 pm on October 6, 2023: contributor

No unfortunately I don’t have the time now, should I close it?

DrahtBot commented at 2:34 pm on February 5, 2024: contributor

🤔 There hasn’t been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

maflcko closed this on Feb 5, 2024

instagibbs commented at 2:54 pm on February 5, 2024: member

@Crypt-iQ could you just open a parallel issue for this for tracking?

Crypt-iQ deleted the branch on Feb 8, 2024

p2p: ensure mapBlockSource is removed from in ProcessBlock #28235

Code Coverage

Reviews