Adding ticking functionality for block polling #35

paweljakubas · 2019-03-06T15:46:18Z

Issue Number

Overview

I have added bracket with ticking function that takes block logic, action taken when blocks are received and tick time
I added tests that tests ticking function

Comments

KtorZ

I think you were a bit off track here and went for more than what was needed. It's important to separate concerns in your implementation and make sure that code isn't needlessly coupled to some other code.

It would have been nice to get a PR much sooner actually to prevent this; as per our new development process, we want to keep on at least one PR a day so make sure to break a task down to iterations that are achievable within a day. Mocking isn't usually a good idea, but inversion of control may help a lot.

Also, make sure to go through and acknowledge the coding standards once again. It's important for us all to be consistent on the practices.

KtorZ · 2019-03-06T18:16:09Z

src/Cardano/Wallet/BlockSyncer.hs

+      getBlock      :: (Hash "BlockHeader") -> IO Block
+    , getEpoch      :: Word64 -> IO [Block]
+    , getNetworkTip :: IO ((Hash "BlockHeader"),BlockHeader)
+    }


This, and the implementation below of the mockup are only for testing I presume 🤔 ? I wouldn't expect this to be needed for writing and testing the ticking function. Cf next comment.

it was supposed to act like that. in the light of the refactoring it is not used anymore

KtorZ · 2019-03-06T18:17:56Z

src/Cardano/Wallet/BlockSyncer.hs

+    :: Int
+    -> [((Hash "BlockHeader"),Block)]
+    -> Int
+    -> IO BlockLayerMockup


General remark about function API, but primitive types like Int or String communicate badly about their usage. It's always a good idea to add a comment explaining what the parameter is for. It's usually an even better idea to create a proper type to encapsulate that semantic.

KtorZ · 2019-03-06T18:18:50Z

src/Cardano/Wallet/BlockSyncer.hs

+blockSyncerBracket blocksPersisted blockLayer period =
+    bracket
+      (return ()) --(putStrLn "setting up for ticking function")
+      (\_->return ()) --(\_ -> putStrLn "cleaning for ticking function")


Commented-out code shouldn't be pushed / committed (unless it serves as an example for documentation), which isn't the case here 👍

I put it like that for the sake of future logging.

Don't :)
If it's not needed, don't do it. We favor simple code and refactor when we need to add functionalities 👍

KtorZ · 2019-03-06T18:19:16Z

src/Cardano/Wallet/BlockSyncer.hs

+                                         missingHashes <- retrieveMissingBlocks [previousHeaderHash]
+                                                          headerHashesConsumed
+                                                          blockLayer
+                                         pure $ currentHeaderHash : (missingHashes ++ headerHashesConsumed)


Make sure to follow the coding standards on variable-length indentation 🙏

KtorZ · 2019-03-06T18:19:34Z

src/Cardano/Wallet/BlockSyncer.hs

+    :: IORef BlocksConsumed
+    -> BlockLayerMockup
+    -> Int
+    -> ()


This is very dubious 😶

I still wonder why have this unit argument here?

KtorZ · 2019-03-06T18:39:31Z

src/Cardano/Wallet/BlockSyncer.hs

+                                         pure $ currentHeaderHash : (missingHashes ++ headerHashesConsumed)
+          writeIORef initialState $ BlocksConsumed updatedHeaderHashes
+          threadDelay (delayPeriod*1000*1000)
+          go previousState


Okay, so here's the thing. This function seems a bit too explicit and conflates several responsabilities into one. As a rule of thumb, function should do one and one thing, and try not to work at different abstraction level. If we step back a little and re-think a bit the requirements / design of such function.

It's about ticking / polling a source of blocks and, proceed with some action when a new block is identified.

This means that we need:

A way to poll a source and get the latest block from a source

An action to execute when a new block is received

We want to leave it to the caller to define such behavior, and the ticking function should just bother
about keeping track of that latest block and calling an action when a new one is discovered.
How blocks are fetched and what action is executed matters not; So we can inject the corresponding
behaviors from the arguments:

tickingFunction :: IO Block -> (Block -> IO ()) -> IO () tickingFunction getNextBlock action = do -- ...

