Refactor chain following #2750

Anviking · 2021-07-01T15:48:31Z

Issue Number

Motivation

Simplify and clean up the chain following code, in the hope of making the wallet more robust against node disconnects. In particular, reduce the number of threads used to run the ChainSync protocol.

Overview

The chainSync function now takes a record of callbacks, ChainFollower, as an argument. These callbacks are used to request the current intersection, as well as react to roll forward and roll backwards messages.
Remove the old Cursor type.
Reduce the number of threads used for implementing the above.

Progress

Comments

rvl

It will be good to simplify chain following.

rvl · 2021-07-05T14:05:51Z

lib/core/src/Cardano/Wallet/DB.hs

@@ -329,6 +331,8 @@ newtype ErrNoSuchWallet
    = ErrNoSuchWallet WalletId -- Wallet is gone or doesn't exist yet
    deriving (Eq, Show)

+instance Exception ErrNoSuchWallet


The possibility of both checked and unchecked exceptions - this is something we want to avoid.

rvl · 2021-07-05T14:09:32Z

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

+            -- TODO: Recover on connection lost exceptions!
+            connectClient tr handlers client versionData conn
+
+        , currentNodeTip =
            fromTip getGenesisBlockHash <$> atomically readNodeTip
        , currentNodeEra =
            -- NOTE: Is not guaranteed to be consistent with @currentNodeTip@
            readCurrentNodeEra
        , watchNodeTip = do


With the reformulation of chainSync, this function will probably be redundant.

I don't see how watchNodeTip would be related to this PR.

We use it for

Caching reward balances in DB

Tx resubmission

It would be good to unify the DB and in-memory caching of rewards, but that's a separate concern. (I think this might contribute to values being slower to be updated, and perhaps to some integration test flakiness).

I guess you mean we could implement watchNodeTip using chainSync?

But I think

having a single tip-sync protocol and multiple STM observers (allow callbacks to be skipped under load)

not having to deserialise block bodies

are good qualities with the current approach, plus I don't see how ChainFollower could allow fast-forwarding the intersection to the tip.

Anviking · 2021-08-10T12:51:18Z

Closing for now.

rvl · 2021-08-14T08:52:49Z

Let's keep the branch and re-open this PR later, because this refactor is something that we definitely need.

HeinrichApfelmus · 2021-10-26T10:54:50Z

bors try

iohk-bors · 2021-10-26T11:41:35Z

try

Build succeeded:

HeinrichApfelmus · 2021-10-26T14:27:16Z

The integration test failures seem to be unrelated to the new chain following code. On my local machine, these failures do pass.

Instead, the failures seem to be related to:

File deletion on windows (when cleaning up old wallets)
Problems with DB concurrency (SQLITE_BUSY)

It appears that these problems will have to be addressed in the database layer.

Anviking

From an incomplete first pass now as well as some commits I saw in the past, lgtm!, left some comments, but will have another look.

Maybe we could regardless have a ~20 minute call about it tomorrow some time after the futurespective?

lib/core/src/Cardano/Wallet.hs

lib/core/src/Cardano/Wallet/Primitive/SyncProgress.hs

lib/core/src/Ouroboros/Network/Client/Wallet.hs

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

Anviking · 2021-10-27T12:13:40Z

lib/core/src/Ouroboros/Network/Client/Wallet.hs

@@ -505,7 +520,7 @@ localStateQuery queue =
        :: LocalStateQueryCmd block m
        -> m (LSQ.ClientStAcquired block (Point block) (Query block) m Void)
    clientStAcquired (SomeLSQ cmd respond) = pure $ go cmd $ \res -> do
-        LSQ.SendMsgRelease (respond res >> clientStIdle)
+        LSQ.SendMsgRelease (respond res >> finalizeCmd >> clientStIdle)


I wonder if not technically a lost connection between respond res and finaliseCmd can lead to the callback being called twice.

But this should be exceedingly rare, and it shouldn't affect queries using our send helper:

send :: MonadSTM m => TQueue m (cmd m) -> ((a -> m ()) -> cmd m) -> m a send queue cmd = do tvar <- newEmptyTMVarIO atomically $ writeTQueue queue (cmd (atomically . putTMVar tvar)) atomically $ takeTMVar tvar

