Allow enabling at most one blockchain client feature #38

notmandatory · 2021-08-05T04:59:30Z

Description

Allow at most one blockchain client feature be enabled at a time for builds. If no blockchain client feature is enabled then online wallet commands are disabled. This will simplify the options shown to the user and make adding new blockchain clients (such as #36) easier. ~~Electrum is still the default, to make a build with a different blockchain client the --no-default-features build option will need to be used.~~ No blockchain client is included in the default features, so if one is needed it must be specified with --features. I also added a default esplora server url so the user doesn't need to specify one if selecting that client, which is how the electrum and compact_filters clients work.

Notes to the reviewers

I changed the server option for both electrum and esplora to --server or -s since that now won't cause a conflict. I also simplified the CHANGELOG to focus on what a user would see as a change while using the bin.

I've also added a build.rs file to prevent more than one blockchain client feature from being enabled.

Checklists

All Submissions:

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

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

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

rajarshimaitra

Thanks @notmandatory for the PR, we are close almost. I just have a few more comments.

Is there any specific use of Repl mode here?

bdk-cli/src/lib.rs

Lines 258 to 262 in 378b33a

 #[structopt(long_about = "REPL command loop mode")] 

 Repl { 

 #[structopt(flatten)] 

 wallet_opts: WalletOpts, 

 },

It just seems to be duplicating WalletOpts and no commands. If not we can remove this, it seems redundant.

One problem here is using --no-default-features will turn off repl too, which is required dep for the binary, so it will not compile any binary. This has been the crux of the problem because cargo doesn't let us selectively disable features. So if we disable default, it disables everything in default.

I think the only way to solve this problem is by having a very minimal default with only the stuffs that we need to create the minimal possible binary. And then add stuff to it with features flag. This will also remove the use of --no-default-feature if we only want esplora but not electrum.

So I propose something like this:

[features]
default = ["repl"]
repl = ["bdk/key-value-db", "clap", "dirs-next", "env_logger", "regex", "rustyline"]
electrum = ["bdk/electrum"]
esplora = ["bdk/esplora"]
compiler = ["bdk/compiler"]
async-interface = ["bdk/async-interface"]
compact_filters = ["bdk/compact_filters"]

[[bin]]
name = "bdk-cli"
path = "src/bdk_cli.rs"
required-features = ["repl"]

This will create the following builds

cargo build : build only repl stuffs, no backend
cargo build --features [backend] : build repl + [backend] blockchain.

I don't think there's any adverse effect of not having a blockchain by default. the only method we have with blockchain is sync and broadcast, everything else can be done without a blockchain. So it's safe to remove it in default.

rajarshimaitra · 2021-08-05T07:25:03Z

Also, I am not sure if we should silently ignore "no binary built" scenarios in CI.

bdk-cli/.github/workflows/cont_integration.yml

Line 43 in 378b33a

run: cargo build --features ${{ matrix.features }} --no-default-features

with current changes for example cargo build --features esplora --no-default-features will not build any binary, but the CI doesn't complain.

you can check that locally with

$ cargo clean
$ cargo build --features esplora --no-default-features
$ ./target/debug/bdk-cli wallet --help
bash: ./target/debug/bdk-cli: No such file or directory

I am not sure how then it's passing the tests without a binary.

notmandatory · 2021-08-06T01:48:53Z

I think tests were passing because they only needed a lib build to run. But I agree it would be nice to not have to use the --no-default-features flag. I did the following to try and address this:

split the repl feature into cli for the required dependencies to build the cli bin, and repl to enable the cli repl commands, then made the cli feature required to build the cli bin, but the cli and repl features on by default.
added cfg statements so someone could build the cli without the repl command if they want fewer dependencies and use the --no-default-features flag
update the CI to test with the default (no blockchain client) features, or each blockchain client, or the 'compiler' feature

rajarshimaitra

Code review ACK. This simplifies the issue.

I had the following observation. It seems if we compile the library using cargo build --features cli --no-default-features, that produces a binary that is perfectly workable with non backend wallet. It has functionality like this

$ ./target/debug/bdk-cli wallet --help
bdk-cli-wallet 0.2.1-dev
Wallet mode

USAGE:
    bdk-cli wallet [FLAGS] [OPTIONS] --descriptor <DESCRIPTOR> <SUBCOMMAND>

FLAGS:
    -v, --verbose    
            Adds verbosity, returns PSBT in JSON format alongside serialized

    -h, --help       
            Prints help information

    -V, --version    
            Prints version information


OPTIONS:
    -c, --change_descriptor <CHANGE_DESCRIPTOR>    
            Sets the descriptor to use for internal addresses

    -d, --descriptor <DESCRIPTOR>                  
            Sets the descriptor to use for the external addresses

    -w, --wallet <WALLET_NAME>                     
            Selects the wallet to use [default: main]


SUBCOMMANDS:
    bump_fee             Bumps the fees of an RBF transaction
    combine_psbt         Combines multiple PSBTs into one
    create_tx            Creates a new unsigned transaction
    extract_psbt         Extracts a raw transaction from a PSBT
    finalize_psbt        Finalizes a PSBT
    get_balance          Returns the current wallet balance
    get_new_address      Generates a new external address
    help                 Prints this message or the help of the given subcommand(s)
    list_transactions    Lists all the incoming and outgoing transactions of the wallet
    list_unspent         Lists the available spendable UTXOs
    policies             Returns the available spending policies for the descriptor
    public_descriptor    Returns the public version of the wallet's descriptor(s)
    sign                 Signs and tries to finalize a PSBT

So that begs the question I had before, what do we need the repl thing for?

Also from here https://github.com/notmandatory/bdk-cli/blob/69b1d80bcd4715a3b78af4aafacebe2fc5ca2b88/src/bdk_cli.rs#L318-L331
it seems all a repl does is to perse existing commands and use them with either wallet or key functions. Maybe I am missing something, but I am not seeing any use of having the same methods called twice in different ways.

If two features cli and repl that does the same thing, it seems confusing to me.

rajarshimaitra · 2021-08-06T07:27:10Z

src/bdk_cli.rs

+ feature = "esplora",
+ any(feature = "electrum", feature = "compact_filters")
+ ))]
+ compile_error!("Only one blockchain client feature can be enabled at a time.");


