bitcoindevkit / bdk-cli

A CLI wallet library and REPL tool to demo and test the BDK library
Other
108 stars 64 forks source link

Satoshi's Calculator. #102

Closed rajarshimaitra closed 2 years ago

rajarshimaitra commented 2 years ago

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

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:

New Features:

rajarshimaitra commented 2 years ago

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

rajarshimaitra commented 2 years ago

@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 commented 2 years ago

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

waterst0ne commented 2 years ago

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:



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

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

rajarshimaitra commented 2 years ago

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

https://github.com/bitcoindevkit/bdk-cli/pull/102/commits/e728577bbf79cbc277abfee9f6e37264d9330408 - Instead of changing the command name, I elaborated the help doc. The command not only generate blocks but also funds the internal wallet.

https://github.com/bitcoindevkit/bdk-cli/pull/102/commits/c1921a7093c06b139ffb7e5eaaa3815885a903dc - 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

https://github.com/bitcoindevkit/bdk-cli/pull/102/commits/e6e602c0d85a5dca96c1ea7fc805a0c1f3c76a8b - 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

rajarshimaitra commented 2 years ago

@notmandatory Should I also add #104 into this PR and get it all merged together??

notmandatory commented 2 years ago

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

Looks like #104 is touching alot of files and already has review comments so let's keep it separate.

Also I love the sub-command change you did for the repl, it makes the subcommands alot more organized and easier to understand what they're for.

rajarshimaitra commented 2 years ago

Yes makes sense.. Let me know if you find any more issue trying it out that we can push into this PR.. To me it seems now ready to merge.. There are few small updates I have in mind, but we can do them later via Issues and PRs..

But getting this and #104 in would complete the major refactorings..

I would rebase #104 again once this gets merged..

rajarshimaitra commented 2 years ago

Also now this Fixes #110

rajarshimaitra commented 2 years ago

Updated with bdk v0.20.0 upgrade..

rajarshimaitra commented 2 years ago

@waterst0ne reported a problem that I wanna document here..

