Skip to content
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.

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

Unleash the power of Bitcoin Core into bdk-cli #92

Closed

Conversation

rajarshimaitra
Copy link
Contributor

@rajarshimaitra rajarshimaitra commented May 13, 2022

Description

fixes #62
fixes #76

This PR does the following

  • Bump bdk version to 0.19.
  • Opens up some basic bitcoin-core commands via a new sub-command bdk-cli node <command> [<args>]. The API of the node commands are kept very similar to bitcoin-cli api. This allows us to control the auto deployed backend node via regtest-* features from bdk-cli itself.
  • The Integration tests are written with std::Command from rust. That can be used to simulate various wallet transaction situations with bdk-cli, like Test watch-only LND wallet and signing PSBT #87.
  • These apis are also exposed in repl mode so now bdk-cli can have real time communication between a backend and a wallet in repl shell itself. Which can be very useful for quick runs of different testing conditions.

Notes to the reviewers

@sandipndev @krtk6160. This is the PR you guys can start working on top of to simulate the intended test situations. At least with bitcoind it can be done with all existing toolings. For LND some other wrapper needes to be built.

@notmandatory let me know what you think about the whole framework.

Also looking for more integration test ideas too add into.

basic node usage looks like this

$ ./target/debug/bdk-cli node --help
bdk-cli-node 0.5.0
Regtest Node mode

USAGE:
    bdk-cli node <SUBCOMMAND>

FLAGS:
    -h, --help       
            Prints help information

    -V, --version    
            Prints version information


SUBCOMMANDS:
    generate         Generate blocks
    getbalance       Get Wallet balance
    getinfo          Get info
    getnewaddress    Get new address from node's test wallet
    help             Prints this message or the help of the given subcommand(s)
    sendtoaddress    Send to an external wallet address

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

@rajarshimaitra rajarshimaitra changed the title Unleas the power of Bitcoin Core into bdk-cli Unleash the power of Bitcoin Core into bdk-cli May 13, 2022
@notmandatory
Copy link
Member

Hey @rajarshimaitra concept ACK! but I need to focus on the next bdk release (taproot!) so won't be able to do a through review until that's out.

@rajarshimaitra
Copy link
Contributor Author

Ya thanks @notmandatory no issues.. I opened this early for discussion.. This can wait till other major things are done..

 - Update BDK to v0.19
 - update electrsd to v0.19 to get latest required upstream changes

Note that `regtest-esplora-*` features aren't working as of now. Some
issues in `elctrsd/esplora` feature. Thus removed from CI tests.
Add a separate subcommand list for for node operation related commands.
Right now they only include basic operations, but can be extended later
as per need.

Add a Backend struct that will hold the running bitcoind or electrsd
process in the background. The electrsd and bitcoind will be connected
together. And the wallet will connect to the backend electrsd via
electrum blockchain config. So in case of `regtest-electrum` too we will
operate the backend via rpc to the underlying core node.
Update the lib tests to accommodate changes of the previous commit.
The database creation functions are broken up and chained together to
create the right database directory for the right context and not reuse
code.
Update the Backend handling logic for new_blockchain() function.
The Backend doesn't contain connection data anymore but the full
bitcoind and electrsd instance.

The Backend struct definition is moved into lib.rs.
Update the Backend handling logic in main() to the new Backend struct.
Update the handle_command() function to handle node command that operates
on the Backend.

Add node commands in REPL mode too to get regtest-* features available
in repl.
This is a simple test framework to using std::Cmd to test custom built
bdk-cli with `regtest-*` feature to quickly simulate integration testing
of any kind of situation involving one/many bdk wallets, and one bitcoind
or electrsd process on the background.

All the bdk-cli command line commands can be used in this framework to
operate all kind of tests. The `bdk-cli wallet <cmd>` and
`bdk-cli node <cmd>` makes bdk-cli the complete integration testing
environment itself.

Each tests can be manually played in the `bdk-cli repl` mode too.
@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Jun 14, 2022

  • Done some major refactoring.
  • Updated all the pending dependencies.
  • Updated to bdk v0.19.0
  • Broken down the commits into smaller chunks for easier review
  • Refactored the existing Backend struct
  • Backend now can be both bitcoind and electrum
  • Some minor update in the integration testing..

@notmandatory this PR is now ready for review..

Edit: The tests failure is due to version conflicts in bdk ocuring from bdk-reserves.. Working on a fix for that.. The PR can be code reviewed in the mean time..

@rajarshimaitra
Copy link
Contributor Author

Updated the PR doc..

The current test failure at cargo build --features reserves,electrum --locked fixes with https://github.com/weareseba/bdk-reserves/pull/5

@rajarshimaitra
Copy link
Contributor Author

Opened an alternate version of the same PR in #102.

Not closing this one yet, in case we need to revert back to previous crate structure..

@notmandatory notmandatory removed this from the Release 0.6.0 milestone Jun 21, 2022
@notmandatory notmandatory added this to the Release 0.7.0 milestone Jun 21, 2022
notmandatory added a commit that referenced this pull request Jun 27, 2022
5b20283 update CI to remove some features (rajarshimaitra)

Pull request description:

  <!-- You can erase any parts of this template not applicable to your Pull Request. -->

  ### Description

  Currently the way `ExternalReserves` functionality is written, it can only be used with `electrum` feature. The tests should fail if the implementation behavior is enforced.. Disabling the tests in CI.

  Also removing the `regtest-esplora` features from the tests, because they won't work when #92 lands.

  ### Notes to the reviewers

  <!-- In this section you can include notes directed to the reviewers, like explaining why some parts
  of the PR were done in a specific way -->

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk-cli/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

ACKs for top commit:
  notmandatory:
    ACK 5b20283.

Tree-SHA512: 09e875376e2a8b1a43c318b66849abdeb518deee32ca39ba1e10c9b9230b4af61a3391e5a1dcad0586643a820eb66fabfea9d2af95f0d97fe8383524c5e050d9
notmandatory added a commit that referenced this pull request Jul 6, 2022
292dd1e Fix repl mode command parsing (Steve Myers)
073f1c3 Update with review comments (rajarshimaitra)
4e8f830 revert author list change (rajarshimaitra)
b09c405 Remove base64 dependency (rajarshimaitra)
1e70ff9 Refactor everything (rajarshimaitra)

Pull request description:

  <!-- You can erase any parts of this template not applicable to your Pull Request. -->

  ### Description

  This is a massive refactoring PR that changes the whole structure of the crate. Previously it was written like a library
  to be used to create the bdk-cli app. But eventually the crate itself became the app. This PR attempts to remove the remaining
  lib like patterns in the code, and make it a pure binary crate.

  This makes the code more modular and makes it look like a typical binary rust crate.

  There was no real good way to structure the change into separate commits, so I made one single big one.. The best way to review is to look at the final structure of the code itself, not the change set.

  The crate has following modules now
   -  `main` : The main app runtime
   -  `commands`: Includes all the structopt commands used by bdk-cli.
   -  `handlers`: Include all the command handlers used buy the app.
   -  `utils`: Include all the utility and helper functions
   - `Backend` : Defines the backend node process, and its related methods. (This will be filled more with #92).

  Apart from the structure changes there are few other changes that took place
   - Almost all of the previous doc comments are removed. As they were written to use bdk-cli as a lib. Instead new structopts "comments" are added to describe the app functionality better. As a result the app `--help` commands are more elaborate and descriptive now. I have also removed few redundant description messages used before, that would mess up the help comments. And as a by product it solves #93.

   - bdk is updated to v0.19.0

   - bdk-reserves is updated with current version pointing to bdk v0.19.0.

   - Default database is now sqlite.

   Overall I think I managed not to break anything.

   Currently this change will remove most of the previous documentation on the crate. But those aren't useful to context of bdk-cli after this change.. My proposal would be reproduce the README instructions itself in doc.rs landing page.

   We also need to update the README to reflect these changes.. I will open that up in a separate PR.

   I also haven't updated changelog yet.. Not sure yet how to describe the change in short.. Will do that once this is almost finalized..

  ### Notes to the reviewers

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk-cli/blob/master/CONTRIBUTING.md)
  * [x] 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`

ACKs for top commit:
  notmandatory:
    ACK 292dd1e

Tree-SHA512: 895d8088bf93a481fd776e2ac5fe85926f13b7b4535f17b9edd3c0363a89dc3689e28c6e13dbcac3970bc00e3ff206f402e94406f3b3688c9e4a7f9d31b20e40
waterst0ne pushed a commit to waterst0ne/bdk-cli that referenced this pull request Jul 7, 2022
292dd1e Fix repl mode command parsing (Steve Myers)
073f1c3 Update with review comments (rajarshimaitra)
4e8f830 revert author list change (rajarshimaitra)
b09c405 Remove base64 dependency (rajarshimaitra)
1e70ff9 Refactor everything (rajarshimaitra)

Pull request description:

  <!-- You can erase any parts of this template not applicable to your Pull Request. -->

  ### Description

  This is a massive refactoring PR that changes the whole structure of the crate. Previously it was written like a library
  to be used to create the bdk-cli app. But eventually the crate itself became the app. This PR attempts to remove the remaining
  lib like patterns in the code, and make it a pure binary crate.

  This makes the code more modular and makes it look like a typical binary rust crate.

  There was no real good way to structure the change into separate commits, so I made one single big one.. The best way to review is to look at the final structure of the code itself, not the change set.

  The crate has following modules now
   -  `main` : The main app runtime
   -  `commands`: Includes all the structopt commands used by bdk-cli.
   -  `handlers`: Include all the command handlers used buy the app.
   -  `utils`: Include all the utility and helper functions
   - `Backend` : Defines the backend node process, and its related methods. (This will be filled more with bitcoindevkit#92).

  Apart from the structure changes there are few other changes that took place
   - Almost all of the previous doc comments are removed. As they were written to use bdk-cli as a lib. Instead new structopts "comments" are added to describe the app functionality better. As a result the app `--help` commands are more elaborate and descriptive now. I have also removed few redundant description messages used before, that would mess up the help comments. And as a by product it solves bitcoindevkit#93.

   - bdk is updated to v0.19.0

   - bdk-reserves is updated with current version pointing to bdk v0.19.0.

   - Default database is now sqlite.

   Overall I think I managed not to break anything.

   Currently this change will remove most of the previous documentation on the crate. But those aren't useful to context of bdk-cli after this change.. My proposal would be reproduce the README instructions itself in doc.rs landing page.

   We also need to update the README to reflect these changes.. I will open that up in a separate PR.

   I also haven't updated changelog yet.. Not sure yet how to describe the change in short.. Will do that once this is almost finalized..

  ### Notes to the reviewers

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk-cli/blob/master/CONTRIBUTING.md)
  * [x] 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`

ACKs for top commit:
  notmandatory:
    ACK 292dd1e

Tree-SHA512: 895d8088bf93a481fd776e2ac5fe85926f13b7b4535f17b9edd3c0363a89dc3689e28c6e13dbcac3970bc00e3ff206f402e94406f3b3688c9e4a7f9d31b20e40
@rajarshimaitra
Copy link
Contributor Author

Closing this as we are moving ahead with #102

@notmandatory notmandatory removed this from the Release 0.7.0 milestone Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Open up rpc control for auto deployed regtest mode Create Integration tests for bdk-cli
2 participants