feat!: use `NetworkKind` #250

luisschwab · 2025-06-01T17:34:39Z

Description

Closes #22.
Closes #94.

This PR replaces bitcoin::Network for bitcoin::NetworkKind where it applies.

I also took the liberty of fixing up and adding comments on the files I touched.

Changelog

wallet_name_from_descriptor() takes NetworkKind instead of Network.
DescriptorToExtract takes NetworkKind instead of Network.
impl IntoWalletDescriptor for <T> takes Network instead NetworkKind.
DescriptorTemplate::build() takes NetworkKind instead of Network.
ExtendedKey::into_xprv() takes NetworkKind instead of Network.
ExtendedKey::into_xpub() takes NetworkKind instead of Network.
DerivableKey::into_extended_key() examples updated to use NetworkKind instead of Network.
any_network() renamed to any_network_kind().
mainnet_network() renamed to mainnet_network_kind().
test_networks() renamed to test_network_kind().
merge_networks() renamed to merge_network_kinds().
ValidNetworks type alias renamed to ValidNetworkKinds.
GeneratedKey::new() takes ValidNetworkKinds instead of ValidNetworks.
DescriptorKey::from_public() takes ValidNetworkKinds instead of ValidNetworks.
DescriptorKey::from_secret() takes ValidNetworkKinds instead of ValidNetworks.
KeyError::InvalidNetwork renamed to KeyError::InvalidNetworkKind.
DescriptorKey::override_valid_networks() renamed to override_valid_network_kinds().
Network validation logic updated to use NetworkKind instead of Network.
Test network validation now uses NetworkKind::Test instead of Network::{Regtest, Signet, Testnet, Testnet4}.
Examples and comments updated to use NetworkKind instead of Network.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo +nightly fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

coveralls · 2025-06-01T17:39:02Z

Pull Request Test Coverage Report for Build 18329864246

Details

372 of 403 (92.31%) changed or added relevant lines in 9 files are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.2%) to 84.995%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/keys/bip39.rs	13	14	92.86%
src/descriptor/dsl.rs	38	40	95.0%
src/wallet/mod.rs	10	13	76.92%
src/descriptor/template.rs	119	123	96.75%
src/descriptor/policy.rs	15	21	71.43%
src/descriptor/mod.rs	48	55	87.27%
src/keys/mod.rs	127	135	94.07%

Files with Coverage Reduction	New Missed Lines	%
src/descriptor/template.rs	4	97.46%

Totals
Change from base Build 18318022693:	0.2%
Covered Lines:	7001
Relevant Lines:	8237

💛 - Coveralls

ValuedMammal · 2025-09-20T02:05:11Z

Concept ACK. To consider this work complete I think we should also strive for a fix to #22. Edit: This is a proposed solution 277ae25. Also I think GeneratedKey should hold the NetworkKind internally (instead of ValidNetworkKinds).

wallet/src/wallet/mod.rs

wallet/src/keys/mod.rs

src/descriptor/mod.rs

ValuedMammal · 2025-09-24T15:55:31Z

You certainly did take some liberties with the code, definitely an improvement overall. In the future it would be nice to make a separate patch for things like documentation and code comments.

ValuedMammal · 2025-09-25T20:57:19Z

ACK 3ee3ae2

ValuedMammal · 2025-09-29T14:16:02Z

Will need a rebase. Sorry for the inconvenience.

src/descriptor/dsl.rs

oleonardolima · 2025-09-30T05:39:59Z

src/descriptor/dsl.rs

+        // As expected, this does not compile due to invalid context:
+        // ```
+        // let desc_key: DescriptorKey<Segwitv0> = (xprv, path.clone()).into_descriptor_key().unwrap();
+        // let (desc, _key_map, _valid_network_kinds) = descriptor!(pkh(desc_key)).unwrap();
+        // ```


question/nit: could we have this as doctest instead and use compile_fail ?

Wouldn't a single doc block look odd among all tests?

src/descriptor/mod.rs

oleonardolima · 2025-09-30T05:46:49Z