bdk-cli doesn't seem to be working with this descriptor wpkh(tprv8ZgxMBicQKsPd5F8dLQA1Wn5a9gcu5Dqt1RKSU1Arw3dTc7qmZPY7wzTbdiygXFE9yFJeUekPmxGXn35fQmmkSNebyXb3cJ86sbe32ACgNz/84'/0'/0'/*).

I don't know how this was generated but this seems like a very valid descriptor like any other.. Here's the full parse of it by bdk..

wpkh(XPub(DescriptorXKey { origin: Some((b62f919d, m/84'/0'/0')), xkey: ExtendedPubKey { network: Testnet, depth: 3, parent_fingerprint: a066e93a, child_number: Hardened { index: 0 }, public_key: PublicKey(c2b157c5a54d046359f17e1c0b2f1a4b1bcc86e6ec6cf95a425585c0364a33c646bcbc849e81fabb4dbd9c8beb1ac07a8109e09559504186b38285697879357a), chain_code: 8329f5f6b2ccf23975f2b28a0802778ce27147a8b071cb68c44bfeb50edcb57a }, derivation_path: m, wildcard: Unhardened }))

Where as when I am creating a fresh descriptor from bdk-cli itself, its working all fine.. Heres an example descriptor generated by bdk-cli "wpkh([cad5ef5e/84'/1'/0'/0]tprv8i78DGMvrqUS15nRQbQd3JtBCL4SsmGMMczTBQhgPgbEGjtnP5boKNidLv3HjkAzdr4FHjH4NVto9ngv2NGVGmSXBqnkiS5d4FSnpi3LWyz/*)"

And here's the parse of it

wpkh(XPub(DescriptorXKey { origin: Some((b62f919d, m/84'/1'/0')), xkey: ExtendedPubKey { network: Testnet, depth: 3, parent_fingerprint: 6b9efee5, child_number: Hardened { index: 0 }, public_key: PublicKey(ffefcdf5298f982aa396bae04b4cbffd13c0c8c85bac0d93cc3f129dcfed97f2e1a353f99329ac610057572d55e14cea765345e19f3e8915371da1efa2cdb617), chain_code: f831fa15022f13eb6fd92b025e54ceb0a4202bf75e3463ac4462c4bee15b764e }, derivation_path: m/0, wildcard: Unhardened }))

I am still not sure whats going on. Things that I have checked for the problematic descriptor

Is there any such thing as "rouge descriptors" that behaves in weird ways??

waterst0ne commented 2 years ago

@waterst0ne reported a problem that I wanna document here..

bdk-cli doesn't seem to be working with this descriptor wpkh(tprv8ZgxMBicQKsPd5F8dLQA1Wn5a9gcu5Dqt1RKSU1Arw3dTc7qmZPY7wzTbdiygXFE9yFJeUekPmxGXn35fQmmkSNebyXb3cJ86sbe32ACgNz/84'/0'/0'/*).

I don't know how this was generated but this seems like a very valid descriptor like any other.. Here's the full parse of it by bdk..

wpkh(XPub(DescriptorXKey { origin: Some((b62f919d, m/84'/0'/0')), xkey: ExtendedPubKey { network: Testnet, depth: 3, parent_fingerprint: a066e93a, child_number: Hardened { index: 0 }, public_key: PublicKey(c2b157c5a54d046359f17e1c0b2f1a4b1bcc86e6ec6cf95a425585c0364a33c646bcbc849e81fabb4dbd9c8beb1ac07a8109e09559504186b38285697879357a), chain_code: 8329f5f6b2ccf23975f2b28a0802778ce27147a8b071cb68c44bfeb50edcb57a }, derivation_path: m, wildcard: Unhardened }))

Where as when I am creating a fresh descriptor from bdk-cli itself, its working all fine.. Heres an example descriptor generated by bdk-cli "wpkh([cad5ef5e/84'/1'/0'/0]tprv8i78DGMvrqUS15nRQbQd3JtBCL4SsmGMMczTBQhgPgbEGjtnP5boKNidLv3HjkAzdr4FHjH4NVto9ngv2NGVGmSXBqnkiS5d4FSnpi3LWyz/*)"

And here's the parse of it

wpkh(XPub(DescriptorXKey { origin: Some((b62f919d, m/84'/1'/0')), xkey: ExtendedPubKey { network: Testnet, depth: 3, parent_fingerprint: 6b9efee5, child_number: Hardened { index: 0 }, public_key: PublicKey(ffefcdf5298f982aa396bae04b4cbffd13c0c8c85bac0d93cc3f129dcfed97f2e1a353f99329ac610057572d55e14cea765345e19f3e8915371da1efa2cdb617), chain_code: f831fa15022f13eb6fd92b025e54ceb0a4202bf75e3463ac4462c4bee15b764e }, derivation_path: m/0, wildcard: Unhardened }))

I am still not sure whats going on. Things that I have checked for the problematic descriptor

* The bdk database doesn't update anything after the sync.

* The corresponding bitcoin core wallet for that descriptor doesn't get any balance either.

Is there any such thing as "rouge descriptors" that behaves in weird ways??

My balance did not update once again on the bdk-cli wallet side when I used sendtoaddress command. Syncing issues. I deleted the .bdk-bitcoin folder, sent the funds againn to the bdk-cli wallet, generated a few more blocks using the node and still no balance. I'm not sure why I have this sync issue on regtest. I will continue to communicate with Raj to see the reason for this case.

waterst0ne commented 2 years ago

@waterst0ne reported a problem that I wanna document here.. bdk-cli doesn't seem to be working with this descriptor wpkh(tprv8ZgxMBicQKsPd5F8dLQA1Wn5a9gcu5Dqt1RKSU1Arw3dTc7qmZPY7wzTbdiygXFE9yFJeUekPmxGXn35fQmmkSNebyXb3cJ86sbe32ACgNz/84'/0'/0'/*). I don't know how this was generated but this seems like a very valid descriptor like any other.. Here's the full parse of it by bdk..

wpkh(XPub(DescriptorXKey { origin: Some((b62f919d, m/84'/0'/0')), xkey: ExtendedPubKey { network: Testnet, depth: 3, parent_fingerprint: a066e93a, child_number: Hardened { index: 0 }, public_key: PublicKey(c2b157c5a54d046359f17e1c0b2f1a4b1bcc86e6ec6cf95a425585c0364a33c646bcbc849e81fabb4dbd9c8beb1ac07a8109e09559504186b38285697879357a), chain_code: 8329f5f6b2ccf23975f2b28a0802778ce27147a8b071cb68c44bfeb50edcb57a }, derivation_path: m, wildcard: Unhardened }))

Where as when I am creating a fresh descriptor from bdk-cli itself, its working all fine.. Heres an example descriptor generated by bdk-cli "wpkh([cad5ef5e/84'/1'/0'/0]tprv8i78DGMvrqUS15nRQbQd3JtBCL4SsmGMMczTBQhgPgbEGjtnP5boKNidLv3HjkAzdr4FHjH4NVto9ngv2NGVGmSXBqnkiS5d4FSnpi3LWyz/*)" And here's the parse of it

wpkh(XPub(DescriptorXKey { origin: Some((b62f919d, m/84'/1'/0')), xkey: ExtendedPubKey { network: Testnet, depth: 3, parent_fingerprint: 6b9efee5, child_number: Hardened { index: 0 }, public_key: PublicKey(ffefcdf5298f982aa396bae04b4cbffd13c0c8c85bac0d93cc3f129dcfed97f2e1a353f99329ac610057572d55e14cea765345e19f3e8915371da1efa2cdb617), chain_code: f831fa15022f13eb6fd92b025e54ceb0a4202bf75e3463ac4462c4bee15b764e }, derivation_path: m/0, wildcard: Unhardened }))

I am still not sure whats going on. Things that I have checked for the problematic descriptor

* The bdk database doesn't update anything after the sync.

* The corresponding bitcoin core wallet for that descriptor doesn't get any balance either.

Is there any such thing as "rouge descriptors" that behaves in weird ways??

My balance did not update once again on the bdk-cli wallet side when I used sendtoaddress command. Syncing issues. I deleted the .bdk-bitcoin folder, sent the funds againn to the bdk-cli wallet, generated a few more blocks using the node and still no balance. I'm not sure why I have this sync issue on regtest. I will continue to communicate with Raj to see the reason for this case.

Turns out that when I send funds to the bdk-cli wallet it doesn't sync and balance doesn't update wether im on repl or not. Same results, not sure why. It was working before.

rajarshimaitra commented 2 years ago

Thanks to @waterst0ne we figured out this is a problem with current bdk v0.20.. I think this needs to wait for the next bdk release where the rpc problem is fixed..

notmandatory commented 2 years ago

Since bdk 0.21 is out now can this PR be fixed? I think the PR that was needed is https://github.com/bitcoindevkit/bdk/pull/672.

rajarshimaitra commented 2 years ago

The balance problem is fixed in bdk v0.21, but we now need https://github.com/bitcoindevkit/bdk/pull/726 into bdk, as bdk-reserves fails for bdk v0.21, and that's the bug fix.. In the last dev call I think we talked about creating a patch release of bdk v0.21.1 and use that here..

Or else we can let the bdk versions diverge in bdk-cli and bdk-reserves, till we have bdk v0.22..

bdk-cli works okay with bdk v0.21, bdk-reserves works ok with v0.20,

notmandatory commented 2 years ago

I added a comment on #726.

rajarshimaitra commented 2 years ago

@notmandatory added a v0.22 update patch here https://github.com/bitcoindevkit/bdk-cli/pull/102/commits/716efe8681be22b55a386516c979cf5b5e6cbe5a pointing to the bdk-reserve open PR https://github.com/bitcoindevkit/bdk-reserves/pull/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.

notmandatory commented 2 years ago

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 https://github.com/bitcoindevkit/bdk-reserves/pull/11. Long story short, I think this PR is only missing dependency updates.

rajarshimaitra commented 2 years ago

Rebased on master and consolidated commit history..

notmandatory commented 2 years ago

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 commented 2 years ago

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. https://github.com/bitcoindevkit/bdk/issues/650#issuecomment-1182990519

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 commented 2 years ago

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.