Currently we’ll stall peers that send us an unknown INV type in a GETDATA message. Be a bit more friendly and just drop the invalid request.
Ditto for blocks-relay-only peers that send us a GETDATA for a transaction.
There’s a test for the first part. The second is difficult to test in the functional test framework since we aren’t able to make blocks-relay-only connections.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
1603- // mempool entries added before this time have likely expired from mapRelay
1604- const std::chrono::seconds longlived_mempool_time = GetTime<std::chrono::seconds>() - RELAY_TX_CACHE_TIME;
1605- const std::chrono::seconds mempool_req = pfrom->m_tx_relay->m_last_mempool_req.load();
1606+ // mempool entries added before this time have likely expired from mapRelay
1607+ const std::chrono::seconds longlived_mempool_time = GetTime<std::chrono::seconds>() - RELAY_TX_CACHE_TIME;
1608+ const std::chrono::seconds mempool_req = pfrom->m_tx_relay->m_last_mempool_req.load();
Co-Authored-By: John Newbery <john@johnnewbery.com>
Co-Authored-By: John Newbery <john@johnnewbery.com>
1647@@ -1642,19 +1648,17 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm
1648 }
1649 } // release cs_main
1650
1651+ // Only process one BLOCK item per call, since they're uncommon and can be
20+
21+ def __init__(self):
22+ super().__init__()
23+ self.blocks = defaultdict(int)
24+
25+ def on_inv(self, message):
Concept ACK.
I think there was discussion in earlier PRs on whether to ignore the unknown data or disconnect the peer. Why did you choose the former / are there legitimate constellations in which peers would send us unknown GETDATA but be worth staying connected to?
Why did you choose the former / are there legitimate constellations in which peers would send us unknown GETDATA but be worth staying connected to?
Just conservatism. Better to not let a logic bug turn into a peer disconnect if possible. In the adversarial case, there are messages that a peer can send us that would be much costlier for us to process, so no need to punish for something that we can just drop.
Co-Authored-By: John Newbery <john@johnnewbery.com>
ACK https://github.com/bitcoin/bitcoin/commit/9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7
Built, run and tested on macOS Catalina 10.15.4.
Functional test p2p_getdata successful.
I think disconnecting the peer because the application doesn’t know how to handle a request would be slightly preferable in this case. It indicates a bug at the side of the peer and explicit error handling is better to track down errors.
There are lots of p2p messages that indicate bugs in the peer which we just silently drop, eg if a peer sends us multiple VERACKs, multiple SENDHEADERs, a GETDATA with no items, etc. I think if the cost to process those messages is low, then dropping and not disconnecting is the safer thing to do.
Why did you choose the former / are there legitimate constellations in which peers would send us unknown GETDATA but be worth staying connected to?
It’s generally important to avoid splitting the p2p network. Even if peers are implementing different rules allowing invalid stuff (eg, an old node after a softfork), they still need to get the blocks from updated peers…
if a peer sends us multiple VERACKs, multiple SENDHEADERs, a GETDATA with no items, etc. I think if the cost to process those messages is low, then dropping and not disconnecting is the safer thing to do.
I think those are different in that they are quantitatively different not qualitatively. There may be spurious messages (or lack of messages) but the messages that are sent have a known meaning. In this case the type is unknown and it is completely unknown how to handle it.
In this case the type is unknown and it is completely unknown how to handle it.
Yes, and so the most cautious action is to drop it. We already drop unknown message types:
I could use the same extensibility argument here. It’s possible that in future another P2P client or a future version of Bitcoin Core might use the inv/getdata/notfound messages for different payload types. We shouldn’t disconnect them unnecessarily.
1615- it++;
1616+ const CInv &inv = *it++;
1617+
1618+ if (pfrom->m_tx_relay == nullptr) {
1619+ // Ignore GETDATA requests for transactions from blocks-only peers.
1620+ continue;
cs_main lock just to skip over MSG_TX and MSG_WITNESS_TX. I think this patch (“ignore tx GETDATA from blocks only peers”) could be dropped entirely, allowing the “ignore unknown INV types in GETDATA messages” patch’s handling to cover it instead.