genesis/origin does not have a slot number #774

avieth · 2019-07-15T19:58:59Z

pointSlot now has type Point block ->WithOrigin SlotNo. There is no slot for the origin/genesis point.

avieth · 2019-07-15T20:25:16Z

@mrBliss I've just pushed a commit that makes ouroboros-consensus build and tests pass, but I may have done some dodgy things so please review and change.

mrBliss

Great!

Can you do the same thing for BlockNo? Because we need BlockNo 0 for the genesis EBB, so the empty chain should have no BlockNo at all (i.e., Origin).

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

mrBliss · 2019-07-16T07:24:31Z

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

@@ -385,7 +385,7 @@ instance (ByronGiven, Typeable cfg, ConfigContainsGenesis cfg)
      lvUB = SlotNo $ unSlotNo currentSlot + (2 * paramK)
      lvLB
        | 2 * paramK > unSlotNo currentSlot
-        = genesisSlotNo
+        = SlotNo 0


@nc6 Is this correct?

Since the lower bound is non-strict, it should be fine.

ouroboros-consensus/src/Ouroboros/Consensus/ChainSyncClient.hs

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

ouroboros-network/test/Test/ChainFragment.hs

avieth · 2019-07-16T14:39:23Z

Can you do the same thing for BlockNo? Because we need BlockNo 0 for the genesis EBB, so the empty chain should have no BlockNo at all (i.e., Origin).

Giving BlockNo 0 for the empty chain may be right. It's basically the length of the chain, so 0 makes sense. Also, in cardano-sl, the genesis block has BlockNo 0, and every EBB has the same BlockNo as its parent block, so there's precedent.

mrBliss · 2019-07-16T15:25:24Z

Can you do the same thing for BlockNo? Because we need BlockNo 0 for the genesis EBB, so the empty chain should have no BlockNo at all (i.e., Origin).

Giving BlockNo 0 for the empty chain may be right. It's basically the length of the chain, so 0 makes sense. Also, in cardano-sl, the genesis block has BlockNo 0, and every EBB has the same BlockNo as its parent block, so there's precedent.

With genesis block, you mean the very first EBB (and thus "thing" on the chain), right? (I call it the genesis EBB)

An argument for WithOrigin BlockNo: during chain selection, we use the BlockNo of the tip of a chain(fragment) as a proxy for the length of the corresponding chain (because we're dealing with fragments, we can't use the fragment length). When we're given the choice between an empty chain (blockNo = Origin) and one containing only the genesis EBB (blockNo = At (BlockNo 0)), we should prefer the latter.

This patches ouroboros-network to build and pass tests. ouroboros-consensus will be patched in a follow-up.

Follow-up to pointSlot type change in ouroboros-network, to make ouroboros-consensus build and tests pass. Must be reviewed by someone more familiar with ouroboros-consensus. I've probably made some changes that aren't right.

The genesis point is never on a fragment

avieth · 2019-07-18T21:41:34Z

@mrBliss I agree that the empty chain should have no BlockNo. Then chain length can be defined as

chainLength :: HasHeader block => Chain block -> Word64
chainLength = maybe 0 (succ . unBlockNo) . headBlockNo

which corresponds to the actual chain length, for valid chains.

I went ahead and tried to do this but there is too much affected, including the ChainDB, so I gave up. Perhaps you should try?

I had done this before. Somehow it was lost in a merge/rebase.

mrBliss · 2019-07-19T06:38:49Z

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

+pointOnChain p@(BlockPoint pslot phash) (c :> b)
+  | pslot >  blockSlot b = False
+  | pslot == blockSlot b = phash == blockHash b
+  | otherwise             = pointOnChain p c


Can you align this last case on = to appease my OCD 🙂

Oops. Will do

mrBliss · 2019-07-19T06:40:56Z

@mrBliss I agree that the empty chain should have no BlockNo. Then chain length can be defined as
chainLength :: HasHeader block => Chain block -> Word64
chainLength = maybe 0 (succ . unBlockNo) . headBlockNo
which corresponds to the actual chain length, for valid chains.

I went ahead and tried to do this but there is too much affected, including the ChainDB, so I gave up. Perhaps you should try?

I'll give it a try.

mrBliss · 2019-07-19T11:30:01Z

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

@@ -144,11 +144,8 @@ pattern BlockPoint { atSlot, withHash } = Point (At (Point.Block atSlot withHash
 {-# COMPLETE GenesisPoint, BlockPoint #-}

 -- Should be


This line should also be removed.

mrBliss · 2019-07-19T13:47:15Z

@mrBliss I agree that the empty chain should have no BlockNo. Then chain length can be defined as
chainLength :: HasHeader block => Chain block -> Word64
chainLength = maybe 0 (succ . unBlockNo) . headBlockNo
which corresponds to the actual chain length, for valid chains.
I went ahead and tried to do this but there is too much affected, including the ChainDB, so I gave up. Perhaps you should try?
I'll give it a try.

I have a branch for this: https://github.com/input-output-hk/ouroboros-network/tree/mrBliss/point_blockno
But after talking with Duncan and Nick, I'm no longer convinced it is really needed.

The main advantage of WithOrigin BlockNo was the ability to distinguish an empty chain (Origin) from a chain containing just the genesis EBB (At (BlockNo 0)). This is handy because we use the block number as a proxy for the chain length. However, as you say, all other EBBs will have the same BlockNo as the block before them. So this advantage is only true for the single edge case.

And quoting Duncan:

I think we generally don't need to worry too much about block number vs chain length.

When we're doing chain selection when we're syncing from the old part of the chain that has EBBs, then every time we find an EBB we will shortly find another block after it. So even if it means we don't immediately select a chain that has just one more block that is an EBB, we will select it as soon as it has an EBB + 1 block.

avieth · 2019-07-19T14:14:38Z

bors r+

774: genesis/origin does not have a slot number r=avieth a=avieth `pointSlot` now has type `Point block ->WithOrigin SlotNo`. There is no slot for the origin/genesis point. Co-authored-by: Alexander Vieth <[email protected]>

iohk-bors · 2019-07-19T14:22:56Z

Build succeeded

avieth mentioned this pull request Jul 15, 2019

Support Byron Epoch boundary blocks #704

Merged

avieth requested a review from mrBliss July 15, 2019 20:00

avieth force-pushed the avieth/point_slot branch from 4b5edf7 to 0eeb798 Compare July 15, 2019 20:29

avieth requested a review from dcoutts July 16, 2019 00:41

mrBliss suggested changes Jul 16, 2019

View reviewed changes

Alexander Vieth added 3 commits July 18, 2019 16:29

genesis/origin does not have a slot number

48d2123

This patches ouroboros-network to build and pass tests. ouroboros-consensus will be patched in a follow-up.

genesis/origin no slot number, ouroboros-consensus

2e9f96d

Follow-up to pointSlot type change in ouroboros-network, to make ouroboros-consensus build and tests pass. Must be reviewed by someone more familiar with ouroboros-consensus. I've probably made some changes that aren't right.

fix ChainFragment assertion

03aaf57

avieth force-pushed the avieth/point_slot branch from 04cc32f to 03aaf57 Compare July 18, 2019 20:30

Alexander Vieth added 4 commits July 18, 2019 16:41

remove genesisSlotNo reference

eb724cf

fix indentation and comment

73fc03c

fix successorBlock

8ac8965

The genesis point is never on a fragment

remove unused binder

a5aff19

fix Consensus.Ledger.Byron

f721eff

I had done this before. Somehow it was lost in a merge/rebase.

mrBliss reviewed Jul 19, 2019

View reviewed changes

mrBliss approved these changes Jul 19, 2019

View reviewed changes

mrBliss reviewed Jul 19, 2019

View reviewed changes

avieth mentioned this pull request Jul 19, 2019

fixups to make it build in cardano-byron-proxy #800

Merged

Alexander Vieth added 2 commits July 19, 2019 10:00

fix alignment of equal sign

a6c4900

remove stale comment

d4453cf

iohk-bors bot merged commit d4453cf into master Jul 19, 2019

iohk-bors bot deleted the avieth/point_slot branch July 19, 2019 14:23

mrBliss mentioned this pull request Jul 22, 2019

Remove usage of Ouroboros.Network.Chain module #809

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

genesis/origin does not have a slot number #774

genesis/origin does not have a slot number #774

avieth commented Jul 15, 2019 •

edited

Loading

avieth commented Jul 15, 2019

mrBliss left a comment

mrBliss Jul 16, 2019

avieth Jul 18, 2019

avieth commented Jul 16, 2019

mrBliss commented Jul 16, 2019

avieth commented Jul 18, 2019

mrBliss Jul 19, 2019

avieth Jul 19, 2019

mrBliss commented Jul 19, 2019

mrBliss Jul 19, 2019

mrBliss commented Jul 19, 2019

avieth commented Jul 19, 2019

iohk-bors bot commented Jul 19, 2019

		@@ -144,11 +144,8 @@ pattern BlockPoint { atSlot, withHash } = Point (At (Point.Block atSlot withHash
		{-# COMPLETE GenesisPoint, BlockPoint #-}

		-- Should be

genesis/origin does not have a slot number #774

genesis/origin does not have a slot number #774

Conversation

avieth commented Jul 15, 2019 • edited Loading

avieth commented Jul 15, 2019

mrBliss left a comment

Choose a reason for hiding this comment

mrBliss Jul 16, 2019

Choose a reason for hiding this comment

avieth Jul 18, 2019

Choose a reason for hiding this comment

avieth commented Jul 16, 2019

mrBliss commented Jul 16, 2019

avieth commented Jul 18, 2019

mrBliss Jul 19, 2019

Choose a reason for hiding this comment

avieth Jul 19, 2019

Choose a reason for hiding this comment

mrBliss commented Jul 19, 2019

mrBliss Jul 19, 2019

Choose a reason for hiding this comment

mrBliss commented Jul 19, 2019

avieth commented Jul 19, 2019

iohk-bors bot commented Jul 19, 2019

Build succeeded

avieth commented Jul 15, 2019 •

edited

Loading