and I don't imagine it would affect anything else either.

If you concur, maybe we should add a note about it though.

Hm, I do not believe that there is a race condition here. 🤔 As far as I can tell, losing the connection to a node is a synchronous exception: The next read or write to the underlying socket will fail. As long as we do not read or write to the socket, there will be no exception related to it.

If this code path receives an asynchronous exception, then this will not be caught by recoveringNodeConnection, and it doesn't matter whether we put anything back into the queue or not.

But I agree that it's best for respond to avoid throwing a synchronous exception (e.g. from trying to read the node) and will add a note to that effect.

As far as I can tell, losing the connection to a node is a synchronous exception: The next read or write to the underlying socket will fail. As long as we do not read or write to the socket, there will be no exception related to it.

Ok, good point, thanks

piotr-iohk · 2021-10-27T13:23:36Z

The branch survives the test from ADP-871 which is great, but each time, after node or wallet server is restarted, all the wallets begin to sync from scratch, that is:

after restarting the node (with wallet server working in bg)
or after restarting the wallet (with node working in bg)
also after restarting both... :)

HeinrichApfelmus · 2021-10-29T15:56:38Z

I rewrote the commit history (though perhaps a bit too aggressively). 🤓

piotr-iohk · 2021-10-29T19:04:59Z

Would you mind rebasing over master 🙏

* Group existing functions into logical sections * Refactor `connectCardanoApiClient` to be more general and to compete directly with `connectClient` * Add `mkLocalTxSubmissionClient` to follow the existing pattern for creating protocol clients.

1. Ensure follow catches asyncronous exceptions from connectClient, such that it can restart with a new connection and cursor. 2. Keep Local State Queries and to-be-submitted Txs queued until their requests finish, not just when they start. If a query is interrupted by the node being disconnected, it will block until a connection is re-established, and then retry.

New type `ChainFollower` provides callbacks that are used to drive and respond to the node-to-client messages.

* Percolate the `ChainPoint` type halfway to the database layer * TODO later: Use the type provided by `Cardano.Api` instead

* Remove unused messages * Remove superstitious printing of exceptions in `connectClient`. * Remove `MsgFollowLog` constructor

* Check whether the block we want to rollback to is the genesis block, use that in `ChainPoint`. * Request genesis explicitly when `MsgIntersectNotFound`. This ensures that the /read-pointer/ on the block producer side points to genesis, avoiding a weird corner case.

… so that the wallet does not sync from the origin anymore. ><

HeinrichApfelmus · 2021-11-01T12:38:45Z

Would you mind rebasing over master 🙏

Sure, no problem. I have fixed the issue with the HasLogger class.

Anviking

LGTM! Thanks for picking this up!

I cannot officially approve since it is was my PR, so feel free to do so yourself.

Anviking · 2021-11-03T08:53:57Z

lib/core/src/Ouroboros/Network/Client/Wallet.hs

+        -- Cave: An empty list is interpreted as requesting the genesis point.
+        let points' = if null points
+                then [Point Origin]
+                else sortBy (flip compareSlot) points -- older points last


Ah wow — does this mean on master we'd always rollback to the oldest common checkpoint? 🤔

I see there's a Asc CheckpointSlot in the wallet DB listCheckpoints
https://github.com/input-output-hk/cardano-wallet/blob/c8cbdb8e40763f13bc58ecc8af9b39f34b0d0314/lib/core/src/Cardano/Wallet/DB/Sqlite.hs#L1416

Is there a case for dropping the sortBy and changing the direction of the sort the DB returns (both wallet db and pool db)?

Pros:

Perhaps negligible performance increase

avoids the cognitive overhead of sorting it in different directions in two different places

Con:

easier for chainSync call-sites to mess up

Ah wow — does this mean on master we'd always rollback to the oldest common checkpoint? 🤔

Fortunately not, as that would have meant rolling back to genesis (this is the issue that Piotr encountered). There was a call to reverse somewhere in the old follow function on master. 😅

Actually, the specification of the ChainSync mini-protocol stipulates that the node should ignore the sort order and returns the youngest point on the chain — but cardano-node behaves differently. See IntersectMBO/ouroboros-network#3443

Is there a case for dropping the sortBy and changing the direction of the sort the DB returns (both wallet db and pool db)?

In light of the ChainSync spec, I would make the case for keeping the sortBy in the chainSync function and instead dropping the sort direction from the DB layer.

In light of the ChainSync spec, I would make the case for keeping the sortBy in the chainSync function and instead dropping the sort direction from the DB layer.

(But do nothing for now, as this would become obsolete in the DB layer redesign anyway.)

lib/core/src/Cardano/Wallet.hs

piotr-iohk

LGTM! Awesome improvement!

Playing around with the branch a little and It looks that it solves multiple stability issues a.k.a. "vanishing wallets", addressed in:

I've also run this branch against e2e tests few times and no "wallet_not_responding" observed, which was rather common before, now passing e.g.:

https://github.com/input-output-hk/cardano-wallet/actions/runs/1415608203
https://github.com/input-output-hk/cardano-wallet/actions/runs/1412956182
https://github.com/input-output-hk/cardano-wallet/actions/runs/1415762485
https://github.com/input-output-hk/cardano-wallet/actions/runs/1412958475

❤️

piotr-iohk · 2021-11-04T09:33:47Z

bors r+

iohk-bors · 2021-11-04T10:29:12Z

Build succeeded:

Anviking self-assigned this Jul 1, 2021

Anviking changed the title ~~Very WIP: Refactor chain following~~ Rough draft: Refactor chain following Jul 1, 2021

rvl reviewed Jul 5, 2021

View reviewed changes

Anviking force-pushed the anviking/ADP-871/refactor-chain-following branch from c83e55a to 59fe9d9 Compare July 6, 2021 19:23

Anviking changed the base branch from anviking/ADP-871/node-connections to master July 6, 2021 19:30

Anviking force-pushed the anviking/ADP-871/refactor-chain-following branch from 5e8501d to efc0229 Compare July 7, 2021 08:32

Anviking mentioned this pull request Jul 7, 2021

Handle node disconnections #2745

Closed

4 tasks

Anviking mentioned this pull request Jul 19, 2021

Fix weeder 2.x config and remove weeds #2765

Merged

Anviking closed this Aug 10, 2021

Anviking reopened this Sep 1, 2021

Anviking force-pushed the anviking/ADP-871/refactor-chain-following branch from d774497 to b1d146f Compare September 2, 2021 12:25

HeinrichApfelmus force-pushed the anviking/ADP-871/refactor-chain-following branch 2 times, most recently from d265671 to dd9ab0a Compare October 18, 2021 13:52

Anviking assigned HeinrichApfelmus and unassigned Anviking Oct 19, 2021

HeinrichApfelmus mentioned this pull request Oct 20, 2021

Flaky test: unit_computeMinimumAdaQuantity_fixedSizeBundle_256 #2982

Closed

1 task

HeinrichApfelmus force-pushed the anviking/ADP-871/refactor-chain-following branch from e792da7 to 405cfb6 Compare October 20, 2021 14:36

HeinrichApfelmus changed the title ~~Rough draft: Refactor chain following~~ Refactor chain following Oct 22, 2021

HeinrichApfelmus force-pushed the anviking/ADP-871/refactor-chain-following branch from 4f86800 to a74655c Compare October 22, 2021 19:39

iohk-bors bot added a commit that referenced this pull request Oct 26, 2021

Try #2750:

87dcd14

HeinrichApfelmus force-pushed the anviking/ADP-871/refactor-chain-following branch from a48f16a to 63781c0 Compare October 26, 2021 15:38

HeinrichApfelmus marked this pull request as ready for review October 26, 2021 15:38

Anviking commented Oct 27, 2021

View reviewed changes

HeinrichApfelmus force-pushed the anviking/ADP-871/refactor-chain-following branch from 2172c52 to 30f9d1c Compare October 28, 2021 15:47

HeinrichApfelmus force-pushed the anviking/ADP-871/refactor-chain-following branch 2 times, most recently from 284c66d to 8f0d750 Compare October 29, 2021 15:55

HeinrichApfelmus and others added 10 commits November 1, 2021 12:39

Refactor ChainSync protocol

7fa865a

New type `ChainFollower` provides callbacks that are used to drive and respond to the node-to-client messages.

Introduce ChainPoint type for chain following

517c7f2

* Percolate the `ChainPoint` type halfway to the database layer * TODO later: Use the type provided by `Cardano.Api` instead

Clean up network logging

865e7e5

* Remove unused messages * Remove superstitious printing of exceptions in `connectClient`. * Remove `MsgFollowLog` constructor

Sort chain points before requesting intersection

b89234c

… so that the wallet does not sync from the origin anymore. ><

Discuss [CheckedExceptionsAndCallbacks]

61e958c

Update documentation for restoreBlocks

2fbafa1

Adapt to new HasLogger type in master

99cfe7c

HeinrichApfelmus force-pushed the anviking/ADP-871/refactor-chain-following branch from 8f0d750 to 99cfe7c Compare November 1, 2021 11:40

piotr-iohk pushed a commit that referenced this pull request Nov 2, 2021

Get latest binaries from Hydra for PR #2750

35ffe9e

Anviking commented Nov 3, 2021

View reviewed changes

HeinrichApfelmus approved these changes Nov 3, 2021

View reviewed changes

piotr-iohk approved these changes Nov 3, 2021

View reviewed changes

iohk-bors bot merged commit 75c0f19 into master Nov 4, 2021

iohk-bors bot deleted the anviking/ADP-871/refactor-chain-following branch November 4, 2021 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor chain following #2750

Refactor chain following #2750

Anviking commented Jul 1, 2021 •

edited by piotr-iohk

Loading

rvl left a comment

rvl Jul 5, 2021

rvl Jul 5, 2021

Anviking Jul 6, 2021

Anviking Jul 6, 2021

Anviking commented Aug 10, 2021

rvl commented Aug 14, 2021

HeinrichApfelmus commented Oct 26, 2021

iohk-bors bot commented Oct 26, 2021

HeinrichApfelmus commented Oct 26, 2021

Anviking left a comment •

edited

Loading

Anviking Oct 27, 2021

HeinrichApfelmus Oct 29, 2021

Anviking Nov 3, 2021

piotr-iohk commented Oct 27, 2021 •

edited

Loading

HeinrichApfelmus commented Oct 29, 2021

piotr-iohk commented Oct 29, 2021

HeinrichApfelmus commented Nov 1, 2021

Anviking left a comment

Anviking Nov 3, 2021 •

edited

Loading

HeinrichApfelmus Nov 3, 2021

HeinrichApfelmus Nov 3, 2021

piotr-iohk left a comment

piotr-iohk commented Nov 4, 2021

iohk-bors bot commented Nov 4, 2021

Refactor chain following #2750

Refactor chain following #2750

Conversation

Anviking commented Jul 1, 2021 • edited by piotr-iohk Loading

Issue Number

Motivation

Overview

Progress

Comments

rvl left a 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 commented Aug 10, 2021

rvl commented Aug 14, 2021

HeinrichApfelmus commented Oct 26, 2021

iohk-bors bot commented Oct 26, 2021

try

HeinrichApfelmus commented Oct 26, 2021

Anviking left a comment • 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

piotr-iohk commented Oct 27, 2021 • edited Loading

HeinrichApfelmus commented Oct 29, 2021

piotr-iohk commented Oct 29, 2021

HeinrichApfelmus commented Nov 1, 2021

Anviking left a comment

Choose a reason for hiding this comment

Anviking Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-iohk left a comment

Choose a reason for hiding this comment

piotr-iohk commented Nov 4, 2021

iohk-bors bot commented Nov 4, 2021

Anviking commented Jul 1, 2021 •

edited by piotr-iohk

Loading

Anviking left a comment •

edited

Loading

piotr-iohk commented Oct 27, 2021 •

edited

Loading

Anviking Nov 3, 2021 •

edited

Loading