net processing: Remove hash and fValidatedHeaders from QueuedBlock #22141

1. 
   jnewbery commented at 2:17 pm on June 3, 2021: member

   The QueuedBlock struct contains a fValidatedHeaders field that indicates whether we have already validated a header for the requested block. Since headers-first syncing, we only request blocks where the header is already validated, so fValidatedHeaders is always true. Remove it and clean up the logic that uses that field.

   Likewise, QueuedBlock contains a hash field that is set to the block hash. Since headers-first syncing, we always have a CBlockIndex, which contains the block hash, so remove the redundant hash field.

   Tidy up the logic and rename functions to better indicate what they’re doing.

2. [net processing] Remove unnecessary hash arg from MarkBlockAsInFlight()

   MarkBlockAsInFlight is always called with a non-null pindex. Just get the block hash
   from that pindex inside the function.

   85e058b191
3. [net processing] Remove QueuedBlock.fValidatedHeaders

   Since headers-first syncing, we only ever request a block if we've already validated its headers.
   Therefore QueuedBlock.fValidatedHeaders is always set to true. Remove it.

   b4e29f2436
4. [net processing] Remove CNodeState.nBlocksInFlightValidHeaders

   nBlocksInFlightValidHeaders always has the same value as nBlocksInFlight, since we only download
   blocks with valid headers.

   b03de9c753
5. scripted-diff: rename nPeersWithValidatedDownloads

   -BEGIN VERIFY SCRIPT-
   ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }
   
   ren nPeersWithValidatedDownloads  m_peers_downloading_from
   -END VERIFY SCRIPT-

   156a19ee6a
6. [net processing] Remove QueuedBlock.hash

   It's redundant with CBlockIndex::GetBlockHash()

   4e90d2dd0e
7. [net processing] Add IsBlockRequested() function

   MarkBlockAsReceived() should not be used for both removing the block
   from mapBlocksInFlight and checking whether it was in the map.

   6299350733
8. [net processing] Tidy up MarkBlockAsReceived() 2c45f832e8
9. scripted-diff: rename MarkBlockAs functions

   -BEGIN VERIFY SCRIPT-
   ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }
   
   ren  MarkBlockAsInFlight BlockRequested
   ren  MarkBlockAsReceived RemoveBlockRequest
   -END VERIFY SCRIPT-

   2f4ad6b7ef
10. fanquake added the label P2P on Jun 3, 2021
11. 
    DrahtBot commented at 3:32 am on June 4, 2021: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19438 (Introduce deploymentstatus by ajtowns)
    • #15606 ([experimental] UTXO snapshots by jamesob)

    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.

12. in src/net_processing.cpp:777 in 2c45f832e8 outdated

    788+    if (it == mapBlocksInFlight.end()) {
    789+        // Block was not requested
    790+        return;
    791+    }
    792+
    793+    auto [node_id, list_it] = it->second;
    

    ---

    ---

    

    sipa commented at 4:40 am on June 4, 2021:

    How dare you make this ancient code so… readable?
13. 
    sipa commented at 4:43 am on June 4, 2021: member

    utACK 2f4ad6b7efa408b8a858e87499bf6cfcdf936d73

    Very nice cleanup.

14. in src/net_processing.cpp:808 in b4e29f2436 outdated

    804@@ -803,9 +805,9 @@ bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const CBlockIndex* pind
    805     MarkBlockAsReceived(hash);
    806 
    807     std::list<QueuedBlock>::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(),
    808-            {hash, pindex, pindex != nullptr, std::unique_ptr<PartiallyDownloadedBlock>(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)});
    809+            {hash, pindex, std::unique_ptr<PartiallyDownloadedBlock>(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)});
    810     state->nBlocksInFlight++;
    811-    state->nBlocksInFlightValidHeaders += it->fValidatedHeaders;
    

    ---

    ---

    

    mjdietzx commented at 5:10 pm on June 7, 2021:

    Interesting, I didn’t even know you could do += true/false like this. Anyways, I like this simplfiication
