Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

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

wants to merge 5 commits into from

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Mar 31, 2023 •
edited
Loading

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 8f647a6 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 5cfa3fb.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 31, 2023 •
edited
Loading

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 DrahtBot added the P2P label Mar 31, 2023
This was referenced Apr 1, 2023
Merged
Merged
Merged
Open
Merged
Closed
Merged
Closed
Closed
Closed
Closed
Closed
@jonatack jonatack force-pushed the 2023-04-extract-network branch 6 times, most recently from 06009b7 to 8f647a6 Compare April 4, 2023 04:01
This was referenced Apr 4, 2023
Merged
Merged
@jonatack jonatack force-pushed the 2023-04-extract-network branch from 8f647a6 to 55f249f Compare April 4, 2023 13:35
@jonatack jonatack changed the title net: extract Network and BIP155Network logic to node/network net, refactor: extract Network and BIP155Network logic to node/network Apr 4, 2023
@jonatack jonatack marked this pull request as ready for review April 4, 2023 15:15
@DrahtBot DrahtBot mentioned this pull request Apr 4, 2023
Merged
@jonatack jonatack force-pushed the 2023-04-extract-network branch from 55f249f to f362fba Compare April 21, 2023 15:38
@jonatack
Copy link
Member Author

Rebased!

@jonatack jonatack force-pushed the 2023-04-extract-network branch from 1cc9a56 to 8efd76b Compare May 31, 2023 13:18
@DrahtBot DrahtBot mentioned this pull request May 31, 2023
Draft
@ccdle12
Copy link
Contributor

ccdle12 commented Jun 2, 2023

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

This was referenced Jun 26, 2023
Merged
Closed
This was referenced Jul 7, 2023
Closed
Closed
@maflcko
Copy link
Member

maflcko commented Jul 25, 2023

which may reduce build size and speed up build times.

May be good to add numbers?

@jonatack jonatack force-pushed the 2023-04-extract-network branch from 8efd76b to c83d137 Compare July 27, 2023 22:33
@fanquake
Copy link
Member

fanquake commented Aug 1, 2023

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

What do you mean by "build size"?

@DrahtBot DrahtBot mentioned this pull request Aug 9, 2023
Merged
@DrahtBot
Copy link
Contributor

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

@jonatack
Copy link
Member Author

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
Copy link
Member

maflcko commented Oct 20, 2023

Are you still working on this?

@jonatack
Copy link
Member Author

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
Copy link
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
Copy link
Member

maflcko commented Jan 21, 2024

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 maflcko closed this Jan 21, 2024
@jonatack jonatack mentioned this pull request Oct 11, 2024
Open
@bitcoin bitcoin locked and limited conversation to collaborators Jan 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fanquake fanquake fanquake left review comments

+1 more reviewer

@ccdle12 ccdle12 ccdle12 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Needs rebase P2P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jonatack @DrahtBot @ccdle12 @maflcko @fanquake