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

Refactor to decouple consensus modes #2978

Merged
merged 1 commit into from
Mar 31, 2021
Merged

Refactor to decouple consensus modes #2978

merged 1 commit into from
Mar 31, 2021

Conversation

jhartzell42
Copy link
Contributor

@jhartzell42 jhartzell42 commented Mar 8, 2021 •
edited
Loading 

Refactor to decouple consensus modes

- Functionality that handles Shelley-based eras in a multi-era consensus
  is moved to ouroboros-consensus-shelley so that it can be shared
  between mainnet consensus (in ouroboros-consensus-cardano) and other
  consensus modes comprising multiple Shelley eras.
- The `Protocol` GADT that enumerates consensus modes and provides a
  wealth of information for clients is eliminated. It is an
  implementation detail only used by `cardano-node`. This way,
  `ouroboros-consensus-cardano` does not need to know about other
  consensus modes.

@jhartzell42 jhartzell42 force-pushed the os/first-pr branch 2 times, most recently from b2e7a41 to 9576047 Compare March 8, 2021 22:15
@madeline-os madeline-os changed the title WIP: Refactor to make consensus modes open, move shelley code out of ourob… WIP: Refactor to decouple consensus modes Mar 16, 2021
@jhartzell42 jhartzell42 force-pushed the os/first-pr branch 2 times, most recently from d0665e1 to dc0e118 Compare March 16, 2021 23:25
@madeline-os madeline-os mentioned this pull request Mar 17, 2021
Merged
@jhartzell42 jhartzell42 marked this pull request as ready for review March 18, 2021 19:59
@nc6 nc6 requested a review from nfrisby March 19, 2021 15:41
@nfrisby
Copy link
Contributor

nfrisby commented Mar 24, 2021

I finally reviewed the previous PR I had promised to review. So I hope to have a pass at this one tomorrow (ie Wednesday). Sorry for the delay.

@jhartzell42 jhartzell42 changed the title WIP: Refactor to decouple consensus modes Refactor to decouple consensus modes Mar 24, 2021
nfrisby
nfrisby requested changes Mar 25, 2021
View reviewed changes
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.

I've reviewed some key parts, and I made some relative large requests, so that's probably plenty for us to work through now. (And it's getting late here.)

I see you're in the US Eastern timezone, so we can relatively easily schedule a call if you think the synchronous bandwidth would help. Thanks much!

I hope to be more responsive now that I've ramped up on the overall intent and looked at the some of the key details; I think my review of the remaining code will mostly be a matter of confirming the copy-paste nature of the relocated Shelley-HFC code.

ouroboros-consensus/src/Ouroboros/Consensus/HardFork.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Protocol.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Protocol.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Protocol.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Protocol.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Protocol.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Protocol.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Protocol.hs Outdated Show resolved Hide resolved
ouroboros-consensus-shelley/ouroboros-consensus-shelley.cabal Outdated Show resolved Hide resolved
@jhartzell42 jhartzell42 force-pushed the os/first-pr branch 2 times, most recently from d44acbe to 9884916 Compare March 29, 2021 16:13
nfrisby
nfrisby requested changes Mar 29, 2021
View reviewed changes
Copy link
Contributor

@nfrisby nfrisby left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Phew! I used some throwaway awk and Haskell scripting to confirm that the relocations were indeed only relocations. All the code changes look good. I didn't check the export lists or module pragmas as carefully, but those are less important and easy to fixup.

Other than that, I made some StyleGuide.md-based comments throughout. Thank you for your patience.

(For the record, I applied these semi-automated checks to your commit 9884916, after rebasing it onto origin/master; the rebase was automatic 👍.)

I don't think there's much more work to be done, once you've validated this PR by getting a working cardano-node PR in place 👍

@jhartzell42 jhartzell42 force-pushed the os/first-pr branch 2 times, most recently from 32285a5 to ec9cc1c Compare March 30, 2021 18:15
Refactor to decouple consensus modes
0698994 
- Functionality that handles Shelley-based eras in a multi-era consensus
  is moved to ouroboros-consensus-shelley so that it can be shared
  between mainnet consensus (in ouroboros-consensus-cardano) and other
  consensus modes comprising multiple Shelley eras.
- The `Protocol` GADT that enumerates consensus modes and provides a
  wealth of information for clients is eliminated. It is an
  implementation detail only used by `cardano-node`. This way,
  `ouroboros-consensus-cardano` does not need to know about other
  consensus modes.
nfrisby
nfrisby reviewed Mar 31, 2021
View reviewed changes
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.

LGTM! Thanks for all those fixups.

Please update

  • the commit message
  • and also the PR's description -- bors or GitHub (I'm not sure which) copies that into the merge commit, so it should be accurate too.

They both mention the Protocol class etc which is no longer in this PR.

I'll Approve after that.

@nfrisby
Copy link
Contributor

nfrisby commented Mar 31, 2021

and also the PR's description

Though for a single commit PR like this one, I wouldn't mind "single commit; see its message" as the entirely of the PR description. Please choose whatever point along that spectrum you feel is appropriate.

nfrisby
nfrisby approved these changes Mar 31, 2021
View reviewed changes
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.

Thanks for this PR! It's a good refactoring.

I've clicked Approve.

@jhartzell42
Copy link
Contributor Author

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 31, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 66dd5eb into master Mar 31, 2021
@iohk-bors iohk-bors bot deleted the os/first-pr branch March 31, 2021 16:18
iohk-bors bot added a commit to IntersectMBO/cardano-node that referenced this pull request Apr 22, 2021
@iohk-bors @madeline-os
Merge #2498
a593cec 
2498: Adapt packages to use Protocol type class r=Jimbo4350 a=jhartzell42

This PR depends on IntersectMBO/ouroboros-network#2978

In that PR, the `Protocol` type that the cardano-node packages use is removed. This is so that ouroboros-network does not need to enumerate all the consensus modes it implements. The majority of changes in this PR are in reaction to that, creating a type class to replace it.

We found that it would take a significant increase in code complexity to remove the explicit enumeration of supported consensus modes in two places: 
- cardano-node/src/Cardano/Node/Configuration/Logging.hs:nodeBasicInfo
- cardano-node/src/Cardano/Node/Query.hs

For that purpose, we have written the GADT `BlockType` which provides the little bit of type discovery these places need. 

In addition, we have removed the `Enum` instance for `AnyCardanoEra`. The motivation is that we anticipate the inclusion of experimental eras and consensus modes in the code base which cannot reasonably be said to be enumerable among the mainnet eras. This instance does not appear to be used anywhere. If it needs to be preserved then we will need an answer on how to handle eras we don't expect to be part of mainnet (at least when they are first implemented).

Co-authored-by: Madeline Haraj <madeline.haraj@obsidian.systems>
@jorisdral jorisdral mentioned this pull request May 11, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nfrisby nfrisby nfrisby approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jhartzell42 @nfrisby