Skip to content

Conversation

@tvpeter
Copy link
Collaborator

@tvpeter tvpeter commented Feb 3, 2025

Description

This PR update bdk-cli to use bdk_wallet v1

Issue: #172 (comment)

Notes to the reviewers

  • replaced list_transactions with transactions
  • set enable_rbf to be true by default
  • update creating and serializing Psbts
  • update new_wallet to return PersistedWallet<Connection>
  • default fee_rate to use FeeRate::BROADCAST_MIN
  • update zeroize to version 1.8.1 from <1.4.0
  • update bdk-reserves from 0.27.1 to 0.29.0
  • replace bdk/electrum with bdk_electrum for the electrum feature
  • add bdk_esplora for the bdk/use-esplora-ureq and bdk/use-esplora-reqwest features
  • add bdk_bitcoind_rpc lib to replace bdk/rpc feature
  • add hwi library to replace bdk/hardware-signer feature

Changelog notice

  • set enable_rbf to be true by default
  • update using PartiallySignedTransaction to Psbt
  • default fee_rate to use FeeRate::BROADCAST_MIN
  • update zeroize to version 1.8.1 from <1.4.0
  • update bdk-reserves from 0.27.1 to 0.29.0
  • replace bdk/electrum with bdk_electrum for the electrum feature
  • replace bdk/use-esplora-ureq and bdk/use-esplora-reqwest with bdk_esplora lib
  • replace bdk/rpc with bdk_bitcoind_rpc lib
  • replace bdk/hardware-signer with hwi lib

Checklists

All Submissions:

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

Bugfixes:

  • This pull request breaks the existing API
  • I'm linking the issue being fixed by this PR

add bdk_wallet to Cargo.toml

* the bdk crate is still in using because of the
error enum, which will be removed after feedback
[Ticket: X]
-update imports from bdk to use bdk_wallet
- set `enable_rbf` command option to be `true` by
default
- add `parse_address` fn to parse address from
string
- replace `ExtendedPrivateKey` with `Xpriv` from
bdk_wallet
- update derving address from string to use
`assumed_check`

[Ticket: X]
- replace `list_transactions` method  with
`transactions`
- format `recipients` to be a vec of `ScriptBuf`
and `Amount`
- update creating and serializing `Psbt`s
- update `fee_rate` method and default to
`FeeRate::BROADCAST_MIN`
- replace `allow_shrinking` method with `drain_to`
method
- update `handle_offline_wallet_subcommands` to
use `PersistedWallet<D>` instead of `Wallet<D>`
[Ticket: X]
- update imports from bdk to bdk_wallet
- fix compilation warnings

[Ticket: X]
- add `parse_address` method
- remove `key-value-db` feature as it uses `sled`
- update `open_database` fn to return
rusqlite::Connection
- update `new_wallet` fn to return
`PersistedWallet<Connection>`

[Ticket: X]
@notmandatory notmandatory added the enhancement New feature or request label Feb 3, 2025
- Replace `bdk` with `bdk_wallet` v1 in Cargo.toml
- replace removed features in bdk features/mods
with crates

[Ticket: X]
-add error enum for handling errors

[Ticket: X]
@tvpeter tvpeter force-pushed the feat/bdk-wallet-v1-update branch from 6653428 to c7bd335 Compare February 6, 2025 12:45
- replace bdk::error with custom error enum
- update imports and handling of errors

[Ticket: X]
@tvpeter tvpeter force-pushed the feat/bdk-wallet-v1-update branch from c7bd335 to 50233dd Compare February 6, 2025 14:04
- update deps imports
- refactor reserves feature

[Ticket: X]
- replace `bitcoincore_rpc` client with
`electrsd::Node` client
- update rpc commands
- update `electrsd` to v0.31.0

[Ticket: X]
@tvpeter tvpeter force-pushed the feat/bdk-wallet-v1-update branch from 4bf989c to 58f07a1 Compare February 12, 2025 00:17
-update error enum
- update creating a new_backend

[Ticket: X]
@notmandatory
Copy link
Member

notmandatory commented Feb 18, 2025

I've been doing some cleanup on your branch, sorry it's squashed all into one commit, see here: https://github.com/notmandatory/bdk-cli/tree/feat/bdk-wallet-v1-update

It works for electrum, esplora, sqlite, and repl mode, but not completely tested. I remove rpc and cbf for now, and cleaned up the features so they can all be enabled together, the recommended way for rust. Do you mind if I push my branch into your PR so you can continue working on it to add rpc and cbf back in. It will overwrite your last commit.

@notmandatory
Copy link
Member

I also removed the hwi support and proof of reserves. Both of these still need updating for 1.0 so we can leave them out of this PR.

I'd also like to re-think what sort of testing we need, the old tests I commented out. And re-think what we want to do for the github CI.

@tvpeter
Copy link
Collaborator Author

tvpeter commented Feb 18, 2025

Do you mind if I push my branch into your PR so you can continue working on it to add rpc and cbf back in. It will overwrite your last commit.

I don’t mind; you can go ahead and push. Thank you so much.

tvpeter and others added 3 commits February 18, 2025 21:49
-update all online wallet subcommands

