Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

Optimize wallet and/or monitoring #2158

Closed
bonomat opened this issue Jun 2, 2022 · 5 comments · Fixed by #2172 or #2174
Closed

Optimize wallet and/or monitoring #2158

bonomat opened this issue Jun 2, 2022 · 5 comments · Fixed by #2172 or #2174
Assignees

Comments

@bonomat
Copy link
Collaborator

bonomat commented Jun 2, 2022

I'm not sure which part of our code base is causing this, but electrs is printing the following warnings:

[2022-06-02T07:35:12.036Z INFO  electrs::electrum] your wallet uses less efficient method of querying electrs, consider contacting the developer of your wallet. Reason: blockchain.scripthash.get_history called for unsubscribed scripthash: 2b3ce0a1810e2255eb556532a94273496bdeb705323adcfbd30e7fa673d8ba84
[2022-06-02T07:35:12.044Z INFO  electrs::electrum] your wallet uses less efficient method of querying electrs, consider contacting the developer of your wallet. Reason: blockchain.scripthash.get_history called for unsubscribed scripthash: 1787b9cfe70daf31e0cbebbf684d1565ef866e350e6031ee4051e5094ef99275
[2022-06-02T07:35:12.053Z INFO  electrs::electrum] your wallet uses less efficient method of querying electrs, consider contacting the developer of your wallet. Reason: blockchain.scripthash.get_history called for unsubscribed scripthash: 89be2a62b1ae5b1f74396bcbeb131c888222ec74b305169f81a5402fb0b8f9b7
[2022-06-02T07:35:12.061Z INFO  electrs::electrum] your wallet uses less efficient method of querying electrs, consider contacting the developer of your wallet. Reason: blockchain.scripthash.get_history called for unsubscribed scripthash: 1772e81bbfbe92d289a175b4e366a4312e267c003bf381f662716bf693a846ea
[2022-06-02T07:35:12.069Z INFO  electrs::electrum] your wallet uses less efficient method of querying electrs, consider contacting the developer of your wallet. Reason: blockchain.scripthash.get_history called for unsubscribed scripthash: 0b45a072c427744ffb7bb5df0557c3c40eb4d6ef1c8d6f4fb085fb2823195810
...

It's a never ending flood of these logs and electrs needs 100% cpu

@da-kami
Copy link
Contributor

da-kami commented Jun 2, 2022

Likely a cause of warnings of #2083

@bonomat
Copy link
Collaborator Author

bonomat commented Jun 2, 2022

my bet is that it's the wallet and these scripthashes are the 1000 address we are trying to cache.

@bonomat
Copy link
Collaborator Author

bonomat commented Jun 3, 2022

I digged a bit deeper into this:

Conclusion: my understanding is that we cannot easily get around this warning (which is probably harmless) but should reconsider if we really need to sync the wallet 1/m for 1000 addresses.

@bonomat
Copy link
Collaborator Author

bonomat commented Jun 3, 2022

I've created bitcoindevkit/bdk#618, let's see what bkd devs say about this.

@bonomat
Copy link
Collaborator Author

bonomat commented Jun 3, 2022

Let me summarize the issue:

TLDR

I propose to sync less often and offer a manual sync button in the UI so that a user can manually trigger sync to speed up his deposits.
Besides that, I don't think there is much we need to do for now.

The issue

What we want:

  • we want the wallet to pick-up mempool transactions as fast as possible so that a user who deposits funds can open a position even with unconfirmed transactions
  • because of this, we need to sync regularly

What we do:

  • we call ensure_derived_addresses to ensure that we have at least 1000 addresses derived from the beginning
  • this is an overkill for normal users but needed for us, i.e. our testnet maker and taker have several hundred used addresses, meaning, the default sync of only 100 is not enough, hence, we derive 1000 addresses
  • we then sync regularly (i.e. every SYNC_INTERVAL) the wallet

The issue

  • The issue with this is that bdk's electrum implementation is naiiv and always syncs all cached addresses, which means, the longer we run, the more addresses we cache, the more requests we send to electrum. This is subobtimal and needs a long time.

There is no big issue in this besides that we bombard public infrastructure or each user's personal electrum server.
It will only become a real issue if public infrastructure is down or starts banning users (or us)

The solution

  1. We could do nothing and leave it as is. It is not ideal but should not hurt us that much besides that occasionally we don't see balance.
  2. We do a minor thing and sync less often, e.g. every 5 minutes. This means, depositing money could take up to 5 minutes until it's seen. To overcome this, we could add a manual sync button to the UI, which allows the user to trigger sync
  3. We implement a proper sync in bdk's electrum client which syncs only unused addresses.

Additionally, I believe that bdk could be more performant if we use a proper db and not only store it in memory

What I would not do

Run infrastructure for users: with the current design this is likely that a few users would exhaust a small machine and we would need to upgrade it fast. This leads to extensive expenses which are probably not worth the hazzle.

@bonomat bonomat self-assigned this Jun 6, 2022
bors bot added a commit that referenced this issue Jun 7, 2022
2172: Minor wallet improvements r=bonomat a=bonomat

Fixes #2158 

Minor UI change: 

Maker UI: 
<img width="486" alt="image" src="https://user-images.githubusercontent.com/224613/172093037-b1dfc492-9789-4524-a781-afc77c9de069.png">

Taker UI: 

<img width="750" alt="image" src="https://user-images.githubusercontent.com/224613/172093106-79a31577-4d09-4e33-9dbb-e2f3a51d3a08.png">


While syncing: 

<img width="803" alt="image" src="https://user-images.githubusercontent.com/224613/172093127-a58e6f27-44f9-499d-866b-55dcfa2243f6.png">



Co-authored-by: Philipp Hoenisch <[email protected]>
bors bot added a commit that referenced this issue Jun 7, 2022
2172: Minor wallet improvements r=bonomat a=bonomat

Fixes #2158 

Minor UI change: 

Maker UI: 
<img width="486" alt="image" src="https://user-images.githubusercontent.com/224613/172093037-b1dfc492-9789-4524-a781-afc77c9de069.png">

Taker UI: 

<img width="750" alt="image" src="https://user-images.githubusercontent.com/224613/172093106-79a31577-4d09-4e33-9dbb-e2f3a51d3a08.png">


While syncing: 

<img width="803" alt="image" src="https://user-images.githubusercontent.com/224613/172093127-a58e6f27-44f9-499d-866b-55dcfa2243f6.png">



Co-authored-by: Philipp Hoenisch <[email protected]>
@bors bors bot closed this as completed in 51eb120 Jun 7, 2022
bors bot added a commit that referenced this issue Jun 15, 2022
2174: Use file-based db for wallet r=bonomat a=bonomat

This will improve performance because
- we don't have to generate the addresses on every restart, we can just read them from the db
- we don't have to generate 1000 addresses after are restart to miss unspent addresses: if the wallet db grows naturally, i.e. all addresses are cached, we don't have to pre-cache addresses
- we use less memory as we don't have to store the whole db in memory: particularly when having 1000s spent addresses this is a significant amount of data
- sync is faster because we don't have re-fetch the whole raw tx which is stored in the db

Note: if you have used the daemon for some time in the past the first restart can be scary because it will not immediately fetch all addresses. It will take some time until the wallet has caught up because we only sync for 100 addresses at a time.

Fixes #2158 

Note 2: this depends on a bdk fork "https://github.com/coblox/bdk", branch = "sqlite-version-bumps"  because of incompatible dependency versions. I'll see to contribute this upstream


Co-authored-by: Philipp Hoenisch <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants