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

Fix PeerStateActions bugs and update testing #4385

Merged
merged 24 commits into from Feb 23, 2023
Merged

Conversation

coot
Copy link
Contributor

@coot coot commented Feb 22, 2023

Description

This PR fixes three bugs (two in production code, one in testing)

  • fixed a hot -> warm -> hot cycle; we used to forget the reason for hot -> warm demotion and then use 0 promtion delay.

  • fixed exception handling in deactivatePeerConnection (part of
    PeerStateActions), which can result in a tight loop (eventually broken by mux shutdown)

  • fixed a deadlock of chain-sync & block-fetch in sim-net: we used two
    different FetchClientRegistry which blocked chain-sync to start.

This PR also includes a test which can reproduce the first bug and various
smaller code changes.

Checklist

  • Branch
    • 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
    • Any changes affecting Consensus packages must have an entry in the appropriate changelog.d directory created using scriv. If in doubt, see the Consensus release process.
    • If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • 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

Rename `MuxTraceShutdown` to `MuxTraceStopping` and add
`MuxTraceStopped`.  This is more consistent with the errors mux can
throw.
The purpose of this commit is to be able to emulate a client which
refuses to talk to a remote peer.  This makes it easier to simulate
a case where the remote part is too far behind.

This commit only introduces the new functionality but it's not yet used.
This will be done in a subsequent commit.
Otherwise `block-fetch` mini-protocol deadlocks (and thus `chain-sync`
deadlocks too).
@coot coot marked this pull request as ready for review February 22, 2023 22:01
@coot coot linked an issue Feb 22, 2023 that may be closed by this pull request
It seems we are using it in all tests, then it should be part of the
node itself.
Exit chain-sync when looking for a common point on the chain.  This
simulates chain being too far behind.
Some ConnectionManager traces should not be emitted by the simulation.
When the monitoring loop notices a hot to warm or cold demotion it
executes `deactivatePeerConnection` or `closePeerConnection` to make
sure all hot or hot & warm & established mini-protocols terminate.  Both
operations can throw, if they do they shutdown mux.  In this case we
should not continue running the monitoring loop but let it die,
otherwise we have a tight busy loop which will is eventually exited when
mux exits.
When a hot mini-protocol exits we need to preserve the information why
it did so, otherwise the outbound governor might miss that information
and thus not insert any reactivation delay.
Comment on lines +298 to +300
quota = if numberOfNodes > 0
then 20 `div` numberOfNodes
else 100
Copy link
Contributor

Choose a reason for hiding this comment

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

eheh nice find!

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.

All looks good! :)

@coot
Copy link
Contributor Author

coot commented Feb 23, 2023

bors merge

@iohk-bors iohk-bors bot merged commit 6d5a968 into master Feb 23, 2023
@iohk-bors iohk-bors bot deleted the coot/peer-state-actions branch February 23, 2023 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] - connection is not recreated after mux stops
2 participants