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

jonatack commented at 10:40 pm on March 31, 2023: member

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.

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.

DrahtBot added the label P2P on Mar 31, 2023

jonatack force-pushed on Apr 3, 2023

jonatack force-pushed on Apr 4, 2023

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

jonatack marked this as ready for review on Apr 4, 2023

DrahtBot added the label Needs rebase on Apr 21, 2023

jonatack force-pushed on Apr 21, 2023

DrahtBot removed the label Needs rebase on Apr 21, 2023

jonatack commented at 5:32 pm on April 21, 2023: member

Rebased!

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.

DrahtBot added the label Needs rebase on May 10, 2023

jonatack force-pushed on May 11, 2023

DrahtBot removed the label Needs rebase on May 11, 2023

jonatack commented at 12:37 pm on May 11, 2023: member

Rebased ⚡

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.

DrahtBot added the label Needs rebase on May 17, 2023

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?

jonatack force-pushed on May 31, 2023

jonatack commented at 12:56 pm on May 31, 2023: member

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.

DrahtBot removed the label Needs rebase on May 31, 2023

jonatack force-pushed on May 31, 2023

DrahtBot added the label CI failed on May 31, 2023

DrahtBot removed the label CI failed on May 31, 2023

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 :)

DrahtBot added the label Needs rebase on Jul 25, 2023

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?

p2p, refactor: extract Network code to node/network 98c3eb5015

net, refactor: make SOCKS5Atyp an enum class

to avoid enumerator naming collisions with enum BIP155Network.

9c88294ae7

p2p, refactor: move BIP155Network and GetBIP155Network to node/network 69f16d951d

p2p, refactor: make BIP155Network an enum class 0eef87cb02

p2p, refactor: declare node/network methods nodiscard c83d137594

jonatack force-pushed on Jul 27, 2023

DrahtBot removed the label Needs rebase on Jul 27, 2023

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”?

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

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

DrahtBot added the label Needs rebase on Aug 15, 2023

jonatack commented at 5:32 pm on September 19, 2023: member

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.

maflcko commented at 9:35 am on October 20, 2023: member

Are you still working on this?

jonatack commented at 5:45 pm on October 23, 2023: member

Are you still working on this?

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

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.

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.

maflcko closed this on Jan 21, 2024

bitcoin locked this on Jan 20, 2025

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

Code Coverage

Reviews

Conflicts