refactor: Remove WithParams serialization helper, use SER_PARAMS_OPFUNC #28503

1. 
   maflcko commented at 9:10 AM on September 19, 2023: member

   Every serialization parameter struct already has the SER_PARAMS_OPFUNC, except for one in the tests.

   For consistency, and to remove verbose code, convert the test to SER_PARAMS_OPFUNC, and use it everywhere, then remove the WithParams helper.

2. 
   DrahtBot commented at 9:10 AM on September 19, 2023: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	ajtowns, TheCharlatan
   Concept ACK	theuni

   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:

   • #28451 (Remove unused SER_DISK, SER_NETWORK, SER_GETHASH by maflcko)

   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 Refactoring on Sep 19, 2023
4. 
   fanquake commented at 12:54 PM on October 2, 2023: member

   cc @theuni

5. maflcko force-pushed on Oct 3, 2023
6. 
   theuni commented at 2:24 PM on October 4, 2023: member

   Concept ACK.

7. test: Use SER_PARAMS_OPFUNC in serialize_tests.cpp fae9054793
8. scripted-diff: Use ser params operator

   -BEGIN VERIFY SCRIPT-
    sed -i 's|WithParams(\([a-zA-Z:._]\+\), |\1(|g' $( git grep -l WithParams )
   -END VERIFY SCRIPT-

   ffffb4af83
9. maflcko force-pushed on Oct 13, 2023
10. maflcko requested review from TheCharlatan on Oct 27, 2023
11. TheCharlatan approved
12. 
    TheCharlatan commented at 10:38 AM on October 30, 2023: contributor

    ACK fa664dc335f02c392e3268f403f32bcf5bd9eeee

13. DrahtBot requested review from theuni on Oct 30, 2023
14. fanquake requested review from ajtowns on Oct 30, 2023
15. in src/net_processing.cpp:5380 in fa664dc335 outdated

    5385 | -    }
    5386 | -    m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(msg_type, WithParams(CAddress::SerParams{{ser_enc}, CAddress::Format::Network}, peer.m_addrs_to_send)));
    5387 | +    const bool wants_addrv2{peer.m_wants_addrv2};
    5388 | +    const auto& msg_type{wants_addrv2 ? NetMsgType::ADDRV2 : NetMsgType::ADDR};
    5389 | +    const auto& ser_params{wants_addrv2 ? CAddress::V2_NETWORK : CAddress::V1_NETWORK};
    5390 | +    m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(msg_type, ser_params(peer.m_addrs_to_send)));
    

    ---

    ---

    

    ajtowns commented at 12:16 PM on October 30, 2023:

    Might have been clearer to write:

    CNetMsgMaker mm(node.GetCommonVersion());
    if (peer.m_wants_addrv2) {
        m_connman.PushMessage(&node, mm.Make(NetMsgType::ADDRV2, CAddress::V2_NETWORK(peer.m_addrs_to_send)));
    } else {
        m_connman.PushMessage(&node, mm.Make(NetMsgType::ADDR, CAddress::V1_NETWORK(peer.m_addrs_to_send)));
    }
    

    Was thinking about moving PushMessage from connman to cnode though (#28686 etc), so could reconsider this at that time.

    ---

    

    maflcko commented at 12:50 PM on October 30, 2023:

    Thanks, I'll re-push the last commit.

16. 
    ajtowns commented at 12:40 PM on October 30, 2023: contributor

    ACK fa664dc335f02c392e3268f403f32bcf5bd9eeee

17. Remove WithParams serialization helper 99990194ce
18. maflcko force-pushed on Oct 30, 2023
19. maflcko requested review from ajtowns on Oct 30, 2023
20. 
    ajtowns commented at 4:25 AM on October 31, 2023: contributor

    reACK 99990194ce26af89308fab5ad0b1c8c26e45f80c

21. DrahtBot removed review request from ajtowns on Oct 31, 2023
22. DrahtBot requested review from TheCharlatan on Oct 31, 2023
23. TheCharlatan approved
24. 
    TheCharlatan commented at 8:49 AM on October 31, 2023: contributor

    Re-ACK 99990194ce26af89308fab5ad0b1c8c26e45f80c

25. fanquake merged this on Oct 31, 2023
26. fanquake closed this on Oct 31, 2023

    ---

27. maflcko deleted the branch on Nov 1, 2023
28. bitcoin locked this on Dec 3, 2024

Contributors

maflckoDrahtBotfanquaketheuniTheCharlatanajtowns

Review Requested
theuni