net, refactor: extract Network and BIP155Network logic to node/network #27385

pull jonatack wants to merge 5 commits into bitcoin:master from jonatack:2023-04-extract-network changing 44 files +242 −167
  1. jonatack commented at 10:40 pm on March 31, 2023: contributor

    This extracts the Network and BIP155Network logic to node/network. The code has been living between netaddress and netbase and some compilation units include these large files when they only need a Network enum or related method. Separating the code to a standalone unit in node improves code separation and helps with using only what is needed.

    I verified the include headers with https://cirrus-ci.com/task/6749578737745920 generated by https://github.com/bitcoin/bitcoin/pull/27385/commits/8f647a65d3484c7acd2d97f4b055c582d7734b6f while this was in draft and carefully narrowed them down to the most relevant ones.

    Possible todos for a follow-up: upgrade Network to an enum class, e.g. NET_I2P becomes Network::I2P and https://github.com/bitcoin/bitcoin/pull/27385/commits/5cfa3fb8b5815aaf96483a63526e5f0bf3c0a06b.

  2. DrahtBot commented at 10:40 pm on March 31, 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
    Stale ACK ccdle12

    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:

    • #28240 (refactor: Remove unused boost signals2 from torcontrol by MarcoFalke)
    • #26859 (fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses by vasild)
    • #26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)
    • #26152 (Bump unconfirmed ancestor transactions to target feerate by murchandamus)
    • #25284 (net: Use serialization parameters for CAddress serialization by MarcoFalke)

    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 31, 2023
  4. jonatack force-pushed on Apr 3, 2023
  5. jonatack force-pushed on Apr 3, 2023
  6. jonatack force-pushed on Apr 3, 2023
  7. jonatack force-pushed on Apr 3, 2023
  8. jonatack force-pushed on Apr 4, 2023
  9. jonatack force-pushed on Apr 4, 2023
  10. jonatack force-pushed on Apr 4, 2023
  11. jonatack renamed this:
    net: extract Network and BIP155Network logic to node/network
    net, refactor: extract Network and BIP155Network logic to node/network
    on Apr 4, 2023
  12. jonatack marked this as ready for review on Apr 4, 2023
  13. DrahtBot added the label Needs rebase on Apr 21, 2023
  14. jonatack force-pushed on Apr 21, 2023
  15. DrahtBot removed the label Needs rebase on Apr 21, 2023
  16. jonatack commented at 5:32 pm on April 21, 2023: contributor
    Rebased!
  17. in src/test/fuzz/addrman.cpp:31 in f362fba03c outdated 
    27@@ -26,6 +28,14 @@
28 namespace {
29 const BasicTestingSetup* g_setup;
30 
31+static const std::map<uint8_t, uint8_t> NET_LEN_MAP = {
    fanquake commented at 3:26 pm on May 9, 2023:
    In 9bcbbc68682c1a92316d6a2dd8142839c58fe247 (fuzz, refactor: hoist net_len_map in addrman fuzzer to constant): Isn’t this already a constant? Seems to just be moving the code out of the function where it’s used?
    jonatack commented at 10:18 am on May 11, 2023:
    Good point; removed that commit. Thanks for having a look @fanquake.
  18. DrahtBot added the label Needs rebase on May 10, 2023
  19. jonatack force-pushed on May 11, 2023
  20. DrahtBot removed the label Needs rebase on May 11, 2023
  21. jonatack commented at 12:37 pm on May 11, 2023: contributor
    Rebased ⚡
  22. in src/netbase.cpp:344 in 64565570b7 outdated 
    376@@ -377,7 +377,7 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a
377     vSocks5.push_back(SOCKSVersion::SOCKS5); // VER protocol version
378     vSocks5.push_back(SOCKS5Command::CONNECT); // CMD CONNECT
    ccdle12 commented at 11:13 am on May 17, 2023:
    Could be a candidate for another PR - since we are changing SOCKS5Atyp to an enum class, would it make sense to also change the following enums to enum classes SOCKSVersion, SOCKS5Command, SOCKS5Method and SOCKS5Reply?
    jonatack commented at 12:37 pm on May 31, 2023:

    It wasn’t necessary to do here (no global scope name collisions), but I think it would make sense. Per doc/developer-notes.md:

    0- Prefer `enum class` (scoped enumerations) over `enum` (traditional enumerations) where possible.
1
2  - *Rationale*: Scoped enumerations avoid two potential pitfalls/problems with
3    traditional C++ enumerations: implicit conversions to `int`, and name clashes
4    due to enumerators being exported to the surrounding scope.
  23. DrahtBot added the label Needs rebase on May 17, 2023
  24. ccdle12 commented at 3:41 pm on May 17, 2023: contributor

    tACK for creating smaller compilation units.

    I’m just curious, for the motivation of future changes, should there be more node networking related logic moved to the node sub folder (e.g. logic related to the node making decisions on networking but not the actual networking implementations themselves) or is it simply classes/enums that are called frequently in isolation, in different parts of the code base but they exist in bigger files with unused imports like in netbase and netaddress?

  25. jonatack force-pushed on May 31, 2023
  26. jonatack commented at 12:56 pm on May 31, 2023: contributor
    Rebased 🥉 @ccdle12 Thanks! Mind re-acking? As to motivation, I think it’s a mix of both reasons you mentioned, with the first one being the direction to go when doing so – see doc/design/libraries.md and merged changes like c741d748d4.
  27. DrahtBot removed the label Needs rebase on May 31, 2023
  28. jonatack force-pushed on May 31, 2023
  29. DrahtBot added the label CI failed on May 31, 2023
  30. DrahtBot removed the label CI failed on May 31, 2023
  31. ccdle12 commented at 8:52 am on June 2, 2023: contributor

    Rebased 3rd_place_medal

    @ccdle12 Thanks! Mind re-acking? As to motivation, I think it’s a mix of both reasons you mentioned, with the first one being the direction to go when doing so – see doc/design/libraries.md and merged changes like c741d74.

    Of course! re-tACK at 8efd76b, thanks for explaining the motivation and pointing out the design/doc :)

  32. DrahtBot added the label Needs rebase on Jul 25, 2023
  33. maflcko commented at 11:10 am on July 25, 2023: member

    which may reduce build size and speed up build times.

    May be good to add numbers?

  34. p2p, refactor: extract Network code to node/network 98c3eb5015
  35. net, refactor: make SOCKS5Atyp an enum class 
    to avoid enumerator naming collisions with enum BIP155Network.
    9c88294ae7
  36. p2p, refactor: move BIP155Network and GetBIP155Network to node/network 69f16d951d
  37. p2p, refactor: make BIP155Network an enum class 0eef87cb02
  38. p2p, refactor: declare node/network methods nodiscard c83d137594
  39. jonatack force-pushed on Jul 27, 2023
  40. DrahtBot removed the label Needs rebase on Jul 27, 2023
  41. fanquake commented at 8:51 am on August 1, 2023: member

    and helps with using only what is needed, which may reduce build size

    What do you mean by “build size”?

  42. DrahtBot commented at 10:28 pm on August 15, 2023: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

  43. DrahtBot added the label Needs rebase on Aug 15, 2023
  44. jonatack commented at 5:32 pm on September 19, 2023: contributor

    May be good to add numbers?

    What do you mean by “build size”?

    Dropped “which may reduce build size and speed up build times” from the pull description.

  45. maflcko commented at 9:35 am on October 20, 2023: member
    Are you still working on this?
  46. jonatack commented at 5:45 pm on October 23, 2023: contributor

    Are you still working on this?

    Yes, just have not been prioritizing it as there’s been no positive interest other than one ACK.

  47. DrahtBot commented at 0:02 am on January 21, 2024: contributor

    ⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
  48. maflcko commented at 12:04 pm on January 21, 2024: member
    Closing for now. Feel free to open a new pull, if this is still relevant, or leave a comment here, to have it reopened.
  49. maflcko closed this on Jan 21, 2024


jonatack DrahtBot fanquake ccdle12 maflcko

Labels
P2P Needs rebase

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-06-29 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me