Skip to content

Conversation

edsko
Copy link
Contributor

@edsko edsko commented Mar 5, 2020

This is my third attempt at doing this, but it's now almost done. Only things left to do:

  • Update the immutable DB QSM tests to also test with a ChunkSize that doesn't support EBBs
  • Check for mentions of epoch that should be renamed to chunk.

@edsko
Copy link
Contributor Author

edsko commented Mar 6, 2020

I'm not adding a migration to rename the chunk files; see #1755.

edsko added 26 commits March 6, 2020 13:27
Unlike the immutable DB, Praos actually does care about epoch boundaries
(this is why the Shelley spec takes an EpochInfo as argument). So we
will leave this as EpochInfo, but use the one from cardano-slotting.
And just use the one from cardano-slotting (soon we'll not use it at all
anymore).
This commit doesn't change the semantics of the code, merely updates it
to account for the fact that the ChunkInfo API is no longer monadic.
This is just a simple renaming. In the next commit we'll start making
more significant changes.
The only consequence (so far) of this is that `maxRelativeSlot` must now
have access to the internals.
`Enum` still implies a translation from and to `Int`. All we need is a
successor function.
This doesn't (yet) _change_ these definitions, merely moves them.
This replaces `epochInfoRelative`.
And provide some related infrastructure.
This reintroduces `EpochInfo` for the analysis (which really do care
about EpochInfo rather than `ChunkInfo`).
Only `.Cache` and `.Primary` use the raw definition from `.Internal`.
This at least allows us to find such use cases more easily.
This is a huge commit, that replaces `EpochNo` with `ChunkNo` _almost_,
but crucially not _quite_, everywhere. Where they remain they are
invariably related to EBB (the `EpochNo` of an EBB). During review we
should make sure that these use cases are correct.

Tested this separately (`test-storage`), and it passes 11,100 tests.
This adds a bunch of invariants, and forces us to be more precise with
`RelativeSlot`. It also strengthens the `PrimaryIndex` tests, equips
them with a more general generator, as well as a basic shrinker.

Passes 100,000 `PrimaryIndex` tests and 10,000 Immutable DB QSM tests.
Again passed 1M `PrimaryIndex` tests and 10k immutable DB QSM tests.
In particular, `EpochNo` and `EpochSize` should not be exported from
`Common` as most of the core consensus infrastructure now is entirely
agnostic about epochs.
This adds the plumbing to provide `ChunkInfo` where needed, but the
generator still uses an epoch size of 10; just testing the plumbing so
far. Passes 10k tests.
Still passes 10k tests. (We do get test failures if we allow the
generator to pick `0` for `numRegularBlocks`.)
This is a reasonably large commit, but most of it is propagation of
HasCallStack for better test failures.

Really only needed to make two changs:

* Update the ChainDB and immutable DB generators not to produce EBBs
* Fix one bug in `slotMightBeEBB`

Both the chain DB and immutable DB passed 10k tests.
This commit doesn't change anything semantically, just renames things
that were still called "epoch" but should by rights be called "chunk".
Copy link
Contributor

@mrBliss mrBliss left a comment

Choose a reason for hiding this comment

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

Monster PR! LGTM

@edsko
Copy link
Contributor Author

edsko commented Mar 6, 2020

bors r+

@mrBliss mrBliss added the consensus issues related to ouroboros-consensus label Mar 6, 2020
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 6, 2020

@iohk-bors iohk-bors bot merged commit d555a77 into master Mar 6, 2020
@iohk-bors iohk-bors bot deleted the edsko/no-epochinfo-3.0 branch March 6, 2020 17:13
@mrBliss mrBliss mentioned this pull request Mar 10, 2020
iohk-bors bot added a commit that referenced this pull request Mar 10, 2020
1777: Fix thunks in ChunkSize r=mrBliss a=mrBliss

Introduced in #1750.

Co-authored-by: Thomas Winant <thomas@well-typed.com>
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.

2 participants