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

Expose OutboundConnectionState for consensus #4846

Merged
merged 4 commits into from
Apr 29, 2024
Merged

Expose OutboundConnectionState for consensus #4846

merged 4 commits into from
Apr 29, 2024

Conversation

coot
Copy link
Contributor

@coot coot commented Apr 6, 2024 •
edited
Loading

Description

This PR subsumes #4836. It provides API for Genesis which allow consensus to
know if the node is connected just to local root peers or has any external
peers. Fixes #4815.

TODO:

Checklist

  • Branch
    • Updated changelog files.
    • Commit sequence broadly makes sense
    • Commits have useful messages
    • The documentation has been properly updated
    • New tests are added if needed and existing tests are updated
    • 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

@coot coot mentioned this pull request Apr 6, 2024
Closed
@coot coot force-pushed the coot/outbound-connections-state branch 2 times, most recently from 7ff94b6 to 62f31df Compare April 6, 2024 14:07
@coot coot added consensus issues related to ouroboros-consensus outbound-governor Issues / PRs related to outbound-governor churn Issues / PRs related to churn labels Apr 6, 2024
@coot coot self-assigned this Apr 6, 2024
@coot coot force-pushed the coot/outbound-connections-state branch 3 times, most recently from daa9886 to 2eaba8f Compare April 8, 2024 16:30
@coot coot marked this pull request as ready for review April 8, 2024 16:33
@coot coot requested a review from a team as a code owner April 8, 2024 16:33
@coot coot linked an issue Apr 8, 2024 that may be closed by this pull request
Outbound Governor Counters #4845
Closed
@coot coot force-pushed the coot/outbound-connections-state branch 2 times, most recently from 5910027 to c0acf44 Compare April 9, 2024 08:39
karknu
karknu requested changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@karknu karknu left a comment

Choose a reason for hiding this comment

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

More can be done by having churn look at the governor's counters.

ouroboros-network/src/Ouroboros/Network/PeerSelection/Churn.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Churn.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Churn.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Churn.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Churn.hs Outdated Show resolved Hide resolved
@coot coot force-pushed the coot/outbound-connections-state branch from c0acf44 to 77cb6fe Compare April 10, 2024 10:38
@coot coot force-pushed the coot/outbound-connections-state branch 2 times, most recently from ce5bd2a to 1451f26 Compare April 10, 2024 16:56
@coot coot force-pushed the coot/outbound-connections-state branch 2 times, most recently from 399e05f to 07c233b Compare April 25, 2024 16:25
@coot coot removed the request for review from newhoggy April 26, 2024 07:37
@coot coot force-pushed the coot/outbound-connections-state branch from 07c233b to b079270 Compare April 26, 2024 08:25
@coot coot force-pushed the coot/outbound-connections-state branch from b079270 to 91a88e6 Compare April 26, 2024 08:52
bolt12
bolt12 requested changes Apr 26, 2024
View reviewed changes
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor/Types.hs Outdated
Comment on lines 405 to 419
-- | Interfaces required by the peer selection governor, which do not need to
-- be shared with actions and thus are not part of `PeerSelectionActions`.
--
-- TODO: add other `TVar`s which outbound governor is using.
--
data PeerSelectionInterfaces m = PeerSelectionInterfaces {
readUseLedgerPeers :: STM m UseLedgerPeers
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Perhaps move peerSharing, readLocalRootPeers, readPeerSelectionTargets, readUseBootstrapPeers and readLedgerStateJudgement from PeerSelectionActions to here since they are not actions as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what happened in the following commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed the following commit added more things but not the ones I mentioned here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are useful in guarded decisions, so I don't think this is the right move. Also, I didn't add readUseLedgerPeers and the other StrictTVars to PeerSelectionActions because they are unnecessary in guarded decisions.

ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor/Types.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor/Types.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/State/LocalRootPeers.hs Show resolved Hide resolved
ouroboros-network/sim-tests-lib/Test/Ouroboros/Network/PeerSelection.hs Outdated Show resolved Hide resolved
@coot coot force-pushed the coot/outbound-connections-state branch 4 times, most recently from 23a353e to b61635a Compare April 26, 2024 13:25
karknu
karknu requested changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@karknu karknu left a comment

Choose a reason for hiding this comment

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

more feedback.

ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor.hs Outdated Show resolved Hide resolved
@coot coot force-pushed the coot/outbound-connections-state branch 3 times, most recently from 8bc5759 to d710c00 Compare April 29, 2024 09:49
coot added 4 commits April 29, 2024 12:26
@coot
peer-selection: added publicly visible OutboundConnectionsState
306a3b3 
Consensus needs to know if the outbound governor is connected just to
local roots or any external roots.  Diffusion takes a callback
`OutboundConnectionsState -> STM m ()` used by the outbound governor.

Co-authored-by: Armando Santos (@bolt12)
Co-authored-by: Marcin Szamotulski (@coot)
@coot
peer-selection: extended PeerSelectionInterfaces
1f448dd 
Include `TVar`s which the outbound governor is writing to.
@coot
local-roots: added Config type alias
484734e 
Most changes in P2P module are imposed by `stylish-haskell`.
@coot
peer-selection: added tests for association mode
a38e5a1 
* Extended the `GovernorMockEnvironment` with a `TimedScript UseLedgerPeers`.
* added `prop_governor_association_mode`
@coot coot force-pushed the coot/outbound-connections-state branch from d710c00 to a38e5a1 Compare April 29, 2024 10:26
karknu
karknu approved these changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@karknu karknu left a comment

Choose a reason for hiding this comment

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

LGTM

@coot coot enabled auto-merge April 29, 2024 11:30
@coot coot added this pull request to the merge queue Apr 29, 2024
Merged via the queue into master with commit dbf4d4f Apr 29, 2024
13 checks passed
@coot coot deleted the coot/outbound-connections-state branch April 29, 2024 19:34
@amesgen amesgen mentioned this pull request Apr 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@bolt12 bolt12 bolt12 approved these changes

@karknu karknu karknu approved these changes

Assignees

@coot coot

@bolt12 bolt12

Labels
churn Issues / PRs related to churn consensus issues related to ouroboros-consensus outbound-governor Issues / PRs related to outbound-governor
Projects
Ouroboros Network
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inform Consensus Layer whether a bootstrap peer should be considered remote
4 participants
@coot @karknu @bolt12 @crocodile-dentist