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

Integrate the Conway era #3971

Merged
merged 1 commit into from
Feb 1, 2023
Merged

Integrate the Conway era #3971

merged 1 commit into from
Feb 1, 2023

Conversation

nfrisby
Copy link
Contributor

@nfrisby nfrisby commented Aug 21, 2022 •
edited by dnadales
Loading

Fixes #3962. Integrates the Conway era.

Checklist

  • Branch
    • Commit sequence broadly makes sense
    • Commits have useful messages
    • New tests are added if needed and existing tests are updated
    • If this branch changes Consensus and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
  • Pull Request
    • Self-reviewed the diff
    • Useful pull request description at least containing the following information:
      • What does this PR change?
      • Why these changes were needed?
      • How does this affect downstream repositories and/or end-users?
      • Which ticket does this PR close (if any)? If it does, is it linked?
    • Reviewer requested

@nfrisby nfrisby added consensus issues related to ouroboros-consensus conway Conway era + chang hard fork labels Aug 21, 2022
nfrisby
nfrisby commented Aug 21, 2022
View reviewed changes
ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Node/Praos.hs Outdated
} =
protocolInfoPraosShelleyBased
protocolParamsShelleyBased
(error "Conway currently pretending to be Alonzo", error "Conway currently pretending to be Alonzo")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO I don't understand this; just copied it from Babbage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fixed this now.

I still don't understand why it was that way. I've simply added those stubs as arguments to the function being defined. I'm fairly certain that function is currently dead-code within the greater codebase, so this shouldn't induce any work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm Unresolving this, just to double-check it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've come to the same conclusion just now as I did in my Sept 7th message earlier in this same comment thread: this function is likely dead, but some downstream 3rd-party might be using it; as a compromise, I've added the stubbed value as an argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO for @dnadales :

  • mention this additional parameter in the INTERFACE-changelog.md

  • or else make the executive decision to simply remove this function (and also the corresponding new Conway one) instead of adding an argument to it. (Also mention that in the changelog)

@nfrisby nfrisby changed the title Nfrisby/issue 3962 conway Integrate the Conway era Aug 30, 2022
@dnadales
Copy link
Member

Do we need to add the golden files for the new era?

@nfrisby nfrisby force-pushed the nfrisby/issue-3962-conway branch 3 times, most recently from c6ce33b to f7b5f92 Compare September 6, 2022 23:44
@nfrisby
Copy link
Contributor Author

nfrisby commented Sep 7, 2022

The three remaining CI failures are because we don't have a ConwayGenesis.json file yet for the cardano-tools test suite. I've pinged Jared and Jordan and John Ky for their thoughts on that.

@nfrisby
Copy link
Contributor Author

nfrisby commented Sep 7, 2022

A status update. I think this PR is now feature complete. However, the commit history remains messy and there are likely some comments to add.

@JaredCorduan
Copy link
Contributor

The three remaining CI failures are because we don't have a ConwayGenesis.json file yet for the cardano-tools test suite. I've pinged Jared and Jordan and John Ky for their thoughts on that.

I think that this ledger PR IntersectMBO/cardano-ledger#3022 should unblock John's work to add a ConwayGenesis.json.

@nfrisby
Copy link
Contributor Author

nfrisby commented Sep 20, 2022

Update: until we get further notice, the Consensus team is going to park this Conway PR.
Please tell us if you need anything along these lines, if you are continuing to work on Conway-related things.

@JaredCorduan
Copy link
Contributor

@nfrisby is there any reason not to merge it now? it would be nice to have the new era integrated all the way into node.

@dnadales dnadales self-assigned this Sep 28, 2022
@dnadales dnadales force-pushed the nfrisby/issue-3962-conway branch 2 times, most recently from a4fa369 to 5f46f41 Compare October 7, 2022 16:07
@dnadales
Copy link
Member

dnadales commented Oct 10, 2022 •
edited
Loading

@disassembler @JaredCorduan @nfrisby I think these are the tasks that remain:

  • Review the pending comments (@nfrisby and I should do this)
  • Review the new commits I added.
  • Add b9e5768 to this PR

I do not know if there are additional tasks that I did not capture.

nfrisby
nfrisby commented Nov 22, 2022
View reviewed changes
Copy link
Contributor Author

@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 did a pass. (I would Request Changes, but since I originally opened the PR, I can't 😄)

ouroboros-consensus-cardano-tools/src/Cardano/Tools/DBAnalyser/Block/Cardano.hs Show resolved Hide resolved
ouroboros-consensus-cardano-tools/src/Cardano/Tools/DBAnalyser/Block/Cardano.hs Show resolved Hide resolved
ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Node/Praos.hs Outdated Show resolved Hide resolved
ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Node/Praos.hs Outdated
} =
protocolInfoPraosShelleyBased
protocolParamsShelleyBased
(error "Conway currently pretending to be Alonzo", error "Conway currently pretending to be Alonzo")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm Unresolving this, just to double-check it again.

