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

Satoshi's Calculator. #102

Merged

Conversation

rajarshimaitra
Copy link
Contributor

@rajarshimaitra rajarshimaitra commented Jun 20, 2022

Description

fixes #62
fixes #76

On the name :
If Bitcoin wallet devs had to have a "calculator", this is what it might look like. One single interface to do all wallet and node operations.

This PR does the following

  • Update electrsd to latest version. We need this to use it for out auto deployed backends.
  • Add the remaining codes to nodes.rs module to define basic node operation apis on the Nodes enum.
  • Add a new node command in CliSubCommand which has its own subcommand NodeSubCommand, These subcommands are essentially bitcoin-cli calls. And only the basic ones are included so far. We can also compose multiple bitcoin-cli calls to create our own commands in future.
  • Update the handlers to handle newly added NodeSubCommands.
  • Update in utils :
    • This includes handling auto blockchain client creation for regtest-* features.
    • update on the directory creation workflow. We are creating many directories for many stuffs, and I was trying to figure out the easiest way to keep the whole datadir struct intact, no matter where the user points its datadir to. Note that now datadir is a global app option, not just available for regtest nodes. Default datadir is the previous ~/.bdk-bitcoin.
    • Finally add an integration test in tests/integration.rs that runs a basic wallet operation with a bdk-cli wallet connected to auto deployed regtest-bitcoin/electrum.

Notes to the reviewers

@sandipndev @krtk6160. I feel this PR is now ready to try out #87 .

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

The biggest benefit is I think in the repl mode. That now kinda becomes an integrated tool the like python interpreter to operate a bdk-cli and a bitcoin backend from one single interface.

I am excited to see what kind of demonstrations with bdk-cli we can create with this..

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
Copy link
Contributor Author

Pushed some more updates and refactoring.. Rearranged the commit history itself for easier reviews instead of new commits..

@rajarshimaitra rajarshimaitra force-pushed the node-update-2-alternate branch 6 times, most recently from 1204412 to 4b61e64 Compare June 27, 2022 19:35
@rajarshimaitra rajarshimaitra mentioned this pull request Jun 27, 2022
3 tasks
@rajarshimaitra rajarshimaitra force-pushed the node-update-2-alternate branch 2 times, most recently from 168554f to 5d2556c Compare July 8, 2022 10:15
@rajarshimaitra
Copy link
Contributor Author

@notmandatory Thanks for the merge of #99. This one though needs a little more looks because a lot of codes and behaviors are updated.. This finishes up the remaining works of adding integration testing with auto deployed regtest nodes.

Rebased on top of master. Updated the PR description..

This still lacks many crate level documentation, that will be completed in #104 which commits on top of this..

@rajarshimaitra rajarshimaitra changed the title [alternate] Backend Node update Satoshi's Calculator. 😄 Jul 8, 2022
@rajarshimaitra rajarshimaitra changed the title Satoshi's Calculator. 😄 Satoshi's Calculator. Jul 8, 2022
@notmandatory notmandatory added this to the Release 0.7.0 milestone Jul 14, 2022
@notmandatory notmandatory added the enhancement New feature or request label Jul 14, 2022
@rajarshimaitra
Copy link
Contributor Author

@notmandatory I feel like #104 could also be included in this PR itself.. That has no code changes but documentation updates only..

@waterst0ne
Copy link
Contributor

Hi @rajarshimaitra
I tested out all the backend node subcommands generate , getbalance, getinfo, getnewaddress,and sendtoaddress. Everything seems to be operating smoothly on its own.

After playing with it for a few hours I noticed a few things:

  • when running repl on the help menu it is not clear to which commands belong to to backend-node wallet vs the bdk-cli wallet

  • when running repl the generate 100 subcommand has conflicting issues with the bdk-cli generate subcommand. I suggest changing the node generate subcommand to node generateblock

  • This is a preview of what the help menu on repl looks like right now.

  • A new dev would not know which commands are for backend or the bdk wallet.

    generate             Generate blocks
    get_balance          Returns the current wallet balance
    get_new_address      Generates a new external address
    getbalance           Get Wallet balance for Node
    getinfo              Get info
    getnewaddress        Get new address from node's test wallet
  • I suggest changing the description to something that differentiates backend node vs bdk-cli wallet commands to something like this.
    generate             [backend-node] Generate blocks
    get_balance          Returns the current wallet balance
    get_new_address      Generates a new external address
    getbalance           [backend-node] Get Wallet balance for Node
    getinfo              [backend-node]Get info
    getnewaddress        [backend-node] Get new address from node's test wallet

