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

Move TPraos protocol into its own package. #3513

Merged
merged 1 commit into from
Dec 8, 2021
Merged

Conversation

nc6
Copy link
Contributor

@nc6 nc6 commented Nov 30, 2021

Intention will be to implement the new Praos protocol also in this package.

This PR simply moves things around. I didn't move the PBFT protocol into this package, since it seemed reasonable to just isolate it in Byron.

@nc6 nc6 requested a review from nfrisby November 30, 2021 12:15
@nc6 nc6 added consensus issues related to ouroboros-consensus praos labels Nov 30, 2021
@bolt12 bolt12 removed their request for review November 30, 2021 15:59
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

Approved. Thanks!

The only key request is to copy over the ghc-options block.

@@ -8,7 +8,7 @@
-- | Hot key
--
-- Intended for qualified import
module Ouroboros.Consensus.Shelley.Protocol.HotKey (
module Ouroboros.Consensus.Protocol.Ledger.HotKey (
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking out loud: is the hotkey more a Ledger concern or more a BlockForging concern?

Actually, I'm less sure how to distinguish which parts of this are unique to BlockForging than I had thought I was. EG only a block-producing node would ever use evolve, sign, and mkHotKey, if I understand correctly.

I suppose it's natural to leave this module unbroken, and so Ledger really is the most appropriate point in the conceptual hierarchy for this bag of definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think I might end up moving some of this logic around anyway. Currently forging isn't really keyed to the protocol, only to the block.

Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

Approved! Sorry -- I meant to Approve instead of just Comment last time.

@nc6
Copy link
Contributor Author

nc6 commented Dec 7, 2021

bors merge

iohk-bors bot added a commit that referenced this pull request Dec 7, 2021
3513: Move TPraos protocol into its own package. r=nc6 a=nc6

Intention will be to implement the new Praos protocol also in this package.

This PR simply moves things around. I didn't move the PBFT protocol into this package, since it seemed reasonable to just isolate it in Byron.

Co-authored-by: Nicholas Clarke <nick@topos.org.uk>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 7, 2021

Build failed:

Intention will be to implement the new Praos protocol also in this
package.
@nc6
Copy link
Contributor Author

nc6 commented Dec 8, 2021

bors retry

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 8, 2021

@iohk-bors iohk-bors bot merged commit 67b7c9e into master Dec 8, 2021
@iohk-bors iohk-bors bot deleted the nc/extract-protocol branch December 8, 2021 08:24
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 praos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants