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

No Slotting Arithmetic #126

Closed
wants to merge 2 commits into from
Closed

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Mar 27, 2019

Issue Number

#94

Overview

  • I have remove slotting arithmetic operation from the source code (relying instead on the fact that blocks are linked to each other and points to their parent)
  • I have added a few tests to make sure that ordering of slots happens as expect, in a sudden paranoia crisis (since we derive the Ord automatically, it just happened that we defined the epoch index before the slot number in the record declaration. Swapping those two would have dramatic effects ^^")

Comments

As a side-effect, we don't have to test for any of the boundary cases because there are none 🎉

The rational is that, doing slotting arithmetic forces us to keep track of the 'slotsPerEpoch'
which is in practice, rather tricky. We can _probably_ get away by leveraging the _chained_
aspect of the blockchain (that each block points to its previous one) and rewind the chain
when needed.
<$> choose (0 :: Int, 4 * (fromIntegral slotsPerEpoch))
arbitrary = do
ep <- choose (0, 10)
sl <- choose (0, 100)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not sl <- choose (0, 21600)?
Also is it possible to have slot 0? or epoch 0?
It is because it's just an Id I suppose?

Copy link
Contributor

Choose a reason for hiding this comment

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

hm, is it even needed? I mean the whole generator for SlotId? The Slotting ordering test suite ^ is using only some explicit values for SlotId...

Copy link
Member Author

Choose a reason for hiding this comment

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

Not only, there's a first property that compare arbitrary slots against the initial SlotId 0 0 (making sure they're all greater than it).

Why not 21600 ? Well, to really show that it doesn't matter here and is completely irrelevant for the testing of this module. The only thing we really test is that we compare slot correctly. There's no other assumption about SlotId (and that's what we try to preserve in the code as well).

Copy link
Contributor

@piotr-iohk piotr-iohk left a comment

Choose a reason for hiding this comment

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

👍

@KtorZ
Copy link
Member Author

KtorZ commented Mar 27, 2019

Merged into #128 to serialize PR / CI builds

@KtorZ KtorZ closed this Mar 27, 2019
@KtorZ KtorZ deleted the KtorZ/94/no-slotting-arithmetic branch April 9, 2019 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants