Skip to content

upgrade(swap): Concurrent syncing, bdk upgrade, refactors #180

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

Merged
merged 128 commits into from
May 18, 2025

Conversation

Einliterflasche
Copy link

@Einliterflasche Einliterflasche commented Nov 16, 2024

This closes #98.

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

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

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.

…ull scan, transmit assumed_total for full scan to tauri, add some icons to progress displays
@binarybaron
Copy link

Another issue is that if the user interrupt the initial full scan, we will not try again...

Fixed

@binarybaron
Copy link

Integration tests are failing. I've manually done a successful swap.

Some issue with the Electrum server I believe...

@binarybaron
Copy link

I tried doing a mainnet swap with this as a client and an old asb as a maker. swap_setup fails.

@binarybaron
Copy link

Alice cannot find the tx_lock in the integration tests:

/// This is from Alice
2025-05-17T18:37:09.622110Z  INFO swap{id=fb9823c5-9a30-4e74-932d-5e4ab9d6f82e}:BitcoinWalletSubscription: swap::bitcoin::wallet: Status of script txid=ed98b02b1f4d2fd57a42bd4df853d9a50e190afe2acf0b94ed1c017c3a5afd4b script=OP_0 OP_PUSHBYTES_32 e718844adac2dccd68596046a2510d8daeb00a4fe6f61c0c01d63d8b0cc949f6 new_status=unseen uuid=7ddca8e4-99e6-486a-810f-3ad9d0c263f6
/// This is from Bob
2025-05-17T18:37:09.641311Z  INFO BitcoinWalletSubscription: swap::bitcoin::wallet: Status of script txid=ed98b02b1f4d2fd57a42bd4df853d9a50e190afe2acf0b94ed1c017c3a5afd4b script=OP_0 OP_PUSHBYTES_32 e718844adac2dccd68596046a2510d8daeb00a4fe6f61c0c01d63d8b0cc949f6 new_status=in mempool uuid=b2916251-d462-4122-963e-ceff66b9d48c

@binarybaron
Copy link

Tests are passing!!

@binarybaron
Copy link

binarybaron commented May 17, 2025

TODO

  • Wallet syncing sometimes fails with Made one or multiple attempts ... unexpected end of file (os error 32). This happens almost instantly after pressing refresh and is fixed by trying again. This makes me think this might be an issue with a lock.
  • Refactor src-gui/src/renderer/components/alert/DaemonStatusAlert.tsx
  • Make sure we do not spam the logging
  • Patch bdk_chain to use rusqlite 0.32, then upgrade arti-client to 0.30
  • Figure out if this is a breaking Network change. Can the new version do swaps with the old version?

TODO (optional)

  • Refactor Electrum server checking to be done in Rust purely
  • Enforce Electrum servers returning valid fee estimation before choosing them (no -1 sats/kbyte)
  • Route electrum-client calls through internal Tor client. We can construct a RawClient<_> that uses our Data Stream from Tor under the hood. However the electrum-client is likely not permissive in it's exposed public API to allow this without patching electrum-client. Alternatively we can use arti to spawn our own socks5 proxy using run_socks_proxy and then configure the client to use that proxy. electrum-client has native support for socks5 proxies. We can also use bdk_electrum_streaming / electrum_streaming_client. It's still experimental but is fully transport agnostic.
  • Electrum server load balancing
  • Use the bdk electrum testing harness instead of our own
  • Unit tests for new Tauri connector
  • Write changelog entry

@binarybaron binarybaron changed the title upgrade(swap): Upgrade bdk library upgrade(swap): Concurrent syncing, bdk upgrade May 17, 2025
@binarybaron binarybaron changed the title upgrade(swap): Concurrent syncing, bdk upgrade upgrade(swap): Concurrent syncing, bdk upgrade, refactors May 17, 2025
@binarybaron
Copy link

This is fine to merge. It's been fairly thoroughly tested and if there are small hickups, all of the issues coming from that will be at worst bad UX. We cannot let this PR take any more time.

@binarybaron binarybaron marked this pull request as ready for review May 18, 2025 20:53
@binarybaron binarybaron merged commit 3f4cbdd into master May 18, 2025
29 checks passed
binarybaron added a commit that referenced this pull request May 21, 2025
* upgrade sqlx to 0.8, add bdk_wallet and bdk_electrum

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.

* move old wallet code to extra module

* fix fee estimation for old client

* bump bitcoin crate, add new wallet constructor

* remove unused old Client, move code around for better readibility

* make Wallet generic over Persister (database) and move more code around for readibility

* add script history, start reimplementing client methods

* update some imports

* cargo fmt

* Add comments, fix fee estimation, address generation and status_of_script

* redo state update and wallet sync

* fix bitcoin address validation and more imports, use Amount everywhere

* fix tx cancel, lock, punish, redeem, refund

* fix bitcoin::Address de-/serialisation

* fix more address validation

* fix more address parsing and validation, also some more imports

* cargo fmt

* fix wallet initialization, start wallet migration

* fail test instead of ignoring it

* perform full scan on creation, load from db if it exists

* add more wallet info, fix wallet initialization

* fix: default to null in config

* migrate from old wallet if needed

* change something

* fix some tests

* temporarily patch bdk_wallet and bdk_electrum

* fix more tests

* fix missing rustls

* asb: only start tor client if register_hidden_service=true in the config

* fix: use p2wsh_signature_hash instead of p2wpkh_signature_hash

* fix some bitcoin address parsing and fee rate parsing

* dprint fmt

* add bitcoin-harness to this project and update to the new bitcoin version

* fix max_givible again

* create electrum client separately from wallet, clean up some code

* add comment

* ignore .env.development

* log config file path on ./asb config

* feat(monero-sys): Initial commit. Regtest integration test. Wrapper around basic Wallet functions, depends on monero#9464

* Revert "feat(monero-sys): Initial commit. Regtest integration test. Wrapper around basic Wallet functions, depends on monero#9464"

This reverts commit 14a5b4c.

* upgrade to rust toolchain 1.81

* Use new bdk update for code from master

* fix

* remove

* fix: add empty .gitmodules file to fix Docker build

* fix: clean up submodule references

* fix: properly declare monero submodule with ignore flag

* fix(wallet, bdk): only reveal new address if absolutely necessary

* fix: private keys not loaded into bdk wallet

* refactor: sync wallet progress log

* dprint fmt

* refactor: move bitcoin-harness to outside repo

* refactor: remove redundant log message

* Display sync progress

* Remove redundant arg to  swap/tests/harness/mod.rs function

* fix: call rustls::crypto::ring::default_provider()

* dprint fmt

* refactor: remove debug code

* refactor: move old bdk wallet export to own function, clear log messages

* remove old migr for testnets (checksum mismatch), remove balance and stringified last revealed addresses from migration export

* use revalidate_network function, remove redundant drop

* Display progress of background tasks, TauriBackgroundProgressHandle struct

* fix: almost satisfy clippy

* fix: gen-bindings error

* feat: add BackgroundRefund background type

* feat: use builder pattern for constructing Bitcoin wallet

* dprint ftm

* sync electrum in seperate thread

* do not allow user to start sync while sync is in progress

* remove redundant log message

* display random buffer in AlertWithLinearProgress progress

* fix: use TauriContextStatusEvent.Available), dont show syncing wallet spinner if not syncing

* differentiate between TestWalletBuilder and WalletBuilder

* satisfy clippy

* remove custom BackgroundRefund event, move into background process architecture

* refactor

* dprint fmt

* progress: get unit tests compiling

* fix: bitcoin unit tests specify const values like sync_interval

* fix: get unit tests passing

* make clippy happy

* feat: display full sync progress, fix unit test import issues

* dprint fmt

* make clippy happy, use u32 for target_block and not usize

* always spawn tor for asb

* refactor: remove gen_background_progress_id and just use Uuid::new_v4()

* refactor(hooks.ts): clarify comment on useConservativeBitcoinSyncProgress

* fix typo

* refactor: do not let WalletBuilder take entire env struct

* dprint fmt

* refactor: remove default feature from workspace patch of bdk

* first try for concurrent syncing

* refactor: concurrent syncing

* fix(wallet.rs): Safely convert FeeRate from btc / kb to sats / kwu

* feat(wallet.rs): persist published Bitcoin transactions without requiring re-scan

This allows us to compute an updated Bitcoin balance without requiring a re-scan

* refactor(wallet.rs): use just 5 concurrent sync requests

* refactor: display snackbar error when Wallet refresh fails

* fix: add missing space

* dprint fmt

* refactor: fancy traits for the CumulativeProgress struct, allow limiting amount of callback calls

* make clippy happy

* dprint fmt

* refactor: clearly differntiate between SyncMutex and TokioMutex, use traits for converting to Arc<Mutex<_>>, move sync_ext into own moid

* fix: skip syncing if no spks in wallet

* fix: update bdk.sh to test migration from old wallet (pre 1.0.0 bdk) to new bdk

* fix: increase bitcoin_lock_confirmed_timeout in RegTest env to 5 minutes

* refactor: avoid usize where possible, create persistence only after full scan, transmit assumed_total for full scan to tauri, add some icons to progress displays

* make clippy happy

* fix(ci): change rust toolchain 1.81

* fix(cross compilation arm): use ring instead of aws-lc-rs

* fmt

* ignore failing rendezvous tests

* fix printing_status_change_doesnt_spam_on_same_status

* fix: given_bitcoin_address_network_mismatch_then_error test

* ignore list_sellers_should_report_all_registered_asbs_with_a_quote test

* feat: add tor icon

* refactor(wallet.rs): reorder struct by abstraction level

* refactor(bitcoin wallet): chunk size for syncing

* fix(integration tests): decrease sync interval to 3s

* fix(integration tests): parse_rpc_err method to take new bdk error, not old one

* add changelog entry

---------

Co-authored-by: Binarybaron <[email protected]>
Co-authored-by: Mohan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Priority 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade bdk library
2 participants