Rename `network` module and move/clean up types #1854

tcharding · 2023-05-18T01:19:45Z

This PR has grown.

Rename the network module to p2p (fix: network module could be more descriptively named #1855)
add BorrowMut to prelude
Move the Network, ChainHash, and Magic structs to a newly create p2p::network module
Do a few cleanups

tcharding · 2023-05-18T01:21:09Z

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.

Kixunil

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 · 2023-05-19T21:37:41Z

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 · 2023-05-22T14:56:47Z

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.

apoelstra · 2023-05-25T12:42:40Z

@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 · 2023-05-25T13:05:27Z

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

tcharding · 2023-05-26T07:22:56Z

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 · 2023-05-26T07:40:05Z

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

tcharding · 2023-05-27T05:56:00Z

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 · 2023-05-27T12:57:03Z

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 · 2023-05-30T06:29:11Z

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 · 2023-05-30T06:37:14Z

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 · 2023-05-30T09:24:09Z

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

stevenroose · 2023-06-07T06:02:02Z

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.
isn't chainhash a lightning concept?
I would say, keep Param in consensus and wire can have Magic
Network could be more general, doesn't necessarily have to occur in either consensus nor wire/p2p

tcharding · 2023-06-08T05:29:37Z

Thanks man, I'll go back over the types and process your message when I'm thinking clearly.

tcharding · 2023-06-09T06:22:09Z

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 · 2023-06-09T12:29:21Z

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 · 2023-06-09T15:29:15Z

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 · 2023-06-09T15:42:01Z

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 · 2023-06-09T15:42:43Z

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

apoelstra

ACK fd36a6b

tcharding · 2023-06-10T03:19:04Z

Cheers bro

tcharding · 2023-06-29T01:37:07Z

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

tcharding · 2023-07-08T01:25:48Z

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

apoelstra

ACK a14ad88

We already have `Borrow`, add `BorrowMut`.

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.

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.

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.

In preparation for removing the `p2p::constants` module; move the `p2p::constants::Magic` type to the `p2p` module.

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 · 2023-08-01T06:50:35Z

Rebased, no other changes.

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

ACK d4e8f49

apoelstra · 2023-08-01T18:41:37Z

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

tcharding force-pushed the 05-18-network branch from 555254e to 89f4344 Compare May 18, 2023 01:23

tcharding marked this pull request as draft May 18, 2023 01:29

tcharding force-pushed the 05-18-network branch from 89f4344 to 22956d7 Compare May 18, 2023 01:50

tcharding marked this pull request as ready for review May 18, 2023 01:51

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 force-pushed the 05-18-network branch from 22956d7 to e5af87e Compare May 18, 2023 01:53

tcharding changed the title ~~Move Network and ChainHash to lib.rs~~ Move Network, ChainHash, and Magic to lib.rs May 18, 2023

tcharding changed the title ~~Move Network, ChainHash, and Magic to lib.rs~~ Create chain module May 18, 2023

tcharding force-pushed the 05-18-network branch from e5af87e to 1fb54a5 Compare May 18, 2023 02:05

tcharding mentioned this pull request May 18, 2023

network module does not get many eyes #1856

Closed

Kixunil reviewed May 19, 2023

View reviewed changes

tcharding force-pushed the 05-18-network branch from 1fb54a5 to 208f1b8 Compare May 25, 2023 01:22

tcharding changed the title ~~Create chain module~~ Rename network module and move/clean up types May 25, 2023

tcharding force-pushed the 05-18-network branch from 208f1b8 to 367fc23 Compare May 25, 2023 02:33

tcharding force-pushed the 05-18-network branch from 367fc23 to ac5e50e Compare May 30, 2023 06:28

tcharding force-pushed the 05-18-network branch from ac5e50e to affa715 Compare May 30, 2023 06:35

tcharding force-pushed the 05-18-network branch from affa715 to 5c8bb93 Compare May 30, 2023 09:23

tcharding force-pushed the 05-18-network branch from 4be5513 to 8ce8358 Compare June 9, 2023 09:00

tcharding force-pushed the 05-18-network branch from 8ce8358 to fd36a6b Compare June 9, 2023 15:42

apoelstra approved these changes Jun 10, 2023

View reviewed changes

tcharding force-pushed the 05-18-network branch from fd36a6b to a14ad88 Compare July 8, 2023 01:25

apoelstra previously approved these changes Jul 12, 2023

View reviewed changes

tcharding mentioned this pull request Jul 18, 2023

policy: Add refactor carve out #1945

Merged

tcharding added 7 commits August 1, 2023 16:36

Add BorrowMut to prelude

dac9707

We already have `Borrow`, add `BorrowMut`.

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.

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.

Improve rustdocs on PROTOCOL_VERSION

99d8ae1

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.

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 dismissed apoelstra’s stale review via d4e8f49 August 1, 2023 06:50

tcharding force-pushed the 05-18-network branch from a14ad88 to d4e8f49 Compare August 1, 2023 06:50

apoelstra approved these changes Aug 1, 2023

View reviewed changes

apoelstra merged commit fc562e9 into rust-bitcoin:master Aug 1, 2023
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename `network` module and move/clean up types #1854

Rename `network` module and move/clean up types #1854

tcharding commented May 18, 2023 •

edited

tcharding commented May 18, 2023

Kixunil left a comment

tcharding commented May 19, 2023 •

edited

apoelstra commented May 22, 2023

apoelstra commented May 25, 2023

tcharding commented May 25, 2023

tcharding commented May 26, 2023

tcharding commented May 26, 2023

tcharding commented May 27, 2023

apoelstra commented May 27, 2023

tcharding commented May 30, 2023

tcharding commented May 30, 2023

tcharding commented May 30, 2023

stevenroose commented Jun 7, 2023

tcharding commented Jun 8, 2023

tcharding commented Jun 9, 2023

apoelstra commented Jun 9, 2023

tcharding commented Jun 9, 2023

tcharding commented Jun 9, 2023 •

edited

tcharding commented Jun 9, 2023

apoelstra left a comment

tcharding commented Jun 10, 2023

tcharding commented Jun 29, 2023

tcharding commented Jul 8, 2023

apoelstra left a comment

tcharding commented Aug 1, 2023

apoelstra left a comment

apoelstra commented Aug 1, 2023

Rename network module and move/clean up types #1854

Rename network module and move/clean up types #1854

Conversation

tcharding commented May 18, 2023 • edited

tcharding commented May 18, 2023

Kixunil left a comment

Choose a reason for hiding this comment

tcharding commented May 19, 2023 • edited

apoelstra commented May 22, 2023

apoelstra commented May 25, 2023

tcharding commented May 25, 2023

tcharding commented May 26, 2023

tcharding commented May 26, 2023

tcharding commented May 27, 2023

apoelstra commented May 27, 2023

tcharding commented May 30, 2023

tcharding commented May 30, 2023

tcharding commented May 30, 2023

stevenroose commented Jun 7, 2023

tcharding commented Jun 8, 2023

tcharding commented Jun 9, 2023

apoelstra commented Jun 9, 2023

tcharding commented Jun 9, 2023

tcharding commented Jun 9, 2023 • edited

tcharding commented Jun 9, 2023

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Jun 10, 2023

tcharding commented Jun 29, 2023

tcharding commented Jul 8, 2023

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Aug 1, 2023

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Aug 1, 2023

Rename `network` module and move/clean up types #1854

Rename `network` module and move/clean up types #1854

tcharding commented May 18, 2023 •

edited

tcharding commented May 19, 2023 •

edited

tcharding commented Jun 9, 2023 •

edited