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.

Sign up for GitHub

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

Jump to bottom

Fix more thunks #1793

Merged
merged 4 commits into from
Mar 13, 2020
Merged

Fix more thunks #1793

merged 4 commits into from
Mar 13, 2020

Conversation

mrBliss
Copy link
Contributor

@mrBliss mrBliss commented Mar 12, 2020

  • Fix thunks in Praos

  • ImmutableDB.Cache: fix concurrency bug

  • Bump dependency on cardano-ledger

@mrBliss mrBliss added the consensus issues related to ouroboros-consensus label Mar 12, 2020
@mrBliss mrBliss requested a review from edsko March 12, 2020 11:07
@mrBliss
Copy link
Contributor Author

mrBliss commented Mar 12, 2020 •
edited
Loading

See the commits messages for more details. I'm still waiting for IntersectMBO/cardano-ledger#746 to be merged.

UPDATE: merged & updated this PR.

@mrBliss
ImmutableDB.Cache: fix concurrency bug
c406823 
When running the mock Praos ThreadNet tests with the `checktvarinvariant` flag
on, we were seeing the following failure:

   simple convergence:                             FAIL (52.75s)
      *** Failed! (after 26 tests and 1 shrink):
      Exception:
        Invariant violation: nbPastChunks (2) /= PSQ.size pastChunksInfo (1)
        CallStack (from HasCallStack):
          error, called at src/Control/Monad/Class/MonadSTM/Strict.hs:174:31 in ..
          checkInvariant, called at src/Ouroboros/Consensus/Util/MonadSTM/StrictMVar.hs:167:5 in ..
          updateMVar, called at src/Ouroboros/Consensus/Storage/ImmutableDB/Impl/Index/Cache.hs:572:22 in ..
          getChunkInfo, called at src/Ouroboros/Consensus/Storage/ImmutableDB/Impl/Index/Cache.hs:665:5 in ..

When there are multiple concurrent cache misses for the same epoch, multiple
threads will concurrently try to insert the same epoch in the cache,
incrementing `nbPastChunks` each time, even when the same epoch is already in
the cache, violating the invariant `nbPastChunks == PSQ.size pastChunksInfo`.

Fix it by only incrementing `nbPastChunks` when the new chunk was not already
in the cache.
@mrBliss mrBliss force-pushed the mrBliss/fix-thunks branch from 6d8e7ca to ed1f096 Compare March 12, 2020 11:33
@mrBliss mrBliss mentioned this pull request Mar 12, 2020
Merged
@mrBliss mrBliss force-pushed the mrBliss/fix-thunks branch from ed1f096 to 0b33f54 Compare March 12, 2020 19:07
@mrBliss
Copy link
Contributor Author

mrBliss commented Mar 12, 2020

CI is failing because the Byron LedgerState and CC.UPI.State golden tests fail. These data types must have changed in cardano-ledger. cc: @nc6.

This means that users would have to replay their ledger from scratch, taking many minutes on startup. With the current elision done in cardano-node, this is not very user-friendly (IntersectMBO/cardano-node#562).

@mrBliss mrBliss mentioned this pull request Mar 13, 2020
Closed
@mrBliss
Copy link
Contributor Author

mrBliss commented Mar 13, 2020

CI is failing because the Byron LedgerState and CC.UPI.State golden tests fail. These data types must have changed in cardano-ledger. cc: @nc6.

This means that users would have to replay their ledger from scratch, taking many minutes on startup. With the current elision done in cardano-node, this is not very user-friendly (input-output-hk/cardano-node#562).

I have updated the golden tests because of #1798 (comment).

mrBliss added 2 commits March 13, 2020 08:35
@mrBliss
Bump dependency on cardano-ledger
fe89ec1 
This brings in IntersectMBO/cardano-ledger#746,
fixing a thunk.
@mrBliss
Update golden tests that broke because ledger format changed
a88cad4 
The LedgerState type changed, which will require a replay from genesis for
nodes upgrading to a version including this change.

The UPIState is only sent through the LocalStateQuery protocol, which is not
actively being used yet.
@mrBliss mrBliss force-pushed the mrBliss/fix-thunks branch from c4838c9 to a88cad4 Compare March 13, 2020 07:36
@mrBliss
Copy link
Contributor Author

mrBliss commented Mar 13, 2020

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 13, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 10c725f into master Mar 13, 2020
@iohk-bors iohk-bors bot deleted the mrBliss/fix-thunks branch March 13, 2020 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edsko edsko edsko approved these changes

Assignees
No one assigned
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mrBliss @edsko