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).
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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);
should describe what the argument does and when it should be used
Consider it done.
Concept ACK. Will start running this when CI checks out.
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).
<details><summary>Show signature data</summary> <p>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4 ([`jamesob/ackr/27608.1.sdaftuar.p2p_avoid_prematurely_cl`](https://github.com/jamesob/bitcoin/tree/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).
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmRalB8ACgkQepNdrbLE
TwXObg//dcajHcXotfGB5mdQGv0a+jXDv1mjXlXr551R1Lv+rhXMbyipB80GNdmu
IqOXGauUUeqnK2dwxwMMgYlJDa92ErQzAOB7vm44m1iNuSStGh9aqupbznl3D2WK
1D3DMiO5+ZfPmMRhGgVPHwr8ydsuJJ3suna6I5SEF4cbET0x6aLfkMrJ0NK11TfY
IUz+1XFpxMNiosUxWroUSUKlkZMI83LqjP9JFFyTwCNrfO3jEYTQRT+BABbTC2yk
k7OWkWBNdoeCSEgVyJ0fOWS5Fr438sL7xpNbWx3MOvU6EC6fV0oLxlE/R6TovQzN
U/s28D/Z/BlEGFYZbW/dB5JkMfllF8sj8SGOL9b/3u7AC6zJI5i3Q6/89f20lhVK
8iD1eo+iM7hdwUnQkgkd4D6Gh3e3GVf1643pVI3ioqWrDRpDrCzThSW4mUKic2vJ
0xDEuqHO/RWsUCoL+6uV3Rw7henmAZsaHzmV6Qf0G4T1apeBq21U7z8YjJmfLDvg
3q1rmvwYciGJzVkLdBHACgdLPpyL+aMDAY22mM3XQqafnNI2hG5qbB/ScueDF0H3
WLHdHkcsymY1pMUVyJc2CX9AiuckxW4vJX/t/D7RawsgO4KW78vkx/igBOI/QJYG
hMDyOm4kKIRhdXbqSZphxDwaFxcX+8wc6WJAUkEB5DbLN0tYx0g=
=6fob
-----END PGP SIGNATURE-----
</p></details>
ACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4
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);
nit: Could have moved the lock outside the if..else and removed the duplicate line in the else below.
Code review ACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4
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
nit: requested from another peer
Oops -- will fix in a followup. Thanks for catching.
"by another peer" reads fine to me?
peer didn't request, WE requested that peer
ah right, suggested text is also ambiguous....
"Block was requested by us from another peer" ?
Code Review ACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4
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);
Is this block required anymore now that ProcessBlock will call it for us the first time when it's new?
Can't we have a race condition where the block is not 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:
// When processing block related messages from our peers,
// we treat requested blocks different from unsolicited ones.
// When making this distinction we only keep track of the
// last peer we requested from.
post-merge Code review ACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4
utACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4