Then, the implementation shouldn't do much more than keeping track of the
previous block fetched, and regularly polling the chain to fetch the one,
calling the action whenever the block fetched is different from the latest
block fetched.

There's no particular need for mocking here, because the behavior of how you get blocks doesn't matter
for the ticking function; this incidentally makes testing way easier because you get to choose that
behavior in your testing code!

complied. Thanks for detailed directions

KtorZ · 2019-03-06T18:39:58Z

src/Cardano/Wallet/BlockSyncer.hs

+    -> [(Hash "BlockHeader")]
+    -> BlockLayerMockup
+    -> IO [(Hash "BlockHeader")]
+retrieveMissingBlocks missing headerHashesConsumed layer = do


This is out of scope for this task.

yes, I removed that

KtorZ · 2019-03-06T18:41:01Z

test/unit/Cardano/Wallet/BlockSyncerSpec.hs

+import Cardano.Wallet.BlockSyncer
+    ( BlocksConsumed (..), blockSyncerBracket, runBlockLayerMockup )
+import Cardano.Wallet.Primitive
+    ( Block (..), BlockHeader (..), Hash (..) )


Make sure to follow the coding standards for imports 🙏 (two groups, qualified vs explicit imports. We do not make any more groups based on whether the code comes from our code base or not).

KtorZ · 2019-03-06T18:41:56Z

test/unit/Cardano/Wallet/BlockSyncerSpec.hs

+        testBlocksPolling
+            :: Int
+            -> Int
+            -> Int


In such case, think about using a record type to make the intent clear about what the parameters are.

KtorZ · 2019-03-06T18:43:34Z

test/unit/Cardano/Wallet/BlockSyncerSpec.hs

+            threadDelay (testTimeEvalInSec*1000*1000)
+            killThread threadID
+            (BlocksConsumed finalHeaderHashes) <- readIORef initialHeaderData
+            finalHeaderHashes `shouldBe` (map fst consecutiveBlocks)


Also, this function would deserve to be split in multiple ones that each encapsulate a particular responsibility. It would make the organization of the code a bit clearer between how the test is being set up, what the test action is and what are the expectations.

I will do it in the final commit

paweljakubas · 2019-03-07T15:45:15Z

test/unit/Cardano/Wallet/BlockSyncerSpec.hs

+            (BlocksConsumed consumedData) <- readIORef consumerData
+
+            consumedData `shouldBe` ((map fst . reverse) consecutiveBlocks)
+


there is further refactoring under way

KtorZ · 2019-03-07T15:59:36Z

src/Cardano/Wallet/BlockSyncer.hs

+    bracket
+      (return ())
+      (\_->return ())
+      (tickingFunction getNextBlocks action tickTime)


So, this is the kind of "not needed yet abstractions" I am often referring too. The bracket doesn't buy us anything here, and there's no particular need to have such bracket. Maybe later we'll have to get some resources before calling the ticking function, but that's later. As for now, we just want to have a ticking function and test it.

removed bracket as requested

KtorZ · 2019-03-07T16:00:42Z

src/Cardano/Wallet/BlockSyncer.hs

+tickingFunction
+    :: IO [Block]
+    -> (Block -> IO ())
+    -> Int


Would be better to get a better type here which convey a clear meaning -- or... not having any at all. The time can just be a hard-coded constant on top of the module, if we ever need to change that, we'll make it a parameter. But again, for now, this parameter isn't needed or required.

went with Second from time-units

KtorZ · 2019-03-07T16:02:24Z

src/Cardano/Wallet/BlockSyncer.hs

+    :: IORef BlocksConsumed
+    -> BlockLayerMockup
+    -> Int
+    -> ()


I still wonder why have this unit argument here?

KtorZ · 2019-03-07T16:04:52Z

src/Cardano/Wallet/BlockSyncer.hs

+    (\(e :: SomeException) ->
+            let msg = "Terminating tickingFunction of BlockSyncer due to " % F.shown
+            in (putStrLn . showText) $ sformat msg e
+    )


That is rather problematic to handle the error here in such way.. If the ticking function dies... as for now, we probably wants the wallet to die too. That's a fatal error and here, we'll just be showing a message in the console and silently fade away. The wallet server will keep running and... nothing will happen until we eventually notice that there's no block getting fetched anymore.

So, until further needs, let just do nothing and let the exception bubbles to the caller.

removed and also formatting dependency

src/Cardano/Wallet/BlockSyncer.hs

KtorZ · 2019-03-07T16:13:49Z

test/unit/Cardano/Wallet/BlockSyncerSpec.hs

+
+            (BlocksConsumed consumedData) <- readIORef consumerData
+
+            consumedData `shouldBe` ((map fst . reverse) consecutiveBlocks)


It is rather unclear to me what the tests are testing here 🤔 ? A comment or a longer scenario would definitely helps. ASCII diagrams are also very welcome.

I wanted to check first ExactlyOnce delivery. When the ticking function gained not-consuming-the-same-block-guarrantee I added also AtLeastOnce delivery test

KtorZ · 2019-03-08T09:23:29Z

src/Cardano/Wallet/BlockSyncer.hs

+newtype BlockHeadersConsumed = BlockHeadersConsumed [BlockHeader] deriving (Show, Eq)
+
+storingLimit :: Int
+storingLimit = 2140


Did you mean 2160 🤔 ?

yeah, when we later tackle rollback security parameter comes here (I thought about it but made blunder here putting 2140 instead of 2160). For the time being I wanted to just add some upper bound just in order to not surpass memory limit.

paweljakubas · 2019-03-08T09:33:47Z

cardano-wallet.cabal

@@ -37,13 +37,16 @@ library
    , cborg
    , containers
    , deepseq
+    , text


not needed - will remove in the final commit

paweljakubas · 2019-03-08T13:23:55Z

problems with tests are because of IORef use. I will refactor the code with MVar

KtorZ · 2019-03-08T17:43:56Z

src/Cardano/Wallet/BlockSyncer.hs

+          -> Block
+          -> Bool
+      checkIfAlreadyConsumed consumedHeaders (Block theHeader _) =
+          theHeader `L.notElem` consumedHeaders


Okay, this looks good now. If I may throw a naming suggestion:

~~BlockHeaderConsumed~~ ---> Known with a type parameter, BlockHeader is rather unnecessary here, it's already in the type: Known [BlockHeader]

~~checkIfAlreadyConsumed~~ ---> isKnown, less prose is good :)

~~tickTime~~ --> delay, well, that's what it is in the end.

~~blocksDownloaded~~ --> blocks, again, no need to be extra verbose about this. By keeping your function small, the context is enough to understand this. And, in the end, it doesn't really matter if blocks have been downloaded, or fetched or unserialized .. Unless it helps disambiguate things, go for a short name.

~~blocksToProcess~~ --> block' or unknownBlock, just to be aligned with the above suggestions

~~consumedHeaders~~ --> knownHeaders, also, to be aligned

~~theHeader~~ --> h is probably enough because the function is really short, or, do not pattern match and go for header block or header b