It seems for every combination of electrum, esplora and compact_filters, this error is being duplicated with the one defined few lines below.

Instead, if we write it like this

#[cfg(all( any( feature = "electrum", feature = "esplora", feature = "compact_filters"), any( feature = "electrum", feature = "esplora", feature = "compact_filters") ))]

this will activate for any combination of the above three. And we won't need to specify compiler_error! multiple times.

Also for future extension, if we just add a new backend to the list above, that will taker care of multiple backend activation errors.

Unfortunately the cfg all any any approach doesn't work, when I added this code:

#[cfg(all( any( feature = "electrum", feature = "esplora", feature = "compact_filters"), any( feature = "electrum", feature = "esplora", feature = "compact_filters") ))] compile_error!("Only one blockchain client feature can be enabled at a time.");

I get the error even with only one blockchain client feature enabled (ie. cargo build --features esplora).

Looks like other's have had this same problem and are proposing a Rust RFC to allow a set of features to provide another feature in a mutually exclusive way.

Until mutually exclusive features are possible I can fix this in a simplistic way in the build.rs file as below or just leave it as it.. What do you think?

fn main() { let electrum = env::var_os("CARGO_FEATURE_ELECTRUM").map(|_| "electrum".to_string()); let esplora = env::var_os("CARGO_FEATURE_ESPLORA").map(|_| "esplora".to_string()); let compact_filters = env::var_os("CARGO_FEATURE_COMPACT_FILTERS").map(|_| "compact_filters".to_string()); let blockchain_features : Vec<String> = vec!(electrum, esplora, compact_filters) .iter() .map(|f| f.to_owned()) .flatten() .collect(); if blockchain_features.len() > 1 { panic!("At most one blockchain client feature can be enabled but these features were enabled: {:?}", blockchain_features) } }