Overall everything looks good, Satoshi's Calculator is pretty awesome for learning I enjoyed using it Thanks!

@rajarshimaitra
Copy link
Contributor Author

Thanks for the look @waterst0ne .. Those are good suggestions, will update them soon..

@rajarshimaitra rajarshimaitra force-pushed the node-update-2-alternate branch 2 times, most recently from e6e602c to 831f339 Compare July 25, 2022 12:24
@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Jul 25, 2022

Thanks @waterst0ne for the review.. Those were helpful.. I have addressed your comments on last 3 commits.

e728577 - Instead of changing the command name, I elaborated the help doc. The command not only generate blocks but also funds the internal wallet.

c1921a7 - Thanks for trying out the repl mode, and I agree having all the commands together is confusing.. I restructured the repl command and have them categorized in node, wallet and key subcommand. Is this more clear the n before?

So repl now looks like this

$ ./target/debug/bdk-cli repl --descriptor "wpkh(tprv8ZgxMBicQKsPeuazF16EdPZw84eHj55AU8ZKgZgdhu3sXcHnFgjzskfDvZdTaAFHYNCbKqrurFo9onSaT7zGT1i3u3j7LKhVZF5sJA39WPN/86'/1'/0'/0/*)#40hv8z77"
>> help
bdk-cli 0.5.0

USAGE:
    bdk-cli <SUBCOMMAND>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

SUBCOMMANDS:
    exit      Exit REPL loop
    help      Prints this message or the help of the given subcommand(s)
    key       Execute Key Commands
    node      Execute Node Commands
    wallet    Execute wallet Commands

>> 

And for each subcommand you can ask for help.

>> node help
node 0.5.0
Execute Node Commands

USAGE:
    node <SUBCOMMAND>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

SUBCOMMANDS:
    generate         Generate given number of blocks and fund the internal wallet with coinbases
    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
>> key help
key 0.5.0
Execute Key Commands

USAGE:
    key <SUBCOMMAND>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

SUBCOMMANDS:
    derive      Derive a child key pair from a master extended key and a derivation path string (eg. "m/84'/1'/0'/0"
                or "m/84h/1h/0h/0")
    generate    Generates new random seed mnemonic phrase and corresponding master extended key
    help        Prints this message or the help of the given subcommand(s)
    restore     Restore a master extended key from seed backup mnemonic words

e6e602c - Adds a generic bitcoin-cli rpc commands into our node subcommand.. So the below now works too.. But they won't be listed in the command list, because bdk-cli doesn't know about them.

>> node listwallets
[
  "default"
]
>> node getbalances
{
  "mine": {
    "immature": 150.0,
    "trusted": 0.0,
    "untrusted_pending": 0.0
  }
}
>> 

We don't have these in our command list but we can use generic Client.call() to get them executed. But we don't have it in our help messages. Thats confusing..

Things to figure out

  • How to convey this to bdk-cli users?
  • How to propagate bitcoin-cli command help to bdk-cli.
  • Is having all the bitcoin-cli` commands available through bdk-cli useful?

@notmandatory
Copy link
Member

I added a comment on #726.

src/commands.rs Show resolved Hide resolved
src/commands.rs Outdated Show resolved Hide resolved
@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Sep 2, 2022

@notmandatory added a v0.22 update patch here 716efe8 pointing to the bdk-reserve open PR bitcoindevkit/bdk-reserves#11..

But for some reason the git link is not working in CI for bdk-reserve. It seems to work fine in my local..

Did the v0.22.0 update here instead of a new PR, because we have many modification over master here, and wanted to cover them.

Not Much changes in the code due to update. But we might need to change the RpcOpts as many new options are added in RpcSyncParam in bdk v0.22.. skip_block is not used in RPC anymore, so we might remove that from cli options.. But all these can be done later in separate PR.

@rajarshimaitra rajarshimaitra force-pushed the node-update-2-alternate branch 2 times, most recently from 7e67ca3 to 6cb4acf Compare September 5, 2022 17:41
@notmandatory
Copy link
Member

prelim ACK, @waterst0ne and I did testing on earlier versions in July of this and the main suggestion from that testing was addressed when @rajarshimaitra added sub-menus in the repl. I'd like to go through it again one more time once #118 is merged and this PR re-based on it. Plus we need the next bdk-reserves release including bitcoindevkit/bdk-reserves#11. Long story short, I think this PR is only missing dependency updates.

 - Update dependency
 - Disable `regtest-esplora-*` backends for further testing
- Add a new NodeCommand to handle backend node operation.
- Fix the REPL mode to show commands via category
- Other auxiliary fixes.
 - Update the handlers to handle node commands
 - Main `handle_command()` function signature changed to only take
   in a `CliOpts`. This is helpful for wasm or any other binding
   produced on bdk-cli.
Add the backend Node mechanism.
Various fixes and moves in utility module
@rajarshimaitra rajarshimaitra force-pushed the node-update-2-alternate branch from 6cb4acf to 322f6f1 Compare September 9, 2022 15:46
@rajarshimaitra
Copy link
Contributor Author

Rebased on master and consolidated commit history..

@notmandatory notmandatory force-pushed the node-update-2-alternate branch from 33185d7 to fa0454e Compare September 10, 2022 03:02
@notmandatory
Copy link
Member

notmandatory commented Sep 10, 2022

Everything looks good with the electrum backend and bdk client, I did a bunch of testing with repl mode too. I pushed a small commit so that in repl mode the backend node is only created once, otherwise it's too slow to use. I also found that with rpc mode the bdk wallet is unable to sync, though it looks like it works in the integration test in CI. I'm using a mac and the error I'm getting is below for cli or local integration testing. I think this is likely related to this issue in the ord project. I'll make an issue for what I found with the mac, I still want to confirm it's working on a linux system in cli and repl mode, if so then I'm ready to put this PR in.

RUST_LOG=info cargo run --features rpc -- --network regtest wallet --descriptor "wpkh(tprv8ZgxMBicQKsPdusu4AAEfp6mxEfdoLHjvPnNwKVSr6A4XdqWE8vTEcxuJE2AsdFuscZDuSLfU8PEwC5aBJBTS4wMZhnC4Esy5bodQAGFRQ5/*)" --cookie "/Users/steve/.bdk-bitcoin/bitcoind/regtest/.cookie" --node "127.0.0.1:59848" -s 10000000000 sync
    Finished dev [unoptimized + debuginfo] target(s) in 0.07s
     Running `target/debug/bdk-cli --network regtest wallet --descriptor 'wpkh(tprv8ZgxMBicQKsPdusu4AAEfp6mxEfdoLHjvPnNwKVSr6A4XdqWE8vTEcxuJE2AsdFuscZDuSLfU8PEwC5aBJBTS4wMZhnC4Esy5bodQAGFRQ5/*)' --cookie /Users/steve/.bdk-bitcoin/bitcoind/regtest/.cookie --node '127.0.0.1:59848' -s 10000000000 sync`
[2022-09-10T02:18:23Z INFO  bdk::database::sqlite] db up to date, no migration needed
[2022-09-10T02:18:23Z INFO  bdk::blockchain::rpc] connected to 'http://127.0.0.1:59848/wallet/l32feux5' with auth: Cookie { file: "/Users/steve/.bdk-bitcoin/bitcoind/regtest/.cookie" }
[2022-09-10T02:18:23Z INFO  bdk::blockchain::rpc] wallet already loaded: l32feux5
[2022-09-10T02:18:23Z INFO  bdk::blockchain::rpc] initial db state: txs=0 utxos=0
[2022-09-10T02:18:38Z ERROR bdk_cli] Rpc(JsonRpc(Transport(SocketError(Os { code: 35, kind: WouldBlock, message: "Resource temporarily unavailable" }))))
cargo test --features regtest-bitcoin 
...
     Running tests/integration.rs (target/debug/deps/integration-7d63cc92aed675c4)

running 1 test
test test::test_basic_wallet_op_bitcoind has been running for over 60 seconds
test test::test_basic_wallet_op_bitcoind ... FAILED

failures:

---- test::test_basic_wallet_op_bitcoind stdout ----
BDK-CLI Config : BdkCli {
    target: "./target/debug/bdk-cli",
    network: "regtest",
    verbosity: false,
    recv_desc: None,
    chang_desc: None,
    node_datadir: Some(
        "/var/folders/rc/blk8txzx1zd71ft30rtl_zww0000gn/T/.tmp9MOXbS",
    ),
}
thread 'test::test_basic_wallet_op_bitcoind' panicked at 'called `Result::unwrap()` on an `Err` value: CmdExec("[2022-09-10T03:19:40Z ERROR bdk_cli] Rpc(JsonRpc(Transport(SocketError(Os { code: 35, kind: WouldBlock, message: \"Resource temporarily unavailable\" }))))\n")', tests/integration.rs:224:40
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    test::test_basic_wallet_op_bitcoind

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 72.24s

error: test failed, to rerun pass '--test integration'

@rajarshimaitra
Copy link
Contributor Author

Thanks @notmandatory for the report. Unfortunately I am not being able to create this in local. But the error message looks very familiar as Evan reported in this issue. bitcoindevkit/bdk#650 (comment)

It probably means there's an bitcoind instance already running that is accessing the node data directory, thus the Resource temporarily unavailable error.. Can you check that all the bitcoind processes are being cleared after each run?

Also from the command

RUST_LOG=info cargo run --features rpc -- --network regtest wallet --descriptor "wpkh(tprv8ZgxMBicQKsPdusu4AAEfp6mxEfdoLHjvPnNwKVSr6A4XdqWE8vTEcxuJE2AsdFuscZDuSLfU8PEwC5aBJBTS4wMZhnC4Esy5bodQAGFRQ5/*)" --cookie "/Users/steve/.bdk-bitcoin/bitcoind/regtest/.cookie" --node "127.0.0.1:59848" -s 10000000000 sync

It seems you are running regular rpc mode.. Not the regtest-bitcoin mode.. A command like this working both normal as well as repl mode for me

cargo run --features regtest-bitcoin -- --network regtest wallet --descriptor "wpkh(tprv8ZgxMBicQKsPdusu4AAEfp6mxEfdoLHjvPnNwKVSr6A4XdqWE8vTEcxuJE2AsdFuscZDuSLfU8PEwC5aBJBTS4wMZhnC4Esy5bodQAGFRQ5/*)" sync

I checked your changes too.. ACK on taking the backend creation to global.. Though new_backend() probably doesn't need feature gate, as it can run for all features, and for non regtes-* feature it just won't do anything.. But this can be done later..

I tested the work flow again in linux machine and all seems to be working okay..

@notmandatory
Copy link
Member

I tested the work flow again in linux machine and all seems to be working okay..

Thanks for retesting on Linux, the problem I'm seeing must just be on MacOS. And for ACKing my global repl new_backend() change, I had to put the feature gate so CI clippy doesn't give a warning when no backend feature is enabled.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

tACK fa0454e

@notmandatory notmandatory merged commit 7869bdb into bitcoindevkit:master Sep 10, 2022
rajarshimaitra added a commit that referenced this pull request Sep 20, 2022
f8a5999 Minor grammar and puctuation fixes (Steve Myers)
e7b6854 Update with Readme fixes (rajarshimaitra)
52e8c61 Add all `possible_values` to network command option (Leonardo Lima)
179618c Update crate documentation (rajarshimaitra)

Pull request description:

  ### Description

  After #99 the previous documentation have been removed and new docs as per `structopts` documentation. This PR adds more documentation across the crate..

  This PR is above #102 , to accommodate all the further refactoring changes.

  The Readme About section have been updated with more details.. Readme format made aligned with the BDK project itself..

  The Readme file is used itself as the crate level documentation in docs.rs too..

  ### 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 f8a5999

Tree-SHA512: 26c5b3903b0215aa9841c4d1079bbdeb9f9d9d458c7e27dddb625db24eb364b73ca978bb2018f486215878f3601b5572ca58d5c202cb74325c992f3e7107d850
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
4 participants