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

For node 10.0 release: Integrate ledger & o-network #1262

Merged
merged 8 commits into from
Oct 14, 2024
Merged

For node 10.0 release: Integrate ledger & o-network #1262

merged 8 commits into from
Oct 14, 2024

Conversation

lehins
Copy link
Contributor

@lehins lehins commented Sep 24, 2024 •
edited by crocodile-dentist
Loading

Description

This PR integrates latest ledger and o-network for node 10.0 release. Notably, the following changed:

  • Support for NodeToNodeV_12 and below was removed from o-network as these are unable to cross the HF boundary
  • NodeToNodeV_14 was added to supported versions to track adoption rate
  • o-network does not track pipelining support as this was only necessary in previous NodeToNode versions. To preserve generality of consensus, this functionality was moved here.
  • Added new GetFuturePParams query
  • Account for removal of minVersion in SL.Globals
  • o-network and ledger package bumps

@lehins lehins marked this pull request as draft September 24, 2024 16:03
@crocodile-dentist crocodile-dentist force-pushed the lehins/integrate-latest-ledger branch 5 times, most recently from b0b9371 to 333f5e6 Compare October 3, 2024 10:42
@crocodile-dentist crocodile-dentist changed the title Integrate latest ledger Node 9.3 release: Integrate ledger & o-network Oct 3, 2024
@crocodile-dentist crocodile-dentist changed the title Node 9.3 release: Integrate ledger & o-network For node 9.3 release: Integrate ledger & o-network Oct 3, 2024
@crocodile-dentist crocodile-dentist force-pushed the lehins/integrate-latest-ledger branch 2 times, most recently from 9c3dca7 to 65466f8 Compare October 11, 2024 09:58
@crocodile-dentist crocodile-dentist changed the title For node 9.3 release: Integrate ledger & o-network For node 10.0 release: Integrate ledger & o-network Oct 11, 2024
@crocodile-dentist crocodile-dentist marked this pull request as ready for review October 11, 2024 14:48
@crocodile-dentist crocodile-dentist force-pushed the lehins/integrate-latest-ledger branch 2 times, most recently from 90b79c5 to d6fd467 Compare October 11, 2024 16:20
amesgen
amesgen reviewed Oct 11, 2024
View reviewed changes
Copy link
Member

@amesgen amesgen left a comment

Choose a reason for hiding this comment

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

LGTM!

Only minor comments, which shouldn't be hard to address, but can could also be postponed to a later PR

Also needs changelog entries

...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs Outdated Show resolved Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs Outdated Show resolved Hide resolved
...sensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Network/NodeToNode.hs Outdated Show resolved Hide resolved
ouroboros-consensus-cardano/src/ouroboros-consensus-cardano/Ouroboros/Consensus/Cardano/Node.hs Outdated Show resolved Hide resolved
nfrisby
nfrisby previously requested changes Oct 11, 2024
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'm Requesting Changes only because there are couple TODO-ish comments that should not be merged.

ouroboros-consensus-cardano/src/shelley/Ouroboros/Consensus/Shelley/Ledger/Mempool.hs Outdated Show resolved Hide resolved
...s-consensus-cardano/src/shelley/Ouroboros/Consensus/Shelley/Ledger/NetworkProtocolVersion.hs Outdated Show resolved Hide resolved
...sensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Network/NodeToNode.hs Outdated Show resolved Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs Outdated Show resolved Hide resolved
@amesgen amesgen mentioned this pull request Oct 11, 2024
Merged
@nfrisby
Copy link
Contributor

nfrisby commented Oct 11, 2024

For the record, I know you'll be working on this first thing Monday. But Monday is a holiday for me.

So please feel free to dismiss my "Changes requested" review once the concerns described in my comments have been addressed. cc @dnadales @crocodile-dentist

@crocodile-dentist crocodile-dentist force-pushed the lehins/integrate-latest-ledger branch 3 times, most recently from 24443fb to 3e22cb5 Compare October 14, 2024 08:20
@crocodile-dentist crocodile-dentist force-pushed the lehins/integrate-latest-ledger branch 2 times, most recently from cc1e387 to 6ef51d7 Compare October 14, 2024 10:00
@crocodile-dentist crocodile-dentist force-pushed the lehins/integrate-latest-ledger branch 9 times, most recently from 4f769a8 to a37dbed Compare October 14, 2024 15:03
lehins and others added 8 commits October 14, 2024 18:18
@lehins @amesgen
Account for removal of minVersion from Globals
9e1e181 
Fix bounds on ledger packages

integrate latest ledger
@crocodile-dentist @amesgen
Fix bounds on network packages
7788686 
Remove support for NodeToNodeV_12 and earlier

Integrate o-network

Add NodeToClientV_18

Bump latest supported o-network protocol versions

simplify `SupportedNetworkProtocolVersion ByronBlock`
@crocodile-dentist @amesgen
Hardcode some constants for tests
3500b66
@dnadales @amesgen
Add haddock comments to maxTxSizeUTxO
b9a6164
@amesgen
Bump CHaP flake input
22726b2
@dnadales @amesgen
Add a new GetFuturePParams query
cc65689
@dnadales @amesgen
Add files for the GetFuturePParams query golden tests
721b338
@crocodile-dentist @amesgen
changelog updates
770e9a0
@amesgen amesgen force-pushed the lehins/integrate-latest-ledger branch from a37dbed to 770e9a0 Compare October 14, 2024 16:19
@amesgen amesgen dismissed nfrisby’s stale review October 14, 2024 16:20

Comments have been addressed

@amesgen amesgen mentioned this pull request Oct 14, 2024
Merged
@amesgen amesgen added this pull request to the merge queue Oct 14, 2024
Merged via the queue into main with commit c8b83a6 Oct 14, 2024
20 of 21 checks passed
@amesgen amesgen deleted the lehins/integrate-latest-ledger branch October 14, 2024 22:25
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

@nfrisby nfrisby nfrisby left review comments

@crocodile-dentist crocodile-dentist crocodile-dentist left review comments

@amesgen amesgen amesgen approved these changes

@jasagredo jasagredo Awaiting requested review from jasagredo jasagredo is a code owner

@fraser-iohk fraser-iohk Awaiting requested review from fraser-iohk fraser-iohk is a code owner

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

Successfully merging this pull request may close these issues.
5 participants
@lehins @nfrisby @dnadales @amesgen @crocodile-dentist