fix: multiple fixes to conditions we check while opening new connections #6538

UdjinM6 · 2025-01-23T21:55:54Z

Issue being fixed or feature implemented

Changes:

c4819b9: we backported 15486 in backport: misc net backports #4213 incorrectly, feeler connections from the same net group should be allowed
e5a19c3: fAllowMultiplePorts was introduced in Switch CNetFulfilledRequestManager and CMasternodeMan maps/funcs to CService #1967 to preserve the behaviour of the old non-deterministic masternode list while fixing netfulfilledman. Most of that code is gone because we use DMN now and fAllowMultiplePorts is kind of useless (and even confusing). Should be safe to drop it completely imo. Inspired by 6451dd7 in feat(net): add "system ports" and Bitcoin ports to "bad" ports list #6535.
47f47d8: there is no reason not to check MN addresses for required net services
8872ba3: we shouldn't break the loop if addr belongs to the set of already connected masternodes, should simply continue just like we do when we find addr from the same net group
db7f4cf: should allow non-Dash-specific conditions to trigger first for better compatibility

What was done?

See above/individual commits

How Has This Been Tested?

Run tests
Run testnet/mainnet masternode for a day or two

Breaking Changes

It's now possible to have regular nodes with the same address but different ports on mainnet too but that's not a breaking change hopefully.

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone

We backported 15486 incorrectly...

…ions Dropped conditions: - do not check masternode ports in `ThreadOpenConnections`, it's done on consensus level on DMN registration/updates - do not check connections on IP-only basis, masternodes handle duplicate mn connections via mnauth, regular nodes don't need that at all

…nodes

…hile `loop Allows non-specific conditions to trigger first for better compatibility

coderabbitai · 2025-01-27T09:51:24Z

removed for simplified merge message

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (3)

src/netfulfilledman.cpp (2)
Line range hint 51-55: Fix post-increment iterator usage with erase.

While the address handling change is good, there's a potential issue with the iterator usage. The current code uses post-increment (it++) with erase, which is unnecessary since erase returns the next iterator.

Consider simplifying:
-    if (it != mapFulfilledRequests.end()) {
-        mapFulfilledRequests.erase(it++);
+    if (it != mapFulfilledRequests.end()) {
+        mapFulfilledRequests.erase(it);
     }
Line range hint 35-55: Consider documenting the network behavior changes.

The removal of address squashing logic across these functions represents a significant architectural change in how network requests are tracked. While the code changes are correct, it would be valuable to document the expected changes in network behavior, particularly around:

How this affects request tracking for nodes with multiple ports

Any impact on network request rate limiting

Potential changes in memory usage patterns
src/net.cpp (1)
3520-3521: Consider consolidating duplicate self-connection checks.

This self-connection check is duplicated from lines 496-497. Consider extracting this check into a helper function to avoid duplication.
+bool IsConnectingToSelf(const CAddress& addr) {
+    return addr.GetPort() == GetListenPort() && IsLocal(addr);
+}

-if (addr.GetPort() == GetListenPort() && IsLocal(addr)) {
+if (IsConnectingToSelf(addr)) {
    break;
}

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0972dfe and db7f4cf.

📒 Files selected for processing (4)

src/chainparams.cpp (0 hunks)
src/chainparams.h (0 hunks)
src/net.cpp (5 hunks)
src/netfulfilledman.cpp (2 hunks)

💤 Files with no reviewable changes (2)

src/chainparams.h
src/chainparams.cpp

🔇 Additional comments (5)

src/netfulfilledman.cpp (2)

35-35: LGTM! Simplified address handling.

The removal of address squashing logic aligns with the broader changes to remove the fAllowMultiplePorts functionality. The simplified code now directly uses the address for request tracking.

41-41: LGTM! Consistent with storage changes.

The simplified lookup matches the storage pattern in AddFulfilledRequest, maintaining consistency in how addresses are handled.

src/net.cpp (3)

496-497: LGTM!

The self-connection check is correctly implemented by comparing ports and checking if the address is local.

3516-3518: LGTM!

The address validation check is correctly implemented with an appropriate early return pattern.

3562-3565: LGTM!

The masternode duplicate connection check is correctly implemented, preventing redundant connections to masternodes we're already connected to.

… ports list cf32c03 net: extend bad ports to include Bitcoin ports (Kittywhiskers Van Gogh) 25d1e96 net: consider all ports that require privileges (<1024) as bad ports (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Ports <1024 are considered to be "system ports" (see [RFC 6335, Section 6](https://datatracker.ietf.org/doc/html/rfc6335#section-6)) and on some platforms, require superuser privileges in order to bind to them ([source](https://linux.die.net/man/5/services)). > Port numbers below 1024 (so-called "low numbered" ports) can only be bound to by root (see bind(2), tcp(7), and udp(7)). This is so clients connecting to low numbered ports can trust that the service running on the port is the standard implementation, and not a rogue service run by a user of the machine. Well-known port numbers specified by the IANA are normally located in this root-only space. This is undesirable as: * We don't want users to run Dash Core with elevated privileges; and * We don't want deployments to resort to carving out exceptions to allow for using these ports. To deter this behavior, specifically the latter, all "system ports" have been added to the "bad" ports list. This comes with the coincidental benefit of trimming down the blocklist of ports substantially. * To avoid conflicts with Bitcoin, as an additional measure, the RPC and P2P ports for mainnet and testnet have been included in the blocklist. ## Breaking Changes * ~~Dash Core will now permits peers on mainnet to connect to each other using a non-default P2P port.~~ Superseded by [dash#6538](#6538). * ~~This port restriction still _de facto_ applies on masternodes, as masternodes will not connect to other masternodes if they're using the differing listening port from themselves ([source](https://github.com/dashpay/dash/blob/0972dfe13c958a2866a189e386dd3ae38ac1ce46/src/net.cpp#L3563)).~~ * The "bad" ports list has been extended to include all ports <1024 and the RPC and P2P ports for Bitcoin mainnet and testnet. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK cf32c03 PastaPastaPasta: utACK cf32c03 Tree-SHA512: b0dae7f5c0a0d2c1a102857294e75d53617dc4158849de9c26197b6b9f89f1e21b2448acc933fd9be0c724374cf4498701b570af25b6f31e123fc0bb792689f3

PastaPastaPasta · 2025-01-28T15:16:25Z

src/net.cpp

+            const uint16_t default_port{Params().GetDefaultPort(addr.GetNetwork())};
+            assert(!IsBadPort(default_port)); // Make sure we never set the default port to a bad port


maybe we just drop this now? or figure out a way to static assert it

how about 8ee01a0?

PastaPastaPasta

generally LGTM; utACK db7f4cf

Do this in CConnman's ctor instead of doing it for every addr we try to connect to

kwvg

utACK 8ee01a0

PastaPastaPasta

utACK 8ee01a0

UdjinM6 added 5 commits January 24, 2025 00:23

fix: 4213 follow-up

c4819b9

We backported 15486 incorrectly...

fix: don't skip mns when checking for required services

47f47d8

fix: do not break when addr is from a set of already connected master…

8872ba3

…nodes

fix: move Dash-specific GetMNByService() check to the end of the `w…

db7f4cf

…hile `loop Allows non-specific conditions to trigger first for better compatibility

UdjinM6 mentioned this pull request Jan 23, 2025

feat(net): add "system ports" and Bitcoin ports to "bad" ports list #6535

Merged

5 tasks

UdjinM6 marked this pull request as ready for review January 27, 2025 09:45

coderabbitai bot reviewed Jan 27, 2025

View reviewed changes

PastaPastaPasta added this to the 22.1 milestone Jan 28, 2025

PastaPastaPasta reviewed Jan 28, 2025

View reviewed changes

PastaPastaPasta previously approved these changes Jan 28, 2025

View reviewed changes

feat: check that no network has its default port set to a "bad" one

8ee01a0

Do this in CConnman's ctor instead of doing it for every addr we try to connect to

UdjinM6 dismissed PastaPastaPasta’s stale review via 8ee01a0 January 28, 2025 21:15

kwvg approved these changes Jan 29, 2025

View reviewed changes

PastaPastaPasta approved these changes Jan 29, 2025

View reviewed changes

PastaPastaPasta merged commit 78f242b into dashpay:develop Jan 29, 2025
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: multiple fixes to conditions we check while opening new connections #6538

fix: multiple fixes to conditions we check while opening new connections #6538

UdjinM6 commented Jan 23, 2025 •

edited

Loading

coderabbitai bot commented Jan 27, 2025 •

edited by PastaPastaPasta

Loading

coderabbitai bot left a comment

PastaPastaPasta Jan 28, 2025

UdjinM6 Jan 28, 2025

PastaPastaPasta left a comment •

edited

Loading

kwvg left a comment

PastaPastaPasta left a comment

		const uint16_t default_port{Params().GetDefaultPort(addr.GetNetwork())};
		assert(!IsBadPort(default_port)); // Make sure we never set the default port to a bad port

fix: multiple fixes to conditions we check while opening new connections #6538

fix: multiple fixes to conditions we check while opening new connections #6538

Conversation

UdjinM6 commented Jan 23, 2025 • edited Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

coderabbitai bot commented Jan 27, 2025 • edited by PastaPastaPasta Loading

coderabbitai bot left a comment

Choose a reason for hiding this comment

PastaPastaPasta Jan 28, 2025

Choose a reason for hiding this comment

UdjinM6 Jan 28, 2025

Choose a reason for hiding this comment

PastaPastaPasta left a comment • edited Loading

Choose a reason for hiding this comment

kwvg left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

UdjinM6 commented Jan 23, 2025 •

edited

Loading

coderabbitai bot commented Jan 27, 2025 •

edited by PastaPastaPasta

Loading

PastaPastaPasta left a comment •

edited

Loading