p2p, refactor: add CInv transaction message helpers; use in net processing #19590

1. 
   jonatack commented at 4:16 pm on July 26, 2020: member

   Following the merge of wtxid relay in #18044, this is the first of three refactoring PRs (this one, #19610, and #19611) with no change in behavior, tightly scoped to ease review, to simplify the net processing code and improve encapsulation:

   • add CInv transaction message helper methods, defined in the class

   • use the new helpers in net_processing.cpp to simplify the code and improve encapsulation

   Test coverage is provided by the functional p2p tests, notably (from seeing which tests failed when breaking things to test coverage) p2p_segwit, p2p_tx_download, p2p_feefilter, and p2p_permissions.

2. 
   DrahtBot commented at 6:31 pm on July 26, 2020: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #19621 ([RFC] Package-relay: sender-initiated by ariard)
   • #19569 (Enable fetching of orphan parents from wtxid peers by sipa)
   • #18766 (Disable fee estimation in blocksonly mode by darosior)

   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 Jul 26, 2020
4. 
   jonasschnelli commented at 7:36 am on July 27, 2020: contributor

   Code Review ACK 3faeaab673ad3fd0f9c80407b0e625a1fa5ff4ae - would it be an wrong to make the new calls inline?
5. p2p: add CInv transaction message helper methods 4254cd9f8f
6. p2p, refactoring: use CInv helpers in net_processing.cpp

   to simplify the code and reach less from it into the CInv class internals

   c251d710a4
7. jonatack force-pushed on Jul 27, 2020
8. 
   jonatack commented at 9:25 am on July 27, 2020: member

   Thanks @jonasschnelli, good idea, done. Made the new helper functions implicitly inline by defining them in the class definition. Change from last push: git diff 3faeaab c251d710
9. vasild approved
10. 
    vasild commented at 1:38 pm on July 27, 2020: member

    ACK c251d71

    • Would it be possible to also handle the rest of MSG_* values and make CInv::type private?
    • Would it make sense to change CInv::type from int to enum GetDataMsg? Or maybe to uint32_t (ac88e2eb6 has changed enum GetDataMsg from int to uint32_t)?
11. 
    jonatack commented at 10:06 am on July 28, 2020: member

    Thanks for reviewing, @vasild. Done in https://github.com/jonatack/bitcoin/commits/CInv-type-refactoring. Will propose separately to keep the changes small and not invalidate review.
12. theStack approved
13. 
    theStack commented at 1:26 pm on July 28, 2020: member

    Code-Review ACK c251d710a4c2981c6d52362a9a89db84da3d4a67
14. hebasto approved
15. 
    hebasto commented at 12:33 pm on July 29, 2020: member

    ACK c251d710a4c2981c6d52362a9a89db84da3d4a67, I have reviewed the code and it looks OK, I agree it can be merged.
16. 
    fjahr commented at 12:40 pm on July 29, 2020: member

    Code review ACK c251d710a4c2981c6d52362a9a89db84da3d4a67
17. 
    laanwj commented at 2:07 pm on July 30, 2020: member

    Code review ACK c251d710a4c2981c6d52362a9a89db84da3d4a67
18. laanwj merged this on Jul 30, 2020
19. laanwj closed this on Jul 30, 2020

    ---

20. jonatack deleted the branch on Jul 30, 2020
21. sidhujag referenced this in commit fc7030c8fe on Jul 31, 2020
22. 
    jonatack commented at 7:37 am on July 31, 2020: member

    Thanks to everyone for reviewing. The follow-up #19610 which makes the same change for INV block messages, is rebased and ready with tests green, if you’re inclined to have a look.
23. laanwj referenced this in commit 505b39e72b on Sep 2, 2020
24. laanwj referenced this in commit bd60a9a8ed on Sep 3, 2020
25. sidhujag referenced this in commit 98e01510c0 on Sep 3, 2020
26. sidhujag referenced this in commit addddc0053 on Sep 3, 2020
27. Fabcien referenced this in commit 3cb957dbb3 on May 12, 2021
28. DrahtBot locked this on Feb 15, 2022

Contributors
jonatack DrahtBot jonasschnelli vasild theStack hebasto fjahr laanwj

Labels
P2P