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

Reconsider ledgerTipPoint #1559

Closed
mrBliss opened this issue Feb 3, 2020 · 2 comments · Fixed by #1562
Closed

Reconsider ledgerTipPoint #1559

mrBliss opened this issue Feb 3, 2020 · 2 comments · Fixed by #1562
Labels
consensus issues related to ouroboros-consensus
Milestone

Comments

@mrBliss
Copy link
Contributor

mrBliss commented Feb 3, 2020

Quoting from #1505 (comment):


Also, I think that the idea of ledgerStatePoint is not correct. Say we have an empty ledger state with no applied blocks, so the last hash is GenesisHash. When we now "tick" it using a SlotNo s, which Point would we return? We can't return GenesisPoint, but we can't return BlockPoint either, because we don't have a hash.

A simple fix would be to return (WithOrigin SlotNo, ChainHash blk) instead of Point blk, but that would allow for (Origin, BlockHash h), which is an invalid combination. A new type would be better, e.g.:

data LedgerTip blk
  = NoAppliedBlock !(WithOrigin SlotNo)
    -- ^ No block has been applied to the ledger. When the ledger hasn't been
    -- \"ticked\", the argument will be 'Origin', otherwise, it will be @'At'
    -- slotNo@ where @slotNo@ is the ticked slot number.
  | AppliedBlock !(HeaderHash blk) !SlotNo
    -- ^ A block with the given hash has been applied. The 'SlotNo' will be
    -- that of the block, unless the ledger has been \"ticked\" with a more
    -- recent slot number.
@mrBliss mrBliss added the consensus issues related to ouroboros-consensus label Feb 3, 2020
@edsko
Copy link
Contributor

edsko commented Feb 3, 2020

Something related I've been thinking about: TickedLedgerState should really contain the slot number that it was ticked for. This would help in the Shelley integration also, as well as in some other places.

I wonder if we do that whether we could perhaps leave the semantics of the underlying LedgerState alone?

@nfrisby
Copy link
Contributor

nfrisby commented Feb 3, 2020

Last week (?) @edsko and I discussed a similar concern with the Mock ledger state that my WIP branch for Issue #1297 revealed. Synchronicity has struck: @mrBliss hypothesized something similar to explain Issue 1505 (hence this Issue) while @nc6 recently noticed similar issues with TPraos.

We all had a call just now. @edsko is fixing the bug (with a better fix than he and I originally came up with). I expect that the tests on my Issue 1297 branch will pass once I rebase it to include that fix. I will continue to debug Issue 1505 to determine if this Issue 1559 is indeed the underlying cause -- simply cherry-picking Edsko's fix might merely mask the repro since it depends on a race condition/subtle timing, so I want to understand its mechanics better.

Also, I should be able to rebase my PR for Issue 1489 onto this fix; Issue 1505 is the only remaining failure I've seen on that branch. Progress!

edsko added a commit that referenced this issue Feb 3, 2020
and leave the tip of the underlying ledger unchanged. I modified the
Byron ledger and spec to do this; the mock ledgers were also leaving the
tip unchanged, but without the additional SlotNo in TickedLedgerState
this was causing trouble (as in a bug that @nrisby found when looking at

Closes #1559.
edsko added a commit that referenced this issue Feb 3, 2020
and leave the tip of the underlying ledger unchanged. I modified the
Byron ledger and spec to do this; the mock ledgers were also leaving the
tip unchanged, but without the additional SlotNo in TickedLedgerState
this was causing trouble (as in a bug that @nrisby found when looking at
issue #1297).

Closes #1559.
iohk-bors bot added a commit that referenced this issue Feb 3, 2020
1562: Add `SlotNo` to `TickedLedgerState` r=edsko a=edsko

and leave the tip of the underlying ledger unchanged. I modified the Byron ledger and spec to do this; the mock ledgers were also leaving the tip unchanged, but without the additional `SlotNo` in `TickedLedgerState` this was causing trouble (as in a bug that @nfrisby found when looking at #1297).

Closes #1559.

Co-authored-by: Edsko de Vries <edsko@well-typed.com>
@iohk-bors iohk-bors bot closed this as completed in bc24dc2 Feb 3, 2020
@mrBliss mrBliss added this to the S6 2020-02-13 milestone Feb 6, 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 a pull request may close this issue.

3 participants