Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ChainSyncClient: No BlockNo when tip is genesis #1585

Merged
merged 1 commit into from
Feb 5, 2020

Conversation

edsko
Copy link
Contributor

@edsko edsko commented Feb 5, 2020

The Tip data-type was

data Tip b = Tip
  { tipPoint   :: !(Point b)
  , tipBlockNo :: !BlockNo
  }

where Point uses WithOrigin. This is not correct. When tipPoint is Origin, then there is no tipBlockNo (genesisBlockNo is the block number of the first block on the chain). In this commit we change this to

data Tip b =
    -- | The tip is genesis
    TipGenesis

    -- | The tip is not genesis
  | Tip !SlotNo !(HeaderHash b) !BlockNo

It doesn't, however, make any real changes, providing instead some "legacy" API that pretends that we do always have a BlockNo. This primarily affects consensus only, in networking this only appears in examples and tests.

We also use this legacy format to avoid changing the binary presentation of Tip, so that this PR does not break backwards compatibility.

The `Tip` data-type was

```haskell
data Tip b = Tip
  { tipPoint   :: !(Point b)
  , tipBlockNo :: !BlockNo
  }
```

where `Point` uses `WithOrigin`. This is not correct. When `tipPoint` is
`Origin`, then there _is_ no `tipBlockNo` (`genesisBlockNo` is the block
number of the first block on the chain). In this commit we change this
to

```haskell
data Tip b =
    -- | The tip is genesis
    TipGenesis

    -- | The tip is not genesis
  | Tip !SlotNo !(HeaderHash b) !BlockNo
```

It doesn't, however, make any real changes, providing instead some
"legacy" API that pretends that we _do_ always have a `BlockNo`. This
primarily affects consensus only, in networking this only appears in
examples and tests.

We also use this legacy format to avoid changing the binary presentation
of `Tip`, so that this PR does _not_ break backwards compatibility.
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Do we have a ticket for this TODO? If not lets make one.

@edsko
Copy link
Contributor Author

edsko commented Feb 5, 2020

bors r+

@edsko
Copy link
Contributor Author

edsko commented Feb 5, 2020

Will create the ticket.

iohk-bors bot added a commit that referenced this pull request Feb 5, 2020
1585: ChainSyncClient: No `BlockNo` when tip is genesis r=edsko a=edsko

The `Tip` data-type was

```haskell
data Tip b = Tip
  { tipPoint   :: !(Point b)
  , tipBlockNo :: !BlockNo
  }
```

where `Point` uses `WithOrigin`. This is not correct. When `tipPoint` is `Origin`, then there _is_ no `tipBlockNo` (`genesisBlockNo` is the block number of the first block on the chain). In this commit we change this to

```haskell
data Tip b =
    -- | The tip is genesis
    TipGenesis

    -- | The tip is not genesis
  | Tip !SlotNo !(HeaderHash b) !BlockNo
```

It doesn't, however, make any real changes, providing instead some "legacy" API that pretends that we _do_ always have a `BlockNo`. This primarily affects consensus only, in networking this only appears in examples and tests.

We also use this legacy format to avoid changing the binary presentation of `Tip`, so that this PR does _not_ break backwards compatibility.

Co-authored-by: Edsko de Vries <[email protected]>
@edsko
Copy link
Contributor Author

edsko commented Feb 5, 2020

#1587 is the ticket.

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 5, 2020

@iohk-bors iohk-bors bot merged commit bab13e5 into master Feb 5, 2020
@iohk-bors iohk-bors bot deleted the edsko/fix-network-chainsync-Tip branch February 5, 2020 18:25
@avieth
Copy link
Contributor

avieth commented Feb 5, 2020

LGTM.

Used to be we had to have a block number for some reason, and we chose 0 for the genesis, and each EBB would have the same block number as the 0th block number of its epoch. If this isn't needed anymore then good riddance!

@edsko
Copy link
Contributor Author

edsko commented Feb 6, 2020

Used to be we had to have a block number for some reason, and we chose 0 for the genesis, and each EBB would have the same block number as the 0th block number of its epoch. If this isn't needed anymore then good riddance!

This might still be needed, but if so, it should be Byron specific, and not live in the general infrastructure. The addition to of WithOrigin to Point was already a step towards that direction. My hope is genesisSlotNo and genesisBLockNo can be banished altogether IntersectMBO/cardano-base#78.

@mrBliss mrBliss added the consensus issues related to ouroboros-consensus label Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants