Call getAccountBalance for all wallets at once on tip change #2034

Anviking · 2020-08-12T18:07:47Z

Issue Number

#2005

Overview

Only send the LSQ query GetFilteredDelegationsAndRewardAccounts when the tip changes
Query all wallet reward accounts in the same GetFilteredDelegationsAndRewardAccounts

Listing stake pools with 10 wallets now takes ~5 seconds instead of 2-3 minutes!

Make the implementation easier to follow and actually sane

Comments

Not completely sure this doesn't mess up some some intricate interactions between the reward balance state and utxo state. The integration tests pass. But maybe there are some rollback scenarios which could lead to the reward balance being incorrectly reported for a while… Would be nice to think through properly and synchronise them.

I think this happens on tip changes:

Wallet worker updates UTxO
Tip worker re-feches balances and writes to TVar
(I think on next tip change) Wallet reads the balance from the TVar and writes it to Sqlite
New reward balance is queryable from API

With this PR, I wonder if it may not risk being out of date for one more tip-change

Anviking · 2020-08-12T18:08:04Z

bors try

iohk-bors · 2020-08-12T18:48:54Z

try

Build failed

ci/hydra-build:required

KtorZ · 2020-08-13T06:08:09Z

lib/shelley/src/Cardano/Wallet/Shelley/Network.hs

@@ -328,6 +349,8 @@ withNetworkLayer tr np addrInfo versionData action = do
    connectNodeTipClient
        :: HasCallStack
        => RetryHandlers
+        -> TVar IO (Map W.ChimericAccount (Maybe W.Coin))


What about using 0 as a default value instead of Nothing. We do already default to 0 when it comes to the reward balance and it is quite plausible / will not cause any particular harm to report a balance as 0 until we fetch it.

I wanted to keep "we already default to 0" separate from the core caching logic.

With the slight refactoring, I made new entries get added to a separate toBeObserved :: Set, instead of Nothing values inside the Map, but the "query" still returns Maybe W.Coin like here, requiring an explicit fromMaybe.

KtorZ

I like the principle here of fetching them all in one query. We need to make sure however to clean accounts from the list when wallets are deleted. Otherwise, we end up fetching unused account over and over. We need some form "pub / sub" mechanism.

Anviking · 2020-08-17T10:51:43Z

bors r+

iohk-bors · 2020-08-17T10:51:52Z

👎 Rejected by too few approved reviews

Anviking · 2020-08-17T10:52:52Z

Meant:
bors try

iohk-bors · 2020-08-17T11:09:14Z

try

Build failed

buildkite/cardano-wallet

Anviking · 2020-08-17T11:21:28Z

bors try

Anviking · 2020-08-17T11:37:29Z