15. in src/net_processing.cpp:4720 in b03de9c753 outdated

    4711@@ -4717,7 +4712,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
    4712         // to unreasonably increase our timeout.
    4713         if (state.vBlocksInFlight.size() > 0) {
    4714             QueuedBlock &queuedBlock = state.vBlocksInFlight.front();
    4715-            int nOtherPeersWithValidatedDownloads = nPeersWithValidatedDownloads - (state.nBlocksInFlightValidHeaders > 0);
    

    ---

    ---

    

    mjdietzx commented at 5:26 pm on June 7, 2021:

    So I guess here, since we know state.vBlocksInFlight.size() > 0 you’re able to get rid of this condition in the subtraction: state.nBlocksInFlightValidHeaders > 0?

    Is it always true that state.vBlocksInFlight.size() == state->nBlocksInFlight?

    ---

    

    mjdietzx commented at 10:06 pm on June 7, 2021:

    Along these lines, is mapBlocksInFlight and vBlocksInFlight tracking the same underlying state, just as different data structures? I think the ordering of vBlocksInFlight is important though?

    ---

    

    jnewbery commented at 9:42 am on June 9, 2021:

    Is it always true that state.vBlocksInFlight.size() == state->nBlocksInFlight?

    Yes. There are only two places where vBlocksInFlight is updated, and nBlocksInFlight is updated at the same time. if vBlocksInFlight.size() was not the same as nBlocksInFlight, it’d be a bug.

    is mapBlocksInFlight and vBlocksInFlight tracking the same underlying state, just as different data structures? I think the ordering of vBlocksInFlight is important though?

    mapBlocksInFlight is a global data structure, and holds iterators into the various vBlocksInFlight for the different peers. So you can think of the vBlocksInFlight lists as tracking the state, and mapBlocksInFlight as an index into that state.

    Eventually, I’d like to extract all of the block request tracking logic and state into its own submodule like the txrequest module, which was added in #19988. A boost multi-index might be the right way to track this state if we did that.

    ---

    

    mjdietzx commented at 2:22 pm on June 9, 2021:

    Thanks for the explanations - makes sense.

    Eventually, I’d like to extract all of the block request tracking logic and state into its own submodule like the txrequest module, which was added in #19988. A boost multi-index might be the right way to track this state if we did that.

    Sounds great, I was trying to figure out if something like this can/should be done with these questions and seems like you’re already thinking about it 👍

    Anyways very nice cleanup in this PR, code reads much better now

16. 
    practicalswift commented at 7:41 pm on June 7, 2021: contributor

    Concept ACK
17. 
    mjdietzx commented at 10:09 pm on June 7, 2021: contributor

    crACK 2f4ad6b7efa408b8a858e87499bf6cfcdf936d73
18. 
    jnewbery commented at 9:42 am on June 9, 2021: member

    Thanks for the review @mjdietzx. I’ve answered your review questions. Feel free to ask more if there’s anything that’s still unclear about this PR.
19. in src/net_processing.cpp:787 in 85e058b191 outdated

    781@@ -782,8 +782,11 @@ bool PeerManagerImpl::MarkBlockAsReceived(const uint256& hash)
    782     return false;
    783 }
    784 
    785-bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex* pindex, std::list<QueuedBlock>::iterator** pit)
    786+bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const CBlockIndex* pindex, std::list<QueuedBlock>::iterator** pit)
    787 {
    788+    assert(pindex);
    

    ---

    ---

    

    MarcoFalke commented at 2:49 pm on June 9, 2021:

    85e058b19145b5068f2f71a90c1182bf2a93c473: I think an easier way to enforce this is to pass a reference from the call site. If needed, you can use & to get a pointer.

    ---

    

    jnewbery commented at 3:29 pm on June 9, 2021:

    Totally agree. I’ll do this in a follow-up or here if I need to retouch the branch.

    ---

    

    MarcoFalke commented at 8:01 am on June 11, 2021:

    Done in #22221
20. MarcoFalke approved
21. 
    MarcoFalke commented at 3:02 pm on June 9, 2021: member

    review ACK 2f4ad6b7efa408b8a858e87499bf6cfcdf936d73 📊

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3review ACK 2f4ad6b7efa408b8a858e87499bf6cfcdf936d73 📊
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUhXzwv9HniRZ2DZea8vtfP9W0XCvKLSaLzohfoLcy8LV+kuq0kzasQQWYfqy6s+
     89ZUkOV/pxBsrl55Pfk/MvhbHOMw2t4Y/2RP7foRm/GZVbF2W5eMkg58L3R0CIXsi
     9k4/dxINftIkNObRMtsYSjNWBwVaYiMHbAB8CHYH+B43/yE3otQ8QweT8IG7/r9VV
    10ZWSkw9P2Z7tUQw9FeiaOYxteDK/W+dxec0m3+iIvOJeloc/UHKXM+wI15E/KoSK/
    11S9ttkHCRzius/M3a54JqmgkA73vnDwcjQe7NLVphl2f/UbIgf2cE2PH/BIiL4k72
    12tNye+KKxMqknxsdMRjQXVs6biQp2crxu/pNSUS5kw6z6vvb0SQgF6zZtWzs2c77D
    131r+Zo0eHhzxyLodbxwgtZzZmr5HvMzHMnAqhp/7aDWTqpeGRYeGC0Klqko9wXipB
    14dxs8nre5S/5XnJHt5DJ3DHiVLRGk0g7xldCDrv0A2KAJOI8whbHt8aEp1SojfM5I
    150eVnuNvO
    16=itrW
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash f0a6a932acf4e1e73bf07c8f0433dd6b510a91f4061198f5afafddba6da52c6d -

22. laanwj merged this on Jun 10, 2021
23. laanwj closed this on Jun 10, 2021

    ---

24. jnewbery deleted the branch on Jun 10, 2021
25. sidhujag referenced this in commit eab85be1d0 on Jun 10, 2021
26. gwillen referenced this in commit 3cc06ba3d2 on Jun 1, 2022
27. DrahtBot locked this on Aug 16, 2022

Contributors
jnewbery DrahtBot sipa mjdietzx practicalswift MarcoFalke

Labels
P2P