p2p local root peers #3006
Merged
p2p local root peers #3006
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The test/Ouroboros/ heirarchy is not for test modules, those live in test/Test/. That heirarchy is for extra source modules that happen only to be used within tests. Clear as mud?
The module was rather on the huge side.
Improve the generation and shrinking of pick scripts, and make their show instances much smaller so the counterexamples are easier to read. Also there were several pick scripts in the mock env that were not being shrunk at all. Fix that, and adjust so that any future additions will be noticed.
karknu
previously requested changes
Mar 22, 2021
ouroboros-network/src/Ouroboros/Network/PeerSelection/LocalRootPeers.hs
Outdated
Show resolved
Hide resolved
bolt12
reviewed
Mar 22, 2021
ouroboros-network/src/Ouroboros/Network/PeerSelection/LocalRootPeers.hs
Outdated
Show resolved
Hide resolved
bolt12
reviewed
Mar 22, 2021
| -- > LocalRootPeers.size (LocalRootPeers.clampToLimit sz lrps) | ||
| -- > == min sz (LocalRootPeers.size lrps) | ||
| -- | ||
| clampToLimit :: Ord peeraddr |
There was a problem hiding this comment.
Should this be more fair and try to clamp a little from each group?
There was a problem hiding this comment.
I think it's not worth it, as noted in the comment above
-- It is unlikely in practice that there are so many local root peers
-- configured that it goes over this targets, so it's ok to resolve it pretty
-- arbitrarily. We just take the local roots in left to right order up to the
-- limit.
There was a problem hiding this comment.
What we should do though is trace it if it happens, so it can appear as a warning in the logs.
bolt12
reviewed
Mar 22, 2021
ouroboros-network/test/Test/Ouroboros/Network/PeerSelection/LocalRootPeers.hs
Show resolved
Hide resolved
bolt12
requested changes
Mar 23, 2021
ouroboros-network/test/Test/Ouroboros/Network/PeerSelection/LocalRootPeers.hs
Show resolved
Hide resolved
dcoutts
force-pushed
the
dcoutts/p2p-local-root-peers
branch
from
c124383
to
bb88209
Compare
Not yet used. It will replace the current simple representation in the governor state. Add tests for the QC generator and the non-trivial clampToLimits function.
That is, change the representation in the governor state for the local root peers from Map peeraddr PeerAdvertise to LocalRootPeers peeraddr. Then follow through the minimally necessary changes. In particular this does not yet introduce the governor targets of the local root peers being established or active peers. Also update the QC tests, using the new generator for the LocalRootPeers from the previous patch.
the enforcement of the invariant that says Public/Local Root peer sets to be disjoint.
dcoutts
force-pushed
the
dcoutts/p2p-local-root-peers
branch
from
bb88209
to
e0978c3
Compare
Just like it is now with local roots, resolving DNS names to IP addresses has to be done by the provider, so it's outside of the p2p governor directly. Adding a suitable provider will be later in this line of work.
I moved them in this patch since they were in the wrong place (and in such a way that extending them was awkward). |
bolt12
approved these changes
Mar 29, 2021
dcoutts
dismissed
karknu’s stale review
We concluded we'd both merge this to the p2p-master branch and to master. And there are follow-up PRs that depend on this so it's good to unblock it.
|
bors merge |
|
Build succeeded: |
coot
pushed a commit
that referenced
this pull request
May 16, 2022
3006: p2p local root peers r=dcoutts a=dcoutts First parts of integrating the new scheme for local root peers. This changes to the new representation but does not yet introduce the governor targets of the local root peers being established or active peers. That'll be in follow-up patches (either extending this PR or after). I have those changes locally, but they need rebasing and tidying up. This is what I have ready to merge so far. Co-authored-by: Duncan Coutts <duncan@well-typed.com> Co-authored-by: Armando Santos <armando@well-typed.com> Co-authored-by: Armando Santos <armandoifsantos@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First parts of integrating the new scheme for local root peers.
This changes to the new representation but does not yet introduce the governor targets of the local root peers being established or active peers. That'll be in follow-up patches (either extending this PR or after).
I have those changes locally, but they need rebasing and tidying up. This is what I have ready to merge so far.