Use file-based db for wallet #2174

bonomat · 2022-06-06T04:59:23Z

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

da-kami

Can we test it before merging, i.e. take the testnet seed start for first time expect the balance to be 0. Trigger sync until the balance is picked up. Then restart and the balance should be picked up without triggering another sync (because the addresses are cached in the db).

da-kami · 2022-06-06T10:57:06Z

Cargo.toml

@@ -26,3 +26,4 @@ maia = { git = "https://github.com/comit-network/maia", rev = "a963084189e1598ca
 maia-core = { git = "https://github.com/comit-network/maia", rev = "e419cb2cf19bed6ec53eaa3672408a6205a9b705", package = "maia-core" } # Unreleased version, pinned before maia's breaking change
 xtra_productivity = { git = "https://github.com/comit-network/xtra-productivity" } # Unreleased
 electrum-client = { git = "https://github.com/da-kami/rust-electrum-client/", branch = "fix/ignore-empty-lines" }
+bdk = { git = "https://github.com/coblox/bdk", branch = "sqlite-version-bumps" }


Why not a fork on itchysats?

No particular reason and we had a fork on coblox already.

bonomat · 2022-06-07T00:03:43Z

Can we test it before merging, i.e. take the testnet seed start for first time expect the balance to be 0. Trigger sync until the balance is picked up. Then restart and the balance should be picked up without triggering another sync (because the addresses are cached in the db).

It takes a long time. I'd rather go for: #2184 and start from scratch.

da-kami · 2022-06-07T00:27:59Z

Can we test it before merging, i.e. take the testnet seed start for first time expect the balance to be 0. Trigger sync until the balance is picked up. Then restart and the balance should be picked up without triggering another sync (because the addresses are cached in the db).

It takes a long time. I'd rather go for: #2184 and start from scratch.

I would have done it to make sure the behavior is as expected. But if you are confident enough to just go with the logs that's fine with me :)

bonomat · 2022-06-07T00:53:19Z

Can we test it before merging, i.e. take the testnet seed start for first time expect the balance to be 0. Trigger sync until the balance is picked up. Then restart and the balance should be picked up without triggering another sync (because the addresses are cached in the db).

It takes a long time. I'd rather go for: #2184 and start from scratch.

I would have done it to make sure the behavior is as expected. But if you are confident enough to just go with the logs that's fine with me :)

I'm actually losing confident right now 🙈

I've been running it over night and the funds were there but now I get errors and can't recover from it:

2022-06-07 00:49:39  WARN daemon::wallet: Syncing failed: Failed to sync wallet: Rusqlite(SqliteFailure(Error { code: DatabaseBusy, extended_code: 5 }, Some("database is locked")))

Even after restart the db is locked. This is not good

bonomat · 2022-06-07T05:40:19Z

@da-kami / @klochowicz : What do you think of using sled instead?

I did not get any errors as with sqlite
we don't need to read that db
we don't get into maintaining a fork for a long time because my version bump was only scheduled for 0.20.

See c03babf

da-kami · 2022-06-07T10:32:32Z

Maybe we can re-consider the configurable wallet address cache again?
That would allow us to configure the addresses cache in different environments. More of an in-between solution though.

da-kami · 2022-06-07T10:36:41Z

@da-kami / @klochowicz : What do you think of using sled instead?

* I did not get any errors as with sqlite

* we don't need to read that db

* we don't get into maintaining a fork for a long time because my version bump was only scheduled for 0.20.

See c03babf

How sure are we that we won't run into similar issues? Maybe try running it longer and see. We can consider going for this but we would have to test it over a longer period on testnet.

bonomat · 2022-06-15T00:04:32Z

@da-kami : cleaned-up the git history and made the test compile.

The last commit believe it will give us a better test performance.

Let's give this a try?

klochowicz

looks good, I'm keen to try it out! 👍

da-kami

Happy to give it a try. Worst case we should be able to go back to the in-memory code without any migration problems.

da-kami · 2022-06-15T11:02:09Z

maker/src/main.rs

@@ -65,7 +65,14 @@ async fn main() -> Result<()> {

    let mut tasks = Tasks::default();

-    let (wallet, wallet_feed_receiver) = wallet::Actor::new(opts.network.electrum(), ext_priv_key)?;
+    let mut wallet_dir = data_dir.clone();
+    wallet_dir.push("maker-wallet");


🙃 nit: We only use it twice but maybe we still want to extract it into a variable to avoid errors in the future.
Same for taker main

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 Signed-off-by: Philipp Hoenisch <[email protected]>

This unnecessary slows down syncs in most cases because all 1000 addresses need to be synced on every sync interval. The wallet itself checks that atleast 100 addresses are cached when sync is called. This should be plenty.

This allows us to use an in-memory wallet for tests which gives us a better test performance. Signed-off-by: Philipp Hoenisch <[email protected]>

bonomat · 2022-06-15T22:31:28Z

bors r+

bors · 2022-06-15T22:47:40Z

bonomat force-pushed the wallet-db branch 2 times, most recently from 83b943b to 2f6b54d Compare June 6, 2022 05:39

bonomat force-pushed the wallet-manual-sync branch from b0307a8 to d0ccadb Compare June 6, 2022 05:41

bonomat force-pushed the wallet-db branch from 2f6b54d to 5b2de0f Compare June 6, 2022 05:44

da-kami approved these changes Jun 6, 2022

View reviewed changes

da-kami mentioned this pull request Jun 7, 2022

Upgrade to latest BDK and remove patched version dependencies #2187

Closed

bonomat force-pushed the wallet-manual-sync branch from d0ccadb to 867915a Compare June 7, 2022 03:40

Base automatically changed from wallet-manual-sync to master June 7, 2022 04:31

bonomat force-pushed the wallet-db branch from 5b2de0f to c03babf Compare June 7, 2022 05:39

bonomat force-pushed the wallet-db branch from c03babf to 43dd02e Compare June 7, 2022 05:52

bonomat force-pushed the wallet-db branch 3 times, most recently from d75e925 to d61e271 Compare June 15, 2022 00:04

bonomat requested a review from da-kami June 15, 2022 00:04

bonomat changed the title ~~Use SQLite db for wallet~~ Use file-based db for wallet Jun 15, 2022

bonomat force-pushed the wallet-db branch from d61e271 to 3644b60 Compare June 15, 2022 00:06

klochowicz approved these changes Jun 15, 2022

View reviewed changes

da-kami approved these changes Jun 15, 2022

View reviewed changes

da-kami mentioned this pull request Jun 15, 2022

Maker wallet address cache size configurable #2125

Closed

bonomat added 3 commits June 16, 2022 08:30

Don't cache 1000 addresses

30f93dc

This unnecessary slows down syncs in most cases because all 1000 addresses need to be synced on every sync interval. The wallet itself checks that atleast 100 addresses are cached when sync is called. This should be plenty.

Make Wallet generic

5a2b016

This allows us to use an in-memory wallet for tests which gives us a better test performance. Signed-off-by: Philipp Hoenisch <[email protected]>

bonomat force-pushed the wallet-db branch from 3644b60 to 5a2b016 Compare June 15, 2022 22:31

bors bot merged commit 4abf47d into master Jun 15, 2022

bors bot deleted the wallet-db branch June 15, 2022 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use file-based db for wallet #2174

Use file-based db for wallet #2174

bonomat commented Jun 6, 2022 •

edited

Loading

da-kami left a comment

da-kami Jun 6, 2022

bonomat Jun 6, 2022

bonomat commented Jun 7, 2022 •

edited

Loading

da-kami commented Jun 7, 2022

bonomat commented Jun 7, 2022 •

edited

Loading

bonomat commented Jun 7, 2022 •

edited

Loading

da-kami commented Jun 7, 2022 •

edited

Loading

da-kami commented Jun 7, 2022

bonomat commented Jun 15, 2022

klochowicz left a comment

da-kami left a comment

da-kami Jun 15, 2022

bonomat commented Jun 15, 2022

bors bot commented Jun 15, 2022

Use file-based db for wallet #2174

Use file-based db for wallet #2174

Conversation

bonomat commented Jun 6, 2022 • edited Loading

da-kami left a comment

Choose a reason for hiding this comment

da-kami Jun 6, 2022

Choose a reason for hiding this comment

bonomat Jun 6, 2022

Choose a reason for hiding this comment

bonomat commented Jun 7, 2022 • edited Loading

da-kami commented Jun 7, 2022

bonomat commented Jun 7, 2022 • edited Loading

bonomat commented Jun 7, 2022 • edited Loading

da-kami commented Jun 7, 2022 • edited Loading

da-kami commented Jun 7, 2022

bonomat commented Jun 15, 2022

klochowicz left a comment

Choose a reason for hiding this comment

da-kami left a comment

Choose a reason for hiding this comment

da-kami Jun 15, 2022

Choose a reason for hiding this comment

bonomat commented Jun 15, 2022

bors bot commented Jun 15, 2022

bonomat commented Jun 6, 2022 •

edited

Loading

bonomat commented Jun 7, 2022 •

edited

Loading

bonomat commented Jun 7, 2022 •

edited

Loading

bonomat commented Jun 7, 2022 •

edited

Loading

da-kami commented Jun 7, 2022 •

edited

Loading