refactor: Remove WithParams serialization helper, use SER_PARAMS_OPFUNC #28503

pull maflcko wants to merge 3 commits into bitcoin:master from maflcko:2309-nuke-with-params-force-opfunc- changing 7 files +79 −82
  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

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

    Code Coverage

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

    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.

    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:

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

    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

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-28 22:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me