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

Use a single set of credentials for all Shelley-based eras #2753

Merged
merged 7 commits into from Nov 20, 2020
Merged

Use a single set of credentials for all Shelley-based eras #2753

merged 7 commits into from Nov 20, 2020

Conversation

mrBliss
Copy link
Contributor

@mrBliss mrBliss commented Nov 17, 2020 •
edited

We add the ability to share a single KES key by multiple Shelley-based eras.

Also, instead of a different thread per era, the hard fork combinator can now
combine BlockForging records from multiple eras into one BlockForging record
which picks the appropriate BlockForging based on the current era.

This means that the KES key shared by the Shelley-based eras will only be tried
to be evolved once, instead of once for each Shelley-based era (in the previous
approach, all threads would try to evolve it, but all but one would be an
identity operation). This should also improve tracing: instead of tracing
messages for each era, only messages of the current era are traced.

Note that multiple credentials per era (i.e. to benchmark multiple stakepools
on one node) will still result in multiple forging threads.

We refactor the protocol parameters for Shelley/Allegra/Mary to handle the
shared key: we now have a ProtocolParamsShelleyBased record which contains
everything needed for a Shelley-based chain, i.e., the genesis config and the
credentials. This record should be combined with the right era-specific ones.
This also paves the way for adding a Mary-only mode.

@mrBliss mrBliss requested a review from edsko November 17, 2020 15:13
@mrBliss mrBliss force-pushed the mrBliss/share-shelley-credentials branch from e0cc9e3 to 8d3ee08 Compare November 17, 2020 15:14
@mrBliss mrBliss added the consensus issues related to ouroboros-consensus label Nov 17, 2020
This was referenced Nov 17, 2020
Merged
Open
@mrBliss mrBliss force-pushed the mrBliss/share-shelley-credentials branch 3 times, most recently from 99036af to e704c7b Compare November 19, 2020 12:50
@mrBliss
Correct era names for Shelley-based eras
c875188 
We had an instance of `SingleEraBlock` for `ShelleyBlock` that was parametric in
the era, causing all Shelley-based eras (Shelley, Allegra, Mary) to have the
"Shelley" name.

Fix this by adding a `shelleyBasedEraName` method to the new `ShelleyBasedEra`
class, which combines the new method with the ledger-defined class of the same
name. See its docstring for more details.
@mrBliss
ProtocolInfo: 'sequence' pInfoBlockForging
9aa3cb0 
Make the following change:
```diff
-      , pInfoBlockForging :: [m (BlockForging m b)]
+      , pInfoBlockForging :: m [BlockForging m b]
```

This is more general and will allow us to, e.g., do create multiple
`BlockForging` records in one monadic call.
@mrBliss
Use a single set of credentials for all Shelley-based eras
2194d81 
For each era, there will be a thread periodically trying to evolve the KES key
and securely forget the previous one.

Previously, we required a set of credentials for each Shelley-based era, e.g.,
Shelley, Allegra, and Mary. In practice, most users will want to use the same
credentials for, e.g., Shelley and Allegra. However, when using the same
in-memory KES key for multiple eras, multiple threads will try to evolve and
secure forget the *same KES key*. Depending on the implementation of secure
forgetting for KES keys, this could lead to segfaults.

Instead of having a separate set of credentials per Shelley-based era (this
remains independent of how Byron credentials are handled), we now share a single
set of credentials for all of them. We use the new `shelleySharedBlockForging`
function to do this in a safe way, i.e., by sharing the same thread-safe
`HotKey` across the threads.

We refactor the protocol parameters for Shelley/Allegra/Mary to handle the
shared key: we now have a `ProtocolParamsShelleyBased` record which contains
everything needed for a Shelley-based chain, i.e., the genesis config and the
credentials. This record should be combined with the right era-specific ones.
This also paves the way for adding a Mary-only mode.
@mrBliss
Swap type argument order of Index
ac38c8c 
This will make it possible to use it in combination with `NP`.
@mrBliss mrBliss force-pushed the mrBliss/share-shelley-credentials branch from e704c7b to 19fba7e Compare November 19, 2020 13:00
@mrBliss mrBliss force-pushed the mrBliss/share-shelley-credentials branch from 19fba7e to e9f4574 Compare November 19, 2020 15:41
@mrBliss
BlockForging refactor: single forging thread for all eras
378f927 
Instead of a different thread per era, the hard fork combinator can now combine
`BlockForging` records from multiple eras into one `BlockForging` record which
picks the appropriate `BlockForging` based on the current era.

This means that the KES key shared by the Shelley-based eras will only be tried
to be evolved once, instead of once for each Shelley-based era (in the previous
approach, all threads would try to evolve it, but all but one would be an
identity operation). This should also improve tracing: instead of tracing
messages for each era, only messages of the current era are traced.

Note that multiple credentials *per era* (i.e. to benchmark multiple stakepools
on one node) will still result in multiple forging threads.
@mrBliss
Use h(c)imap and h(c)izipWithX instead of injections
d362ec6
@mrBliss mrBliss force-pushed the mrBliss/share-shelley-credentials branch from e9f4574 to d362ec6 Compare November 20, 2020 08:54
edsko
edsko approved these changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@edsko edsko left a comment

Choose a reason for hiding this comment

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

This was trickier than anticpated!

@mrBliss
Copy link
Contributor Author

mrBliss commented Nov 20, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 20, 2020

Build succeeded:

@iohk-bors iohk-bors bot merged commit 10998cc into master Nov 20, 2020
@iohk-bors iohk-bors bot deleted the mrBliss/share-shelley-credentials branch November 20, 2020 12:56
mrBliss added a commit to IntersectMBO/cardano-node that referenced this pull request Nov 23, 2020
@mrBliss
Update dependency on ouroboros-network
b00e292 
Notable changes:

* Share a single set of credentials for all Shelley-based eras:
  IntersectMBO/ouroboros-network#2753
* Add a pure Mary mode
  IntersectMBO/ouroboros-network#2754
@mrBliss mrBliss mentioned this pull request Nov 23, 2020
Merged
mrBliss added a commit to IntersectMBO/cardano-node that referenced this pull request Nov 24, 2020
@mrBliss
Update dependency on ouroboros-network
6ced601 
Notable changes:

* Share a single set of credentials for all Shelley-based eras:
  IntersectMBO/ouroboros-network#2753
* Add a pure Mary mode
  IntersectMBO/ouroboros-network#2754
iohk-bors bot added a commit to IntersectMBO/cardano-node that referenced this pull request Nov 24, 2020
@iohk-bors @mrBliss
Merge #2114
ee130d0 
2114: Update dependency on ouroboros-network r=mrBliss a=mrBliss

Notable changes:

* Share a single set of credentials for all Shelley-based eras:
  IntersectMBO/ouroboros-network#2753
* Add a pure Mary mode
  IntersectMBO/ouroboros-network#2754

Co-authored-by: Thomas Winant <thomas@well-typed.com>
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.

None yet
2 participants
@mrBliss @edsko