Avoid letting one peer send us data that clears out the download request (and related timers etc) from another peer.
The exception is if a block is definitely stored to disk, in which case we’ll clear the download state (just as we do for compact blocks).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | jamesob, instagibbs, fjahr, mzumsande |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
ProcessCompactBlockTxns by brunoerg)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.
883@@ -884,7 +884,7 @@ class PeerManagerImpl final : public PeerManager
884 * - the block has been received from a peer
885 * - the request for the block has timed out
886 */
887- void RemoveBlockRequest(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
888+ void RemoveBlockRequest(const uint256& hash, std::optional<NodeId> from_peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
first glance, looks right to me. I was literally writing the same code as part of as learn-as-I-go resurrection of #10984
this would be required for any parallel download implementation as well
ACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4 (jamesob/ackr/27608.1.sdaftuar.p2p_avoid_prematurely_cl)
Built, reviewed code locally. Deployed to b-02.slug on https://bmon.info; all metrics there look normal (peer count, tip connection).
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4 ([`jamesob/ackr/27608.1.sdaftuar.p2p_avoid_prematurely_cl`](https://github.com/jamesob/bitcoin/tree/ackr/27608.1.sdaftuar.p2p_avoid_prematurely_cl))
4
5Built, reviewed code locally. Deployed to b-02.slug on https://bmon.info; all
6metrics there look normal (peer count, tip connection).
7
8-----BEGIN PGP SIGNATURE-----
9
10iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmRalB8ACgkQepNdrbLE
11TwXObg//dcajHcXotfGB5mdQGv0a+jXDv1mjXlXr551R1Lv+rhXMbyipB80GNdmu
12IqOXGauUUeqnK2dwxwMMgYlJDa92ErQzAOB7vm44m1iNuSStGh9aqupbznl3D2WK
131D3DMiO5+ZfPmMRhGgVPHwr8ydsuJJ3suna6I5SEF4cbET0x6aLfkMrJ0NK11TfY
14IUz+1XFpxMNiosUxWroUSUKlkZMI83LqjP9JFFyTwCNrfO3jEYTQRT+BABbTC2yk
15k7OWkWBNdoeCSEgVyJ0fOWS5Fr438sL7xpNbWx3MOvU6EC6fV0oLxlE/R6TovQzN
16U/s28D/Z/BlEGFYZbW/dB5JkMfllF8sj8SGOL9b/3u7AC6zJI5i3Q6/89f20lhVK
178iD1eo+iM7hdwUnQkgkd4D6Gh3e3GVf1643pVI3ioqWrDRpDrCzThSW4mUKic2vJ
180xDEuqHO/RWsUCoL+6uV3Rw7henmAZsaHzmV6Qf0G4T1apeBq21U7z8YjJmfLDvg
193q1rmvwYciGJzVkLdBHACgdLPpyL+aMDAY22mM3XQqafnNI2hG5qbB/ScueDF0H3
20WLHdHkcsymY1pMUVyJc2CX9AiuckxW4vJX/t/D7RawsgO4KW78vkx/igBOI/QJYG
21hMDyOm4kKIRhdXbqSZphxDwaFxcX+8wc6WJAUkEB5DbLN0tYx0g=
22=6fob
23-----END PGP SIGNATURE-----
3163@@ -3155,6 +3164,11 @@ void PeerManagerImpl::ProcessBlock(CNode& node, const std::shared_ptr<const CBlo
3164 m_chainman.ProcessNewBlock(block, force_processing, min_pow_checked, &new_block);
3165 if (new_block) {
3166 node.m_last_block_time = GetTime<std::chrono::seconds>();
3167+ // In case this block came from a different peer than we requested
3168+ // from, we can erase the block request now anyway (as we just stored
3169+ // this block to disk).
3170+ LOCK(cs_main);
if..else and removed the duplicate line in the else below.
1131@@ -1129,6 +1132,12 @@ void PeerManagerImpl::RemoveBlockRequest(const uint256& hash)
1132 }
1133
1134 auto [node_id, list_it] = it->second;
1135+
1136+ if (from_peer && node_id != *from_peer) {
1137+ // Block was requested by another peer
ah right, suggested text is also ambiguous….
“Block was requested by us from another peer” ?
4413@@ -4400,7 +4414,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4414 // process from some other peer. We do this after calling
4415 // ProcessNewBlock so that a malleated cmpctblock announcement
4416 // can't be used to interfere with block relay.
4417- RemoveBlockRequest(pblock->GetHash());
4418+ RemoveBlockRequest(pblock->GetHash(), std::nullopt);
ProcessBlock will call it for us the first time when it’s new?
new? In any case this function is extremely fast if called twice.
1172@@ -1164,7 +1173,7 @@ bool PeerManagerImpl::BlockRequested(NodeId nodeid, const CBlockIndex& block, st
1173 }
1174
1175 // Make sure it's not listed somewhere already.
This comment could be improved:
0// When processing block related messages from our peers,
1// we treat requested blocks different from unsolicited ones.
2// When making this distinction we only keep track of the
3// last peer we requested from.
