Conversation
karknu
force-pushed
the
karknu/max_recon_main
branch
from
9e31084 to
395a527
Compare
coot
approved these changes
Nov 28, 2025
There was a problem hiding this comment.
Pull request overview
This PR implements a maximum reconnection attempt limit for peer connections in the Ouroboros Network peer selection governor. Peers that fail to connect repeatedly are now forgotten after exceeding a configurable retry limit, with exceptions for localroot peers, bootstrap relays, and manually configured public root peers which can retry indefinitely.
Key changes:
- Adds
policyMaxConnectionRetriesfield (default: 5) toPeerSelectionPolicy - Implements retry limit enforcement in the cold peer promotion failure handler
- Adds a
forgottenboolean flag to promotion failure trace events to indicate when a peer is removed - Updates all trace pattern matches and JSON serialization to handle the new parameter
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
ouroboros-network/lib/Ouroboros/Network/PeerSelection/Governor/Types.hs |
Adds policyMaxConnectionRetries field and forgotten parameter to trace constructors |
ouroboros-network/lib/Ouroboros/Network/PeerSelection/Governor/EstablishedPeers.hs |
Implements retry limit logic to forget peers after max failures, exempting localroots and bootstrap peers |
ouroboros-network/lib/Ouroboros/Network/Diffusion/Policies.hs |
Sets default policyMaxConnectionRetries value to 5 |
ouroboros-network/orphan-instances/Ouroboros/Network/OrphanInstances.hs |
Updates JSON serialization to include forgotten field |
dmq-node/src/DMQ/Diffusion/PeerSelection.hs |
Adds policyMaxConnectionRetries to DMQ policy |
cardano-diffusion/tests/lib/Test/Cardano/Network/PeerSelection/MockEnvironment.hs |
Updates test trace patterns for new forgotten parameter |
cardano-diffusion/tests/lib/Test/Cardano/Network/PeerSelection.hs |
Updates test trace pattern matches |
cardano-diffusion/tests/lib/Test/Cardano/Network/Diffusion/Testnet.hs |
Updates test trace pattern matches |
ouroboros-network/changelog.d/20251128_094205_karl.fb.knutsson_max_recon_main.md |
Adds changelog entry |
dmq-node/changelog.d/20251128_104246_karl.fb.knutsson_max_recon_main.md |
Adds changelog entry |
cardano-diffusion/changelog.d/20251128_104055_karl.fb.knutsson_max_recon_main.md |
Adds changelog entry |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Comment on lines
+518
to
+521
| unForgetAble = LocalRootPeers.member peeraddr localRootPeers || | ||
| (memberExtraPeers peeraddr (PublicRootPeers.getExtraPeers publicRootPeers)) | ||
| (publicRootPeers', knownPeers'', forgotten) = | ||
| if unForgetAble || failCount < policyMaxConnectionRetries |
There was a problem hiding this comment.
[nitpick] The variable name unForgetAble has inconsistent capitalization. Consider using unforgettable or unForgettable for better consistency with Haskell naming conventions (camelCase for local variables).
Suggested change
| unForgetAble = LocalRootPeers.member peeraddr localRootPeers || | |
| (memberExtraPeers peeraddr (PublicRootPeers.getExtraPeers publicRootPeers)) | |
| (publicRootPeers', knownPeers'', forgotten) = | |
| if unForgetAble || failCount < policyMaxConnectionRetries | |
| unForgettable = LocalRootPeers.member peeraddr localRootPeers || | |
| (memberExtraPeers peeraddr (PublicRootPeers.getExtraPeers publicRootPeers)) | |
| (publicRootPeers', knownPeers'', forgotten) = | |
| if unForgettable || failCount < policyMaxConnectionRetries |
Comment on lines
+518
to
+531
| unForgetAble = LocalRootPeers.member peeraddr localRootPeers || | ||
| (memberExtraPeers peeraddr (PublicRootPeers.getExtraPeers publicRootPeers)) | ||
| (publicRootPeers', knownPeers'', forgotten) = | ||
| if unForgetAble || failCount < policyMaxConnectionRetries | ||
| then ( publicRootPeers | ||
| , KnownPeers.setConnectTimes (Map.singleton peeraddr (delay `addTime` now)) | ||
| knownPeers' | ||
| , False | ||
| ) | ||
| else ( PublicRootPeers.difference differenceExtraPeers publicRootPeers | ||
| (Set.singleton peeraddr) | ||
| , KnownPeers.delete (Set.singleton peeraddr) knownPeers' | ||
| , True | ||
| ) |
There was a problem hiding this comment.
The new behavior of forgetting peers after policyMaxConnectionRetries failed connection attempts lacks test coverage. Consider adding a test that verifies:
- Peers that are not localroots or bootstrap peers are forgotten after the maximum number of connection failures
- Localroot peers and bootstrap peers are never forgotten regardless of connection failure count
- The
forgottenflag is correctly set toTruein the trace when a peer is forgotten
coot
approved these changes
Nov 28, 2025
karknu
force-pushed
the
karknu/max_recon_main
branch
from
4344590 to
72a8bdb
Compare
karknu
force-pushed
the
karknu/max_recon_main
branch
2 times, most recently
from
f41e82c to
8981df7
Compare
coot
approved these changes
Jan 12, 2026
Enforce a maximum limit on the number of times we will attempt to promote a peer to warm. Localroot peers, bootstrap relays and manually configured public root peers are exempt from this limit. The clearing of the reconnection counter is delayd until a connection has managed to be active for a specific time (currently 120s).
Incase of an error use a shorter timeout when waiting for chainsync to exit.
Exclude shutdown peers in active peers calculations. It can take a while for peers to exit because blockfetch has to sync with chainsync as it exits. But we shouldn't count those peers as active or preferred anymore.
With p2p peerselection and the keepalive protocol we are not that dependant on chainsync timeout for detecting bad upstream peers. By bumping the timeout from between 135s and 269s to between 601s and 911s we change the false positive rate from something that happens a few times per epoch to something that happens less than once in a decade.
karknu
force-pushed
the
karknu/max_recon_main
branch
from
0d9de7f to
48bdfb3
Compare
coot
added a commit
that referenced
this pull request
Mar 19, 2026
Since #5253, peer selection can forget know peers. This patch relaxes the property. Peer selection now logs the set of forgotten peers via `TraceForgottenPeers`, which is used by the property.
coot
added a commit
that referenced
this pull request
Mar 19, 2026
Since #5253, peer selection can forget know peers. This patch relaxes the property. Peer selection now logs the set of forgotten peers via `TraceForgottenPeers`, which is used by the property.
This file contains hidden or 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
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.
Description
This is a series a changes that makes the node more robust.
Implements #5252
Checklist
Quality
Maintenance
ouroboros-networkproject.