Net Processing: Move RelayTransaction() into PeerManager #21162

1. 
   jnewbery commented at 1:12 pm on February 12, 2021: member

   This is the first part of #21160. It moves the RelayTransaction() function to be a member function of the PeerManager class. This is required in order to move the transaction inventory data into the Peer object, since Peer objects are only accessible from within PeerManager.
2. fanquake added the label P2P on Feb 12, 2021
3. 
   DrahtBot commented at 10:37 pm on February 12, 2021: member

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

   Conflicts

   No conflicts as of last run.

4. MarcoFalke deleted a comment on Feb 14, 2021
5. 
   jnewbery commented at 1:30 pm on February 16, 2021: member

   Rebased. This is now ready for review.
6. jnewbery force-pushed on Feb 16, 2021
7. jnewbery marked this as ready for review on Feb 16, 2021
8. jnewbery marked this as a draft on Feb 17, 2021
9. jnewbery marked this as ready for review on Feb 17, 2021
10. jnewbery force-pushed on Feb 17, 2021
11. jnewbery marked this as a draft on Feb 17, 2021
12. jnewbery force-pushed on Feb 18, 2021
13. 
    jnewbery commented at 9:25 am on February 18, 2021: member

    I’ve removed the last two commits in this PR, so this is now just moving RelayTransaction() into PeerManagerImp. The final two commits are difficult to separate from the rest of the “Move tx inventory into net_processing” change (#21160) without ugly lock taking/releasing.
14. jnewbery marked this as ready for review on Feb 18, 2021
15. jnewbery force-pushed on Feb 19, 2021
16. 
    ajtowns commented at 1:08 pm on March 3, 2021: member

    Concept ACK, but ignoring for now since drahtbot says it conflicts with txorphanage
17. DrahtBot added the label Needs rebase on Mar 4, 2021
18. [net processing] Move RelayTransaction into PeerManager

    We don't mark RelayTransaction as const. Even though it doesn't mutate
    PeerManagerImpl state, it _is_ mutating the internal state of a CNode
    object, by updating setInventoryTxToSend. In a subsequent commit, that
    field will be moved to the Peer object, which is owned by
    PeerMangerImpl.
    
    This requires PeerManagerImpl::ReattemptInitialBroadcast() to no longer
    be const.

    a38a4e8f03
19. [net processing] Don't pass CConnman to RelayTransactions

    Use the local m_connman instead

    680eb56d82
20. jnewbery force-pushed on Mar 4, 2021
21. 
    jnewbery commented at 10:24 am on March 4, 2021: member

    Rebased

    Concept ACK, but ignoring for now since drahtbot says it conflicts with txorphanage @ajtowns txorphange merged. This is now ready for review :)

22. in src/net_processing.h:51 in a38a4e8f03 outdated

    46@@ -47,6 +47,10 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
    47     /** Whether this node ignores txs received over p2p. */
    48     virtual bool IgnoresIncomingTxs() = 0;
    49 
    50+    /** Relay transaction to all peers. */
    51+    virtual void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman)
    

    ---

    ---

    

    ajtowns commented at 11:59 am on March 4, 2021:

    Hmm, I think this should be marked as const ? Even after setInventoryTxToSend is moved into Peer, updating that doesn’t change PeerManagerImpl, only the contents of an object which PeerManagerImpl has a shared_ptr to. For better or worse const on a class only protects its pointers from changes, it doesn’t protect the things pointed to from changing.

    ---

    

    jnewbery commented at 5:27 pm on March 4, 2021:

    More discussion here: #21236 (review)
23. 
    ajtowns commented at 12:03 pm on March 4, 2021: member

    Approach ACK; not convinced about the non-const bit.
24. DrahtBot removed the label Needs rebase on Mar 4, 2021
25. 
    DrahtBot commented at 4:50 pm on March 15, 2021: member

    🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

26. jnewbery added this to the "Blockers" column in a project

    ---

27. 
    ajtowns commented at 5:01 am on March 18, 2021: member

    ACK 680eb56d828ce358b4e000c140f5b247ff5e6179
28. fanquake merged this on Mar 18, 2021
29. fanquake closed this on Mar 18, 2021

    ---

30. jnewbery deleted the branch on Mar 18, 2021
31. jnewbery removed this from the "Blockers" column in a project

    ---

32. sidhujag referenced this in commit 115c5127f3 on Mar 18, 2021
33. DrahtBot locked this on Aug 18, 2022

Contributors
jnewbery DrahtBot ajtowns

Labels
P2P