[CAD-3723] Add progress to initial chain selection #3518

EncodePanda · 2021-12-01T15:29:23Z

No description provided.

jasagredo

Looks super to me. 👌

ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/InMemory.hs

dnadales

I left some comments. We can have a call to discuss them.

ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/InMemory.hs

dnadales · 2021-12-01T19:57:51Z

ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/InMemory.hs


 -- | Switch to a fork
 ledgerDbSwitch :: (ApplyBlock l blk, Monad m, c)
               => LedgerDbCfg l
               -> Word64          -- ^ How many blocks to roll back
+               -> (UpdateLedgerDbTraceEvent blk -> m ())


In principle, this could be any action. I wonder if we want to be more specific and declare that this should actually be a tracer.

Good question, but at least debatable. Even though I follow your line of thought and you are right, in principle this could be any hook but:
a) this could be a slipper slope to messy code in a future, because it is so easy to just "add one more thing" to a hook.
b) I've followed existing convention in our code were we pass trace functions as arguments

data ChainSelEnv m blk = ChainSelEnv { lgrDB :: LgrDB m blk , trace :: TraceValidationEvent blk -> m ()

ChainSelEnv is using trace and that pollutes our design. I think I will change that to provide a tracer instead but in a follow up PR.

I think it should be Tracer. Tracer is isomorphic to that type, and transparent, so there's no restriction gained by using either type. But Tracer is the tell-tale type throughout our codebase for this kind of concern.

@nfrisby are you ok with doing this in separate PR so this one does not get polluted with too many concerns?

I think for the the new field it should just be Tracer, regardless of new code, but yeah no need to change existing code in this PR.

Edit: eg you could use nullTracer instead of const $ pure () etc. Tracer is the right API for this sort of thing.

Those events tracks changes done to LedgerDB each time block is added/applied to it. User gets notiftied twice: once the block is about to be applied and the moment after it got applied

Also remove PushedBlockToTheLedgerDb

While pushing blocks to ledger we can trace progress. This means that the cardano-node can present that progress to the user of the node, possibly in the for of progress bar or printing out percentage

ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/InMemory.hs

EncodePanda · 2021-12-03T15:54:53Z

@nfrisby last commit (5c59ed7), is that what you were thinking of?

ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/Types.hs

dnadales · 2021-12-03T16:00:34Z

ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/InMemory.hs

-      res <- ledgerDbPush cfg ap db
-      return res
+    pushAndTrace ap db = do
+      traceStep $ StartedPushingBlockToTheLedgerDb (Pushing $ toRealPoint ap)


What about something more direct (with the use of auxiliary mk functions)?

trace $ tartedPushingBlockToTheLedgerDb $ (mkPushing ap ) (mkPushGoal aps)

dnadales · 2021-12-03T16:02:02Z

ouroboros-consensus-test/test-consensus/Test/Consensus/MiniProtocol/LocalStateQuery/Server.hs

@@ -208,6 +208,9 @@ initLgrDB k chain = do

    genesisLedgerDB = LgrDB.ledgerDbWithAnchor testInitExtLedger

+    noopTrace :: blk -> m ()


We have other uses of const $ pure (). Don't we want to replace these with noopTrace? Or just use const $ pure ()

hmm.. good question, i think it's better if we replace with noop helper function, otherwise someone who will read the code will have to check each time to understand "wtf is const pure unit"

dnadales

I left some comments. LGTM.

dnadales · 2021-12-06T15:31:23Z

ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/Types.hs

+      StartedPushingBlockToTheLedgerDb
+        !(Pushing blk)
+        -- ^ Point which block we are about to push
+        (PushGoal blk)


Is there a ! missing here?

EncodePanda · 2021-12-06T16:25:32Z

bors merge

iohk-bors · 2021-12-06T16:53:57Z

Build succeeded:

nfrisby · 2021-12-07T16:46:06Z

ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/OnDisk.hs

@@ -323,6 +325,7 @@ initStartingWith tracer cfg streamAPI initDb = do
                       (ledgerState (ledgerDbCurrent db))
                       (ledgerState (ledgerDbCurrent db'))

+


Style: undue newline

nfrisby · 2021-12-07T16:47:04Z

ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/OnDisk.hs

@@ -533,4 +535,5 @@ data TraceReplayEvent blk replayTo
    -- The @blockInfo@ parameter corresponds replayed block and the @replayTo@
    -- parameter corresponds to the block at the tip of the ImmutableDB, i.e.,
    -- the last block to replay.
+  | UpdateLedgerDbTraceEvent (UpdateLedgerDbTraceEvent blk)


Please add a Haddock comment on this one, describing the general meaning of the UpdateLedgerDbTraceEvent type/category.

nfrisby · 2021-12-07T16:48:21Z

ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/InMemory.hs

+  where
+    pushAndTrace ap db = do
+      let pushing = Pushing . toRealPoint $ ap
+          goal    = PushGoal . toRealPoint . last $ aps


I'd prefer to not call partial functions correctly. So write a case expression with a nice error message instead of calling last.

Edit: Or, depending on the call site, I wouldn't be too surprised if it were possible to change the type from [] to NonEmpty

nfrisby · 2021-12-07T16:50:17Z

Bah --- I just realized this was already Merged :)

EncodePanda requested review from dnadales, nfrisby and jasagredo December 1, 2021 15:29

jasagredo approved these changes Dec 1, 2021

View reviewed changes

ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/InMemory.hs Outdated Show resolved Hide resolved

dnadales reviewed Dec 1, 2021

View reviewed changes

EncodePanda force-pushed the EncodePanda/CAD-3723-init-chain-selec-events branch from f1e6637 to f50db87 Compare December 2, 2021 09:44

EncodePanda added 4 commits December 2, 2021 14:38

Move dedicated initChainSelTracer tracer to initialChainSelection

b3902e5

Added StartedInitChainSelection and InitialChainSelected

8be9a23

Add UpdateLedgerDbTraceEvent events

eee455c

Those events tracks changes done to LedgerDB each time block is added/applied to it. User gets notiftied twice: once the block is about to be applied and the moment after it got applied

Add RealPoint to the StartedPushingBlockToTheLedgerDb

d2f234d

Also remove PushedBlockToTheLedgerDb

EncodePanda force-pushed the EncodePanda/CAD-3723-init-chain-selec-events branch from d76ef34 to d2f234d Compare December 2, 2021 13:38

Add progress to UpdateLedgerDbTraceEvent

5c59ed7

While pushing blocks to ledger we can trace progress. This means that the cardano-node can present that progress to the user of the node, possibly in the for of progress bar or printing out percentage

dnadales reviewed Dec 3, 2021

View reviewed changes

ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/InMemory.hs Show resolved Hide resolved

dnadales reviewed Dec 3, 2021

View reviewed changes

ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/Types.hs Outdated Show resolved Hide resolved

dnadales reviewed Dec 3, 2021

View reviewed changes

dnadales approved these changes Dec 3, 2021

View reviewed changes

EncodePanda added 2 commits December 6, 2021 13:41

Capitalize comments

68b9a6c

Simplify the pushAndTrace

4a5633a

EncodePanda force-pushed the EncodePanda/CAD-3723-init-chain-selec-events branch from b67a2e4 to 4a5633a Compare December 6, 2021 12:57

dnadales reviewed Dec 6, 2021

View reviewed changes

iohk-bors bot merged commit cbd883f into master Dec 6, 2021

iohk-bors bot deleted the EncodePanda/CAD-3723-init-chain-selec-events branch December 6, 2021 16:54

nfrisby reviewed Dec 7, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CAD-3723] Add progress to initial chain selection #3518

[CAD-3723] Add progress to initial chain selection #3518

EncodePanda commented Dec 1, 2021

jasagredo left a comment

dnadales left a comment

dnadales Dec 1, 2021

EncodePanda Dec 2, 2021

EncodePanda Dec 2, 2021

nfrisby Dec 2, 2021

EncodePanda Dec 3, 2021 •

edited

Loading

nfrisby Dec 7, 2021 •

edited

Loading

EncodePanda commented Dec 3, 2021

dnadales Dec 3, 2021

EncodePanda Dec 3, 2021

dnadales Dec 3, 2021

EncodePanda Dec 3, 2021

dnadales left a comment

dnadales Dec 6, 2021

EncodePanda commented Dec 6, 2021

iohk-bors bot commented Dec 6, 2021

nfrisby Dec 7, 2021

nfrisby Dec 7, 2021

nfrisby Dec 7, 2021 •

edited

Loading

nfrisby commented Dec 7, 2021

		@@ -208,6 +208,9 @@ initLgrDB k chain = do

		genesisLedgerDB = LgrDB.ledgerDbWithAnchor testInitExtLedger

		noopTrace :: blk -> m ()

		@@ -323,6 +325,7 @@ initStartingWith tracer cfg streamAPI initDb = do
		(ledgerState (ledgerDbCurrent db))
		(ledgerState (ledgerDbCurrent db'))

[CAD-3723] Add progress to initial chain selection #3518

[CAD-3723] Add progress to initial chain selection #3518

Conversation

EncodePanda commented Dec 1, 2021

jasagredo left a comment

Choose a reason for hiding this comment

dnadales 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

EncodePanda Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

nfrisby Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

EncodePanda commented Dec 3, 2021

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

dnadales left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EncodePanda commented Dec 6, 2021

iohk-bors bot commented Dec 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfrisby Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

nfrisby commented Dec 7, 2021

EncodePanda Dec 3, 2021 •

edited

Loading

nfrisby Dec 7, 2021 •

edited

Loading

nfrisby Dec 7, 2021 •

edited

Loading