net: #27257 follow-ups #27324

1. 
   dergoegge commented at 3:03 PM on March 24, 2023: member

   Follow-up PR for #27257

   • Deletes the copy constructor/assignment operator of CNetMessage
   • Removes trivial getter for the connection type
   • Avoids passing nRecvFloodSize to CNode methods by passing it to CNode on creation
2. 
   DrahtBot commented at 3:03 PM on March 24, 2023: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	theStack, jnewbery

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #27114 (p2p: Allow whitelisting outgoing connections by brunoerg)
   • #26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
   • #25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
   • #25268 (refactor: Introduce EvictionManager by dergoegge)

   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.

3. DrahtBot added the label P2P on Mar 24, 2023
4. fanquake requested review from theStack on Mar 24, 2023
5. fanquake requested review from jnewbery on Mar 24, 2023
6. fanquake requested review from vasild on Mar 24, 2023
7. in src/net.h:243 in f824725be3 outdated

    235 | @@ -236,6 +236,10 @@ class CNetMessage {
    236 |      std::string m_type;
    237 |  
    238 |      CNetMessage(CDataStream&& recv_in) : m_recv(std::move(recv_in)) {}
    239 | +    CNetMessage(CNetMessage&&) = default;
   

   ---

   ---

   

   jnewbery commented at 6:12 PM on March 24, 2023:

   Perhaps add a comment on why we don't want to copy CNetMessage objects.

   ---

   

   dergoegge commented at 2:03 PM on March 27, 2023:

   Done! happy to amend the comment if you had something different in mind.

   ---

   

   jnewbery commented at 9:55 AM on March 28, 2023:

   This is absolutely fine. I would have gone for something like "For performance reasons, once we've deserialized the bytes into a CNetMessage, we avoid re-allocating and copying the message as we pass it up the stack for processing. Delete the copy ctor/assignment to avoid accidentally introducing a copy operation.", which is more-or-less the same as what you came up with.

8. 
   jnewbery commented at 6:20 PM on March 24, 2023: contributor

   utACK f824725be383e89346e2dc3269cc3d6aa533bcd3

   One suggestion inline if you touch this PR again.

9. DrahtBot cross-referenced this on Mar 24, 2023 from issue p2p: Allow whitelisting manual connections by brunoerg
10. DrahtBot cross-referenced this on Mar 25, 2023 from issue refactor: Continue moving application data from CNode to Peer by dergoegge
11. [net] Delete CNetMessage copy constructor/assignment op b5a85b365a
12. [net] Remove trivial GetConnectionType() getter 860402ef2e
13. [net] Pass nRecvFloodSize to CNode cd0c8eeb09
14. dergoegge force-pushed on Mar 27, 2023
15. theStack approved
16. 
    theStack commented at 11:34 PM on March 27, 2023: contributor

    ACK cd0c8eeb0940790b6ba83786d1c9e362d4dc4829

17. DrahtBot requested review from jnewbery on Mar 27, 2023
18. 
    jnewbery commented at 9:56 AM on March 28, 2023: contributor

    utACK cd0c8eeb0940790b6ba83786d1c9e362d4dc4829

19. DrahtBot removed review request from jnewbery on Mar 28, 2023
20. fanquake merged this on Mar 28, 2023
21. fanquake closed this on Mar 28, 2023

    ---

22. DrahtBot cross-referenced this on Mar 28, 2023 from issue refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge
23. DrahtBot cross-referenced this on Mar 28, 2023 from issue refactor: Introduce EvictionManager by dergoegge
24. sidhujag referenced this in commit 1c07f8ff47 on Mar 28, 2023
25. dergoegge cross-referenced this on Apr 19, 2023 from issue test: add end-to-end tests for CConnman and PeerManager by vasild
26. brunoerg cross-referenced this on May 16, 2023 from issue fuzz: net, add `recv_flood_size`, `prefer_evict` in `ConsumeNode` by brunoerg
27. bitcoin locked this on Mar 27, 2024

Contributors

dergoeggeDrahtBotjnewberytheStack

Review Requested
vasild