Short names are usually good. Too short is sometimes not very helpful, though it make sense when working on abstractions (like in filter :: (a -> Bool) -> [a] -> [a], one wouldn't write filter :: (theElement -> Bool) -> theList -> theList).

KtorZ · 2019-03-08T17:53:29Z

test/unit/Cardano/Wallet/BlockSyncerSpec.hs

+                -> Gen [Int]
+            generateBlockChunks numberOfTicks = do
+                let chunkSizeGen = choose (0,15)
+                vectorOf numberOfTicks chunkSizeGen


It would have been nice to have a chat about the approach on testing for this. I think you've got the idea, but the implementation of the tests has gone slightly too complex IMO. I'll make a suggestion a bit later but it would be nice to go through this together.

jonathanknowles · 2019-03-11T06:46:18Z

src/Cardano/Wallet/BlockSyncer.hs

+--
+-- This module contains the ticking function that is responsible for invoking block acquisition functionality and executing it in periodic fashion.
+--
+-- Known limitations: ticking function makes sure action is not executed on already consumed block, but does not check and handle block gaps (aka catching up)


Hi there! We'd need to wrap this to bring the above comment in line with https://github.com/input-output-hk/cardano-wallet/wiki/Coding-Standards#limit-line-length-to-80-characters

Perhaps like this:

-- This module contains the ticking function that is responsible for invoking -- block acquisition functionality and executing it in periodic fashion. -- -- Known limitations: the ticking function makes sure action is not executed on -- already consumed block, but does not check and handle block gaps (aka -- catching up).

jonathanknowles · 2019-03-11T06:47:32Z

src/Cardano/Wallet/BlockSyncer.hs

+import qualified Data.List as L
+
+
+newtype BlockHeadersConsumed = BlockHeadersConsumed [BlockHeader] deriving (Show, Eq)


https://github.com/input-output-hk/cardano-wallet/wiki/Coding-Standards#limit-line-length-to-80-characters

Propose:

newtype BlockHeadersConsumed = BlockHeadersConsumed [BlockHeader] deriving (Show, Eq)

[3] Adding ticking function test and downloading block logic [3] Block syncer working ok plus additional tests [3] Cleaning the code [3] cabal fix [3] Complete refactoring to the review [3] Final refactoring [3] cabal fix [3] hlint and weeding [3] add killing thread at the end of test [3] Replace IORef with MVar refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints refactoring #2 | review indentation of 'where' clause refactoring #3 | inline loop and remove test intrumentation from test logic refactoring #4 | define generator for ticking args in a declarative manner refactoring #5 | purify tickingFunctionTest and make it a monadic property refactoring #6 | review naming in Arbitrary TickingArgs refactoring #7 | use guards in mkConsecutiveTestBlocks refactoring #8 | define single block generator from previous block refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function refactoring #11 | switch argument positions in mkConsecutiveTestBlocks refactoring #12 | replace loop with built-in list 'iterate' refactoring #13 | define Arbitrary instance for creating consecutive blocks refactoring #14 | replace mkConsecutiveBlocks with a property parameter refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar' refactoring #16 | remove unecessary IO in 'writeToIORefAction' refactoring #17 | replace takeMVar + putMVar with modifyMVar refactoring #18 | review naming for 'writeToIORefAction' --> 'reader' refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match refactoring #20 | remove 'BlocksConsumed' wrapper refactoring #21 | generalize reader with polymorphic parametrism refactoring #22 | review pushNextBlocks indentation refactoring #23 | group case pattern matches using tuple refactoring #24 | remove 'Hash BlockHeader' from the block to inject refactoring #25 | use synchronization lock instead of computed times refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks' refactoring #27 | remove 'chunkSizes' in a favor of inline random selection refactoring #28 | remove 'DeliveryMode' in favor of the most general case refactoring #29 | cleanup wrapper types refactoring #30 | generalize pushNextBlocks with parametric polymorphism refactoring #31 | rename pushNextBlocks to 'writer' refactoring #32 | define reader on Block instead of BlockHeaderHash refactoring #33 | replace old reader with reader' refactoring #34 | move creation of writer MVar inside writer action refactoring #35 | remove header hash from 'Blocks' refactoring #36 | rename 'consecutiveBlocks' into 'blocks' refactoring #37 | re-organize module to separate effectful logic from declarations refactoring #38 | move waiting logic into dedicated function refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer refactoring #40 | use Millisecond instead of Second for shorter tests refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks' [3] fix line width [3] aligning the code with other code changes

paweljakubas · 2019-03-11T11:18:03Z

rebased and squashed

paweljakubas · 2019-03-11T11:35:06Z

it seems we have :

5  {-# OPTIONS_GHC -fno-warn-unused-imports #-}                                                                                                                                               
6                                                                                                                                                                                             
7  module Cardano.Wallet.BlockSyncerSpec                                                                                                                                                      
8      ( spec                                                                                                                                                                                 
9      , groups                                                                                                                                                                               
10     , duplicateMaybe                                                                                                                                                                       
11     ) where

and weeder still keeps complaining about groups and duplicateMaybe not used outside module

[3] Adding ticking function test and downloading block logic [3] Block syncer working ok plus additional tests [3] Cleaning the code [3] cabal fix [3] Complete refactoring to the review [3] Final refactoring [3] cabal fix [3] hlint and weeding [3] add killing thread at the end of test [3] Replace IORef with MVar refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints refactoring #2 | review indentation of 'where' clause refactoring #3 | inline loop and remove test intrumentation from test logic refactoring #4 | define generator for ticking args in a declarative manner refactoring #5 | purify tickingFunctionTest and make it a monadic property refactoring #6 | review naming in Arbitrary TickingArgs refactoring #7 | use guards in mkConsecutiveTestBlocks refactoring #8 | define single block generator from previous block refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function refactoring #11 | switch argument positions in mkConsecutiveTestBlocks refactoring #12 | replace loop with built-in list 'iterate' refactoring #13 | define Arbitrary instance for creating consecutive blocks refactoring #14 | replace mkConsecutiveBlocks with a property parameter refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar' refactoring #16 | remove unecessary IO in 'writeToIORefAction' refactoring #17 | replace takeMVar + putMVar with modifyMVar refactoring #18 | review naming for 'writeToIORefAction' --> 'reader' refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match refactoring #20 | remove 'BlocksConsumed' wrapper refactoring #21 | generalize reader with polymorphic parametrism refactoring #22 | review pushNextBlocks indentation refactoring #23 | group case pattern matches using tuple refactoring #24 | remove 'Hash BlockHeader' from the block to inject refactoring #25 | use synchronization lock instead of computed times refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks' refactoring #27 | remove 'chunkSizes' in a favor of inline random selection refactoring #28 | remove 'DeliveryMode' in favor of the most general case refactoring #29 | cleanup wrapper types refactoring #30 | generalize pushNextBlocks with parametric polymorphism refactoring #31 | rename pushNextBlocks to 'writer' refactoring #32 | define reader on Block instead of BlockHeaderHash refactoring #33 | replace old reader with reader' refactoring #34 | move creation of writer MVar inside writer action refactoring #35 | remove header hash from 'Blocks' refactoring #36 | rename 'consecutiveBlocks' into 'blocks' refactoring #37 | re-organize module to separate effectful logic from declarations refactoring #38 | move waiting logic into dedicated function refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer refactoring #40 | use Millisecond instead of Second for shorter tests refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks' [3] fix line width [3] aligning the code with other code changes [3] hlint suggestion

KtorZ · 2019-03-11T12:21:24Z

@paweljakubas Whoops. Sorry about the pragma, I typically do that when developing to iterate quickly and I clean up the imports before making a PR. This should be removed, and imports cleaned up as well 👍

paweljakubas · 2019-03-11T12:27:36Z

ahh, I have already added proper .weeder.yaml being the product of:

[pawel@arch cardano-wallet]$ weeder . --yaml > .weeder.yaml
[pawel@arch cardano-wallet]$ cat .weeder.yaml
- package:
  - name: cardano-wallet
  - section:
    - name: test:unit
    - message:
      - name: Weeds exported
      - module:
        - name: Cardano.Wallet.BlockSyncerSpec
        - identifier:
          - duplicateMaybe
          - groups

We do not want it, right? Just remove pragma and superfluous exports?

[3] Adding ticking function test and downloading block logic [3] Block syncer working ok plus additional tests [3] Cleaning the code [3] cabal fix [3] Complete refactoring to the review [3] Final refactoring [3] cabal fix [3] hlint and weeding [3] add killing thread at the end of test [3] Replace IORef with MVar refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints refactoring #2 | review indentation of 'where' clause refactoring #3 | inline loop and remove test intrumentation from test logic refactoring #4 | define generator for ticking args in a declarative manner refactoring #5 | purify tickingFunctionTest and make it a monadic property refactoring #6 | review naming in Arbitrary TickingArgs refactoring #7 | use guards in mkConsecutiveTestBlocks refactoring #8 | define single block generator from previous block refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function refactoring #11 | switch argument positions in mkConsecutiveTestBlocks refactoring #12 | replace loop with built-in list 'iterate' refactoring #13 | define Arbitrary instance for creating consecutive blocks refactoring #14 | replace mkConsecutiveBlocks with a property parameter refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar' refactoring #16 | remove unecessary IO in 'writeToIORefAction' refactoring #17 | replace takeMVar + putMVar with modifyMVar refactoring #18 | review naming for 'writeToIORefAction' --> 'reader' refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match refactoring #20 | remove 'BlocksConsumed' wrapper refactoring #21 | generalize reader with polymorphic parametrism refactoring #22 | review pushNextBlocks indentation refactoring #23 | group case pattern matches using tuple refactoring #24 | remove 'Hash BlockHeader' from the block to inject refactoring #25 | use synchronization lock instead of computed times refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks' refactoring #27 | remove 'chunkSizes' in a favor of inline random selection refactoring #28 | remove 'DeliveryMode' in favor of the most general case refactoring #29 | cleanup wrapper types refactoring #30 | generalize pushNextBlocks with parametric polymorphism refactoring #31 | rename pushNextBlocks to 'writer' refactoring #32 | define reader on Block instead of BlockHeaderHash refactoring #33 | replace old reader with reader' refactoring #34 | move creation of writer MVar inside writer action refactoring #35 | remove header hash from 'Blocks' refactoring #36 | rename 'consecutiveBlocks' into 'blocks' refactoring #37 | re-organize module to separate effectful logic from declarations refactoring #38 | move waiting logic into dedicated function refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer refactoring #40 | use Millisecond instead of Second for shorter tests refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks' [3] fix line width [3] aligning the code with other code changes [3] hlint suggestion [3] add .weeder.yaml to omit duplicateMaybes and groups to be announce during weeder execution [3] undo weeder ignore file plus remove pragma and unnecessary exports

[3] Adding ticking function test and downloading block logic [3] Block syncer working ok plus additional tests [3] Cleaning the code [3] cabal fix [3] Complete refactoring to the review [3] Final refactoring [3] cabal fix [3] hlint and weeding [3] add killing thread at the end of test [3] Replace IORef with MVar refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints refactoring #2 | review indentation of 'where' clause refactoring #3 | inline loop and remove test intrumentation from test logic refactoring #4 | define generator for ticking args in a declarative manner refactoring #5 | purify tickingFunctionTest and make it a monadic property refactoring #6 | review naming in Arbitrary TickingArgs refactoring #7 | use guards in mkConsecutiveTestBlocks refactoring #8 | define single block generator from previous block refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function refactoring #11 | switch argument positions in mkConsecutiveTestBlocks refactoring #12 | replace loop with built-in list 'iterate' refactoring #13 | define Arbitrary instance for creating consecutive blocks refactoring #14 | replace mkConsecutiveBlocks with a property parameter refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar' refactoring #16 | remove unecessary IO in 'writeToIORefAction' refactoring #17 | replace takeMVar + putMVar with modifyMVar refactoring #18 | review naming for 'writeToIORefAction' --> 'reader' refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match refactoring #20 | remove 'BlocksConsumed' wrapper refactoring #21 | generalize reader with polymorphic parametrism refactoring #22 | review pushNextBlocks indentation refactoring #23 | group case pattern matches using tuple refactoring #24 | remove 'Hash BlockHeader' from the block to inject refactoring #25 | use synchronization lock instead of computed times refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks' refactoring #27 | remove 'chunkSizes' in a favor of inline random selection refactoring #28 | remove 'DeliveryMode' in favor of the most general case refactoring #29 | cleanup wrapper types refactoring #30 | generalize pushNextBlocks with parametric polymorphism refactoring #31 | rename pushNextBlocks to 'writer' refactoring #32 | define reader on Block instead of BlockHeaderHash refactoring #33 | replace old reader with reader' refactoring #34 | move creation of writer MVar inside writer action refactoring #35 | remove header hash from 'Blocks' refactoring #36 | rename 'consecutiveBlocks' into 'blocks' refactoring #37 | re-organize module to separate effectful logic from declarations refactoring #38 | move waiting logic into dedicated function refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer refactoring #40 | use Millisecond instead of Second for shorter tests refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks' [3] fix line width [3] aligning the code with other code changes [3] hlint suggestion [3] add .weeder.yaml to omit duplicateMaybes and groups to be announce during weeder execution [3] undo weeder ignore file plus remove pragma and unnecessary exports [3] remove not needed imports

paweljakubas requested review from jonathanknowles, Anviking, rvl, akegalj and KtorZ March 6, 2019 15:46

KtorZ suggested changes Mar 6, 2019

View reviewed changes

KtorZ assigned paweljakubas Mar 7, 2019

paweljakubas force-pushed the paweljakubas/3/ticking-function branch from 2bc23be to 84ad4f5 Compare March 7, 2019 15:41

paweljakubas commented Mar 7, 2019

View reviewed changes

KtorZ suggested changes Mar 7, 2019

View reviewed changes

paweljakubas force-pushed the paweljakubas/3/ticking-function branch from 84ad4f5 to 26ba984 Compare March 8, 2019 09:21

KtorZ reviewed Mar 8, 2019

View reviewed changes

paweljakubas commented Mar 8, 2019

View reviewed changes

paweljakubas force-pushed the paweljakubas/3/ticking-function branch from 26ba984 to c065308 Compare March 8, 2019 11:58

piotr-iohk mentioned this pull request Mar 8, 2019

Ticking function to retrieve current tip and get next block (no handling of rollbacks) #3

Closed

3 tasks

paweljakubas force-pushed the paweljakubas/3/ticking-function branch from c065308 to bc9da3b Compare March 8, 2019 13:56

paweljakubas requested a review from KtorZ March 8, 2019 14:41

KtorZ suggested changes Mar 8, 2019

View reviewed changes

jonathanknowles reviewed Mar 11, 2019

View reviewed changes

KtorZ added a commit that referenced this pull request Mar 11, 2019

refactoring #35 | remove header hash from 'Blocks'

da229b9

paweljakubas force-pushed the paweljakubas/3/ticking-function branch from 600f3cf to 69e2083 Compare March 11, 2019 11:17

paweljakubas force-pushed the paweljakubas/3/ticking-function branch from 69e2083 to a423b02 Compare March 11, 2019 11:54

paweljakubas force-pushed the paweljakubas/3/ticking-function branch from 60b16ce to 33cd68f Compare March 11, 2019 12:41

paweljakubas force-pushed the paweljakubas/3/ticking-function branch from 33cd68f to a2f3c36 Compare March 11, 2019 13:08

paweljakubas requested a review from KtorZ March 11, 2019 13:27

KtorZ approved these changes Mar 11, 2019

View reviewed changes

KtorZ merged commit 0722b47 into master Mar 11, 2019

KtorZ deleted the paweljakubas/3/ticking-function branch March 13, 2019 18:29

whosystem mentioned this pull request Mar 24, 2021

cabal build all uses the -msse2 FLAG = Compiling on arm64 - how to reconfigure #2578

Closed

		(BlocksConsumed consumedData) <- readIORef consumerData

		consumedData `shouldBe` ((map fst . reverse) consecutiveBlocks)

		import qualified Data.List as L


		newtype BlockHeadersConsumed = BlockHeadersConsumed [BlockHeader] deriving (Show, Eq)

Adding ticking functionality for block polling #35

Adding ticking functionality for block polling #35

Conversation

paweljakubas commented Mar 6, 2019 • edited Loading

Issue Number

Overview

Comments

KtorZ 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

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

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

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

paweljakubas Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

paweljakubas Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

paweljakubas commented Mar 8, 2019

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

jonathanknowles Mar 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Mar 11, 2019

paweljakubas commented Mar 11, 2019 • edited Loading

KtorZ commented Mar 11, 2019

paweljakubas commented Mar 11, 2019

paweljakubas commented Mar 6, 2019 •

edited

Loading

paweljakubas Mar 8, 2019 •

edited

Loading

paweljakubas Mar 8, 2019 •

edited

Loading

paweljakubas Mar 8, 2019 •

edited

Loading

jonathanknowles Mar 11, 2019 •

edited

Loading

paweljakubas commented Mar 11, 2019 •

edited

Loading