Uh oh!
There was an error while loading. Please reload this page.
Improve on (potentially) blocking wallet behaviors - #281
Conversation
3d70262 to
c0f13bcCompare8edbac9 to
d4649e8Compared4649e8 to
abb6926Comparetnull
commented
Jun 1, 2024
Rebased on #141 to resolve minor conflicts with main. |
85affcf to
4abd601Comparetnull
commented
Jun 11, 2024
Rebased on main after #141 landed. |
4abd601 to
fa7690bComparetnull
commented
Jun 14, 2024
Rebased after #307 landed. |
jbesraa
left a comment
There was a problem hiding this comment.
I tested this branch locally with an app with GUI that previously lagged when the wallet synced and I can confirm that with this PR things are much smoother
Yeah, without these workarounds the post-Anchors code is borderline unusable as event handling might grind to a standstill whenever we need to check the balance and wallet syncing is ongoing. |
fa7690b to
840ca4fComparetnull
commented
Jun 17, 2024
Rebased to resolve minor conflict after #303 landed. |
840ca4f to
a85fedeCompareNow also added a commit dropping the immediate-retry behavior in TxBroadcaster, and applying the timeout to |
c1ff7a1 to
0e9cacaCompare| *self.balance_cache.write().unwrap() = balance.clone(); | ||
| balance | ||
| }, | ||
| Err(_) => self.balance_cache.read().unwrap().clone(), |
There was a problem hiding this comment.
can you expand on some of the implications of using a cached balance ?
iiuc, we might bypass some of the balance checks during open-channel (outbound and inbound) and send_to_address.(does bdk allow building tx greater than balance?)
There was a problem hiding this comment.
can you expand on some of the implications of using a cached balance ?
It essentially has no implications: generally the wallet balance would always just be updated when a sync succeeds and here we make sure to always update the cache afterwards, while still holding the wallet Mutex. So the cache should always be up to date. Plus, we'll actually call through and update the cache whenever we can.
iiuc, we might bypass some of the balance checks during open-channel (outbound and inbound) and send_to_address.(does bdk allow building tx greater than balance?)
Yes, but IIUC BDK's wallet would always only pick up on any changes after a successful sync, i.e., it would show the old balance anyways.
Btw, see relatedly: #41
| @@ -175,13 +199,24 @@ where | |||
| pub(crate) fn get_balances( | |||
There was a problem hiding this comment.
might be worth to add doc that result might be cached.
There was a problem hiding this comment.
I honestly don't think it matters: a) this is pub(crate), and b) it shouldn't result in any noticeable behavior changes, as discussed above.
| bdk::Error::Esplora(ref be) => match **be { | ||
| bdk::blockchain::esplora::EsploraError::Reqwest(_) => { | ||
| // Drop lock, sleep for a second, retry. | ||
| drop(wallet_lock); |
There was a problem hiding this comment.
nit: we this and retry here, only to remove it in later commit, can be simplified if we just drop retry before this.
There was a problem hiding this comment.
Fair enough. Are you fine with me leaving it like this or would you prefer to invert the commit ordering?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
afee329 to
fcb824fComparetnull
commented
Jun 20, 2024
Kicked CI. @G8XSU Let me know if I can squash the fixups. |
G8XSU
commented
Jun 20, 2024
Lgtm! |
Unfortunately BDK's current wallet design requires us to have it live in `Mutex` that is locked for long periods of time during syncing. This is especially painful for short-lived operations that just operate locally, such as retrieving the current balance, which we now do in several places to be able to check Anchor channels limitations, e.g., in event handling. In order to avoid blocking during balance retrieval, we introduce a `balance` cache that will be refreshed whenever we're done with syncing *or* when we can successfully get the wallet lock. Otherwise, we'll just return the cached value, allowing us to make progress even though a background sync of the wallet might be in-progress.
Using a `Condvar` could be potentially dangerous in async contexts as `wait`ing on it might block the current thread potentially hosting more than one task. Here, we drop the `Condvar` and adopt a pub/sub scheme instead, similar to the one we already implemented in `ConnectionManager`.
It's not super clear that it achieves much in the face of a rate-limited Esplora server, and having a custom sleep there is just awkward. So we drop it and hope we still get a chance to sync our on-chain wallet now and then.
.. as we're not sure it actually increases reliability. We now only log failures, ignoring HTTP 400 as this is bitcoind's error code for "transaction already in mempool".
.. to make progress and unblock the `Mutex` even if BDK's wallet `sync` would never return.
.. even though we don't expect this to block, we're better safe than sorry and start to introduce timeouts for any calls we make to remote servers.
.. even though we don't expect this to block, we're better safe than sorry and start to introduce timeouts for any calls we make to remote servers.
.. even though we don't expect this to block, we're better safe than sorry and start to introduce timeouts for any calls we make to remote servers.
.. before initiating the Runtime shutdown.
.. as we use `Clone` for `tokio::sync::watch::Sender`, which was only introduced with 1.37.
fcb824f to
f839015Comparetnull
commented
Jun 20, 2024
Squashed without further changes. |
Based on #141.Due to our onchain wallet requiring us to hold a
Mutexfor the duration of the chain sync, many operations might be blocked for seconds at at time.Here, we make several changes that hopefully alleviate any blocking issues from holding the wallet lock(s). In particular we introduce a balance cache, drop our 'immediate retry sync' logic and drop the previous
wallet_lockCondvarin favor of a pub/sub pattern that is beneficial in async runtime environments.