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

Extensible Ouroboros Network Diffusion Stack #5016

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bolt12
Copy link
Contributor

@bolt12 bolt12 commented Nov 27, 2024

Description

This PR refactors ouroboros-network in order to make diffusion layer general/polymorphic and extensible. This will allow us (or 3rd parties) to have access to a flexible network stack which can be extended/configured to ones needs.

For more details about this refactoring, see https://github.com/IntersectMBO/ouroboros-network/wiki/Reusable-Diffusion-Investigation

TODOs:

  • Better document PRP invariants and refactor/cleanup insertion functions
  • remove daBlockFetchMode
  • Consensus uses a entirely different project for Cardano Node consensus layer instantiation, decide what's the best approach for the networking code.

For next steps (e,.g. when developing mithril) this will be in the todo list:

  • Add tests for the general components: Churn, OG, etc..
  • Generalise peerChurnGovernor to only have one implementation and not 2
  • Redesign bootstrap peers as a concept to be general enough to be usable by third party users. Right now it is very cardano-node specific.
  • Create a sub library that holds all cardano-node specific types and functions, this will be better for organising dependencies and the code overall
  • Move Cardano specific things from cardano-node, we should think of moving some of the other things to ouroboros-network as well, e.g. topology file format for example. 3rd parties don't need to reimplement it from scratch.

@bolt12 bolt12 added the diffusion Issues / PRs related to diffusion layer label Nov 27, 2024
@bolt12 bolt12 self-assigned this Nov 27, 2024
@bolt12 bolt12 requested a review from a team as a code owner November 27, 2024 13:10
@bolt12 bolt12 force-pushed the bolt12/reusable-diffusion-3 branch 6 times, most recently from 3cdb9a3 to f00952f Compare November 27, 2024 16:53
@bolt12 bolt12 force-pushed the bolt12/reusable-diffusion-3 branch from c14d5cb to 3b30f43 Compare December 2, 2024 16:42
@yihuang
Copy link

yihuang commented Dec 4, 2024

a step toward haskell blockchain sdk? 😄

@bolt12
Copy link
Contributor Author

bolt12 commented Dec 5, 2024

@yihuang as you can see from the PR description this is some interesting step towards a fully polymorphic diffusion that might be used by others to create their own overlay network! Thank you for noticing 😃

@bolt12 bolt12 force-pushed the bolt12/reusable-diffusion-3 branch 6 times, most recently from f922210 to 3cdf98d Compare December 9, 2024 10:09
@bolt12 bolt12 force-pushed the bolt12/reusable-diffusion-3 branch 5 times, most recently from ce99e1d to 5400a7c Compare January 7, 2025 12:07
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.

I reviewed the first two patches:

git hash commit title
bae4f75 Pull out Cardano specifics to different modules
cb0da21 Add extension points to Diffusion data structures

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.

ouroboros-network/src/Cardano/Diffusion/P2P.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Cardano/PeerSelection/Churn.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
@coot coot changed the title Reusable Diffusion - Make diffusion fully polymorphic Extensible Ouroboros Network Diffusion Stack Jan 20, 2025
@bolt12 bolt12 force-pushed the bolt12/reusable-diffusion-3 branch 2 times, most recently from f4e996b to d048514 Compare January 21, 2025 10:52
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.

This is next batch of review comments...

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.

Next batch of comments. I am only left with one non-testing module.

@bolt12 bolt12 force-pushed the bolt12/reusable-diffusion-3 branch 3 times, most recently from 0935d40 to 9a2980f Compare February 4, 2025 12:02
@bolt12 bolt12 force-pushed the bolt12/reusable-diffusion-3 branch from 9a2980f to e1b0745 Compare February 6, 2025 09:43
@bolt12 bolt12 requested a review from a team as a code owner February 6, 2025 14:44
-- | For Genesis, this sets the floor for minimum number of
-- active big ledger peers we must be connected to in order
-- to be able to signal trusted state (OutboundConnectionsState)
, numberOfBigLedgerPeers :: NumberOfBigLedgerPeers
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think just adding numberOfBigLedgerPeers is enough. If the target for established peers is insufficient, then we won't have a chance to achieve the target of big ledger peers.

I think in main we pass all the targets, not just active ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this somehow related to genesisPeerTargets from Ouroboros.Cardano.Network.PeerSeledction.Churn.ExtraArguments?

If it is the same, we should organise the code better. E.g. pass genesisPeerTargets through AgumentsExtra here rather than the Churn ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the same but this one in ArgumentsExtra comes from configuration file which is going to be passed to Churn.ExtraArguments

peerconn
BootstrapPeersCriticalTimeoutError
m
cardanoPeerSelectionGovernorArgs readUseLedgerPeers peerSharing updateOutboundConnectionsState =
Copy link
Contributor

Choose a reason for hiding this comment

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

It's something wrong if we need to call it in cardano-node. It seems the arguments in Cardano diffusion are too general. Please investigate how to call this function inside of Cardano diffusion.

@@ -366,6 +367,16 @@ data PeerSelectionActions peeraddr peerconn m = PeerSelectionActions {
--
peerConnToPeerSharing :: peerconn -> PeerSharing,

-- | Public Extra Peers Actions
--
extraPeersAPI :: PublicExtraPeersAPI extraPeers peeraddr,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I confused PeerSelectionActions, with PeerStateActions. It's fine to pass it as part of the former. It's already a mixture of things.


-- | Extension point for third party users to be able to add more
-- arguments.
, daExtraArgs :: extraArgs
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used anywhere? I don't see how it is used inside Ouroboros.Network.Diffusion.P2P.run function. This indicates that it could be removed.

(See CHANGELOG for more details)

- Extract Cardano-specific components into separate modules to resolve cyclic dependency issues:
  - `ConsensusModePeerTargets` and `PeerSelectionTargets` were refactored to avoid circular dependencies.

- Introduce extension points in Diffusion data structures:
  - Split P2P functionality into `P2PCardano` (for Cardano Node specifics) and a more general `P2P` module.
  - Added `Minimal/Node.hs` as a placeholder for a minimal Node diffusion instantiation example.

- Refactor Peer Selection:
  - Removed redundant `PeerSelectionActionsArgs`.
  - Reorganized `DNSActions` and `LedgerPeersArgs`.
  - Improved API for `PeerSelectionActions`.

- Rename `Cardano.Node` to `Cardano.Network`.

- Generalize and refactor the following components:
  - `ConsensusModePeerTargets`.
  - `PeerSelection{Views, Counters}` to allow extensibility for third-party users.
  - `PublicRootPeers`.

- Enhance the Outbound Governor:
  - Generalize `MinimalP2P` for diffusion initialization.
  - Update `ArgumentsExtra` with new parameters for a more flexible diffusion setup.
  - Refactor `PeerSelection.Governor.Monitor` to separate Cardano-specific monitoring actions and support third-party custom actions.

- Ensure the `Diffusion` module is fully polymorphic:
  - Removed `CardanoP2P` dependencies.
  - Updated the test suite to align with the new structure.

- Miscellaneous updates:
  - Update CHaP and fix build issues.
  - Remove `daBlockFetchMode`.
  - Address review feedback.
  - Fix churn timeout tests and increase `shortDelay` to stabilize tests.
  - Recover `sigusr1` signal handler.
  - Make addresses polymorphic.
  - Refactor `extraDebugState` and introduce `P2PDecisionType`.
  - Refactor `requestPublicRootPeers` type.
@bolt12 bolt12 force-pushed the bolt12/reusable-diffusion-3 branch from 44a505f to a84ba63 Compare February 7, 2025 11:19
@bolt12 bolt12 force-pushed the bolt12/reusable-diffusion-3 branch from a84ba63 to 7e02959 Compare February 7, 2025 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diffusion Issues / PRs related to diffusion layer
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants