feat(net): add "system ports" and Bitcoin ports to "bad" ports list #6535
Conversation
This file contains 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
kwvg
changed the title
UdjinM6
reviewed
Jan 23, 2025
7 tasks
kwvg
changed the title
|
Warning Rate limit exceeded@kwvg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
UdjinM6
approved these changes
Jan 27, 2025
PastaPastaPasta
added a commit
that referenced
this pull request
Jan 29, 2025
… new connections 8ee01a0 feat: check that no network has its default port set to a "bad" one (UdjinM6) db7f4cf fix: move Dash-specific `GetMNByService()` check to the end of the `while `loop (UdjinM6) 8872ba3 fix: do not break when addr is from a set of already connected masternodes (UdjinM6) 47f47d8 fix: don't skip mns when checking for required services (UdjinM6) e5a19c3 fix: drop useless `fAllowMultiplePorts` and some Dash-specific conditions (UdjinM6) c4819b9 fix: 4213 follow-up (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Changes: - c4819b9: we backported 15486 in #4213 incorrectly, feeler connections from the same net group should be allowed - e5a19c3: `fAllowMultiplePorts` was introduced in #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 #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? - [x] Run tests - [x] 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 ACKs for top commit: kwvg: utACK 8ee01a0 PastaPastaPasta: utACK 8ee01a0 Tree-SHA512: 98bb6013857202ef2716ecb17b1c137db8346abcc0f1ffe640bff4c17150018255288b92004202c01dcb5db45f6eebe4e12d64511261964add5b4be257a76dee
PastaPastaPasta
added a commit
that referenced
this pull request
Jan 30, 2025
8c30f73 test: check port 0 in netbase_tests.cpp (UdjinM6) c5eb184 fix: port 0 is not a bad one (UdjinM6) Pull request description: ## Issue being fixed or feature implemented ``` An ephemeral port is allocated to a socket in the following circumstances: * the port number in a socket address is specified as 0 when calling bind(2); ``` [source](https://man7.org/linux/man-pages/man7/ip.7.html#:~:text=An%20ephemeral%20port%20is%20allocated%20to%20a%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20socket%20in%20the%20following%20circumstances%3A%0A%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%E2%80%A2%20%20the%20port%20number%20in%20a%20socket%20address%20is%20specified%20as%200%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20when%20calling%20bind(2)%3B) #6535 follow-up ## What was done? ## How Has This Been Tested? ## Breaking Changes ## 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 _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: utACK 8c30f73 PastaPastaPasta: utACK 8c30f73; nit, could've been one commit Tree-SHA512: f9a2767c1d9534e592e26a5332deab0d5c764a56ae783aece2f3b5bf0f2c3a258eac4fc2c16bf37fd5c1aae639bda14e43558849ad8229521b0830b9ec042aef
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.
Additional Information
Ports <1024 are considered to be "system ports" (see RFC 6335, Section 6) and on some platforms, require superuser privileges in order to bind to them (source).
This is undesirable as:
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.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).The "bad" ports list has been extended to include all ports <1024 and the RPC and P2P ports for Bitcoin mainnet and testnet.
Checklist