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.

Sign up for GitHub

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

Jump to bottom

upgrade(swap): Upgrade bdk library #180

Draft
wants to merge 41 commits into
base: master
Choose a base branch
from
Draft

upgrade(swap): Upgrade bdk library #180

wants to merge 41 commits into from

Conversation

Einliterflasche
Copy link

This closes #98.

@Einliterflasche
upgrade sqlx to 0.8, add bdk_wallet and bdk_electrum
cc760f0 
The new dependencies are part of the bdk upgrade and
include the improved wallet code.
They, too, depend on sqlite3.
However, they use a newer version than we currently use via sqlx.
This necessitated the sqlx upgrade.
This entailed trivial changes (use Pool directly instead of pool.acquire()).
We might have to fix the CI as well, I kept getting compile
errors from the macro until I ran swap/sqlx_dev_setup.sh.
@Einliterflasche Einliterflasche self-assigned this Nov 16, 2024
@Einliterflasche
Copy link
Author

Einliterflasche commented Nov 16, 2024 •
edited
Loading

Oh boy. It looks like we'll have to upgrade bitcoin as well, since [email protected] uses [email protected] but we use [email protected]. I did that and instantly received ~500 compile errors. This upgrade is not at all trivial.

@binarybaron
Copy link

binarybaron commented Nov 16, 2024 •
edited
Loading

Oh boy. It looks like we'll have to upgrade bitcoin as well, since [email protected] uses [email protected] but we use [email protected]. I did that and instantly received ~500 compile errors. This upgrade is not at all trivial.

That's a bummer but I'd assume it's mostly a few API changes that propogate through the codebase. The Bitcoin protocol hasn't changed.

@Einliterflasche
Copy link
Author

Probably. The changelog isn't very clear on that, sadly.

@binarybaron
Copy link

We also have to consider that this upgrade could be a breaking network upgrade if the serde serialization structures have changed.

@binarybaron
Copy link

I haven't looked too much what you are doing here but fyi you do not really need to do any backwards compatability stuff. Our descriptor is derived from our seed.pem which does not change, and we can just re-scan our wallet at first startup.

@Einliterflasche
Copy link
Author

The docs recommended that we get the revelation indices of the wallet to speed up the migration, so that's why I kept some of the old wallet code around

@Einliterflasche
Copy link
Author

I fixed almost all compile errors now. Only left are Wallet::new and Client::new and how their used, some wallet testing code, as well as migrating to the new wallet when needed.

@binarybaron
Copy link

According to a BDK dev we don't even need to do a full scan when first creating the new wallet. Since we now the derivation indices a normal sync is sufficient.

We still need to do a full scan if we are restoring the wallet from our seed but NOT migrating from the older bdk wallet. For example, if one deleted the wallet while keeping the seed.pem.

@Einliterflasche
Copy link
Author

We have a dependency conflict again, I submitted a PR to bdk to hopefully bump their dependency on rusqlite.

@Einliterflasche
Copy link
Author

I linked to my fork of bdk for now, we will have to change that before we can merge this PR. Hopefully the bdk devs accept my patch.

@Einliterflasche
Copy link
Author

I fixed the bitcoin::Amount conversion error but am now experiencing the same connection issue as on master (failing to get quote because we couldn't dial the peer).

@binarybaron
Copy link

I fixed the bitcoin::Amount conversion error but am now experiencing the same connection issue as on master (failing to get quote because we couldn't dial the peer).

Try on mainnet perhaps?

@binarybaron
Copy link

Starting a swap fails with:

2024-12-17T14:52:55.862976Z ERROR swap{swap_id=4dcf597a-54ea-46de-a58d-5d8b5e861c3f}:buy_xmr{buy_xmr=BuyXmrArgs { seller: /onion3/dmdrgmy27szmps3p5zqh4ujd7twoi2a5ao7mouugfg6owyj4ikd2h5yd:9939/p2p/12D3KooWCa6vLE6SFhEBs3EhsC5tCBoHKBLoLEo1riDDmcExr5BW, bitcoin_change_address: None, monero_receive_address: Address { network: Stagenet, addr_type: SubAddress, public_spend: 22f6e4d45c86a8be2810e2fba7484c8d2c8be4f5d91f62cf28fccc36cf5252fa, public_view: ac7434fb2a7bc9672ef30704a8fef4cda5d20153fd41c6cf14a6a322e36cc4d3 } } swap_id=4dcf597a-54ea-46de-a58d-5d8b5e861c3f method="buy_xmr"}: Failed to complete swap: Failed to build transaction. Insufficient funds: 4264449 sat available of 4812359 sat needed swap_id=4dcf597a-54ea-46de-a58d-5d8b5e861c3f

@binarybaron binarybaron requested a review from delta1 December 17, 2024 17:03
@Einliterflasche
Copy link
Author

I fixed that error, but the testnet asb on the server couldn't read a cbor encoded message now, containing a bitcoin::psbt::Psbt. I suppose it really is a breaking change.

@Einliterflasche
Copy link
Author

When running a local asb the dialing doesn't even work...

@binarybaron
Copy link

binarybaron commented Dec 19, 2024 •
edited
Loading

When running a local asb the dialing doesn't even work...

Probably because we are routing over tor which does not allow dialing local addresses.

umgefahren/libp2p-tor#23

@binarybaron
Copy link

I fixed the bitcoin::Amount conversion error but am now experiencing the same connection issue as on master (failing to get quote because we couldn't dial the peer).

Could be. I'm building our public asb with this branch right now... Will be live in a couple of minutes.

@binarybaron
Copy link

Perfect timing: https://github.com/bitcoindevkit/bdk/releases/tag/wallet-1.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@delta1 delta1 Awaiting requested review from delta1

Assignees

@Einliterflasche Einliterflasche

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade bdk library
2 participants
@Einliterflasche @binarybaron