backport: merge bitcoin#21845, #22732, #23542, #24468, #25119, #25176, #25421, #26248, #26199, #27036, #27270, partial bitcoin#27106 (networking backports: part 10) #6532
Conversation
WalkthroughThis pull request introduces a comprehensive set of changes across multiple files in the Dash Core codebase, focusing on network connectivity, socket handling, transaction relay, and documentation. The modifications include adding a new documentation file about P2P bad ports, refactoring socket and peer management functions, updating transaction relay logic, and enhancing testing frameworks. Key areas of change include the introduction of methods to handle "bad" ports, modifications to how peers are classified and connected, updates to socket selectability and non-blocking operations, and improvements in transaction relay mechanisms. The changes aim to improve network security, connection management, and overall code structure by centralizing certain functionalities and providing more precise control over network interactions. The pull request also includes updates to test frameworks and functional tests to support and validate these new network-related features, ensuring robust implementation of the proposed changes. Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
src/net.cpp (5)
3145-3145: Remove redundant 'net:' prefix in log messageThe logging category
BCLog::NETalready indicates that this log message is related to the network. Including 'net:' in the message is redundant.Apply this diff to remove the redundant prefix:
- LogPrint(BCLog::NET, "net: enabling extra block-relay-only peers\n"); + LogPrint(BCLog::NET, "enabling extra block-relay-only peers\n");
3552-3552: Fix typo in commentThe word 'arbitary' is misspelled. It should be 'arbitrary' in the comment.
Apply this diff to correct the typo:
-// from using a non-default port while others allow any arbitary port so long +// from using a non-default port while others allow any arbitrary port so long
3553-3553: Improve clarity in commentConsider adding 'as' after 'so long' to enhance the clarity of the comment.
Apply this diff to improve the comment:
-// it isn't a bad port (and in the case of masternodes, it matches its listen +// as it isn't a bad port (and in the case of masternodes, it matches its listen
4250-4251: Ensure consistent use ofbilingual_strinThreadSafeMessageBoxThe
ThreadSafeMessageBoxfunction may expect abilingual_strfor proper localization support. Ensure thatstrErroris abilingual_strand that unnecessary conversions are avoided.If
strErroris already abilingual_str, pass it directly:- m_client_interface->ThreadSafeMessageBox(strError, "", CClientUIInterface::MSG_ERROR); + m_client_interface->ThreadSafeMessageBox(strError, /*caption=*/"", CClientUIInterface::MSG_ERROR);If
strErroris astd::string, consider changing it to abilingual_str.
4302-4302: Passbilingual_strtoThreadSafeMessageBoxSimilar to the previous comment, ensure that you're passing a
bilingual_strtoThreadSafeMessageBoxto support localization.If the message is translatable, wrap it with
_()to create abilingual_str:- m_client_interface->ThreadSafeMessageBox( - _("Cannot provide specific connections and have addrman find outgoing connections at the same time."), + m_client_interface->ThreadSafeMessageBox( + _("Cannot provide specific connections and have addrman find outgoing connections at the same time."), "", CClientUIInterface::MSG_ERROR);src/netbase.cpp (1)
708-795: Consider adding more commonly exploited portsThe implementation provides a good baseline of prohibited ports, but consider adding these commonly exploited ports:
- Port 445 (Microsoft-DS)
- Port 3389 (RDP)
- Port 5900 (VNC)
case 6697: // IRC + TLS case 10080: // Amanda +case 445: // Microsoft-DS +case 3389: // RDP +case 5900: // VNC return true;src/test/netbase_tests.cpp (1)
437-455: Enhance test coverage for edge casesWhile the test provides good basic coverage, consider adding:
- Edge case tests for port numbers 0 and 65535
- Tests for sequential port ranges (e.g., IRC ports 6665-6669)
- Tests for related services (e.g., if SMTP port 25 is bad, test SMTPS port 465)
BOOST_AUTO_TEST_CASE(isbadport) { BOOST_CHECK(IsBadPort(1)); BOOST_CHECK(IsBadPort(22)); BOOST_CHECK(IsBadPort(6000)); + BOOST_CHECK(!IsBadPort(0)); + BOOST_CHECK(!IsBadPort(65535)); BOOST_CHECK(!IsBadPort(80)); BOOST_CHECK(!IsBadPort(443)); BOOST_CHECK(!IsBadPort(8333)); + // Test sequential IRC ports + BOOST_CHECK(IsBadPort(6665)); + BOOST_CHECK(IsBadPort(6666)); + BOOST_CHECK(IsBadPort(6667)); + BOOST_CHECK(IsBadPort(6668)); + BOOST_CHECK(IsBadPort(6669)); + + // Test related services + BOOST_CHECK(IsBadPort(25)); // SMTP + BOOST_CHECK(IsBadPort(465)); // SMTPS + BOOST_CHECK(IsBadPort(587)); // SMTP Submission + // Check all ports, there must be 80 bad ports in total.doc/p2p-bad-ports.md (1)
104-114: Improve document formatting for better readability.Consider the following formatting improvements:
- Add a blank line before "For further information see:"
- Format references as a bullet list for better readability
Here's the suggested format:
+ For further information see: -[pull/23306](https://github.com/bitcoin/bitcoin/pull/23306#issuecomment-947516736) - -[pull/23542](https://github.com/bitcoin/bitcoin/pull/23542) - -[fetch.spec.whatwg.org](https://fetch.spec.whatwg.org/#port-blocking) - -[chromium.googlesource.com](https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/net/base/port_util.cc) - -[hg.mozilla.org](https://hg.mozilla.org/mozilla-central/file/tip/netwerk/base/nsIOService.cpp) +* [Bitcoin PR #23306](https://github.com/bitcoin/bitcoin/pull/23306#issuecomment-947516736) +* [Bitcoin PR #23542](https://github.com/bitcoin/bitcoin/pull/23542) +* [WHATWG Fetch Spec - Port Blocking](https://fetch.spec.whatwg.org/#port-blocking) +* [Chromium Port Util Implementation](https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/net/base/port_util.cc) +* [Mozilla Port Blocking Implementation](https://hg.mozilla.org/mozilla-central/file/tip/netwerk/base/nsIOService.cpp)🧰 Tools
🪛 LanguageTool
[uncategorized] ~104-~104: Possible missing comma found.
Context: ...RC + TLS 10080: Amanda For further information see: [pull/23306](https://github.com/b...(AI_HYDRA_LEO_MISSING_COMMA)
src/net_processing.cpp (2)
32-32: Consider reordering includes for consistencyThe addition of
#include <sync.h>appears between#include <streams.h>and#include <timedata.h>, which may not follow the include ordering conventions. It's recommended to group includes by category and sort them alphabetically within each group to maintain consistency.
648-649: Rename_RelayTransactionfor naming consistencyThe method
_RelayTransactionuses a leading underscore, which may not align with the project's naming conventions. Consider renaming it toRelayTransactionImplor similar to maintain consistency and avoid reserved identifier usage.Apply this diff to rename the method:
-void _RelayTransaction(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(cs_main); +void RelayTransactionImpl(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(cs_main);Similarly, update all references to
_RelayTransactiontoRelayTransactionImpl.src/init.cpp (1)
569-570: Remove TODO comment after changes are widely deployed.This comment should be removed once the changes from Bitcoin PR bitcoin#23542 regarding non-default ports are widespread.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
doc/README.md(1 hunks)doc/p2p-bad-ports.md(1 hunks)src/bitcoin-cli.cpp(4 hunks)src/compat/compat.h(0 hunks)src/init.cpp(3 hunks)src/masternode/node.cpp(1 hunks)src/net.cpp(10 hunks)src/net.h(3 hunks)src/net_processing.cpp(15 hunks)src/net_processing.h(1 hunks)src/netaddress.cpp(0 hunks)src/netaddress.h(2 hunks)src/netbase.cpp(4 hunks)src/netbase.h(1 hunks)src/node/transaction.cpp(0 hunks)src/test/dbwrapper_tests.cpp(2 hunks)src/test/fuzz/netaddress.cpp(1 hunks)src/test/fuzz/util.cpp(2 hunks)src/test/fuzz/util.h(2 hunks)src/test/netbase_tests.cpp(1 hunks)src/test/util/net.h(1 hunks)src/util/sock.cpp(2 hunks)src/util/sock.h(1 hunks)test/functional/p2p_blocksonly.py(1 hunks)test/functional/test_framework/p2p.py(1 hunks)test/lint/lint-locale-dependence.py(0 hunks)
💤 Files with no reviewable changes (4)
- src/compat/compat.h
- src/node/transaction.cpp
- test/lint/lint-locale-dependence.py
- src/netaddress.cpp
🧰 Additional context used
🪛 LanguageTool
doc/p2p-bad-ports.md
[uncategorized] ~104-~104: Possible missing comma found.
Context: ...RC + TLS 10080: Amanda For further information see: [pull/23306](https://github.com/b...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (31)
src/net.cpp (1)
3563-3563: Verify masternode port restrictionThe condition
addr.GetPort() != GetListenPort()restricts connections to masternodes only if they are using the same port as our node's listening port. Please verify if this restriction is necessary. Masternodes might operate on different ports, and enforcing this could limit connectivity.src/net_processing.h (1)
114-114: Verify thread safety after lock annotation removal.The removal of
EXCLUSIVE_LOCKS_REQUIRED(cs_main)fromRelayTransactionsuggests that locking is now handled internally. Please ensure that all call sites are properly synchronized.✅ Verification successful
Thread safety is properly maintained after lock annotation removal
The implementation ensures thread safety by acquiring cs_main lock internally in RelayTransaction before calling _RelayTransaction. The change simplifies the interface while maintaining proper synchronization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct calls to RelayTransaction to verify proper synchronization ast-grep --pattern 'RelayTransaction($hash)'Length of output: 212
Script:
#!/bin/bash # Find interface declarations and implementations ast-grep --pattern 'RelayTransaction($_) $_' # Find direct method calls rg "RelayTransaction\(" -A 2 # Check for any locking mechanisms around the implementation rg "cs_main" -A 3 -B 3Length of output: 69911
test/functional/p2p_blocksonly.py (1)
60-60: LGTM! Enhanced test coverage for relay permissions.The added assertion properly verifies that the peer has the expected relay permission when restarted with relay permission and blocksonly mode.
src/netbase.h (1)
249-256: LGTM! Consider adding comprehensive unit tests.The
IsBadPortfunction is well-documented and provides a clear interface for validating ports. This is important for security as it helps prevent connections to potentially dangerous ports.src/util/sock.h (1)
193-204: LGTM! Excellent use of modern C++ practices.The new methods demonstrate good practices:
- Use of
[[nodiscard]]to prevent ignoring critical return values- Virtual methods enabling proper polymorphic behavior
- Clear documentation explaining the purpose of each method
src/masternode/node.cpp (1)
165-165: LGTM: Socket selectability check refactored correctly.The change moves the socket selectability check into the
Sockclass, improving encapsulation and maintainability.src/util/sock.cpp (3)
128-145: LGTM: Well-implemented platform-specific non-blocking socket configuration.The implementation correctly handles both Windows and POSIX systems, with proper error handling and status reporting.
147-154: LGTM: Platform-aware socket selectability check.The implementation correctly handles platform differences:
- Always returns true for systems using poll or Windows
- Checks against FD_SETSIZE for other platforms
185-185: LGTM: Updated Wait method to use new IsSelectable check.Consistent with the socket handling refactoring.
src/test/dbwrapper_tests.cpp (3)
8-8: LGTM: Added required header for string utilities.
341-343: LGTM: Improved deserialization robustness.The new implementation using stream eof check is more reliable for handling serialized data.
352-368: LGTM: Enhanced key generation with better type safety.Replacing string formatting with ToString improves type safety and readability.
src/test/fuzz/util.cpp (3)
17-17: LGTM: Proper initialization of selectability state.Constructor correctly initializes the new m_selectable member with fuzzed data.
260-271: LGTM: Well-implemented fuzz testing for non-blocking socket operations.The implementation correctly simulates possible error conditions with appropriate errno values.
273-276: LGTM: Simple and effective selectability check for fuzz testing.Returns the fuzzed boolean value, providing good test coverage for socket selectability handling.
src/test/fuzz/util.h (1)
56-61: Well-structured socket selectability implementation!Good practices observed:
- Clear documentation of the
m_selectablemember variable- Use of
constfor thread safety- Proper use of
overridekeyword for virtual methodsAlso applies to: 90-92
src/netaddress.h (1)
564-570: Secure implementation of service hashing!Good security practices:
- Using cryptographically secure random number generation for salt initialization
- Proper const-correctness for salt values
- Parameterized constructor enables testing while keeping default constructor secure
Also applies to: 582-583
src/netbase.cpp (2)
291-291: LGTM: Updated documentation reflects API changesThe comment accurately reflects the socket non-blocking functionality changes.
703-705: LGTM: Clean implementation of interrupt mechanismSimple and effective implementation using atomic flag.
test/functional/test_framework/p2p.py (1)
613-613: LGTM: Added relay attribute from version messageThe addition of
self.relayattribute properly captures the relay capability from the version message.src/net.h (1)
1466-1466: LGTM: Method declaration for starting extra block relay peersClean method declaration for managing extra block relay peers.
doc/README.md (1)
79-79: LGTM!The new documentation link is well-placed and follows the existing format.
doc/p2p-bad-ports.md (1)
1-22: LGTM! Clear and comprehensive documentation.The document provides an excellent explanation of P2P bad ports, their implications, and the rationale behind avoiding them. The content is technically accurate and valuable for both developers and users.
src/net_processing.cpp (4)
1459-1460: Ensure correct transaction relay flag in version messageThe
tx_relayflag is correctly set based on theRejectIncomingTxs(pnode)function, ensuring that theVERSIONmessage reflects whether the peer should relay transactions.
1634-1635: Add missing lock before calling_RelayTransactionThe
LOCK(cs_main);statement is added before calling_RelayTransaction(txid);to ensure thread safety, complying with theEXCLUSIVE_LOCKS_REQUIRED(cs_main)annotation on_RelayTransaction.
2434-2438: Consistent naming for private relay transaction methodThe public method
RelayTransactioncalls the private method_RelayTransaction. To maintain naming consistency and avoid leading underscores, consider renaming_RelayTransactionas suggested earlier.
5739-5741: Properly reject incoming transactions from feeler connectionsThe addition of
if (peer.IsFeelerConn()) return true;ensures that transactions from feeler connections are rejected, adhering to the expected behavior for such temporary connections.src/test/util/net.h (1)
206-206: Verify method signature forIsSelectableEnsure that the
IsSelectable()method signature, including theconstqualifier, matches the base class method. If the base class method is non-const, adjust accordingly to avoid overriding issues.src/bitcoin-cli.cpp (1)
437-437: LGTM! Improved naming and logic for transaction relay handling.The changes correctly rename
is_block_relaytois_tx_relayand update the associated logic to better reflect the actual functionality. The implementation aligns with Bitcoin PR bitcoin#25176.Also applies to: 501-501, 507-507, 530-530, 564-564
src/init.cpp (2)
2275-2281: LGTM! Well-designed reusable warning message function.The
BadPortWarninglambda is well-structured and provides consistent warning messages for bad ports, with proper documentation references.
2290-2292: LGTM! Comprehensive port validation with user-friendly warnings.The implementation adds proper validation for potentially problematic ports in both
-bindand-portarguments, with clear warning messages directing users to documentation.Also applies to: 2320-2327
| @@ -4303,7 +4325,9 @@ bool CConnman::Start(CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_met | |||
| LogPrintf("%i block-relay-only anchors will be tried for connections.\n", m_anchors.size()); | |||
| } | |||
|
|
|||
| uiInterface.InitMessage(_("Starting network threads…").translated); | |||
| if (m_client_interface) { | |||
| m_client_interface->InitMessage(_("Starting network threads…").translated); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid unnecessary .translated when passing bilingual_str
The InitMessage function expects a bilingual_str parameter. Using .translated converts it to a std::string, which may not be intended. Pass the bilingual_str directly without calling .translated.
Apply this diff to fix the issue:
- m_client_interface->InitMessage(_("Starting network threads…").translated);
+ m_client_interface->InitMessage(_("Starting network threads…"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| m_client_interface->InitMessage(_("Starting network threads…").translated); | |
| m_client_interface->InitMessage(_("Starting network threads…")); |
There was a problem hiding this comment.
The commit that introduces that change only does the switchover from uiInterface to m_client_interface, anything other than that is out-of-scope for that commit.
There was a problem hiding this comment.
@kwvg You're absolutely right - I apologize for suggesting a change outside the scope of this commit. The commit should stay focused on just the uiInterface to m_client_interface switchover. Thank you for the feedback.
✏️ Learnings added
Learnt from: kwvg
PR: dashpay/dash#6532
File: src/net.cpp:4329-4329
Timestamp: 2025-01-14T09:07:12.264Z
Learning: Keep suggestions focused on the scope of the current commit/PR. Avoid suggesting unrelated improvements that should be handled in separate PRs, even if technically valid.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| (void)CServiceHash()(service); | ||
| (void)CServiceHash(0, 0)(service); |
There was a problem hiding this comment.
Verify the usage of CServiceHash function object
The calls to CServiceHash()(service); and CServiceHash(0, 0)(service); should be verified to ensure that the CServiceHash class supports such constructors and that these calls are valid. The constructor CServiceHash(0, 0) may not exist or may not be intended for use in this way.
Apply this diff to correct the usage:
- (void)CServiceHash()(service);
- (void)CServiceHash(0, 0)(service);
+ (void)CServiceHash(service);Alternatively, ensure that the correct constructor and method are used according to the CServiceHash implementation.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
But both constructors exist and the code cited is from the fuzzer harness, where both variants are tested
Lines 564 to 570 in 01975fb
There was a problem hiding this comment.
@kwvg You are absolutely right. I apologize for the incorrect warning. Both constructors exist in CServiceHash and the fuzzer is correctly testing both variants. Thank you for the clarification.
✏️ Learnings added
Learnt from: kwvg
PR: dashpay/dash#6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:18.984Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| @@ -201,6 +201,10 @@ class StaticContentsSock : public Sock | |||
| return 0; | |||
| } | |||
|
|
|||
| bool SetNonBlocking() const override { return true; } | |||
There was a problem hiding this comment.
Correct constness of SetNonBlocking() method
The SetNonBlocking() method in StaticContentsSock is marked const, but it overrides a base class method that is likely non-const. Marking it const may lead to incorrect overriding and compiler warnings or errors. Consider removing the const qualifier.
Apply this diff to correct the constness:
- bool SetNonBlocking() const override { return true; }
+ bool SetNonBlocking() override { return true; }Ensure that the method signature matches the base class to prevent overriding errors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool SetNonBlocking() const override { return true; } | |
| bool SetNonBlocking() override { return true; } |
There was a problem hiding this comment.
@kwvg You are absolutely correct. I apologize for my earlier review. The const qualifier on SetNonBlocking() is correct as it matches the base class Sock's virtual method declaration and is consistently used across all implementations. Thank you for pointing this out.
✏️ Learnings added
Learnt from: kwvg
PR: dashpay/dash#6532
File: src/test/util/net.h:204-204
Timestamp: 2025-01-14T09:05:11.844Z
Learning: The SetNonBlocking() method in Sock and its derived classes is correctly marked const as it doesn't modify the socket's internal state, only its blocking mode configuration.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Additional Information
Due to changes introduced in bitcoin#21845, a
LOCK(cs_main)had to be added toPeerManagerImpl::ReattemptInitialBroadcast()(source). This addition is in line with upstream (source).While nodes have been allowed to connect to non-default ports since dash#2168, bitcoin#23542 also adds a list of ports considered "bad" that while not outright prohibited, are heavily discouraged from use as they are considered de facto prohibited.
In combination with port restrictions for masternodes (connections only permitted if matching listening port), port validation logic was better served by implementing it in a lambda block that's immediately executed (see
is_prohibited_portfor more information, source).In bitcoin#25176,
is_block_relayis renamed tois_tx_relayas the block relay-only = not transaction-relay assumption no longer holds true (see dash#6365 for more information). One use ofis_block_relaywhich relied on this now-obsolete assumption is incrementingm_block_relay_peers_count. It has been replaced with checking for ablock-relay-onlyconn_type, matching upstream (source).Breaking Changes
None expected.
Checklist