net processing: Don't require locking cs_main before calling RelayTransactions() #21845

1. 
   jnewbery commented at 6:04 PM on May 3, 2021: member

   As part of the general effort to reduce cs_main usage in net_processing, this removes the need to be holding cs_main when calling RelayTransactions() from outside net_processing. Internally, we lock cs_main and call an internal _RelayTransactions() function that does require cs_main.

2. 
   MarcoFalke commented at 6:39 PM on May 3, 2021: member

   unsigned cr ACK 9012512a57283bdcd0cc0562680d5de2f304af9f

3. DrahtBot added the label P2P on May 3, 2021
4. 
   DrahtBot commented at 12:11 AM on May 4, 2021: member

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #21527 (net_processing: lock clean up by ajtowns)

   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.

5. in src/net_processing.cpp:261 in 9012512a57 outdated

    255 | @@ -256,6 +256,9 @@ class PeerManagerImpl final : public PeerManager
    256 |                          const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) override;
    257 |  
    258 |  private:
    259 | +    void _RelayTransaction(const uint256& txid, const uint256& wtxid)
    260 | +        EXCLUSIVE_LOCKS_REQUIRED(cs_main);
   

   ---

   ---

   

   promag commented at 12:19 AM on May 4, 2021:

   9012512a57283bdcd0cc0562680d5de2f304af9f

   Should include sync.h?

   ---

   

   jnewbery commented at 8:31 AM on May 4, 2021:

   Added

6. in src/net_processing.cpp:1018 in 9012512a57 outdated

   1017 | @@ -1015,7 +1018,7 @@ void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler)
   1018 |  
   1019 |          if (tx != nullptr) {
   1020 |              LOCK(cs_main);
   1021 | -            RelayTransaction(txid, tx->GetWitnessHash());
   

   ---

   ---

   

   promag commented at 12:23 AM on May 4, 2021:

   9012512a57283bdcd0cc0562680d5de2f304af9f

   Remove the above lock instead?

   ---

   

   jnewbery commented at 8:33 AM on May 4, 2021:

   A future PR is going to replace cs_main in _RelayTransaction() with a new internal net_processing lock, and we'll need to call the internal _RelayTransaction() function here. It makes sense to keep this as it is for now.

   ---

   

   promag commented at 8:35 AM on May 4, 2021:

   I wasn't aware of that, makes sense then (you can resolve this thread).

7. 
   promag commented at 12:29 AM on May 4, 2021: member

   Code review ACK 9012512a57283bdcd0cc0562680d5de2f304af9f.

8. [net processing] Add internal _RelayTransactions()

   Callers of the external RelayTransactions() no longer need to lock cs_main.

   39e19713cd
9. jnewbery force-pushed on May 4, 2021
10. 
    jnewbery commented at 8:33 AM on May 4, 2021: member

    Thanks for the review @promag. I've addressed your review comments.

11. 
    promag commented at 8:37 AM on May 4, 2021: member

    Code review ACK 39e19713cd6594f93db835e8ef7eef5824a9ba02, just included sync.h since last review.

12. 
    MarcoFalke commented at 8:43 AM on May 4, 2021: member

    re-unsigned-code-review ACK 39e19713cd6594f93db835e8ef7eef5824a9ba02

13. fanquake requested review from amitiuttarwar on May 6, 2021
14. fanquake requested review from ajtowns on May 6, 2021
15. in src/net_processing.cpp:1519 in 39e19713cd

    1514 | @@ -1511,6 +1515,11 @@ void PeerManagerImpl::SendPings()
    1515 |  }
    1516 |  
    1517 |  void PeerManagerImpl::RelayTransaction(const uint256& txid, const uint256& wtxid)
    1518 | +{
    1519 | +    WITH_LOCK(cs_main, _RelayTransaction(txid, wtxid););
    

    ---

    ---

    

    ajtowns commented at 3:06 PM on May 6, 2021:

    No need for ;); -- there's already a terminating ; in the WITH_LOCK #define. Could just use a plain LOCK instead of WITH_LOCK too...

16. 
    ajtowns commented at 3:40 PM on May 6, 2021: member

    ACK 39e19713cd6594f93db835e8ef7eef5824a9ba02

    please also help to review the conflicting pull requests

    Hi, fellow reviewers of this PR...

17. fanquake merged this on May 7, 2021
18. fanquake closed this on May 7, 2021

    ---

19. sidhujag referenced this in commit 47b4103a52 on May 7, 2021
20. jnewbery deleted the branch on May 7, 2021
21. gwillen referenced this in commit 086262250a on Jun 1, 2022
22. DrahtBot locked this on Aug 16, 2022

Contributors

jnewberyMarcoFalkeDrahtBotpromagajtowns

Review Requested
amitiuttarwar