ouroboros-network/src/Ouroboros/Network/Diffusion/P2P.hs Outdated Show resolved Hide resolved
ouroboros-consensus-cardano-tools/src/Cardano/Node/Protocol/Conway.hs Show resolved Hide resolved
ouroboros-consensus-cardano-tools/test/disk/config/config.json Show resolved Hide resolved
ouroboros-consensus-cardano-tools/test/disk/config/genesis-conway.json Show resolved Hide resolved
ouroboros-consensus-cardano/src/Ouroboros/Consensus/Cardano/Node.hs Outdated Show resolved Hide resolved
ouroboros-consensus-cardano-tools/src/Cardano/Node/Protocol/Cardano.hs Outdated Show resolved Hide resolved
@dnadales dnadales mentioned this pull request Dec 21, 2022
Merged
9 tasks
@dnadales dnadales marked this pull request as ready for review December 29, 2022 13:14
nfrisby
nfrisby commented Jan 18, 2023
View reviewed changes
ouroboros-consensus-cardano/src/Ouroboros/Consensus/Cardano/Node.hs Outdated
@@ -468,7 +522,8 @@ instance CardanoHardForkConstraints c
[ (NodeToNodeV_7, CardanoNodeToNodeVersion5)
, (NodeToNodeV_8, CardanoNodeToNodeVersion5)
, (NodeToNodeV_9, CardanoNodeToNodeVersion6)
, (NodeToNodeV_10, CardanoNodeToNodeVersion6)
, (NodeToNodeV_10, CardanoNodeToNodeVersion7)
, (NodeToNodeV_11, CardanoNodeToNodeVersion7) -- TODO: check this!
Copy link
Contributor Author

@nfrisby nfrisby Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the -- TODO: check this! can be removed -- this mapping looks correct to me: V11 enables Conway.

... But the tuple above should stay as (NodeToNodeV_10, CardanoNodeToNodeVersion6), right?

Copy link
Member

Choose a reason for hiding this comment

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

I think so, but I'll confirm with @coot

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's not modify what NodeToNodeV_10 (it will be released in 1.35.5).

We also introduce NodeToNodeV_11 in #4019 - just to note that there will be a conflict on one side or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just Approved this PR; this comment is the only one that wasn't Resolved. I'll let @dnadales resolve it, just to make sure he has seen Marcin's warning.

coot
coot reviewed Jan 20, 2023
View reviewed changes
Copy link
Contributor

@coot coot 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.

When adding new NodeToNodeVersion (and NodeToClientVersion), I'd expect also a modifiction of this instance.

nfrisby
nfrisby commented Jan 24, 2023
View reviewed changes
Copy link
Contributor Author

@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 would Approve this PR if I could. I can't because I opened it. But @dnadales owns it now.

@nfrisby
Copy link
Contributor Author

nfrisby commented Jan 31, 2023

I hate to add another speedbump here, but: if we rebase this PR we should also add a (very celebratory!) scrivy entry 👍

@nfrisby nfrisby mentioned this pull request Jan 31, 2023
Open
@nfrisby @dnadales
Add the Conway era
903bcc6 
- cardano-test: update golden tests for Conway.
- Bump maxMajorProtVer to Conway's version.
- Comment on why certain eras do not need genesis data.
- Remove instructions to update `ShelleyBasedEras` since this is no.
  longer defined. We have a `ShelleyBasedEra` symbol, but that refers to a
  type class.
- Refer to the PR that adds Conway in 'AddingAnEra.md'
- Update out of date haddock for 'ProtocolTransitionParamsShelleyBased'
  and add a FIXME that should be addressed before this PR is merged.
- Remove reference to non-existing data structure. I do not know if we should mention
  'ProtocolTransitionParamsShelleyBased' here instead.
- Update the `ProtocolCardano` type synonym
- Extend changelog with entry describing the Conway era addition
- Add a `PerEraAnalysis` for Conway to `db-analyser`.
coot
coot approved these changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

LGTM

@dnadales
Copy link
Member

dnadales commented Feb 1, 2023

bors r+

iohk-bors bot added a commit that referenced this pull request Feb 1, 2023
@iohk-bors @nfrisby
Merge #3971
41cdb42 
3971: Integrate the Conway era r=dnadales a=nfrisby

Fixes #3962. Integrates the Conway era.



Co-authored-by: Nicolas Frisby <nick.frisby@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 1, 2023

Timed out.

@dnadales
Copy link
Member

dnadales commented Feb 1, 2023

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 1, 2023

Build succeeded:

@iohk-bors iohk-bors bot merged commit 6085c03 into master Feb 1, 2023
@iohk-bors iohk-bors bot deleted the nfrisby/issue-3962-conway branch February 1, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnadales dnadales dnadales left review comments

@JaredCorduan JaredCorduan JaredCorduan left review comments

@lehins lehins lehins left review comments

@coot coot coot approved these changes

@bolt12 bolt12 Awaiting requested review from bolt12

Assignees

@dnadales dnadales

Labels
consensus issues related to ouroboros-consensus conway Conway era + chang hard fork
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integrate the Conway era
5 participants
@nfrisby @dnadales @JaredCorduan @coot @lehins