disable ledger accounts caching

ali-sharif commented 4 years ago

Issue: When multiple ledgers are used during the same session, the derived public key list (HD) in the user interface can be populated with accounts from previously used ledgers.

Root cause: index management in the caching code is erroneous. This causes the first page, first load to not get cache busted for entries with index 1-4. Cache busting works correctly if indices >=5 are retrieved; on initial load. This happens because on initial load, index 0 is not cached.

Proposed fix: The least risky thing to do here is to disable the caching altogether, since:

Since only 5 child addresses are presented at a time, we can't imaging people scrolling very deep into that list to retrieve addresses.
Assuming people use the first few pages of the presented address list, the load times for a 5-address list is ~1.4s (see benchmark attached in comments), which seems acceptable.
Caching might have other unintended consequences (which have not yet surfaced). Since the performance is acceptable without caching, no reason to re-work the caching behaviour.

ali-sharif commented 4 years ago

Average times to retrieve 5 public keys from Ledger (on machine: i7-8550U, 16gb ddr3 ram, SATA ssd):

Ledger Nano X: ~1.40s
Ledger Nano S: ~1.43s

It seems like Nano X is slightly more responsive, but overall, the USB communication overhead dominates the cost of retriving public keys from Ledger. It seems like ~1.4 seconds is an acceptable delay, particularly since most people (most likely) aren’t scrolling very deep into that list.

[Ledger Nano S] Results

19-11-18 17:00:49.856 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(0,5) took 1.434178 seconds
19-11-18 17:00:53.417 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(5,10) took 1.442861 seconds
19-11-18 17:00:55.759 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(10,15) took 1.430114 seconds
19-11-18 17:00:58.798 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(15,20) took 1.435157 seconds
19-11-18 17:01:01.640 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(20,25) took 1.429440 seconds
19-11-18 17:01:04.178 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(25,30) took 1.427664 seconds
19-11-18 17:01:06.956 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(30,35) took 1.429577 seconds
19-11-18 17:01:09.065 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(35,40) took 1.430976 seconds

[Ledger Nano X] Results

19-11-18 16:51:28.157 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(25,30) took 1.402493 seconds
19-11-18 16:51:30.325 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(20,25) took 1.366629 seconds
19-11-18 16:51:32.375 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(15,20) took 1.288601 seconds
19-11-18 16:51:34.574 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(10,15) took 1.409470 seconds
19-11-18 16:51:36.711 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(5,10) took 1.396358 seconds
19-11-18 16:51:38.836 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(0,5) took 1.401626 seconds
19-11-18 16:51:43.445 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(5,10) took 1.398290 seconds
19-11-18 16:51:45.574 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(10,15) took 1.409409 seconds
19-11-18 16:51:47.791 INFO  WLT  [pool-13-thread-1]: getMultipleAccountDetails(15,20) took 1.382700 seconds

ali-sharif commented 4 years ago

The "recovered accounts list" seems to be designed to retrieve the transactions list when you recover the "root account" and all it's children that you've presumably requested the wallet to create for you (the wallet seems to maintain one HD wallet account on it's own).

https://github.com/aionnetwork/Desktop-Wallet/blob/07797d2c3b69879a91ead9d676ef85dd86643ba5/src/main/java/org/aion/wallet/account/AccountManager.java#L123

I think the event firing in that background task was in-error, since it makes no sense to optimistically fetch transactions for all pre-fetched child HD keys. Worst-case scenario, some transaction display features might not work; people should be using the dashboard for that anyways.

@qoire please confirm this line of deduction.

qoire commented 4 years ago

This does seem to be premature fetching. I believe we fetch transactions for a particular account anyways when accounts are changed. So maybe this is some premature optimisation? Anyways LGTM

aionnetwork / Desktop-Wallet

disable ledger accounts caching #257