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

CAD-3444 p2p-governor changes from p2p-master #3377

Merged
merged 59 commits into from
Oct 7, 2021
Merged

CAD-3444 p2p-governor changes from p2p-master #3377

merged 59 commits into from
Oct 7, 2021

Conversation

coot
Copy link
Contributor

@coot coot commented Sep 21, 2021

  • p2p-governor: peer selection without gossip
  • p2p-governor: localRootPersProvider
  • p2p-governor: use ledger as a source for publicRootPeers
  • p2p-governor: improved tracing when ledger peers is disabled
  • p2p-governor: PoolStake: derive Real instance
  • p2p-governor: force the list of ledger peers to WHNF
  • p2p-governor: keep lookup results for different domains separate
  • p2p-governor: basic churnGovernor based on random selection only
  • p2p-governor: sleep between 3300s and 3900s between churns
  • p2p-governor: Adjust number of active peers based on fetchmode
  • p2p-governor: added PeerSelectionCounters and respective tracers.
  • p2p-governor: change LocalRootPeers.toGroups to match fromGroups, and use in Show
  • p2p-governor: fix a bug in the QC shrinker for the p2p governor mock environment
  • p2p-governor: move QC utils to their own module
  • p2p-governor: improve the Script shrinker
  • p2p-governor: Restructure the p2p governor tests a bit: split, add and reorder
  • p2p-governor: Slightly simplify the Arbitrary instance for PeerAddr
  • p2p-governor: Make the arbitraryScriptOf generator more general and use it more
  • p2p-governor: Generalise the LocalRootPeers instance for Arbitrary
  • p2p-governor: Change the PickScript to pick elements not offsets.
  • p2p-governor: Extend the shrinker tests: test that the shrinkers shrink!
  • p2p-governor: Add a "no excessive busyness" test for the p2p governor
  • p2p-governor: Adjust prop_governor_gossip_1hr to allow demotions
  • p2p-governor: Adjust prop_governor_connstatus to allow demotions
  • p2p-governor: Improve counterexamples for +prop_governor_connstatus
  • p2p-governor: Improve the mock env pick script interpretation
  • p2p-governor: Adjust playTimedScript to trace the initial value
  • p2p-governor: Add new env tracers for public roots and gossips
  • p2p-governor: Make gossip failure results take non-zero time
  • p2p-governor: Add a new signal-based abstraction for expressing properties
  • p2p-governor: New governor properties for making progress towards targets
  • p2p-governor: Add a few misc comments and TODOs
  • p2p-governor: Order p2p governor tests after the livelock test
  • p2p-governor: Update the comment on the list of properties
  • p2p-governor: Add review feedback
  • p2p-governor: Fix prop_governor_target_known_1_valid_subset
  • p2p-governor: Add more Signal primitives
  • p2p-governor: Replace one use of Signal.primitiveTransformEvents
  • p2p-governor: Adjust LocalRootPeers to require targets > 0
  • p2p-governor: Minor correction in a comment
  • p2p-governor: Fix the prop_governor_target_known_above property
  • p2p-governor: Adjust established and active target properties for local roots
  • p2p-governor: Add support for hitting the local root peer targets
  • p2p-governor: Refactored localRootPeersProvider
  • p2p-governor: Scale pool's stake by sqrt
  • p2p-governor: new root peers configuration (Refactored DiffusionArguments for new root peers configuration #3079)
  • p2p-governor: Refactored DNS resolution to use io-sim-classes
  • p2p-governor: Added dns resolution tests
  • p2p-governor: Process synchronous hot promotion errors
  • p2p-governor: add some randomness to the reconnection delay

@coot coot requested review from dcoutts and removed request for nfrisby September 21, 2021 16:54
@coot coot changed the title CAD_3444 p2p-governor changes from p2p-master CAD-3444 p2p-governor changes from p2p-master Sep 21, 2021
@coot coot force-pushed the coot/p2p-p2p-governor branch 3 times, most recently from cba61b8 to 0b9328b Compare September 28, 2021 07:04
@coot coot force-pushed the coot/p2p-p2p-governor branch 2 times, most recently from 69f4985 to 6f2a117 Compare October 1, 2021 12:47
karknu and others added 4 commits October 1, 2021 14:51
@karknu @coot
p2p-governor: add some randomness to the reconnection delay
46a159a 
In case of a TCP sim open the handshake will fail, and both
nodes will retry at exactly the same time causing the same
failure to repeat forever. The random delay breaks the cycle.

The random delay is between -2s to +2s which is more than
then the expected RTT around the world.
@coot
p2p-governor: interface to ledger peers
cd03e92 
withLedgerPeers runs ledger peers worker thread, creates a two way
communication channel with it and exposes a function which allows to
request 'NumberOfPeer' and blocks until the ledger peers will respond.
@coot
RelayAddress renamed as RelayAccessPoint
be571be 
'DomainAddress' is renamed to 'DomainAccessPoint'.

'RelayAccessPoint' constructors are uniform: they take either ip or
domain and address.  We also provide 'RelayDomainAccessPoint'
bidirectional pattern which links 'RelayAccessDomain' and
'DomainAccessPoint'

Both types, and their instances are moved to their own module.
@coot
p2p-governor: use MonadTimer.timeout
8db9c66 
We should not use `network-mux` timeout as it requires to be used in
a synchronous way.  This is not the case for outbound-governor.  Also
outbound-governor is using timeouts for dns resolution which are in
seconds, we don't need to use `network-mux` high frequency timeouts.

issue #3267
@coot
ledger-peers: don't expose low leverl pickPeers
86ab706 
This required to rewrite the tests using `withLedgerPeers`.
karknu
karknu approved these changes Oct 5, 2021
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 mentioned this pull request Oct 5, 2021
Closed
@coot
p2p-governor: simplified dns resolution tests
c50e069 
fixup: p2p-governor: Added dns resolution tests
@coot
peer-selection: re-organise tests
81ecc81 
Instead of setting up all peer selection tests in
'Test.Ouroboros.Network.PeerSelection', we do that in the 'Main' module.
The advantage of this approach is that the tasty path of all the tests
is visible at the top module and does not require chasing between
different files to find out how to run the specific test.   This patch
preserves tasty groups, so all the tests are still available under
"Ouroboros.Network.PeerSelection" group.
@coot
peer-selection: add a shrinker for MockRoots
2e2e67e
@coot
peer-selection: fix tests bases on mockLocalRootPeersProvider
777c921 
Instead of taking last 1000 events, timeout the simulation after 1hr of
simulated time.  The `mockLocalRooPeersProvider` is a recursive loop
which never returns, it could only terminate by an exception.  When we
used rng to generate delays and timeouts this would eventually happen
but in some cases this would never happen for some tests.
@coot
peer-selection: fix prop_public_resolvesDomainsCorrectly test
c4ac7ef 
It could deadlock; We resolve the deadlock by:

* supplying dns timeouts and delays script, which eventually succeeds;
* the termination condition checks that we resolved all the addresses.

We no longer need to use `Test.QuickCheck.within`.
bolt12
bolt12 approved these changes Oct 6, 2021
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! (At least the changes regarding DNS tests)

@bolt12
Copy link
Contributor

bolt12 commented Oct 6, 2021

Looks like the test still failed in CI

@coot
peer-selection: fixed prop_local_resolvesDomainsCorrectly
1fcf3e9 
Verify that:
* we try to resolve all local root domains
* the resolved domains are a subset of the local root domains available
  in dns map
@coot
Copy link
Contributor Author

coot commented Oct 7, 2021

@bolt12 I think I fixed this one too. Please check the last commit.

bolt12
bolt12 approved these changes Oct 7, 2021
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

@coot
Copy link
Contributor Author

coot commented Oct 7, 2021

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 7, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 31f79d9 into master Oct 7, 2021
@iohk-bors iohk-bors bot deleted the coot/p2p-p2p-governor branch October 7, 2021 10:51
@bolt12 bolt12 mentioned this pull request Nov 22, 2021
Closed
@coot coot mentioned this pull request Feb 9, 2022
Closed
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

@karknu karknu karknu approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@coot @bolt12 @karknu @dcoutts