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 network & consensus packages #4155

Merged
merged 12 commits into from
Nov 27, 2022
Merged

Refactor network & consensus packages #4155

merged 12 commits into from
Nov 27, 2022

Conversation

coot
Copy link
Contributor

@coot coot commented Nov 17, 2022 •
edited
Loading

A refactorisation of ouroboros-network and ouroboros-consensus. It includes the following packages:

  • ouroboros-network-api - no other ouroboros depenencies
  • ouroboros-network-framework
  • ouroboros-network-mock - mock chain
  • ouroboros-network-protocols - implementation of all (except Handshake) mini-protocols, also includes an ouroboros-network-protocols:testlib visible testing library.
  • ouroboros-network - peer selection & diffusion
  • ouroboros-consensus-diffusion - the top level package which was created out of ouroboros-consensus

pkgs

@coot coot requested a review from nfrisby November 17, 2022 13:07
@ghost
Copy link

ghost commented Nov 17, 2022

The idea is to have a separate package that ties the know between network and consensus? Seems like a good idea to me.

@coot
Copy link
Contributor Author

coot commented Nov 17, 2022

There are two other that I'd like to experiment with:

  • ouroboros-network-protocols - includes implementation of all the protocols
  • ouroboros-network-consensus - includes API shared between network & consensus

The idea is that ouroboros-consensus would only depend on the two packages, but not on ouroboros-network.

@coot coot force-pushed the coot/consensus-split branch 3 times, most recently from 0974ec4 to ec00c35 Compare November 18, 2022 15:18
@coot coot marked this pull request as ready for review November 18, 2022 15:33
@coot coot requested review from newhoggy, a team, dnadales and bolt12 as code owners November 18, 2022 15:33
@ghost
Copy link

ghost commented Nov 18, 2022

I am not too sure what's behind each package, it would help to group them as Network and Consensus I guess.
My intuition is that the Consensus component and the Network component should depend on some Interface package that contains only the "interfaces" between the 2 components, and the "wiring" would be done in another component, possibly in the test packages or the node.
But perhaps this is a different line of thought than yours @coot ?

@coot
Copy link
Contributor Author

coot commented Nov 18, 2022

Future tasks:

  • the network team should move the simulated snocket into its own package, this might remove the ouroboros-network-framework -> ouroboros-network-testing dependency
  • the consensus team might want to try remove the ouroboros-consensus -> ouroboros-network-mock dependency

Then the production code will be isolated from testing.

@coot
Copy link
Contributor Author

coot commented Nov 18, 2022 •
edited
Loading

But perhaps this is a different line of thought than yours @coot?

The API used by ouroboros-consensus is isolated in ouroboros-network-api package. ouroboros-consensus depends on:

  • ouroboros-network-api
  • ouroboros-network-protocols

This is a bit masked (in the graph above) by the fact that ouroboros-network-protocols also depends on ouroboros-network-framework. I should look if it's easy to solve.

@coot
Copy link
Contributor Author

coot commented Nov 18, 2022

I am not too sure what's behind each package, it would help to group them as Network and Consensus I guess.

They are grouped using naming convetion: all network packages match: ouroboros-network*
All cosnensus packages match ouroboros-cosnensus*. Plus the top level ouroboros-node. There are also generic packages like:

  • network-mux
  • monoidal-synchronisation
  • ntp-client
  • cardano-client - top level integration package for cardano-db-sync and maybe cardano-wallet

The first two will at some point be put in seprate git repos and published on Hackage.

@coot
Copy link
Contributor Author

coot commented Nov 18, 2022

Updated graph
pkgs

The last commit made ouroboros-consensus independent of ouroboros-network-framework. If the consensus team will remove its dependency on ouroboros-network-mock (which is only for some testing stuff) then the dependency graph of ouroboros-consensus will be even clearer.

@coot
Copy link
Contributor Author

coot commented Nov 18, 2022

ouroboros-network-api pulls TraceLabelPeer type from network-mux (no reason to solve this dependency).

@coot
Copy link
Contributor Author

coot commented Nov 18, 2022

Another remark:
ouroboros-consensus-test depends on ouroboros-network-framework because it pulls the Channel abstraction.