lib/shelley/src/Cardano/Wallet/Shelley/Network.hs

            [ "Querying the reward account balance for"
-            , pretty acct
+            , fmt $ listF accts


With 10 wallets:

[cardano-wallet.network:Info:15] [2020-08-17 10:37:49.31 UTC] Querying the reward account balance for [1ed0bdd4, 2a98402c, 5237aa81, 634bea28, 69e2c672, 75765c1e, 84132baa, c662444a, d55deafa, f4ea9095] at 70617265<-[c90c882f-6094377#4570261]

iohk-bors · 2020-08-17T12:21:31Z

try

Build succeeded

Anviking · 2020-08-17T13:52:45Z

lib/shelley/src/Cardano/Wallet/Shelley/Network.hs

+            , stopObserving = \k ->
+                atomically $ do
+                    modifyTVar' toBeObservedVar (Set.delete k)
+                    modifyTVar' cacheVar (Map.delete k)


Integration tests / STAKE_POOLS_JOIN_01 pass, meaning this should work.

But if this Observer abstraction is something we want to keep, it would be nice to add more extensive unit tests. newRewardBalanceFetcher would probably have to be made more abstract. (Because the record itself provides very little structure and can't be tested)

I also guess there is similarities between this and the WorkerRegistry, that perhaps could be unified in the future.

getAccountBalance is called by each wallet worker, I think on each restoration step (which is often). For some reason, sending multiple getAccountBalance queries at the same time slows it down from e.g. 0.002s to 40s. The LSQ query even supports querying multiple accounts in a single query, so let's use it! This commit makes getAccountBalance merely lookup in a Map in a TVar, which is updated with a single query on every tip-change. Somewhat crude implementation so far. I'm yet not familiar with the code that is calling getAccoutBalance. I suspect there are layers of indirection that can be removed.

Seems slightly nicer.

KtorZ · 2020-08-19T06:47:15Z

lib/shelley/src/Cardano/Wallet/Shelley/Network.hs

+        -> TVar IO (Set W.ChimericAccount)
+        -> Tip (CardanoBlock sc)
+        -> IO ()
+    refresh cacheVar toBeObservedVar tip = do


I find it surprising that refresh is not part of the Observer "interface". The function clearly has a privileged access to both TVar and make strong assumption about how they are manipulated. So to me, it seems like it belong to the same abstraction and having it as a separate function is a bit confusing.

To me, Observer is concerned about subscribing to and accessing values. It theoretically be passed around and escape the Network module.

refresh is coupled to the creation of the Observer. It's more like an implementation detail, specific to the Network module.

If say the wallet registry were to have access to an Observer, it should not be allowed to manually call refresh.

KtorZ · 2020-08-19T06:55:19Z

lib/shelley/src/Cardano/Wallet/Shelley/Network.hs

+          , Tip (CardanoBlock sc) -> IO ()
+            -- Call on tip-change to refresh
+          )
+newRewardBalanceFetcher tr gp queryRewardQ = do


It'd be nice to be able to test this bit of logic in isolation. This is currently hard because the management of the keys / accounts is mixed with the logic for fetching the reward. I'd suggest to replace the ChimericAccount with a Ord k => k and to replace Coin with an abstract a, and pass a function that yield a as argument. So, something like:

data Observer m key value = Observer { startObserving :: key -> m () , stopObserving :: key -> m () , query :: key -> m (Maybe value) , refresh :: tip -> m tip } newObserver :: Ord key => (tip -> [key] -> m (Map key value)) -> Observer m key value newObserver getValues = ...

This way, we can separate the network logic from the observer logic and test them separately (at least, test the observer).

It'd be nice to be able to test this bit of logic in isolation.

Yes 👍 I alluded to it in #2034 (comment), but didn't want to explore it before hearing your feedback.

Before this commit keys would start in `toBeObserved`. They would then get passed to the `fetch` function. If the supplied `fetch` function chose to not include the same keys in the resulting map of values, they would silently dissappear. This way, `newObserver` takes full responsibility of maintaining the set of keys which should be observed, by replacing the TVars (cache, toBeObserved) with (cache, observedKeys)

KtorZ · 2020-08-20T09:13:56Z

lib/shelley/src/Cardano/Wallet/Shelley/Network.hs

+            { startObserving = \k -> do
+                atomically $ do
+                    modifyTVar' observedKeysVar (Set.insert k)
+                traceWith tr $ MsgAddedObserver k


Since "startObserving" is called every time getAccountBalance is called, this would log a line MsgAddedObserver in a rather misleading way and much more than it needs. We could use something like Set.alterF and return Just _ or Nothing depending on whether the element was added (and log accordingly).

Yup. It's connected to nullTracer now, so one doesn't notice, but should connect to the real tracer and change this.

KtorZ · 2020-08-20T09:22:37Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/NetworkSpec.hs

+    describe "Observer" $ do
+        describe "(query k) with typical use" $ beforeAll mockObserver $ do
+            -- Using monadic-property tests /just/ for the sake of testing with
+            -- multiple keys seem worthless.


It'd be nice to test with multiple keys, since this is the main point of this PR actually. Coming up with some properties would help testing possible edge-cases that aren't caught here, although may be a bit tricky here. At least, we should add two additional test scenarios:

registering the same key twice

registering multiple keys

I should say, I’m much more worried about this causing further balance discrepancies than that it doesn’t handle registering the same key twice, or multiple keys, etc (though sure, good to test).

KtorZ · 2020-08-20T14:21:46Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/NetworkSpec.hs

+                            [ MsgAddedObserver k
+                            , MsgWillFetch $ Set.singleton k
+                            , MsgDidFetch $ Map.singleton k v
+                            ]


I don't get this test, unless this test depends on the previous ones which would be quite bad / misleading. Tests should work in isolation.

I wanted to experiment with slightly different usage of describe/it for more readable test output.

Here we get:

when refresh fails (query k) returns the existing v only MsgWillFetch is traced

which I think is quite neat.

(And in total:)

typical use startObserving (query k) returns Nothing before startObserving (query k) returns v after (startObserving k >> refresh) traced MsgAddedObserver, MsgWillFetch, MsgDidFetch calling startObserving a second time (query k) is still v when refresh fails (query k) returns the existing v only MsgWillFetch is traced stopObserving makes (query k) return Nothing

With no shared state, the whole typical use tree would be a single it.

I'll admit it gets a bit weird here though, where the refresh False belongs to the whole when refresh fails tree, but is run in the first it. (We cannot have it as a runIO or beforeAll, because we cannot access the refresh function from outside an it).

We'd need some

mapBeforeAll :: (a -> IO b) -> SpecWith b -> Spec a

which doesn't exist

We could probably add a Arbitrary (Observer, refresh), and do run most of these checks in isolation though 🤔

I admit that the separation is neat, but it is puzzling. So at the very least, there should be a clear note as comment explaining basically what you just explained here.

Anviking · 2020-08-20T15:20:18Z

bors try

Anviking · 2020-08-20T15:22:07Z

lib/shelley/src/Cardano/Wallet/Shelley/Network.hs

+        Observer
+            { startObserving = \k -> do
+                wasAdded <- atomically $ do
+                    notAlreadyThere <- Set.notMember k <$> readTVar observedKeysVar


Seems Set.alterF doesn't exist with our version of containers , and I didn't figure out how it worked, but think a separate readTVar inside atomically is perfectly fine.

iohk-bors · 2020-08-20T15:57:19Z

try

Build failed

ci/hydra-build:required

     Private Key
        +++ OK, passed 100 tests (59% conflicting db entries).
    parallel puts replace values in _any_ order
      Checkpoint
building of '/nix/store/wx550kfdn839bdg2racdn5755ng85fhl-cardano-wallet-core-test-unit-2020.8.3-check' timed out after 900 seconds of silence

Fell-x27 · 2020-08-20T19:49:27Z

There is a bug. This build doesn't sync reward balance for fresh-restored wallets. Probably it also doesn't get reward from pools incoming after wallet sync.

Fell-x27 · 2020-08-20T20:53:42Z

Hmm..looks like the very last build (03ed329) doesn't have this problem. Prev had.

KtorZ · 2020-08-21T08:25:22Z

Do you mind describing the context, expected behavior and observed behavior?

There are a few things that are counter intuitive at first when dealing with reward balances. And there's also a known "latency" issue we are tracking as part of anothrr ticket.

Fell-x27 · 2020-08-21T09:16:41Z

Do you mind describing the context, expected behavior and observed behavior?

There are a few things that are counter intuitive at first when dealing with reward balances. And there's also a known "latency" issue we are tracking as part of anothrr ticket.

I mean, fresh-restored wallets were restored without reward balance. And even pre-restored wallets with non-zero reward balance randomly "lost" rewards. But with the last build problem is gone. And rewards were back. But there is another one issue with fees described in #2005 (comment)

KtorZ · 2020-08-24T07:01:55Z

bors r+

iohk-bors · 2020-08-24T08:36:34Z

Build succeeded

iohk-bors bot added a commit that referenced this pull request Aug 12, 2020

Try #2034:

df86f5c

Anviking self-assigned this Aug 12, 2020

Anviking mentioned this pull request Aug 12, 2020

The pools list fetching takes 3-10 mins #2005

Closed

Anviking changed the base branch from master to anviking/2005/time-lsq-queries August 12, 2020 18:36

KtorZ reviewed Aug 13, 2020

View reviewed changes

Anviking mentioned this pull request Aug 13, 2020

Cleanup observed reward accounts #2042

Closed

1 task

Anviking force-pushed the anviking/2005/batch-getAccountBalance branch from 22ac654 to d7dc020 Compare August 17, 2020 10:45

Anviking changed the base branch from anviking/2005/time-lsq-queries to master August 17, 2020 10:46

iohk-bors bot added a commit that referenced this pull request Aug 17, 2020

Try #2034:

efbaf6d

Anviking force-pushed the anviking/2005/batch-getAccountBalance branch from d7dc020 to f66a86e Compare August 17, 2020 11:10

iohk-bors bot added a commit that referenced this pull request Aug 17, 2020

Try #2034:

9607bb8

Anviking commented Aug 17, 2020

View reviewed changes

Anviking marked this pull request as ready for review August 17, 2020 11:39

Anviking changed the title ~~WIP: Call getAccountBalance for all wallets at once on tip change~~ Call getAccountBalance for all wallets at once on tip change Aug 17, 2020

Anviking requested a review from KtorZ August 17, 2020 13:49

Anviking commented Aug 17, 2020

View reviewed changes

Anviking added 2 commits August 17, 2020 16:07

Add Observer abstraction for fetching rewards

7d1ff70

Seems slightly nicer.

Anviking force-pushed the anviking/2005/batch-getAccountBalance branch from f66a86e to 7d1ff70 Compare August 17, 2020 14:07

Anviking added the RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG label Aug 18, 2020

KtorZ reviewed Aug 19, 2020

View reviewed changes

Anviking added 3 commits August 19, 2020 13:53

Separate newRewardBalanceFetcher and newObserver

321c318

Add basic unit tests for Observer

f892b0b

Anviking force-pushed the anviking/2005/batch-getAccountBalance branch from f4a4d39 to 99ee84a Compare August 20, 2020 09:04

KtorZ reviewed Aug 20, 2020

View reviewed changes

Fixup: whitespace

b2ddd1f

KtorZ reviewed Aug 20, 2020

View reviewed changes

More extensive testing and logging

0312071

Anviking force-pushed the anviking/2005/batch-getAccountBalance branch from 72100ff to 0312071 Compare August 20, 2020 14:04

KtorZ reviewed Aug 20, 2020

View reviewed changes

Anviking added 2 commits August 20, 2020 17:02

Add comments explaining statefulness and small it blocks

e41a4b7

Minor documentation comment adjustments

03ed329

Anviking force-pushed the anviking/2005/batch-getAccountBalance branch from 5ddfc4c to 03ed329 Compare August 20, 2020 15:14

Anviking requested a review from KtorZ August 20, 2020 15:20

iohk-bors bot added a commit that referenced this pull request Aug 20, 2020

Try #2034:

8848a76

Anviking commented Aug 20, 2020

View reviewed changes

KtorZ approved these changes Aug 24, 2020

View reviewed changes

iohk-bors bot merged commit b5b292b into master Aug 24, 2020

iohk-bors bot deleted the anviking/2005/batch-getAccountBalance branch August 24, 2020 08:36

Call getAccountBalance for all wallets at once on tip change #2034

Call getAccountBalance for all wallets at once on tip change #2034

Conversation

Anviking commented Aug 12, 2020 • edited Loading

Issue Number

Overview

Comments

Anviking commented Aug 12, 2020

iohk-bors bot commented Aug 12, 2020

try

Build failed

Choose a reason for hiding this comment

Anviking Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

KtorZ left a comment

Choose a reason for hiding this comment

Anviking commented Aug 17, 2020

iohk-bors bot commented Aug 17, 2020

Anviking commented Aug 17, 2020

iohk-bors bot commented Aug 17, 2020

try

Build failed

Anviking commented Aug 17, 2020

Choose a reason for hiding this comment

iohk-bors bot commented Aug 17, 2020

try

Build succeeded

Anviking Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

KtorZ Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

Anviking Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

KtorZ Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

Anviking commented Aug 20, 2020

Choose a reason for hiding this comment

iohk-bors bot commented Aug 20, 2020 • edited by Anviking Loading

try

Build failed

Fell-x27 commented Aug 20, 2020

Fell-x27 commented Aug 20, 2020 • edited Loading

KtorZ commented Aug 21, 2020 • edited Loading

Fell-x27 commented Aug 21, 2020 • edited Loading

KtorZ commented Aug 24, 2020

iohk-bors bot commented Aug 24, 2020

Build succeeded

Anviking commented Aug 12, 2020 •

edited

Loading

Anviking Aug 17, 2020 •

edited

Loading

Anviking Aug 17, 2020 •

edited

Loading

KtorZ Aug 19, 2020 •

edited

Loading

Anviking Aug 20, 2020 •

edited

Loading

Anviking Aug 20, 2020 •

edited

Loading

KtorZ Aug 20, 2020 •

edited

Loading

iohk-bors bot commented Aug 20, 2020 •

edited by Anviking

Loading

Fell-x27 commented Aug 20, 2020 •

edited

Loading

KtorZ commented Aug 21, 2020 •

edited

Loading

Fell-x27 commented Aug 21, 2020 •

edited

Loading