Ah ok.. I thought it would work because my analyzer didn't throw. Yes there should be a way to handle mutually exclusive features in cargo itself.

I like the build script approach. It's clear and concise. Till it's available in cargo we can use a guard like above.

rajarshimaitra · 2021-08-06T07:44:15Z

The following structure and function call could use feature guard. They exist in binary compiled without a backend, but are never used.

https://github.com/notmandatory/bdk-cli/blob/69b1d80bcd4715a3b78af4aafacebe2fc5ca2b88/src/lib.rs#L892

https://github.com/notmandatory/bdk-cli/blob/69b1d80bcd4715a3b78af4aafacebe2fc5ca2b88/src/lib.rs#L638

rajarshimaitra

tACK 9bb1c60

With some minor nits.

CHANGELOG.md

rajarshimaitra · 2021-08-07T11:49:26Z

Cargo.toml

@@ -38,7 +40,7 @@ compact_filters = ["bdk/compact_filters"]
 [[bin]]
 name = "bdk-cli"
 path = "src/bdk_cli.rs"
-required-features = ["repl", "electrum"]
+required-features = ["cli"]


Is this required-features value necessary? By putting cli in default we are ensuring that the build will always include cli. (we also removed any necessity of using --no-default-features).

I think we still need cli as required to force anyone building the bin without repl (using --no-default-features) to still include cli.

Ya makes sense.

README.md

rajarshimaitra · 2021-08-07T12:00:18Z

README.md