@coot
Copy link
Contributor Author

coot commented Nov 18, 2022 •
edited
Loading

Requires #4159 (to fix github windows CI)

@coot coot requested a review from njd42 as a code owner November 18, 2022 18:38
@coot coot removed request for newhoggy and njd42 November 19, 2022 08:19
@coot coot force-pushed the coot/consensus-split branch 6 times, most recently from 743187f to a91fed4 Compare November 22, 2022 06:57
@coot
Copy link
Contributor Author

coot commented Nov 23, 2022

@bolt12 I merged the two packages.

@coot coot force-pushed the coot/consensus-split branch from 601417b to 2cd0673 Compare November 23, 2022 16:18
nfrisby
nfrisby requested changes Nov 23, 2022
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.

Thanks for addressing my comments.

There's one left: the typo in PR description (I commented there to try to clarify). I care because these end up in the merge commit messages.

Edit: this one #4155 (comment)

@amesgen amesgen mentioned this pull request Nov 24, 2022
Open
@coot coot force-pushed the coot/consensus-split branch 2 times, most recently from c9baec0 to 1a6f3ca Compare November 24, 2022 21:47
@coot coot changed the title Split Ouroboros-Consensus Refactor network & consensus packages Nov 24, 2022
@coot coot force-pushed the coot/consensus-split branch from 1a6f3ca to 63ea756 Compare November 25, 2022 07:19
@coot
Copy link
Contributor Author

coot commented Nov 25, 2022

I fixed ghc-9.2 support. The -Wunused-packages was improved in 9.2 (as compared to 8.10).

@coot
Copy link
Contributor Author

coot commented Nov 25, 2022

I also deleted the script/test.sh.

nfrisby
nfrisby approved these changes Nov 25, 2022
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.

Approved 👏

ouroboros-network-api/src/Ouroboros/Network/SizeInBytes.hs
@@ -0,0 +1,5 @@
module Ouroboros.Network.SizeInBytes (SizeInBytes) where
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to Approve without this being resolved.

coot added 7 commits November 25, 2022 22:54
@coot
Moved ipv6 cabal flag to ouroboros-network-framework
eed81fd
@coot
ouroboros-consensus-diffusion package
f0cd4af 
Top level integration of ouroboros-consensus & ouroboros-network.
@coot
ouroboros-network-protocols package
01e0bd9 
`ouroboros-network-protocols` contains implementation of all
mini-protocols except for `Handshake`.  It contains three components:

* `ouroboros-network-protocols`         - the main library
* `ouroboros-network-protocols:testlib` - a public testing library
* `ouroboros-network-protocols:test`    - the testing component
@coot
ouroboros-network-{api,mock} packages
3a2c7c2 
* `ouroboros-network-mock` contains a testing chain.
* `ouroboros-network-api` contains shared api between various
  `ouroboros-network*` packages and the `ouroboros-consensus` package.
@coot
Make ouroboros-network-api independent of `ouroboros-network-framew…
a9ab8d9 
…ork`

This way `ouroboros-network-protocols` does not contain
`ouroboros-network-framework` as its transitive dependency.
@coot
stylish-haskell: refactor the script
2642d8d
@coot
Updated nix & network-spec
4c75803
@coot coot force-pushed the coot/consensus-split branch from 63ea756 to c907007 Compare November 25, 2022 22:27
@coot coot force-pushed the coot/consensus-split branch from c907007 to 12d8a1d Compare November 26, 2022 17:07
@coot
Copy link
Contributor Author

coot commented Nov 27, 2022

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 27, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit 2793b69 into master Nov 27, 2022
@iohk-bors iohk-bors bot deleted the coot/consensus-split branch November 27, 2022 15:08
@coot coot mentioned this pull request Jun 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amesgen amesgen amesgen left review comments

@nfrisby nfrisby nfrisby approved these changes

@bolt12 bolt12 bolt12 approved these changes

@dnadales dnadales Awaiting requested review from dnadales

Assignees
No one assigned
Labels
consensus issues related to ouroboros-consensus technical debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@coot @nfrisby @amesgen @bolt12