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

Remove MonadRandom constraints from consensus #2036

Closed
edsko opened this issue May 3, 2020 · 5 comments · Fixed by #2372
Closed

Remove MonadRandom constraints from consensus #2036

edsko opened this issue May 3, 2020 · 5 comments · Fixed by #2372
Assignees
@edsko
Labels
consensus issues related to ouroboros-consensus technical debt
Milestone
S17 2020-07-16

Comments

@edsko
Copy link
Contributor

edsko commented May 3, 2020

@dcoutts refactored the crypto classes in IntersectMBO/cardano-base#107 to remove MonadRandom constraints (as well as various other improvements), but those constraints still exist in consensus. We should remove them our side also.
@edsko edsko added consensus issues related to ouroboros-consensus priority medium labels May 3, 2020
@dcoutts
Copy link
Contributor

dcoutts commented May 3, 2020

This should also fix #1460

@edsko
Copy link
Contributor Author

edsko commented May 6, 2020

This seems to be blocked only on the use of MonadRandom for evalCertified for the VRF, which should be gone once we have @tdammers 's VRF implementation.

@edsko edsko mentioned this issue May 9, 2020
Closed
@edsko
Copy link
Contributor Author

edsko commented May 28, 2020 •
edited
Loading

Once this is gone, the use of ChaChaT should also be removed from the entire codebase (it's used in tests only).

mrBliss added a commit that referenced this issue Jun 22, 2020
@mrBliss
Introduce ChainIndepState
a642aa2 
Closes #2058, #2229.

* Get rid of the `TPraosForgeState` wrapper around `HotKey` and track more.
  information about the `HotKey`.
* Rename `ConsensusState` to `ChainDepState`.
* Introduce `ChainIndepState` and use it to track the `HotKey` for `TPraos`.
* Rename `ForgeState` to `ExtraForgeState` (block-specific, unused, but for
  future use, e.g., hardware devices, online opcert renewal) and define
  `ForgeState` to be the product of `ChainIndepState` and `ExtraForgeState`.
* Pass the `ChainIndepState` to `checkIsLeader` so that `TPraos` can check the
  validity of the KES key (original goal of this PR).
* Remove `Update`.
* Make `forgeBlock` pure by removing `MonadRandom` (work towards #2036).
* Also remove `MonadRandom` from `ProtocolInfo`.
@mrBliss mrBliss mentioned this issue Jun 22, 2020
Merged
mrBliss added a commit that referenced this issue Jun 22, 2020
@mrBliss
Introduce ChainIndepState
52a2a22 
Closes #2058, #2229.

* Get rid of the `TPraosForgeState` wrapper around `HotKey` and track more.
  information about the `HotKey`.
* Rename `ConsensusState` to `ChainDepState`.
* Introduce `ChainIndepState` and use it to track the `HotKey` for `TPraos`.
* Rename `ForgeState` to `ExtraForgeState` (block-specific, unused, but for
  future use, e.g., hardware devices, online opcert renewal) and define
  `ForgeState` to be the product of `ChainIndepState` and `ExtraForgeState`.
* Pass the `ChainIndepState` to `checkIsLeader` so that `TPraos` can check the
  validity of the KES key (original goal of this PR).
* Remove `Update`.
* Make `forgeBlock` pure by removing `MonadRandom` (work towards #2036).
* Also remove `MonadRandom` from `ProtocolInfo`.
iohk-bors bot added a commit that referenced this issue Jun 22, 2020
@iohk-bors @mrBliss
Merge #2300
9a7d0e0 
2300: Introduce ChainIndepState r=mrBliss a=mrBliss

Closes #2058, #2229.

* Get rid of the `TPraosForgeState` wrapper around `HotKey` and track more.
  information about the `HotKey`.
* Rename `ConsensusState` to `ChainDepState`.
* Introduce `ChainIndepState` and use it to track the `HotKey` for `TPraos`.
* Rename `ForgeState` to `ExtraForgeState` (block-specific, unused, but for
  future use, e.g., hardware devices, online opcert renewal) and define
  `ForgeState` to be the product of `ChainIndepState` and `ExtraForgeState`.
* Pass the `ChainIndepState` to `checkIsLeader` so that `TPraos` can check the
  validity of the KES key (original goal of this PR).
* Remove `Update`.
* Make `forgeBlock` pure by removing `MonadRandom` (work towards #2036).
* Also remove `MonadRandom` from `ProtocolInfo`.

Co-authored-by: Thomas Winant <[email protected]>
@edsko
Copy link
Contributor Author

edsko commented Jun 30, 2020

This blocked on IntersectMBO/cardano-base#136 .

@edsko edsko mentioned this issue Jul 7, 2020
Closed
@mrBliss
Copy link
Contributor

mrBliss commented Jul 7, 2020

After IntersectMBO/cardano-base#139, we can finally do this. We first have to wait for IntersectMBO/cardano-ledger#1621, though.

@mrBliss mrBliss added this to the S17 2020-07-16 milestone Jul 7, 2020
mrBliss added a commit that referenced this issue Jul 8, 2020
@mrBliss
Remove MonadRandom from consensus
c0d6911 
Fixes #2036.

Update the dependencies on `cardano-base` and `cardano-ledger-specs`, bringing
in IntersectMBO/cardano-base#139 and
IntersectMBO/cardano-ledger#1621, which
remove `MonadRandom` from the VRF Crypto class.

This means we can remove `MonadRandom` from `checkIsLeader`.

The implementation code no longer mentions `MonadRandom`, but the `TxGen` used
in the ThreadNet tests still does.
@mrBliss mrBliss mentioned this issue Jul 8, 2020
Merged
mrBliss added a commit that referenced this issue Jul 8, 2020
@mrBliss
Remove MonadRandom from consensus
e094a15 
Fixes #2036.

Update the dependencies on `cardano-base` and `cardano-ledger-specs`, bringing
in IntersectMBO/cardano-base#139 and
IntersectMBO/cardano-ledger#1621, which
remove `MonadRandom` from the VRF Crypto class.

This means we can remove `MonadRandom` from `checkIsLeader`.

The implementation code no longer mentions `MonadRandom`, but the `TxGen` used
in the ThreadNet tests still does.
iohk-bors bot added a commit that referenced this issue Jul 9, 2020
@iohk-bors @mrBliss @nfrisby
Merge #2372
8ca6cd8 
2372: Remove MonadRandom from consensus r=mrBliss a=mrBliss

**WARNING:** I believe the changes in `cardano-ledger-specs` break compatibility with deployed testnets.

# Remove MonadRandom from consensus

Fixes #2036.

Update the dependencies on `cardano-base` and `cardano-ledger-specs`, bringing
in IntersectMBO/cardano-base#139 and
IntersectMBO/cardano-ledger#1621, which
remove `MonadRandom` from the VRF Crypto class.

This means we can remove `MonadRandom` from `checkIsLeader`.

The implementation code no longer mentions `MonadRandom`, but the `TxGen` used
in the ThreadNet tests still does.

---

# test-infra: use Gen instead of MonadRandom in TxGen class

Replace `Test.Util.Random` with `Test.ThreadNet.Util.Seed`, which uses
QuickCheck instead of `MonadRandom`.

Remove all mentions of `MonadRandom` from the testsuite and thus from the
repository.

This means the seeds mentioned in the regression tests are no longer the ones
that produced the original failures. However, the seed is mainly used to
generate transactions, so the regression tests that don't mention transactions
are left in place with `Seed 0`. As we don't have a transaction generator for
Byron, we can leave *all* those regression tests in place. A few tests for the
mock protocols that relied on transactions were removed.

---

# Shelley: more efficient reapplyLedgerBlock

Take advantage of IntersectMBO/cardano-ledger#1630.

Co-authored-by: Thomas Winant <[email protected]>
Co-authored-by: Nicolas Frisby <[email protected]>
@iohk-bors iohk-bors bot closed this as completed in 3177cea Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@edsko edsko

Labels
consensus issues related to ouroboros-consensus technical debt
Projects
None yet
Milestone
S17 2020-07-16
Development

Successfully merging a pull request may close this issue.

Remove MonadRandom from consensus IntersectMBO/ouroboros-network
3 participants
@mrBliss @edsko @dcoutts