src/descriptor/template.rs

                .unwrap();
        check(
-            P2Pkh(prvkey).build(Network::Bitcoin),
+            P2Pkh(prvkey).build(NetworkKind::Test),


question: any reason to change this from mainnet to test ?

I believe we agreed to use NetworkKind::Test on all tests.

oleonardolima · 2025-09-30T05:47:20Z

src/descriptor/template.rs

        );
        check(
-            Bip44Public(pubkey, fingerprint, KeychainKind::Internal).build(Network::Bitcoin),
+            Bip44Public(pubkey, fingerprint, KeychainKind::Internal).build(NetworkKind::Test),


question: same here.

I believe we agreed to use NetworkKind::Test on all tests.

thunderbiscuit · 2025-09-30T12:49:31Z

I have just a quick feedback that might help you in further PRs @luisschwab. Last week I saw your message on Discord looking for review on this, and I decided to take a look. In the end the diffs are really big because it's so much docs changes all mixed up with code changes, and I didn't have enough time before my day was over.

My advice for further work would be to separate docs and code changes in different commits (I don't actually mind that they are in the same PR, and I know that sometimes we're in a given part of the codebase and it makes sense to fix docs while we're there and thinking about it). I actually like these little ongoing fixes/polish of the docs. But when the diff is big, I review one commit at a time, and that makes it much easier if the two ideas are separate.

luisschwab · 2025-09-30T14:54:34Z

Yeah, Mammal mentioned that as well. This PR is pretty old, but I'll keep that in mind for the future.

..to require the NetworkKind when generating the key. This fixes an issue where keys generated for mainnet were incorrectly marked valid for any network. test(keys): Add `test_xpriv_generate_options` fixes bitcoindevkit#22

github-project-automation bot added this to BDK Wallet Jun 1, 2025

luisschwab force-pushed the feat/networkkind branch 2 times, most recently from 22d27a6 to e6e6266 Compare June 1, 2025 17:41

luisschwab self-assigned this Jun 1, 2025

luisschwab added the api A breaking API change label Jun 1, 2025

luisschwab moved this to Needs Review in BDK Wallet Jun 1, 2025

luisschwab modified the milestones: Wallet 3.1.0, Wallet 3.0.0 Jun 1, 2025

luisschwab force-pushed the feat/networkkind branch from e6e6266 to ecefb4c Compare June 30, 2025 21:22

luisschwab force-pushed the feat/networkkind branch from ecefb4c to 3e25aef Compare September 22, 2025 20:39

luisschwab requested a review from ValuedMammal as a code owner September 22, 2025 20:39

luisschwab added the bug Something isn't working label Sep 24, 2025

ValuedMammal reviewed Sep 24, 2025

View reviewed changes

wallet/src/wallet/mod.rs Outdated Show resolved Hide resolved

wallet/src/keys/mod.rs Outdated Show resolved Hide resolved

wallet/src/keys/mod.rs Outdated Show resolved Hide resolved

wallet/src/keys/mod.rs Outdated Show resolved Hide resolved

src/descriptor/mod.rs Show resolved Hide resolved

luisschwab force-pushed the feat/networkkind branch from 03fb39c to 3ee3ae2 Compare September 25, 2025 18:24

luisschwab requested a review from ValuedMammal September 25, 2025 18:30

ValuedMammal approved these changes Sep 25, 2025

View reviewed changes

luisschwab requested a review from oleonardolima September 25, 2025 22:12

luisschwab force-pushed the feat/networkkind branch from 3ee3ae2 to 34fe4d6 Compare September 30, 2025 00:06

luisschwab requested a review from ValuedMammal September 30, 2025 00:12

oleonardolima reviewed Sep 30, 2025

View reviewed changes

luisschwab added 2 commits October 1, 2025 09:36

feat!(descriptor): use NetworkKind

5d2ee01

feat!(keys): use NetworkKind

45e196f

luisschwab added 2 commits October 1, 2025 09:36

feat!(wallet): use NetworkKind

17530f8

feat!(examples,test): use NetworkKind

a04fa29

luisschwab force-pushed the feat/networkkind branch from 34fe4d6 to 24da1b6 Compare October 1, 2025 12:36

fix: GeneratableKey options for PrivateKey, Xpriv

554bb75

..to require the NetworkKind when generating the key. This fixes an issue where keys generated for mainnet were incorrectly marked valid for any network. test(keys): Add `test_xpriv_generate_options` fixes bitcoindevkit#22

luisschwab force-pushed the feat/networkkind branch from 24da1b6 to 554bb75 Compare October 8, 2025 00:23

luisschwab mentioned this pull request Oct 8, 2025

refactor: rename merge_networks to intersect_networks #324

Open

3 tasks

feat!: use NetworkKind #250

Are you sure you want to change the base?

feat!: use NetworkKind #250

Uh oh!

Conversation

luisschwab commented Jun 1, 2025 • edited by ValuedMammal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changelog

Checklists

All Submissions:

New Features:

Bugfixes:

Uh oh!

coveralls commented Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 18329864246

Details

💛 - Coveralls

Uh oh!

ValuedMammal commented Sep 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ValuedMammal commented Sep 24, 2025

Uh oh!

ValuedMammal commented Sep 25, 2025

Uh oh!

ValuedMammal commented Sep 29, 2025

Uh oh!

Uh oh!

oleonardolima Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

luisschwab Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oleonardolima Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

luisschwab Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

oleonardolima Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

luisschwab Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

thunderbiscuit commented Sep 30, 2025

Uh oh!

luisschwab commented Sep 30, 2025

Uh oh!

Uh oh!

feat!: use `NetworkKind` #250

feat!: use `NetworkKind` #250

luisschwab commented Jun 1, 2025 •

edited by ValuedMammal

Loading

coveralls commented Jun 1, 2025 •

edited

Loading

ValuedMammal commented Sep 20, 2025 •

edited

Loading