[Ticket: X]
- add ci test for features
- add "compiler" feature to bdk_wallet dependency
- remove duplicate fn in utils.rs
- replace "possible_values" with "value_parser"
as it is deprecated in clap 4.5

[Ticket: X]
@tvpeter tvpeter marked this pull request as ready for review February 21, 2025 03:39
- remove unavailable features in code_coverage workflow
- add more features for the cont_integration workflow
- update local compilation test script

[Ticket: X]
src/commands.rs Outdated
Comment on lines 533 to 534
// #[cfg(test)]
// mod test {

Choose a reason for hiding this comment

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

Will all of the tests here ultimately need to be updated in a follow-up PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now, the emphasis is not to test inputs but to test that features are building correctly, which has been included in the ci directory

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

However, if you notice any outputs that seem incorrect or if you want specific features or sections tested, please indicate them here.

- re-enable `rpc` feature
@tvpeter tvpeter force-pushed the feat/bdk-wallet-v1-update branch 3 times, most recently from c163b95 to b20683a Compare March 9, 2025 18:56
-add more error types to error enum
- update the handlers file to replace Generic
error arm

[Ticket: X]
@tvpeter tvpeter force-pushed the feat/bdk-wallet-v1-update branch from b20683a to cb10873 Compare March 12, 2025 13:57
@notmandatory notmandatory force-pushed the feat/bdk-wallet-v1-update branch 3 times, most recently from f001a48 to 62ca1c4 Compare March 12, 2025 16:45
@notmandatory notmandatory force-pushed the feat/bdk-wallet-v1-update branch 2 times, most recently from 897f7e5 to 2828cee Compare March 12, 2025 17:11
@notmandatory notmandatory force-pushed the feat/bdk-wallet-v1-update branch from 2828cee to efc5fc5 Compare March 12, 2025 17:19
@tvpeter tvpeter force-pushed the feat/bdk-wallet-v1-update branch from ff99cbd to 4a66557 Compare March 12, 2025 20:30
Also when doing an rpc sync, re-sync the prior 200 blocks to
handle any possible reorg scenarios.
@notmandatory
Copy link
Member

notmandatory commented Mar 13, 2025

Code looks good but I wasn't able to do a round trip test of receiving and sending back bitcoin to a signet faucet using the new rpc feature. I was able to:

  1. sync against local signet bitcoind
  2. get a new_address
  3. receive signet coins from https://signet25.bublina.eu.org/ to my address
  4. see the unconfirmed balance change while the mempool tx was being RBF'ed by the faucet
  5. see the confirmed balance and verify it with https://mempool.space/signet
  6. create and sign a tx that send sSATs back to the signet faucet

I was not able to broadcast this signed TX however. I don't know why, I get the error "PsbtError: invalid magic". Were you able to successfully receive and send coins back to a signet or testnet wallet?

@tvpeter
Copy link
Collaborator Author

tvpeter commented Mar 13, 2025

Code looks good but I wasn't able to do a round trip test of receiving and sending back bitcoin to a signet faucet using the new rpc feature. I was able to:

  1. sync against local signet bitcoind
  2. get a new_address
  3. receive signet coins from https://signet25.bublina.eu.org/ to my address
  4. see the unconfirmed balance change while the mempool tx was being RBF'ed by the faucet
  5. see the confirmed balance and verify it with https://mempool.space/signet
  6. create and sign a tx that send sSATs back to the signet faucet

I was not able to broadcast this signed TX however. I don't know why, I get the error "PsbtError: invalid magic". Were you able to successfully receive and send coins back to a signet or testnet wallet?

I haven’t tested signing and broadcasting on either signet or testnet yet, as I have primarily been working on regtest. I will test on them now to see the outcome.

- fix the returned data after signing or
finalizing `psbt`

[Ticket: X]
@tvpeter
Copy link
Collaborator Author

tvpeter commented Mar 14, 2025

Code looks good but I wasn't able to do a round trip test of receiving and sending back bitcoin to a signet faucet using the new rpc feature. I was able to:

  1. sync against local signet bitcoind
  2. get a new_address
  3. receive signet coins from https://signet25.bublina.eu.org/ to my address
  4. see the unconfirmed balance change while the mempool tx was being RBF'ed by the faucet
  5. see the confirmed balance and verify it with https://mempool.space/signet
  6. create and sign a tx that send sSATs back to the signet faucet

I was not able to broadcast this signed TX however. I don't know why, I get the error "PsbtError: invalid magic". Were you able to successfully receive and send coins back to a signet or testnet wallet?

The error occurred because the returned PSBT data after signing or finalizing was the original unsigned PSBT instead of the signed PSBT. I have fixed the bug and can now broadcast signed transactions. Apologies for not catching it at my end.

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.

ACK e0bd147

I retested and verified sign and broadcast with rpc is working. Thanks for hanging in there and getting this first big step done!

@notmandatory notmandatory merged commit 2fa18a0 into bitcoindevkit:master Apr 13, 2025
7 checks passed
@notmandatory notmandatory mentioned this pull request May 28, 2025
3 tasks
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

None yet

Development

Successfully merging this pull request may close these issues.

3 participants