backend/account: fix bug in account discovery #3330
Conversation
backend/accounts.go
Outdated
| @@ -1533,6 +1533,9 @@ func (backend *Backend) checkAccountUsed(account accounts.Interface) { | |||
| log.WithError(err).Error("error initializing account") | |||
| return | |||
| } | |||
| for !account.Synced() { | |||
| time.Sleep(100 * time.Millisecond) | |||
There was a problem hiding this comment.
That is not a robust solution 🙈 How about calling checkAccountUsed once the account is synced instead?
edit to clarify the downsides of this:
- it might loop forever if the account does not sync
- up to 100ms of delay before proceeding
There was a problem hiding this comment.
I agree is not very elegant, but why do you think it is not robust?
Beerosagos
force-pushed
the
account-discovery
branch
from
2ae0e8c to
f1b75b8
Compare
|
@benma updated, PTAL. While working on this I also noticed a weird bug that was sometimes showing a wrong account list in the sidebar: after some debugging I found the cause in |
backend/accounts.go
Outdated
| } | ||
| account.Observe(func(event observable.Event) { | ||
| if event.Subject == string(accountsTypes.EventSyncDone) { | ||
| f() |
There was a problem hiding this comment.
There is already an account.Observe() in addAccount(), imho it's better to just move the go backend.checkAccountUsed(account) there instead of having this big local f := func() ... here. Wdyt?
Technically it's not guaranteed that the SyncDone event will be fired after we start to observe it 🙈 But that's probably a super-edge case that we don't need to worry about 😄
Now it also occurs to me that this PR probably also fixes the bug if you receive coins on the last account, you can't add another one until you re-open the app and it realizes the previous account was used. Now it will re-check once coins arrive 👍
There was a problem hiding this comment.
There is already an account.Observe() in addAccount(), imho it's better to just move the go backend.checkAccountUsed(account) there instead of having this big local f := func() ... here. Wdyt?
I tested that, but it waits until the sync is done before scanning the next account: the whole discovery is much slower.
Now it also occurs to me that this PR probably also fixes the bug if you receive coins on the last account, you can't add another one until you re-open the app and it realizes the previous account was used. Now it will re-check once coins arrive 👍
Exactly :D
Technically it's not guaranteed that the SyncDone event will be fired after we start to observe it 🙈 But that's probably a super-edge case that we don't need to worry about 😄
I could check if the account is already synced before setting the observer, wdyt?
Beerosagos
force-pushed
the
account-discovery
branch
from
f1b75b8 to
2361ac6
Compare
|
@benma updated, PTAL 🙏 |
backend/backend.go
Outdated
| return backend.accounts | ||
| accounts := AccountsList{} | ||
| accounts = append(accounts, backend.accounts...) | ||
| return accounts |
There was a problem hiding this comment.
Use slices.Clone() instead, which I recently learned about here #3322 :P
Recent commit 726289c made the account Initialization asynchronous, moving the `account.ensureAddresses()` call on a separate thread. This caused a regression inside `backend.checkAccountUsed()` where the method wasn't waiting for the complete account synchronization before marking it as used. This commit fixes the issue waiting for account sync before checking if it used. This also fixes the `backend.Accounts()` method, that was returning the original `backend.accounts` object instead of a copy. This caused the caller to read the account list without a proper lock, causing weird behaviors during account discovery.
Beerosagos
force-pushed
the
account-discovery
branch
from
2361ac6 to
c5b2896
Compare
Recent commit 726289c made the account Initialization asynchronous, moving the
account.ensureAddresses()call on a separate thread. This caused a regression insidebackend.checkAccountUsed(), where the method wasn't waiting for the complete account synchronization before marking it as used.This commit fixes the issue waiting for account sync before checking if it used.
Note: The tests didn't catch the regression: I couldn't find an easy way to add some latency in the mocked addresses verification, but it would be nice to introduce it in a test to make it more reliable. Any suggestions?