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

Branch to collect post-fork improvements #2446

Merged
merged 7 commits into from
Aug 6, 2020

Conversation

edsko
Copy link
Contributor

@edsko edsko commented Jul 21, 2020

Any PRs that change the consensus layer, unless they are high priority bug fixes that should be included on mainnet prior to the hard fork transition, should be opened against this one (rather than against master). We will collect them here, and after the transition has happened, merge them into master. This is just a temporary measure to make sure that we can respond quickly should any problem require urgent fixes.

@mrBliss mrBliss added the consensus issues related to ouroboros-consensus label Jul 21, 2020
@edsko edsko force-pushed the consensus-improvements-after-hardwork branch from a60fdd9 to 0d71416 Compare July 23, 2020 12:43
@mrBliss mrBliss force-pushed the consensus-improvements-after-hardwork branch 3 times, most recently from 63e4b17 to 0317a6d Compare July 31, 2020 07:16
@edsko edsko force-pushed the consensus-improvements-after-hardwork branch 2 times, most recently from ffd4fae to 4f054dc Compare August 6, 2020 08:18
mrBliss and others added 7 commits August 6, 2020 10:25
Until quite a while ago, we were using `ProtocolInfo` in our tests, which meant
that `ProtocolInfo` had to include the mock blocks in order for us to run our
tests with the mock blocks.

While it was useful in the beginning to test/demo `cardano-node` with mock
blocks, since the introduction of `ByronBlock`, I don't think anyone has run
`cardano-node` using a mock block in quite a while.

Removing the mock blocks from `ProtocolInfo` will probably allow `cardano-node`
to simplify/remove quite a bit of stuff, e.g., command line argument handling.

Note that initially including the mock blocks in `ProtocolInfo` (in addition to
`ByronBlock`) has served its purpose: a lot of code in `cardano-node` was
written to be parametric in the `blk` type, instead of hard-coding it to
`ByronBlock`, which made it much easier to add support for `ShelleyBlock` and
`CardanoBlock` in the last months.
They are the same thing, just with different names in different libs. We
are standardising on NetworkMagic so this patch removes most uses of
ProtocolMagicId.
* Remove all things related to `ChainIndepState` and `ForgeState`.

* Let `checkIsLeader` return `Maybe (IsLeader p)` instead of
  `LeaderCheck` (deleted).

* Replace `BlockProduction`, `CanForge`, and `MaintainForgeState` with
  `BlockForging`: This record has four fields:

  1. `canBeLeader`, a proof that the node can be a leader, previously stored in
     `ProtocolInfo`.
  2. `updateForgeState`, called before the leader check, is used for Shelley to
     evolve the KES key and returns the new status of the key. The stateful key
     can be in the closure. Returns `ForgeStateInfo`, a new type family, which
     is traced. For Shelley, this will be the status of the KES key.
  3. `checkCanForge`, which does the extra checks (in addition to
     `checkIsLeader`) that we can actually forge a block. It returns a value of
     the new `CannotForge` type family. E.g., because the KES key is no longer
     valid.
  3. `forgeBlock`, moved from `CanForge`.

  Only when a node has credentials to produce blocks will it have a
  `BlockForging` record (see next bullet).

* `ProtocolInfo`: replace

  ```haskell
  pInfoLeaderCreds :: Maybe ( CanBeLeader (BlockProtocol b)
                            , MaintainForgeState m b
                            )
  ```
  with
  ```haskell
  pInfoBlockForging :: Maybe (m (BlockForging m b))
  ```
  Note the outer `m`: this allows monadic initialisation of the `BlockForging`,
  e.g., create an `MVar` to store the KES key or other `IOLike` operations.

* Provide a stateful API to the KES key with operations to sign something,
  evolve the key, and return info about the key. This means the signing key is
  not exposed in a pure way, allowing us to securely erase it after evolving in
  the future.

Co-authored-by: Edsko de Vries <edsko@well-typed.com>
Fixes #2367.

As the `forgetSignKeyKES` method lives in IO but we live in `IOLike m`, we
can't call it directly. We add it as a method to `IOLike`.
* Introduce `ForgeStateUpdateError`

* Let `updateForgeState` return `ForgeStateUpdateInfo`, which is a sum of
  *updated*, *unchanged*, *failed* (`ForgeStateUpdateError`).
  In case of *updated* or *unchanged*, pass the resulting `ForgeStateInfo` to
  `checkCanForge`.

* Resurrect `LeaderCheck` in the form of `ShouldForge`.

* Split off `OptNP` in a new module. Change its index from `allowedEmpty` to
  `empty` and adapt accordingly.
While `Qry` still provides a monadic interface, most of the actual calculation
happens in the interpreter for `Expr`, and any past-horizon exceptions come
from the evaluation of an `Expr`. Unlike `Qry`, `Expr` uses PHOAS, which means
that we can actually `Show` expressions, despite them containing functions.
This means that we can include the offending expression in the exception.

This is also a strict generalization: previously, an entire `Qry` was always
evaluated against a single era; with the split, every `Expr` is evaluated
against a single era, but every `Expr` embedded in a `Qry` can be evaluated
against a _different_ era.

Closes #2463.
@edsko edsko force-pushed the consensus-improvements-after-hardwork branch from 4f054dc to 2bfbb3b Compare August 6, 2020 08:34
@edsko edsko marked this pull request as ready for review August 6, 2020 08:35
@edsko
Copy link
Contributor Author

edsko commented Aug 6, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 6, 2020

@iohk-bors iohk-bors bot merged commit 83baf7e into master Aug 6, 2020
@iohk-bors iohk-bors bot deleted the consensus-improvements-after-hardwork branch August 6, 2020 09:31
coot pushed a commit that referenced this pull request May 16, 2022
2446: Branch to collect post-fork improvements r=edsko a=edsko

Any PRs that change the consensus layer, unless they are high priority bug fixes that should be included on mainnet prior to the hard fork transition, should be opened against this one (rather than against `master`). We will collect them here, and after the transition has happened, merge them into master. This is just a temporary measure to make sure that we can respond quickly should any problem require urgent fixes.

Co-authored-by: Thomas Winant <thomas@well-typed.com>
Co-authored-by: Duncan Coutts <duncan@well-typed.com>
Co-authored-by: Edsko de Vries <edsko@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.

None yet

3 participants