PeerSelectionPolicy for dmq-node by coot · Pull Request #27 · IntersectMBO/dmq-node

coot · 2026-01-19T16:33:40Z

List of changes

improved PeerSelectionPolicy for dmq-node

Checklist

related issue
My changes generate no new warnings
My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have added tests that prove my fix is effective or that my feature works

Copilot

Pull request overview

This PR improves the peer selection policy for dmq-node and refactors the validateSig function to remove the hashing function parameter.

Changes:

Replaced trivial peer selection policy with a weighted selection algorithm that considers peer state (tepid flag) and failure history
Refactored validateSig to use Ledger.hashKey (Ledger.VKey coldKey) directly instead of accepting a hashing function parameter
Added a tracer lock to prevent interleaved trace output
Updated ouroboros-network dependency to a development branch

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
dmq-node/src/DMQ/Diffusion/PeerSelection.hs	Replaced trivial peer selection with weighted algorithm based on peer state and failures
dmq-node/src/DMQ/Protocol/SigSubmission/Validate.hs	Removed hashing function parameter from validateSig, refactored to use Ledger.hashKey directly, reordered validation checks for performance
dmq-node/test/DMQ/Protocol/SigSubmission/Test.hs	Updated test to match new validateSig signature
dmq-node/app/Main.hs	Added tracer lock for synchronized output, updated to pass RNG TVar to policy, removed unused imports
dmq-node/src/DMQ/Diffusion/NodeKernel.hs	Changed to qualified imports for Ledger modules
dmq-node/dmq-node.cabal	Removed unused cardano-ledger-core dependency from executable
cabal.project	Updated ouroboros-network dependency to development branch
dmq-node/changelog.d/20260122_162026_coot_peer_selection_policy.md	Documented breaking and non-breaking changes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-26T12:45:04Z

dmq-node/src/DMQ/Diffusion/PeerSelection.hs

+  inRng <- readTVar rngVar
+
+  let (rng, rng') = split inRng
+      rns = take (Set.size available) $ unfoldr (Just . random)  rng :: [Word32]


There are two spaces between random) and rng on this line. Consider removing the extra space for consistency.

Suggested change

rns = take (Set.size available) $ unfoldr (Just . random) rng :: [Word32]

rns = take (Set.size available) $ unfoldr (Just . random) rng :: [Word32]

Copilot · 2026-01-26T12:45:04Z

cabal.project

+  -- from coot/dmq-related-changes
+  tag: 625296c92363b8c5e77cddee40de4525421d2660


This PR depends on an unreleased branch (coot/dmq-related-changes) of ouroboros-network. Consider documenting what changes in that branch are required for this PR to work, or ensure that branch is merged and released before merging this PR. Using unreleased dependencies can make builds unstable and difficult to reproduce.

Copilot · 2026-01-26T12:45:04Z

dmq-node/src/DMQ/Diffusion/PeerSelection.hs

+                -> Word32
+                -> (peerAddr, Word32)
+    failWeight failCnt peer r =
+        (peer, r `div` fromIntegral (failCnt peer + 1))


The failWeight function uses integer division which could result in a weight of 0 when failCnt peer is large. When multiple peers have a weight of 0, their ordering becomes non-deterministic (dependent only on the random number). Consider using a minimum weight (e.g., max 1 (r \div` ...)`) to ensure some randomness is preserved, or document that this behavior is intentional.

Suggested change

(peer, r `div` fromIntegral (failCnt peer + 1))

(peer, max 1 (r `div` fromIntegral (failCnt peer + 1)))

Copilot · 2026-01-26T12:45:04Z

dmq-node/src/DMQ/Diffusion/PeerSelection.hs

 -- | Trivial peer selection policy used as dummy value
 --


The comment describes this as a "Trivial peer selection policy used as dummy value" but the implementation is now quite sophisticated with weighted selection based on tepid flags and failure counts. Consider updating the comment to reflect the actual implementation, e.g., "Weighted peer selection policy that prioritizes peers based on their state and failure history".

Suggested change

-- | Trivial peer selection policy used as dummy value

--

-- | Weighted peer selection policy that prioritizes peers based on

-- their state and failure history.

Copilot · 2026-01-26T12:45:05Z

dmq-node/src/DMQ/Diffusion/PeerSelection.hs

+                          else (peer, r)
+
+
+ -- Add scaled random number in order to prevent ordering based on SockAddr


The comment has incorrect indentation. It should start at column 1 like the function definition below it, not with a leading space.

Suggested change

-- Add scaled random number in order to prevent ordering based on SockAddr

-- Add scaled random number in order to prevent ordering based on SockAddr

Copilot · 2026-01-26T12:45:05Z

dmq-node/changelog.d/20260122_162026_coot_peer_selection_policy.md

+
+### Non-Breaking
+
+- Added a lock to avoid race conditions between trace events.


The changelog entry mentions "Added a lock to avoid race conditions between trace events" but this appears to be unrelated to the PR title "PeerSelectionPolicy for dmq-node". Consider either updating the PR title to reflect all changes, or split this into a separate PR if it's an independent change.

Copilot · 2026-01-26T12:45:05Z

dmq-node/changelog.d/20260122_162026_coot_peer_selection_policy.md

@@ -0,0 +1,9 @@
+### Breaking
+
+- `validateSig`: removed the hashing function for cold key from arguments, added required constraints ledger's `hashKey . VKey` usage instead


The changelog mentions using "ledger's hashKey . VKey usage" but the actual implementation uses Ledger.hashKey (Ledger.VKey coldKey). While the meaning is the same, consider updating the changelog to match the actual code pattern for clarity.

Suggested change

- `validateSig`: removed the hashing function for cold key from arguments, added required constraints ledger's `hashKey . VKey` usage instead

- `validateSig`: removed the hashing function for cold key from arguments, added required constraints for using `Ledger.hashKey (Ledger.VKey coldKey)` instead

the-headless-ghost

LGTM

The peer selection policy is based on `simplePeerSelectionPolicy` with two exceptions: * it doesn't take into account `PeerMetric`, since it's not available * it doesn't forget known local root peers The latter is important, since at least now there no other peers in the network.

Included IntersectMBO/ouroboros-network#5289 to `coot/dmq-related-changes`

Cryptographic checks are the most computationally complex, so it makes sense to run them last. Added comments.

It's enough for us to add `DSIGN crypto ~ Ledger.DSIGN` constraint to hash they key.

the-headless-ghost

LGTM

github-project-automation bot added this to Ouroboros Network Jan 19, 2026

github-project-automation bot moved this to In Progress in Ouroboros Network Jan 19, 2026

coot marked this pull request as ready for review January 22, 2026 15:19

coot self-assigned this Jan 22, 2026

coot force-pushed the coot/peer-selection-policy branch from 71f5460 to e6d98e1 Compare January 22, 2026 21:09

coot requested a review from Copilot January 26, 2026 12:37

Copilot started reviewing on behalf of coot January 26, 2026 12:37 View session

Copilot AI reviewed Jan 26, 2026

View reviewed changes

the-headless-ghost approved these changes Jan 27, 2026

View reviewed changes

coot enabled auto-merge January 27, 2026 08:40

coot added 10 commits January 27, 2026 12:39

Updated ouroboros-network dependency

8c52be9

Included IntersectMBO/ouroboros-network#5289 to `coot/dmq-related-changes`

tracer: use a lock to avoid concurrent writes

fcaa6c5

validation: moved cryptographic checks

ed653a6

Cryptographic checks are the most computationally complex, so it makes sense to run them last. Added comments.

validation: removed the hash function from argumentsm

0bcae9d

It's enough for us to add `DSIGN crypto ~ Ledger.DSIGN` constraint to hash they key.

validation: qualified imports from cardano-ledger

edf4667

More qualified imports from ledger & consensus

7b3b7b8

Added changelog.d entry

e4660c8

Churn fix in ouroboros-network

aea70e8

configuration: added some haddocks

363cb28

coot force-pushed the coot/peer-selection-policy branch from e6d98e1 to 363cb28 Compare January 27, 2026 11:39

the-headless-ghost approved these changes Jan 27, 2026

View reviewed changes

coot added this pull request to the merge queue Jan 27, 2026

Merged via the queue into main with commit 6e19ab1 Jan 27, 2026
7 checks passed

github-project-automation bot moved this from In Progress to Done in Ouroboros Network Jan 27, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PeerSelectionPolicy for dmq-node#27

PeerSelectionPolicy for dmq-node#27
coot merged 10 commits intomainfrom
coot/peer-selection-policy

coot commented Jan 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 26, 2026

Uh oh!

Copilot AI Jan 26, 2026

Uh oh!

Copilot AI Jan 26, 2026

Uh oh!

Copilot AI Jan 26, 2026

Uh oh!

Copilot AI Jan 26, 2026

Uh oh!

Copilot AI Jan 26, 2026

Uh oh!

Copilot AI Jan 26, 2026

Uh oh!

the-headless-ghost left a comment

Uh oh!

the-headless-ghost left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	rns = take (Set.size available) $ unfoldr (Just . random) rng :: [Word32]
	rns = take (Set.size available) $ unfoldr (Just . random) rng :: [Word32]

		-- from coot/dmq-related-changes
		tag: 625296c92363b8c5e77cddee40de4525421d2660

	(peer, r `div` fromIntegral (failCnt peer + 1))
	(peer, max 1 (r `div` fromIntegral (failCnt peer + 1)))

--- | Trivial peer selection policy used as dummy value
---
+-- | Weighted peer selection policy that prioritizes peers based on
+--   their state and failure history.

		else (peer, r)


		-- Add scaled random number in order to prevent ordering based on SockAddr


		### Non-Breaking

		- Added a lock to avoid race conditions between trace events.

		@@ -0,0 +1,9 @@
		### Breaking

		- `validateSig`: removed the hashing function for cold key from arguments, added required constraints ledger's `hashKey . VKey` usage instead

Conversation

coot commented Jan 19, 2026

List of changes

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

the-headless-ghost left a comment

Choose a reason for hiding this comment

Uh oh!

the-headless-ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants