Skip to content

Rename network module and move/clean up types#1854

Merged
apoelstra merged 7 commits intorust-bitcoin:masterfrom
tcharding:05-18-network
Aug 1, 2023
Merged

Rename network module and move/clean up types#1854
apoelstra merged 7 commits intorust-bitcoin:masterfrom
tcharding:05-18-network

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented May 18, 2023 •
edited
Loading

This PR has grown.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented May 18, 2023

And BTW @apoelstra, epic win on the recent fuzzing changes, I was able to catch required changes to the fuzzing code during my normal dev workflow instead of getting a red CI job.

@tcharding tcharding marked this pull request as draft May 18, 2023 01:29
@tcharding tcharding marked this pull request as ready for review May 18, 2023 01:51
@tcharding tcharding added API break This PR requires a version bump for the next release code quality Makes code easier to understand and less likely to lead to problems labels May 18, 2023
@tcharding tcharding changed the title Move Network and ChainHash to lib.rs Move Network, ChainHash, and Magic to lib.rs May 18, 2023
@tcharding tcharding changed the title Move Network, ChainHash, and Magic to lib.rs Create chain module May 18, 2023
@tcharding tcharding mentioned this pull request May 18, 2023
Closed
Kixunil
Kixunil reviewed May 19, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, bikeshedding:

  • Is chain the best name? A newbie might get it confused with blockdata.
  • Rename Network to Chain?
  • If we're keeping chain, rename ChainHash to chain::Hash?

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented May 19, 2023 •
edited
Loading

Fair points re naming, I would have used network but could not have a network.rs file in the same directory as the current network/ directory. I'm not sure there is a good name, even in spoken language its annoying/hard to find a term to use since "network" and "chain" are both so heavily overloaded.

Agree module name chain and type name Network being different is odd but changing one ambiguous name to another Network to Chain seems like churn and just makes users of the lib learn a new name for no benefit. Thoughts? Note that the module has three types so naming it after one of them is also imperfect.

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented May 22, 2023

I am tempted actually to pull these things into the existing network/ module (which maybe will be renamed to p2p) rather than creating a new one. The Magic structure definitely seems like it should be in the network module. For Network itself I'm less sure. Maybe it should just live directly in lib.rs.

@tcharding tcharding changed the title Create chain module Rename network module and move/clean up types May 25, 2023
@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented May 25, 2023

@tcharding how about if we add a cargo-public-api check as in #1875 -- we add a .txt file to the repo with the existing API, CI-check that it's consistent, and then any API changes are required to change the .txt file so we can debate them independent of the code.

This PR seems like a good test case for this workflow.

But concept ACK everything here.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented May 25, 2023

Sounds good, I'll have a play with it.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented May 26, 2023

Check out #1880, thanks for prodding me to do that, it was actually very enjoyable and got me a bit excited that we can actually have a stable API and not mess it up accidentally.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented May 26, 2023

So lets wait for #1880 to merge then I'll rebase this one and we can see how the review process goes.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented May 27, 2023

If we decide to leave #1880 open for a while I'd pref to let this in, there will be plenty of chance to see how 1880 effects workflow, we change the API daily.

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented May 27, 2023

If we're still debating #1880 by end of Monday then sure, let's start pushing forward on this. But I think it's in good shape other than the warning thing which is a pretty minor issue.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented May 30, 2023

Changes in force push:

  • rebased on master
  • minor improvement of module level docs for p2p module
  • improved p2p::network module level docs (mention network vs chain terminoligy)

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented May 30, 2023

Changes in force push, fix sloppy work in f20d4bb5 Create p2p::network module:

  • Remove duplicate PROTOCOL_VERSION const declaration
  • Fix the commit log to mention removal of constants module

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented May 30, 2023

In light of recent discussion I broke the code move patch up into a whole bunch of patches so that the review is easier.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jun 9, 2023

Just some quick 2 cents:

  • this MR (or at least the OP of it) seems to conflate consensus and p2p, which are actually entirely unrelated. Params is consensus, and in fact shouldn't even have the network field, though each params is probably related to one Network that's just not really inherent.

My bad, the PR description was stale, this PR does not touch Params.

  • isn't chainhash a lightning concept?

Yes, good point. I will remove any changes to ChainHash from this PR.

  • I would say, keep Param in consensus and wire can have Magic

I'm going to put Magic in p2p/mod.rs as suggested (excl. p2p/wire naming thing)

  • Network could be more general, doesn't necessarily have to occur in either consensus nor wire/p2p

Cool, so I'm going to add a network module at the crate root that holds just the Network type.

Thanks man!

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Jun 9, 2023

Cool, so I'm going to add a network module at the crate root that holds just the Network type.

I think it can just be defined directly in lib.rs, we don't need a module for it.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jun 9, 2023

WTF cargo test --no-default-features --features=no-std in the crate root does not pass the features options to bitcoin when building??? Is this a feature or a bug or my set up, how have I written Rust this long and not known this. (That command passes in rust-bitcoin/ but not in rust-bitcoin/bitcoin/.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jun 9, 2023 •
edited
Loading

I think it can just be defined directly in lib.rs, we don't need a module for it.

You sure? It clutters the lib.rs file a bit, a bunch of tests and things. Currently the lib.rs file is sparse, adding Network there just begs for people to start adding all sorts of stuff in there. I'm not totally against it but I don't see any benefit.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jun 9, 2023

Changes in force push: Fix the no-std build errors after learning how to actually build the code.

apoelstra
apoelstra approved these changes Jun 10, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fd36a6bc1a06c55b1cc76dc86d7825054118ab38

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jun 10, 2023

Cheers bro

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jun 29, 2023

Friendly bump, I believe all review suggestions have been addressed. Thanks

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jul 8, 2023

Rebased on master (to pick up #1927). No other changes.

apoelstra
apoelstra previously approved these changes Jul 12, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a14ad887396b38486585da2ebeaa28c18c8d0685

@tcharding tcharding mentioned this pull request Jul 18, 2023
Merged
@tcharding
Add BorrowMut to prelude
dac9707 
We already have `Borrow`, add `BorrowMut`.
@tcharding
Rename the network module to p2p
1bac1fd 
The `network` module deals with data types and logic related to
internetworking bitcoind nodes, this is commonly referred to as the p2p
layer.

Rename the `network` module to `p2p` and fix all the paths.
@tcharding
Move p2p::constants::PROTOCOL_VERSION to p2p module
4330722 
The `PROTOCOL_VERSION` const is a p2p layer constant. It can live in the
`mod.rs` file of the `p2p` module.

This is a straight code move, the `PROTOCOL_VERSION` replaces the
current re-export.
@tcharding
Improve rustdocs on PROTOCOL_VERSION
99d8ae1
@tcharding
Move p2p::constants::ServiceFlags to p2p module
d9d5a4e 
The `ServiceFlags` type is used by the p2p layer. It can live in the
`mod.rs` file of the `p2p` module. Done in preparation for removing the
`p2p::constants` module.

This is a straight code move, the `ServiceFlags` replaces the
current re-export.
@tcharding
Move p2p::constants::Magic to p2p module
0f78943 
In preparation for removing the `p2p::constants` module; move the
`p2p::constants::Magic` type to the `p2p` module.
@tcharding
Move p2p::constants::Network to crate root
d4e8f49 
The `Network` type is not a p2p construct, it is more general, used
throughout the codebase to define _which_ Bitcoin network we are
operating on.
@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Aug 1, 2023

Rebased, no other changes.

apoelstra added a commit that referenced this pull request Aug 1, 2023
@apoelstra
Merge #1945: policy: Add refactor carve out
1b952de 
55be538 policy: Add refactor carve out (Tobin C. Harding)

Pull request description:

  I have managed to burn out or bore our reviewers/maintainers. Getting two acks is becoming increasingly difficult. I've pestered everyone to the limit that I feel socially comfortable doing so, and as such, am requesting a carve out to the 2-ACK before merge rule.

  The primary justification is that I feel we should have a bit more of BDFL and a bit less total consensus if we are to push forwards.

  ### Example PRs where this change would apply

  - #1925
  - #1854
  - #1862

ACKs for top commit:
  elichai:
    I agree this makes sense for refactors ACK 55be538
  apoelstra:
    ACK 55be538
  sanket1729:
    ACK 55be538. Same reasons as apoelstra. And this is only for re-factors that are not adding any new features.
  RCasatta:
    ACK 55be538

Tree-SHA512: a5e206252015f49245ed282a3be7a35760d16f94dc6e60f31edf589a41ef642eba52a3bd7d1375b6033f3cf0128f47beee4f03e59cad151c64eedd71ac98baac
apoelstra
apoelstra approved these changes Aug 1, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d4e8f49

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Aug 1, 2023

Merging based on new one-ack-2-weeks policy in #1945

@apoelstra apoelstra merged commit fc562e9 into rust-bitcoin:master Aug 1, 2023
@tcharding tcharding deleted the 05-18-network branch May 22, 2024 23:14
@darosior darosior mentioned this pull request Aug 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kixunil Kixunil Kixunil left review comments

@apoelstra apoelstra apoelstra approved these changes

Assignees

No one assigned

Labels

API break This PR requires a version bump for the next release code quality Makes code easier to understand and less likely to lead to problems

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

network module could be more descriptively named

4 participants

@tcharding @apoelstra @Kixunil @stevenroose