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

Diffusion improvements #5070

Merged
merged 7 commits into from
Feb 11, 2025
Merged

Diffusion improvements #5070

merged 7 commits into from
Feb 11, 2025

Conversation

coot
Copy link
Contributor

@coot coot commented Feb 6, 2025

Description

  • diffusion: removed daExtraArgs from ArgumentsExtra
  • diffusion: removed extraActions

There will be more...

Checklist

Quality

  • Commit sequence makes sense and have useful messages, see ref.
  • New tests are added and existing tests are updated.
  • Self-reviewed the PR.

Maintenance

  • Linked an issue or added the PR to the current sprint of ouroboros-network project.
  • Added labels.
  • Updated changelog files.
  • The documentation has been properly updated, see ref.

@coot coot requested a review from a team as a code owner February 6, 2025 20:04
@coot coot force-pushed the coot/bolt12/reusable-diffusion-3 branch from 987d6c9 to cf13ae7 Compare February 7, 2025 08:34
@bolt12 bolt12 force-pushed the bolt12/reusable-diffusion-3 branch from 44a505f to a84ba63 Compare February 7, 2025 11:19
@bolt12 bolt12 requested a review from a team as a code owner February 7, 2025 11:19
bolt12
bolt12 reviewed Feb 7, 2025
View reviewed changes
Copy link
Contributor

@bolt12 bolt12 left a comment

Choose a reason for hiding this comment

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

I don't think my PR is leaking network details. It is setting up a different API. Currently, cardano-node is only one possible client of our diffusion so we must not entirely tailor our API to it, we need to be sensible for others too. For instance, I'd prefer that the extension points were all in the same top level/entry data types and then the diffusion library would make the plumbing itself, also from a type signature perspective it is more clear to me where I should have my extension data types.

Removing extraActions just because they can be computed from extraArgs in the cardano node case doesn't mean it is going to be the same case for all other applications. So we might be simplifying things for cardano-node case and making things difficult for the others...

The same for the extraAPI ... for cardano-node it is easy to optimise for, but I don't like removing the extension point from the main Consensus API data type LedgerConsensusInterface because it makes it harder (imo) to see where one would extend the API.

I don't disagree with these changes but I think they are missing the bigger picture. We can improve our API without making it worse for non-cardano clients.

@bolt12 bolt12 force-pushed the bolt12/reusable-diffusion-3 branch from a84ba63 to 7e02959 Compare February 7, 2025 11:43
@coot coot force-pushed the coot/bolt12/reusable-diffusion-3 branch from 68204a5 to eb5f6a5 Compare February 10, 2025 16:13
@coot coot force-pushed the coot/bolt12/reusable-diffusion-3 branch from eb5f6a5 to 6a1b9d2 Compare February 11, 2025 07:51
coot added 6 commits February 11, 2025 10:53
@coot
diffusion: removed extraActions
be414d5 
They are passed as a closure, no need to leak them outside of `Cardano`
(or any other 3rd party integration).
@coot
diffusion: removed Applications wrapper
817e52c 
Instead of specifying `extraAPI ~ ()` for non-p2p, we can make it
polymorphic.  This way both p2p & non-p2 `Application` type is using the
same type variables, and the wrapper type can be removed.  This is more
in-line with removal of non-p2p code base.
@coot
diffusion: fix confused type variables
65cd0dc 
Also adhere to the convention: first `extraFlags`, then `extraPeers`
@coot
diffusion: code formatting
7ebd959
@coot
peer-selection: updateWithState
3c2605e 
We can pass `PeerSelectionInterfaces` to `updateWithState` function to
get access to `readUseLedgerPeers`.
@coot
diffusion: moved types back to the Diffusion.P2P module
d700faa
@coot coot force-pushed the coot/bolt12/reusable-diffusion-3 branch from 1c37cdf to d700faa Compare February 11, 2025 10:09
bolt12
bolt12 reviewed Feb 11, 2025
View reviewed changes
ouroboros-network/src/Ouroboros/Cardano/Network/PeerSelection/Governor/Types.hs
Comment on lines +215 to +223
, updateWithState = \PeerSelectionInterfaces { readUseLedgerPeers }
PeerSelectionActions { getLedgerStateCtx,
peerSharing }
psv st -> do
associationMode <- readAssociationMode readUseLedgerPeers
peerSharing
(Cardano.bootstrapPeersFlag (extraState st))
updateOutboundConnectionsState
Cardano.updateOutboundConnectionsState
(lpExtraAPI getLedgerStateCtx)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

bolt12
bolt12 approved these changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@bolt12 bolt12 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @coot

@coot coot merged commit 1e6f14a into bolt12/reusable-diffusion-3 Feb 11, 2025
9 of 16 checks passed
@coot coot deleted the coot/bolt12/reusable-diffusion-3 branch February 11, 2025 11:30
@coot coot added diffusion Issues / PRs related to diffusion layer mithril Things related to Mithril. labels Feb 11, 2025
@coot coot self-assigned this Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bolt12 bolt12 bolt12 approved these changes

Assignees

@coot coot

Labels
diffusion Issues / PRs related to diffusion layer mithril Things related to Mithril.
Projects
Ouroboros Network
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@coot @bolt12