Accounts discovery #2086
Accounts discovery #2086
Conversation
benma
force-pushed
the
accountsync2
branch
2 times, most recently
from
93b5664
to
1ae91cc
Compare
|
First 3 commits looks good to me. I'm still looking into the code for the others, but with the first tests I noticed an inconsistency in the account summary view (see pic). What I did:
Am I doing smth wrong? EDIT: Tested again, properly restarting both the backend and the frontend. Everything seems to work, so I'd say that this is not a real problem.
|
|
Code and tests LGTM, really nice! |
Tested again this morning on Android, got the same weird behavior. It was not meant to be a test for this PR, just needed to compile account discovery on testnet for Android, so here is what I did:
I don't see how these changes should break this tho. Closing the app and opening again fixed the issue.
|
There was a problem hiding this comment.
Ok, I think I found the problem:
backend.Accounts() sorts the account list while locking the Read lock (see https:/benma/bitbox-wallet-app/blob/accountsync2/backend/backend.go#LL429C16-L429C17), which doesn't seem correct. Moving the sortAccounts call inside backend.addAccount seems to solve the issue.
Locking the write lock inside backend.Accounts seems to cause a deadlock, and doesn't look good anyway, imho.
A couple notes:
- with the suggested modification I could not reproduce the issue again, but I made a lot of tests removing the accounts.json file and restarting the servewallet. I noticed that sometimes the bb02 seems to take longer in deriving the keys (I can tell from the bb02 screen saver staying still instead of moving around), sometimes up to a minute or so. I thought of another race condition, but it apparently solves by itself. It also happened once that the bb02 apparently aborted some request and needed to be unplugged and replugged (no need to restart the servewallet) to get the app working again. No idea why.
- I noticed that the number of scanned addresses is not always shown by the frontend, I didn't investigate it and neither checked if it is related to this PR or if that happens also on master.
An account could be Initialized() after it was already Closed() if they run at the same time. In ETH, the isClosed check was missing altogether, but even in btc, there was a race condition. We need to use the same lock for both. The `WaitSynchronized()` in the Close function of the ETH account also seems unneccesary and if it blocks the Close indefinitely, the DB cannot be closed and the an account with the same DB cannot be initialized (e.g. in `ReinitializeAccounts()`). Not sure why it was added, the commit that introduced it had no explanation. I ran into these issues while working on unit tests for accounts sync.
The `account.Config().Config` is a pointer to the persisted account stored in `backend.config.accountsConfig.Accounts`. By making the the slice entry also a pointer, we can make sure that the `account.Config().Config` always points to the persisted config. Then, we can simply modify the persisted config to also modify the loaded config. Before that, the same was true but not deterministically: the stored account config and the loaded config would point to the same memory location, but only as long as the persisted account was not moved, which can happen when the slice is extended (so the slice and all contents is moved to a new location while growing the slice). If this happens, then the loaded config would become a shallow copy, which is error prone: modifying a primitive field would modify a copy, while modifying a pointer field (e.g. the signing configuration) would modify both the loaded and the persisted config. The alternative to this commit would have been to instead make a deep copy of the persisted config when loading it. It seemed to me like just sharing the same memory here is simpler overall.
By being able to mock the accounts, it is much easier to write unit tests that rely on specific acocunt behavior. For example, in accounts discovery we check if `.Transactions()` returns an empty or non empty list to decide when to stop scanning accounts. Without mocking the account, we'd have to connect to mocked Electrum/Etherscan servers, which is much harder to do and maintain.
We want to track if an account has been used (i.e. has a transaction history). This is info is required to decide if the next account can be created, according to BIP39.
In order to scan accounts until we find an empty account, we always need to scan one account ahead of what is visible to the user. For this, we introduce a flag "hiddenBecauseUnused", which is false by default and only true if an account is added to be scanned in the background. It is not shown to the user at all, until either: - it is scanned and has a transaction history - the user adds a new account in the settings - this activtes the hidden account if it was automatically added before. Such hidden accounts are persisted for the same reason the regular accounts are persisted: creating the entry requires the xpubs, which requires a keystore (BitBox02), which is slow. By persisting it we do not need to query the keystore for multiple xpubs everytime for the hidden account, but just once. The part where we add the hidden account automatically follows in the next commit.
We implement BIP-44 accounts scanning, meaning: accounts per coin are scanned until we find an unused account. See the docstring of `maybeAddHiddenUnusedAccounts()` for details, especially how and why we deviate from BIP-44.
`Accounts()` only held the read lock but modified the accounts, which could lead to a race condition.
|
Thanks @Beerosagos !! I fixed the race condition and added unit tests. While adding the unit tests I discovered that adding a new account in the settings does not set the name correctly if a previously added hidden account is activated - I fixed that too. |
I open a draft PR without unit tests, to discuss the concept. Unit tests of this nature (lots of async) is tedious to write, so I will do that once the code is somewhat stable.
This implements accounts discovery for BTC/LTC.
This also allows users to create more than 5 accounts. At the moment, the first 5 can always be added like before, and afterwards an account can only be added if the previous one was used. We could limit adding the first 5 accounts in the same way - if so, in another PR.
The first three commits (fix race condition, simplify RenameAccount, mock btc/eth) are kind of independent of the main concept and can be merged if review is ok regardless of the the rest.
The commits are designed to be small enough to be easy to review.