Merge #21525: [Bundle 4.5/n] Followup fixups to bundle 4

693414d27181cf967f787a2ca72344e52c58c7f0 node/ifaces: ChainImpl: Use an accessor for ChainMan (Carl Dong)
98c4e252f0d09bebb2e4ad3289407459c2cda5d5 node/ifaces: NodeImpl: Use an accessor for ChainMan (Carl Dong)
7e8b5ee814b0b8c34acb20637ed4fc988ccba555 validation: Make BlockManager::LookupBlockIndex const (Carl Dong)
88aead263c61d86e5f836028f517cfbf2a575498 node: Avoid potential UB by asserting assumptions (Carl Dong)
1dd8ed7a8491e51b76eeb236b15b794d9254f674 net_processing: Move comments to declarations (Carl Dong)
07156eb387ea580be5e2ce4a1744992ce7575903 node/coinstats: Replace #include with fwd-declaration (Carl Dong)
7b8e976cd5ac78a22f1be2b2fed8562c693af5d9 miner: Add chainstate member to BlockAssembler (Carl Dong)
e62067e7bcad5a559899afff2e4a8e8b7e9f4301 Revert "miner: Pass in chainstate to BlockAssembler::CreateNewBlock" (Carl Dong)
eede0647b06b6009080c4e536a2705e911d6ee19 Revert "scripted-diff: Invoke CreateNewBlock with chainstate" (Carl Dong)
0c1b2bc549aec77b247f0103652d883227841ac5 Revert "miner: Remove old CreateNewBlock w/o chainstate param" (Carl Dong)

Pull request description:

  Chronological history of this changeset:
  1. Bundle 4 (#21270) got merged
  2. Posthumous reviews were posted
  3. These changes were prepended in bundle 5
  4. More reviews were added in bundle 5
  5. Someone suggested that we split the prepended changes up to another PR
  6. This is that PR

  In the future, I will just do posthumous review changes in another PR instead. I apologize for the confusion.

  Addresses posthumous reviews on bundle 4:
    - From jnewbery:
      - https://github.com/bitcoin/bitcoin/pull/21270#issuecomment-796738048
        - I didn't fix this one, but I added a `TODO` comment so that we don't lost track of it
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r592291225
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r592296942
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r592299738
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r592301704
    - From MarcoFalke:
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r593096212
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r593097032
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r593097867
      - https://github.com/bitcoin/bitcoin/pull/21270#discussion_r593100570

  Addresses reviews on bundle 5:
  - Checking chainman existence before locking cs_main
    - MarcoFalke
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r596601776
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r596601876
  - Appropriate locking, usage of chainman, and control flow in `src/node/interfaces.cpp`
    - MarcoFalke
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r596601383
    - jnewbery
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r597029360
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r597029921
    - ryanofsky
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r597163828
  - Style/comment formatting changes
    - jnewbery
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r597026552
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r597027186
  - Making LookupBlockIndex const
    - jnewbery
      - https://github.com/bitcoin/bitcoin/pull/21391#discussion_r597035062

ACKs for top commit:
  MarcoFalke:
    review ACK 693414d27181cf967f787a2ca72344e52c58c7f0 🛐
  ryanofsky:
    Code review ACK 693414d27181cf967f787a2ca72344e52c58c7f0. I reviewed this previously as part of #21391. I am a fan of the increasingly complicated bundle numbering, and kind of hope there in the next round there is some way we can get bundles 5.333333 and 5.666667!
  jamesob:
    ACK 693414d27181cf967f787a2ca72344e52c58c7f0 ([`jamesob/ackr/21525.1.dongcarl.bundle_4_5_n_followup_f`](https://github.com/jamesob/bitcoin/tree/ackr/21525.1.dongcarl.bundle_4_5_n_followup_f))

Tree-SHA512: 9bdc199f70400d01764e1bd03c25bdb6cff26dcef60e4ca3b649baf8d017a2dfc1f058099067962b4b6ccd32d078002b1389d733039f4c337558cb70324c0ee3

1 files changed, 51 insertions, 50 deletions

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 14b628acf0..17d7619ff5 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -482,19 +482,70 @@ private:
     /** Offset into vExtraTxnForCompact to insert the next tx */
     size_t vExtraTxnForCompactIt GUARDED_BY(g_cs_orphans) = 0;
 
+    /** Check whether the last unknown block a peer advertised is not yet known. */
     void ProcessBlockAvailability(NodeId nodeid) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    /** Update tracking information about which blocks a peer is assumed to have. */
     void UpdateBlockAvailability(NodeId nodeid, const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
     bool CanDirectFetch() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+
+    /**
+     * To prevent fingerprinting attacks, only send blocks/headers outside of
+     * the active chain if they are no more than a month older (both in time,
+     * and in best equivalent proof of work) than the best header chain we know
+     * about and we fully-validated them at some point.
+     */
     bool BlockRequestAllowed(const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
     bool AlreadyHaveBlock(const uint256& block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
     void ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& inv);
+
+    /**
+     * Validation logic for compact filters request handling.
+     *
+     * May disconnect from the peer in the case of a bad request.
+     *
+     * @param[in]   peer            The peer that we received the request from
+     * @param[in]   filter_type     The filter type the request is for. Must be basic filters.
+     * @param[in]   start_height    The start height for the request
+     * @param[in]   stop_hash       The stop_hash for the request
+     * @param[in]   max_height_diff The maximum number of items permitted to request, as specified in BIP 157
+     * @param[out]  stop_index      The CBlockIndex for the stop_hash block, if the request can be serviced.
+     * @param[out]  filter_index    The filter index, if the request can be serviced.
+     * @return                      True if the request can be serviced.
+     */
     bool PrepareBlockFilterRequest(CNode& peer,
                                    BlockFilterType filter_type, uint32_t start_height,
                                    const uint256& stop_hash, uint32_t max_height_diff,
                                    const CBlockIndex*& stop_index,
                                    BlockFilterIndex*& filter_index);
+
+    /**
+     * Handle a cfilters request.
+     *
+     * May disconnect from the peer in the case of a bad request.
+     *
+     * @param[in]   peer            The peer that we received the request from
+     * @param[in]   vRecv           The raw message received
+     */
     void ProcessGetCFilters(CNode& peer, CDataStream& vRecv);
+
+    /**
+     * Handle a cfheaders request.
+     *
+     * May disconnect from the peer in the case of a bad request.
+     *
+     * @param[in]   peer            The peer that we received the request from
+     * @param[in]   vRecv           The raw message received
+     */
     void ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv);
+
+    /**
+     * Handle a getcfcheckpt request.
+     *
+     * May disconnect from the peer in the case of a bad request.
+     *
+     * @param[in]   peer            The peer that we received the request from
+     * @param[in]   vRecv           The raw message received
+     */
     void ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv);
 };
 } // namespace
@@ -748,7 +799,6 @@ static bool PeerHasHeader(CNodeState *state, const CBlockIndex *pindex) EXCLUSIV
     return false;
 }
 
-/** Check whether the last unknown block a peer advertised is not yet known. */
 void PeerManagerImpl::ProcessBlockAvailability(NodeId nodeid) {
     CNodeState *state = State(nodeid);
     assert(state != nullptr);
@@ -764,7 +814,6 @@ void PeerManagerImpl::ProcessBlockAvailability(NodeId nodeid) {
     }
 }
 
-/** Update tracking information about which blocks a peer is assumed to have. */
 void PeerManagerImpl::UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) {
     CNodeState *state = State(nodeid);
     assert(state != nullptr);
@@ -1191,16 +1240,6 @@ bool PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationStat
     return false;
 }
 
-
-//////////////////////////////////////////////////////////////////////////////
-//
-// blockchain -> download logic notification
-//
-
-// To prevent fingerprinting attacks, only send blocks/headers outside of the
-// active chain if they are no more than a month older (both in time, and in
-// best equivalent proof of work) than the best header chain we know about and
-// we fully-validated them at some point.
 bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
 {
     AssertLockHeld(cs_main);
@@ -2101,20 +2140,6 @@ void PeerManagerImpl::ProcessOrphanTx(std::set<uint256>& orphan_work_set)
     m_mempool.check(m_chainman.ActiveChainstate());
 }
 
-/**
- * Validation logic for compact filters request handling.
- *
- * May disconnect from the peer in the case of a bad request.
- *
- * @param[in]   peer            The peer that we received the request from
- * @param[in]   filter_type     The filter type the request is for. Must be basic filters.
- * @param[in]   start_height    The start height for the request
- * @param[in]   stop_hash       The stop_hash for the request
- * @param[in]   max_height_diff The maximum number of items permitted to request, as specified in BIP 157
- * @param[out]  stop_index      The CBlockIndex for the stop_hash block, if the request can be serviced.
- * @param[out]  filter_index    The filter index, if the request can be serviced.
- * @return                      True if the request can be serviced.
- */
 bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer,
                                                 BlockFilterType filter_type, uint32_t start_height,
                                                 const uint256& stop_hash, uint32_t max_height_diff,
@@ -2168,14 +2193,6 @@ bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer,
     return true;
 }
 
-/**
- * Handle a cfilters request.
- *
- * May disconnect from the peer in the case of a bad request.
- *
- * @param[in]   peer            The peer that we received the request from
- * @param[in]   vRecv           The raw message received
- */
 void PeerManagerImpl::ProcessGetCFilters(CNode& peer, CDataStream& vRecv)
 {
     uint8_t filter_type_ser;
@@ -2207,14 +2224,6 @@ void PeerManagerImpl::ProcessGetCFilters(CNode& peer, CDataStream& vRecv)
     }
 }
 
-/**
- * Handle a cfheaders request.
- *
- * May disconnect from the peer in the case of a bad request.
- *
- * @param[in]   peer            The peer that we received the request from
- * @param[in]   vRecv           The raw message received
- */
 void PeerManagerImpl::ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv)
 {
     uint8_t filter_type_ser;
@@ -2259,14 +2268,6 @@ void PeerManagerImpl::ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv)
     m_connman.PushMessage(&peer, std::move(msg));
 }
 
-/**
- * Handle a getcfcheckpt request.
- *
- * May disconnect from the peer in the case of a bad request.
- *
- * @param[in]   peer            The peer that we received the request from
- * @param[in]   vRecv           The raw message received
- */
 void PeerManagerImpl::ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv)
 {
     uint8_t filter_type_ser;