@@ -42,7 +49,7 @@ cargo run
 To sync a wallet to the default electrum server:

 ```shell
-cargo run -- wallet --descriptor "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/*)" sync
+cargo run --features electrum -- wallet --descriptor "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/*)" sync


Few lines ago we mentioned how to install bdk-cli with cargo.

Instructing to use cargo run here will build the local files instead of using the installed binary. So if someone is in their cloned repo, using cargo run they will actually build and then run the master branch instead of the release binary he just installed (this might be an unexpected behavior for the user, unless he knows cargo stuffs).

Even worse if someone didn't clone the repo, cargo run will not run anything, even if they have bdk-cli binary installed (and we don't have that instruction anywhere).

I think in the "bdk-cli bin usage examples" section, we should just instruct to use bdk-cli binary instead of crago run, which will make the instruction simpler also.

This doesn't have to be fixed here. Just mentioned as it occurred to me.

I agree we should probably update the README to focus on installing with different features, and then usage with an installed version instead of using cargo run since anyone making local changes should already know how to use cargo. But let's do that cleanup in a different PR.

src/lib.rs

rajarshimaitra · 2021-08-11T09:44:14Z

ACK 062542a

rajarshimaitra

Code review with testing ACK 4fd39b7

notmandatory · 2021-08-13T10:45:46Z

Thanks for the suggestions and review on this @rajarshimaitra, all ready to rebase your #36 PR.

notmandatory force-pushed the single_blockchain_feature branch 4 times, most recently from 65729b9 to 47e0962 Compare August 5, 2021 06:15

notmandatory changed the title ~~Single blockchain feature~~ Only allow enabling at most one blockchain client feature Aug 5, 2021

notmandatory changed the title ~~Only allow enabling at most one blockchain client feature~~ Allow enabling at most one blockchain client feature Aug 5, 2021

notmandatory force-pushed the single_blockchain_feature branch 2 times, most recently from f9cd936 to c81a99e Compare August 5, 2021 06:42

notmandatory marked this pull request as ready for review August 5, 2021 06:43

rajarshimaitra reviewed Aug 5, 2021

View reviewed changes

rajarshimaitra reviewed Aug 6, 2021

View reviewed changes

notmandatory force-pushed the single_blockchain_feature branch from d3c0af3 to 9bb1c60 Compare August 7, 2021 00:27

rajarshimaitra reviewed Aug 7, 2021

View reviewed changes

notmandatory added 9 commits August 12, 2021 16:06

Pin zeroize version to <1.4.0 to fix 1.46.0 build

6e54f12

Require only one blockchain client feature at a time

2460781

Update README

7b7624e

Update CHANGELOG

f7f29dd

Add cli feature and make it required for bin build

85c7201

Add build.rs to abort if more than one blockchain client feature

086496f

Fix clippy warnings

c6ca004

Rename ElectrumOpts.electrum to .server

2de50e5

Minor fixes to README and CHANGELOG

7c4f5e7

notmandatory force-pushed the single_blockchain_feature branch from 062542a to 7c4f5e7 Compare August 12, 2021 14:13

Update Cargo.lock and CHANGELOG

4fd39b7

rajarshimaitra approved these changes Aug 13, 2021

View reviewed changes

notmandatory merged commit 4fd39b7 into bitcoindevkit:master Aug 13, 2021

notmandatory added this to the Release 0.3.0 Feature Freeze milestone Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow enabling at most one blockchain client feature #38

Allow enabling at most one blockchain client feature #38

notmandatory commented Aug 5, 2021 •

edited

Loading

rajarshimaitra left a comment •

edited

Loading

rajarshimaitra commented Aug 5, 2021 •

edited

Loading

notmandatory commented Aug 6, 2021 •

edited

Loading

rajarshimaitra left a comment

rajarshimaitra Aug 6, 2021

notmandatory Aug 6, 2021 •

edited

Loading

rajarshimaitra Aug 7, 2021

rajarshimaitra commented Aug 6, 2021

rajarshimaitra left a comment

rajarshimaitra Aug 7, 2021

notmandatory Aug 10, 2021

rajarshimaitra Aug 11, 2021

rajarshimaitra Aug 7, 2021

notmandatory Aug 10, 2021

rajarshimaitra commented Aug 11, 2021

rajarshimaitra left a comment

notmandatory commented Aug 13, 2021

	#[structopt(long_about = "REPL command loop mode")]
	Repl {
	#[structopt(flatten)]
	wallet_opts: WalletOpts,
	},

Allow enabling at most one blockchain client feature #38

Allow enabling at most one blockchain client feature #38

Conversation

notmandatory commented Aug 5, 2021 • edited Loading

Description

Notes to the reviewers

Checklists

All Submissions:

New Features:

Bugfixes:

rajarshimaitra left a comment • edited Loading

Choose a reason for hiding this comment

rajarshimaitra commented Aug 5, 2021 • edited Loading

notmandatory commented Aug 6, 2021 • edited Loading

rajarshimaitra left a comment

Choose a reason for hiding this comment

rajarshimaitra Aug 6, 2021

Choose a reason for hiding this comment

notmandatory Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

rajarshimaitra Aug 7, 2021

Choose a reason for hiding this comment

rajarshimaitra commented Aug 6, 2021

rajarshimaitra left a comment

Choose a reason for hiding this comment

rajarshimaitra Aug 7, 2021

Choose a reason for hiding this comment

notmandatory Aug 10, 2021

Choose a reason for hiding this comment

rajarshimaitra Aug 11, 2021

Choose a reason for hiding this comment

rajarshimaitra Aug 7, 2021

Choose a reason for hiding this comment

notmandatory Aug 10, 2021

Choose a reason for hiding this comment

rajarshimaitra commented Aug 11, 2021

rajarshimaitra left a comment

Choose a reason for hiding this comment

notmandatory commented Aug 13, 2021

notmandatory commented Aug 5, 2021 •

edited

Loading

rajarshimaitra left a comment •

edited

Loading

rajarshimaitra commented Aug 5, 2021 •

edited

Loading

notmandatory commented Aug 6, 2021 •

edited

Loading

notmandatory Aug 6, 2021 •

edited

Loading