Better Point type factoring, again #706

avieth · 2019-07-04T00:08:55Z

Intended to replace #635 and #690 .

It's factored into 2 commits that could be reviewed separately (the third one is a tiny unrelated fix to byron-proxy).

The first commit introduces the Ouroboros.Network.Point module and uses it throughout ouroboros-network, making it build and tests pass.

The second makes ouroboros-consensus build and tests pass.

This commit retains the mistake pointHash :: Point block -> SlotNo rather than fixing it to be pointHash :: Point block -> WithOrigin SlotNo. The origin point is given SlotNo 0. It's kept this way so that the pull request will remain short and people will be willing to review it.

The ouroboros-network library builds and tests pass. This does not fix the pointSlot mistake: it is still defined, and gives SlotNo 0 to the origin. This will be fixed in a forthcoming commit.

This package builds and tests pass. One perhaps controversial change is in the Byron ledger definition of ledgerTipPoint. If the ledger state has the genesis hash as its latest, then there are no blocks, so the tip point is set to Origin, rather than a point with slot number 0. I believe this is the how it ought to be.

Apparently an update to iohk-monitoring broke it. But how did that get past CI?

avieth · 2019-07-04T00:09:31Z

byron-proxy/src/exec/Logging.hs

@@ -81,6 +81,8 @@ defaultLoggerConfig = Monitoring.Representation
  , Monitoring.hasPrometheus   = Nothing
  , Monitoring.hasGUI          = Nothing
  , Monitoring.options         = mempty
+  , Monitoring.hasGraylog      = Nothing
+  , Monitoring.logOutput       = Nothing


Apparently a new build of iohk-monitoring has been brought in recently, and bryon-proxy wasn't updated. How did this pass CI?

@jbgi could you take a look why this slipped through CI

mrBliss

Looking good. I requested a few changes that will make my life easier.

Even without switching the slot and hash to WithOrigin I was expecting a bigger diff.

Thanks for taking the time to do this.

ouroboros-network/src/Ouroboros/Network/Point.hs

ouroboros-consensus/src/Ouroboros/Consensus/Ledger/Byron.hs

mrBliss · 2019-07-04T12:48:48Z

ouroboros-network/src/Ouroboros/Network/Point.hs

+  , block
+  ) where
+
+data WithOrigin t = Origin | At t


Why Origin instead of Genesis?

Because Genesis is a constructor of Chain block. I thought it better not to overload.

Ok, fine for me. (What about data WithGenesis t = Gen | At t? 😈)

We might have places in haddock where we refer to Genesis, which now becomes Origin.

The Chain type is a model / mock impl. We shouldn't give it primacy in names. If we want Genesis for the point types we can use it, and we can rename the Chain one (e.g. GenesisBlock) or use qualified names.

ouroboros-consensus/src/Ouroboros/Storage/ChainDB/API.hs

ouroboros-network/src/Ouroboros/Network/Block.hs

mrBliss · 2019-07-04T12:56:05Z

ouroboros-consensus/src/Ouroboros/Storage/ChainDB/API.hs

@@ -319,17 +321,30 @@ data UnknownRange blk =
 --
 -- > StreamFromExclusive (Point { pointSlot = SlotNo 3 , .. }
 -- > StreamToInclusive   (Point { pointSlot = SlotNo 3 , .. }
+--
+-- FIXME StreamFrom and StreamTo can be refined to not admit origin points


Ok, I will do that when this is merged.

ouroboros-consensus/src/Ouroboros/Storage/ChainDB/API.hs

Suggested by mrBliss

mrBliss · 2019-07-05T06:53:44Z

@avieth shall we wait for @dcoutts to give his blessing?

coot · 2019-07-05T07:08:37Z

ouroboros-network/src/Ouroboros/Network/Point.hs

+  fmap _ Origin    = Origin
+  fmap f (At t) = At (f t)
+
+data Block slot hash = Block


What about calling this BlockPoint?

In this module it is clear that it is a point since (apart from the type, the module is named Point), in most places we use unqualified imports so this might be confusing. Or maybe just leave a comment at the top that this module is supposed to be imported qualified.

I see now it is imported qualified in other places, just dropping a comment at the top might be useful.

My intuition is also for BlockPoint and not using module-qualified names elsewhere.

coot · 2019-07-05T07:13:32Z

ouroboros-network/src/Ouroboros/Network/Point.hs

+at = At
+
+origin :: WithOrigin t
+origin = Origin


Is it worth to duplicate the api?

Not clear it's worth using functions rather than constructors directly.

coot

LGTM, I left a two comments, feel free to decide upon them.

coot · 2019-07-05T07:51:04Z

@avieth @mrBliss I am fine merging without @dcoutts blessing, 😎 ;). I see this PR as an improvement on top of Duncan's #690.

avieth · 2019-07-09T14:50:28Z

bors r+

706: Better Point type factoring, again r=avieth a=avieth Intended to replace #635 and #690 . It's factored into 2 commits that could be reviewed separately (the third one is a tiny unrelated fix to byron-proxy). The first commit introduces the `Ouroboros.Network.Point` module and uses it throughout ouroboros-network, making it build and tests pass. The second makes ouroboros-consensus build and tests pass. This commit retains the mistake `pointHash :: Point block -> SlotNo` rather than fixing it to be `pointHash :: Point block -> WithOrigin SlotNo`. The origin point is given `SlotNo 0`. It's kept this way so that the pull request will remain short and people will be willing to review it. Co-authored-by: Alexander Vieth <[email protected]>

iohk-bors · 2019-07-09T15:00:30Z

Build succeeded

dcoutts

So I'm still not happy with the names and extra layers of indirection. The pattern synonyms are good, but this should just be the actual representation, and we can provide conversion functions when we need something else.

dcoutts · 2019-07-17T12:48:50Z

ouroboros-network/src/Ouroboros/Network/Point.hs

+  , block
+  ) where
+
+data WithOrigin t = Origin | At t


The Chain type is a model / mock impl. We shouldn't give it primacy in names. If we want Genesis for the point types we can use it, and we can rename the Chain one (e.g. GenesisBlock) or use qualified names.

dcoutts · 2019-07-17T12:50:35Z

ouroboros-network/src/Ouroboros/Network/Point.hs

+  fmap _ Origin    = Origin
+  fmap f (At t) = At (f t)
+
+data Block slot hash = Block


My intuition is also for BlockPoint and not using module-qualified names elsewhere.

dcoutts · 2019-07-17T12:51:53Z

ouroboros-network/src/Ouroboros/Network/Point.hs

+at = At
+
+origin :: WithOrigin t
+origin = Origin


Not clear it's worth using functions rather than constructors directly.

ouroboros-network/src/Ouroboros/Network/Block.hs

706: Better Point type factoring, again r=avieth a=avieth Intended to replace #635 and #690 . It's factored into 2 commits that could be reviewed separately (the third one is a tiny unrelated fix to byron-proxy). The first commit introduces the `Ouroboros.Network.Point` module and uses it throughout ouroboros-network, making it build and tests pass. The second makes ouroboros-consensus build and tests pass. This commit retains the mistake `pointHash :: Point block -> SlotNo` rather than fixing it to be `pointHash :: Point block -> WithOrigin SlotNo`. The origin point is given `SlotNo 0`. It's kept this way so that the pull request will remain short and people will be willing to review it. Co-authored-by: Alexander Vieth <[email protected]>

Alexander Vieth added 3 commits July 3, 2019 19:22

point type factoring in ouroboros-network

7b35a6d

The ouroboros-network library builds and tests pass. This does not fix the pointSlot mistake: it is still defined, and gives SlotNo 0 to the origin. This will be fixed in a forthcoming commit.

byron-proxy logging fix

53a9dad

Apparently an update to iohk-monitoring broke it. But how did that get past CI?

avieth commented Jul 4, 2019

View reviewed changes

avieth requested review from dcoutts, mrBliss and coot and removed request for mrBliss July 4, 2019 00:10

fix ouroboros-network demo

7c4e456

mrBliss suggested changes Jul 4, 2019

View reviewed changes

WithOrigin derives Functor and others

7c679aa

avieth commented Jul 4, 2019

View reviewed changes

ouroboros-consensus/src/Ouroboros/Storage/ChainDB/API.hs Outdated Show resolved Hide resolved

pattern synonyms for Point

3648d99

Suggested by mrBliss

mrBliss approved these changes Jul 5, 2019

View reviewed changes

coot reviewed Jul 5, 2019

View reviewed changes

coot approved these changes Jul 5, 2019

View reviewed changes

iohk-bors bot merged commit 3648d99 into master Jul 9, 2019

iohk-bors bot deleted the avieth/point_type_redux branch July 9, 2019 15:00

avieth mentioned this pull request Jul 9, 2019

better Point type factoring #635

Closed

This was referenced Jul 15, 2019

Support Byron Epoch boundary blocks #704

Merged

Make it clearer that Ouroboros.Network.Chain is a test/mock module #771

Closed

dcoutts reviewed Jul 17, 2019

View reviewed changes

mrBliss added the consensus issues related to ouroboros-consensus label Aug 2, 2019

coot mentioned this pull request Aug 19, 2019

Introduce distinguished GenesisPoint :: Point #690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better Point type factoring, again #706

Better Point type factoring, again #706

avieth commented Jul 4, 2019

avieth Jul 4, 2019

coot Jul 8, 2019

mrBliss left a comment

mrBliss Jul 4, 2019

avieth Jul 4, 2019

mrBliss Jul 4, 2019

coot Jul 5, 2019

dcoutts Jul 17, 2019

mrBliss Jul 4, 2019

mrBliss commented Jul 5, 2019

coot Jul 5, 2019

coot Jul 5, 2019

coot Jul 5, 2019

dcoutts Jul 17, 2019

coot Jul 5, 2019

dcoutts Jul 17, 2019

coot left a comment

coot commented Jul 5, 2019

avieth commented Jul 9, 2019

iohk-bors bot commented Jul 9, 2019

dcoutts left a comment

dcoutts Jul 17, 2019

dcoutts Jul 17, 2019

dcoutts Jul 17, 2019

Better Point type factoring, again #706

Better Point type factoring, again #706

Conversation

avieth commented Jul 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrBliss 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

mrBliss commented Jul 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coot left a comment

Choose a reason for hiding this comment

coot commented Jul 5, 2019

avieth commented Jul 9, 2019

iohk-bors bot commented Jul 9, 2019

Build succeeded

dcoutts 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