First commit adds coverage to a similar case to #13370 . h/t dergoegge for detecting this issue independently. Documentation of this behavior with a test is then followed by a proposed change where anytime we considered something VALID_TRANSACTIONS and potentially advertised via compact blocks, we should respond to requests for the getdata if the peer fell back for whatever reason.
If the second commit is deemed controversial it can be shelved and current behavior documented alone.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32869.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | dergoegge |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/45297064338
LLM reason (✨ experimental): The CI failure is caused by an unfixable lint error detected by rust and Python linters, leading to the overall build failure.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Concept ACK
h/t dergoegge for detecting this issue independently.
For context, I found this case (as demonstrated by the first commit in the PR) by adding a netsplit oracle (see https://gist.github.com/dergoegge/3036796551095a9ce7535fb5d74e6656#detecting-network-split-bugs) to fuzzamoto. While extremely unrealistic/unlikely to trigger in the wild, I’d still consider this timeout case a bug, especially because it could cause disconnects across the whole network (as opposed to isolated instances with short id collisions).
Resolving this will help avoid special casing the oracle for this scenario.
If a PoW-and-merkle-valid compact block ends up being
invalid during validation, a peer can end up sending a
getdata for the full block and we will end up ignoring that
request.
This scenario is when a block hasn't been mined in 200m+
disallowing direct fetch and causing us to fall back to
parallel block fetching logic, rather than the short txid
collision scenario.
In the case where a block was advertised due to it being
deemed TRANSACTION_VALID at any point, a peer may request
the full block and we currently ignore those requests,
causing disconnection.
Since we already answer requests for valid blocks, there is
no reason to not also answer these requests to avoid
partition risk in the network. The PoW is valid and
"recent", so there is no DoS risk either.
57+ # No disconnect yet, node1 is waiting on response to getdata
58+ assert_equal(len(self.nodes[1].getpeerinfo()), 1)
59+
60+ # Make sure node1 has processed the compact block
61+ self.wait_until(lambda: len(self.nodes[1].getchaintips()) == 2)
62+ assert {"height": 13, "hash": block.hash, "branchlen": 1, "status": "headers-only"} in node.getchaintips()
0 assert {"height": 13, "hash": block.hash, "branchlen": 1, "status": "headers-only"} in self.nodes[1].getchaintips()
40+ self.generate(self.nodes[0], 12)
41+
42+ # Make 200+ minutes pass with wiggle room added
43+ # to disallow direct fetch of block
44+ for node in self.nodes:
45+ node.setmocktime(now + 200 * 60 * 2)
nit:
0 node.bumpmocktime(200 * 60 * 2)
Maybe a dumb question, but why not relax the other side here? I don’t know what the new criteria would be, but in an ideal world, nodes should not be sending block announcements for blocks they have identified as invalid just to avoid upsetting their peers, so maybe the constraints set by PeerManagerImpl::SendMessages() are too rigid?
