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. [net] Delete CNetMessage copy constructor/assignment op b5a85b365a
10. [net] Remove trivial GetConnectionType() getter 860402ef2e
11. [net] Pass nRecvFloodSize to CNode cd0c8eeb09
12. dergoegge force-pushed on Mar 27, 2023
13. theStack approved
14. 
    theStack commented at 11:34 PM on March 27, 2023: contributor

    ACK cd0c8eeb0940790b6ba83786d1c9e362d4dc4829

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

    utACK cd0c8eeb0940790b6ba83786d1c9e362d4dc4829

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

    ---

20. sidhujag referenced this in commit 1c07f8ff47 on Mar 28, 2023
21. bitcoin locked this on Mar 27, 2024

Contributors

dergoeggeDrahtBotjnewberytheStack

Review Requested
vasild