backport: merge bitcoin#22087, #28695, #27375, #29649, #27679, #29968, partial bitcoin#26312 (UNIX domain sockets support) #6634
Conversation
WalkthroughThis update introduces comprehensive support for UNIX domain sockets throughout the networking and proxy subsystems, affecting both core logic and test infrastructure. The build configuration is enhanced to detect UNIX socket support and define a corresponding macro. The ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (18)
test/functional/test_framework/netutil.py (1)
159-166: Good addition of UNIX socket availability detection.The function correctly identifies UNIX socket support by checking for the presence of
socket.AF_UNIX. However, there's a small issue with the implementation.The expression
socket.AF_UNIXon line 162 is never used, which may trigger a "useless expression" warning. Consider modifying to check for the attribute's existence more explicitly:- try: - socket.AF_UNIX - except AttributeError: + try: + return hasattr(socket, 'AF_UNIX') + except: return False - else: - return True🧰 Tools
🪛 Ruff (0.8.2)
162-162: Found useless expression. Either assign it to a variable or remove it.
(B018)
src/test/netbase_tests.cpp (1)
86-92: Consider renaming the parameter to avoid confusion with the local variable
validPortis both a parameter and (with the suffixOut) a local variable. Having two nearly‑identical names in the same small scope can be error‑prone when the code evolves. A trivial rename (e.g.expectedValidPort) keeps the meaning intact while eliminating the risk of mixing the two up.-bool static TestSplitHost(const std::string& test, const std::string& host, uint16_t port, bool validPort=true) +bool static TestSplitHost(const std::string& test, + const std::string& host, + uint16_t port, + bool expectedValidPort = true) … - bool validPortOut = SplitHostPort(test, portOut, hostOut); - return hostOut == host && portOut == port && validPortOut == validPort; + bool validPortOut = SplitHostPort(test, portOut, hostOut); + return hostOut == host && portOut == port && validPortOut == expectedValidPort;src/i2p.h (3)
11-11: Header order / hidden dependency
#include <netbase.h>introduces a dependency onProxy, but the order places it after<netaddress.h>. That is perfectly legal, yet to prevent accidental reliance on transitive includes elsewhere, keepnetbase.hnext to the headers that actually useProxy(here, immediately before the first usage or grouped with other net headers).Non‑blocking – informational only.
70-73: Docs still talk about “host”The constructor doc‑block references “Location of the SAM proxy” and previously took a
CService. Now that it accepts aProxy, the wording should clarify that it is either a TCP or UNIX‑socket proxy (depending on theProxy’snetworkmember).- * @param[in] control_host Location of the SAM proxy. + * @param[in] control_host Proxy (TCP or UNIX domain socket) used to reach the SAM SAM control service.
231-234: Immovable but copy‑constructedProxyStoring
const Proxy m_control_hostmakes theSessionobject non‑assignable but copy‑constructible. If that is not intended (e.g. we only ever allocateSessionon the heap or stack immovably), mark the typeSessionexplicitly as non‑copyable / non‑movable to avoid surprises:Session(const Session&) = delete; Session& operator=(const Session&) = delete;Otherwise, all good.
src/i2p.cpp (1)
480-487:m_control_sock.reset()is sufficient – drop redundantifguardCalling
reset()on an emptyunique_ptris a no‑op, so theif (m_control_sock) { ... m_control_sock.reset(); }block can be simplified to always log and then unconditionallyreset(). This avoids accidentally skipping the log when the pointer is non‑null but already disconnected.- if (m_control_sock) { - … - m_control_sock.reset(); - } + if (m_control_sock) { + LogPrintLevel(…); + } + m_control_sock.reset();src/init.cpp (4)
570-574: CLI help text looks good – just a tiny consistency nitThe wording for the new Unix‑socket aware
-onionhelp line is clear, but the sentence ends with “(default: ‑proxy). May be …”. All other help strings use a full stop before “May be”. Consider inserting a period after the closing parenthesis to stay perfectly aligned with adjacent lines.
1685-1697: Port‑number validation duplicated – consider extracting a helperThe loop that validates
-portand-rpcportre‑implements the same “parse → uint16 → >0” logic that already lives inSplitHostPort/ParseUInt16. Extracting a smallValidateSinglePort(const ArgsManager&, std::string_view opt)would:
• remove the iterate‑over‑vector boilerplate
• avoid future divergence if the rules ever change
• improve testability.
1699-1727:SplitHostPort()is called on Unix‑socket arguments unnecessarilyFor options that explicitly allow Unix‑domain sockets we still try to split
"unix:/path"as host:port first, knowingly making the function fail and then falling back to the#if HAVE_SOCKADDR_UNbranch.
This is harmless but does extra work and produces confusing debug traces. A quick prefix check before callingSplitHostPort()would avoid the failed parse altogether:- std::string host_out; - uint16_t port_out{0}; - if (!SplitHostPort(socket_addr, port_out, host_out)) { + if (unix && socket_addr.rfind(ADDR_PREFIX_UNIX, 0) == 0) { + continue; // valid unix:path, no further checks + } + std::string host_out; + uint16_t port_out{0}; + if (!SplitHostPort(socket_addr, port_out, host_out)) {Would you like a full patch?
1729-1736:substr(0, npos)is safe but a little obscureWhen
'='is absent in-bind,rfindreturnsnposandsubstr(0, npos)yields the whole string. Although this is well‑defined, an explicit check would read clearer:const std::string bind_socket_addr = socket_addr.rfind('=') == std::string::npos ? socket_addr : socket_addr.substr(0, socket_addr.rfind('='));src/test/i2p_tests.cpp (3)
50-53: Lambda can mark parameter unused
[](const sa_family_t&) { … }compiles, but adding an identifier or[[maybe_unused]]suppresses “unused parameter” warnings on more pedantic tool‑chains:CreateSock = [](const sa_family_t& /*af*/) {
56-58: Optional: avoid.value()when the optional cannot be empty
Lookup("127.0.0.1", 9000, false)is guaranteed to succeed here; however, dereferencing with.value()will still throw on the off‑chance the precondition changes.
Consider either*addr(asserts in DEBUG) or an earlyBOOST_REQUIRE(addr)guard to make intent explicit.
134-178: Temporary files left behind afterdamaged_private_keyThe loop writes several malformed key files but never removes them, leaving clutter in the test datadir which can make debugging harder. Adding a small RAII helper or
fs::remove(i2p_private_key_file)at the end of the test keeps the environment clean.@@ } +// Clean up +fs::remove(i2p_private_key_file);src/net.cpp (1)
418-422: Use explicit comparison forgetsockname()success for readabilityThe current check relies on the truth‑value of
!int:if (!sock.GetSockName(&sockaddr_bind, &sockaddr_bind_len)) { … }
getsockname()returns0on success and-1on failure, so the logic is technically correct, but the double‑negative (!) can be confusing. An explicit comparison improves clarity and avoids accidental mis‑reads:-if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) { +if (sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len) == 0) {test/functional/feature_proxy.py (1)
70-73: Conditional node count would speed the test suite
self.num_nodesis hard‑coded to 7, yet nodes 5 and 6 are used only when
UNIX sockets are available. Spawning two extradashdinstances on
platforms without AF_UNIX increases runtime and resource usage.Consider:
self.num_nodes = 7 if test_unix_socket() else 5This keeps the functional surface identical while avoiding needless work.
src/netbase.h (2)
34-36: Useconstexpr std::string_viewfor the address prefix
ADDR_PREFIX_UNIXis a dynamically‑initialisedstd::string, which
incurs static‑initialisation order risks and unnecessary runtime
allocation. A compile‑time constant is sufficient:-const std::string ADDR_PREFIX_UNIX = "unix:"; +inline constexpr std::string_view ADDR_PREFIX_UNIX{"unix:"};Update call‑sites to accept a
std::string_view(or use.data()when a
std::stringis required). This is header‑only so ODR safety matters.
70-71: Avoid an extra copy in the path‑based constructorThe constructor that takes a UNIX socket path copies the string
unnecessarily:explicit Proxy(const std::string path, bool rnd = false);Pass by
const&to prevent one heap allocation perProxyinstance:-explicit Proxy(const std::string path, bool _randomize_credentials = false) +explicit Proxy(const std::string& path, bool _randomize_credentials = false)This change is ABI‑neutral but improves performance in hot paths that
createProxyobjects (e.g. argument parsing).src/netbase.cpp (1)
640-674: Passmanual_connection = falsewhen dialling the proxy
Proxy::Connect()forwards toConnectDirectly(..., /*manual_connection=*/true).
manual_connectioncontrols user‑visible logging; a proxy dial is an
internal operation and should normally use the verbose (BCLog::NET)
channel, not unconditionalLogPrintf, to avoid log spam.- if (!m_is_unix_socket) return ConnectDirectly(proxy, /*manual_connection=*/true); + if (!m_is_unix_socket) return ConnectDirectly(proxy, /*manual_connection=*/false);(Apply the same rationale in the AF_UNIX branch if you keep the helper
call split.) This keeps logs clean while preserving useful debug output
under-debug=net.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 272c951 and d942f13992a6fd22c3b3d9b8831fcad04f34bfdc.
📒 Files selected for processing (26)
configure.ac(1 hunks)doc/release-notes-6634.md(1 hunks)src/compat/compat.h(1 hunks)src/i2p.cpp(5 hunks)src/i2p.h(5 hunks)src/init.cpp(5 hunks)src/masternode/node.cpp(1 hunks)src/net.cpp(6 hunks)src/netaddress.cpp(1 hunks)src/netaddress.h(1 hunks)src/netbase.cpp(10 hunks)src/netbase.h(3 hunks)src/qt/optionsdialog.cpp(1 hunks)src/rpc/net.cpp(1 hunks)src/test/fuzz/fuzz.cpp(1 hunks)src/test/i2p_tests.cpp(5 hunks)src/test/netbase_tests.cpp(2 hunks)src/util/error.cpp(1 hunks)src/util/error.h(1 hunks)src/util/strencodings.cpp(2 hunks)src/util/strencodings.h(1 hunks)src/zmq/zmqnotificationinterface.cpp(2 hunks)src/zmq/zmqutil.h(1 hunks)test/functional/feature_proxy.py(13 hunks)test/functional/interface_zmq.py(6 hunks)test/functional/test_framework/netutil.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
src/masternode/node.cpp (2)
src/netbase.cpp (2)
ConnectDirectly(617-638)ConnectDirectly(617-617)src/netbase.h (1)
ConnectDirectly(293-293)
test/functional/interface_zmq.py (1)
test/functional/test_framework/netutil.py (2)
test_ipv6_local(143-157)test_unix_socket(159-166)
src/qt/optionsdialog.cpp (2)
src/netbase.cpp (6)
IsUnixSocketPath(237-253)IsUnixSocketPath(237-237)LookupHost(184-194)LookupHost(184-184)LookupHost(196-200)LookupHost(196-196)src/netbase.h (3)
IsUnixSocketPath(63-63)LookupHost(208-208)LookupHost(219-219)
test/functional/feature_proxy.py (4)
test/functional/test_framework/socks5.py (2)
Socks5Configuration(36-43)Socks5Server(128-162)test/functional/test_framework/test_framework.py (1)
BitcoinTestFramework(101-1133)test/functional/test_framework/netutil.py (1)
test_unix_socket(159-166)test/functional/test_framework/test_node.py (1)
assert_start_raises_init_error(602-645)
src/init.cpp (6)
src/util/strencodings.h (2)
ParseUInt16(209-209)SplitHostPort(97-97)src/util/strencodings.cpp (4)
ParseUInt16(267-270)ParseUInt16(267-267)SplitHostPort(100-125)SplitHostPort(100-100)src/node/interface_ui.cpp (2)
InitError(68-72)InitError(68-68)src/util/error.cpp (2)
InvalidPortErrMsg(44-47)InvalidPortErrMsg(44-44)src/util/error.h (1)
InvalidPortErrMsg(39-39)src/netbase.cpp (6)
IsUnixSocketPath(237-253)IsUnixSocketPath(237-237)Lookup(202-218)Lookup(202-202)Lookup(220-225)Lookup(220-220)
src/netbase.cpp (5)
src/netbase.h (6)
CreateSockOS(278-278)m_is_unix_socket(77-81)m_is_unix_socket(83-87)m_is_unix_socket(89-93)ConnectDirectly(293-293)Socks5(331-331)src/util/sock.cpp (6)
NetworkErrorString(390-404)NetworkErrorString(390-390)NetworkErrorString(406-410)NetworkErrorString(406-406)Connect(64-67)Connect(64-64)src/i2p.cpp (2)
Connect(218-275)Connect(218-218)src/torcontrol.cpp (2)
Connect(139-174)Connect(139-139)src/netaddress.cpp (4)
IsValid(426-453)IsValid(426-426)IsValid(1085-1088)IsValid(1085-1085)
src/net.cpp (3)
src/i2p.h (3)
sock(167-169)sock(188-188)sock(196-196)src/netbase.cpp (6)
ConnectThroughProxy(724-750)ConnectThroughProxy(724-727)ConnectDirectly(617-638)ConnectDirectly(617-617)GetNameProxy(702-708)GetNameProxy(702-702)src/netbase.h (3)
ConnectThroughProxy(306-309)ConnectDirectly(293-293)GetNameProxy(190-190)
🪛 Ruff (0.8.2)
test/functional/test_framework/netutil.py
162-162: Found useless expression. Either assign it to a variable or remove it.
(B018)
test/functional/interface_zmq.py
193-193: Use a context manager for opening files
(SIM115)
test/functional/feature_proxy.py
67-67: Use a context manager for opening files
(SIM115)
🪛 LanguageTool
doc/release-notes-6634.md
[style] ~4-~4: Consider using “formerly” to strengthen your wording.
Context: ...e now validated at startup. Values that previously worked and were considered valid can no...
(PREVIOUSLY_FORMERLY)
🪛 Cppcheck (2.10-2)
src/test/i2p_tests.cpp
[error] 45-45: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
src/netbase.cpp
[performance] 240-240: Inefficient usage of string
(stlIfStrFind)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: linux64_multiprocess-test / Test source
🔇 Additional comments (27)
configure.ac (2)
1269-1280: Good addition of UNIX domain socket detection.This well-structured check correctly tests for UNIX domain socket support by verifying both required headers and the
sockaddr_unstructure. Following Autoconf best practices, it definesHAVE_SOCKADDR_UNwhen available, which enables conditional compilation of UNIX socket support elsewhere in the codebase.
1265-1266: Minor syntax fix in HAVE_STRONG_GETAUXVAL definition.The semicolon is now properly placed in the AC_DEFINE line, following the standard pattern for Autoconf macros.
src/test/fuzz/fuzz.cpp (1)
82-82: Parameter type updated for CreateSock lambda.The parameter type has been updated from
const CService&tosa_family_t&, aligning with changes to the socket creation API that now supports multiple address families including UNIX domain sockets. The lambda's behavior (terminating on socket creation) remains unchanged.src/util/error.h (1)
39-39: New utility function for port validation errors.The
InvalidPortErrMsgfunction declaration provides consistent error messages for invalid port specifications. This supports enhanced port validation in the command line arguments, which is particularly important for UNIX socket support where traditional port validation doesn't apply.src/zmq/zmqutil.h (1)
12-13: Addition of ADDR_PREFIX_IPC constant is appropriate for Unix socket supportThis constant defines the "ipc://" prefix used by ZeroMQ for Unix domain socket addresses, which is part of the broader Unix socket support being added. The comment clearly explains its purpose and usage context.
src/compat/compat.h (1)
39-44: Windows platform compatibility enhancement for socket family typesThis typedef correctly introduces
sa_family_tfor Windows platforms to align with POSIX systems, ensuring consistent socket address family handling across platforms. The comment effectively explains the rationale and provides a reference to the Microsoft documentation.src/netaddress.h (1)
554-558: Well-defined socket address family getter with appropriate attributeThe
GetSAFamily()method declaration is correctly marked with[[nodiscard]]to indicate the return value should not be ignored. The documentation clearly indicates that it returnsAF_UNSPECfor unspecified address families.src/netaddress.cpp (1)
827-838: Implementation correctly maps network types to socket address familiesThe implementation handles all network types appropriately:
- Maps
NET_IPV4toAF_INET- Maps
NET_IPV6andNET_CJDNStoAF_INET6- Provides a sensible fallback of
AF_UNSPECfor all other typesThis method enables socket creation code to determine the appropriate socket type based on address family, supporting the Unix domain socket enhancements in this PR.
src/rpc/net.cpp (1)
604-605: Properly adjusted proxy field references for Unix socket supportThe changes properly adapt the GetNetworksInfo function to the refactored Proxy class which now supports Unix domain sockets. The
proxy.ToString()method correctly handles both IP:port and Unix socket path representations, while the member variable has been renamed to follow consistent naming conventions with them_prefix.src/util/error.cpp (1)
44-47: Good addition of port validation error reportingThis new function provides consistent error messaging for invalid port specifications, supporting the enhanced port validation logic introduced in this PR. The message is properly internationalized using the translation function.
doc/release-notes-6634.md (1)
1-12: Clear documentation of user-facing changesThe release notes appropriately document the three main user-facing changes:
- Stricter port validation for command-line options
- Unix domain socket support for proxy connections
- Unix socket path support for ZMQ interfaces
The format and examples are clear and will help users understand the new capabilities.
🧰 Tools
🪛 LanguageTool
[style] ~4-~4: Consider using “formerly” to strengthen your wording.
Context: ...e now validated at startup. Values that previously worked and were considered valid can no...(PREVIOUSLY_FORMERLY)
src/util/strencodings.h (1)
88-97: Enhanced host/port splitting with port validationThe function signature has been improved to return a boolean indicating port validity, which supports the new stricter port validation throughout the codebase. The documentation clearly explains the new behavior and return value semantics.
src/zmq/zmqnotificationinterface.cpp (2)
6-9: Added appropriate includes for network functionality.These includes are necessary for the UNIX socket path handling functionality added below. Good practice to only include what's needed.
61-66: Good implementation of UNIX socket support for ZMQ notifications.This code properly handles the prefix conversion from "unix://" to "ipc://" which is required by libzmq for UNIX domain sockets. The approach is consistent with the Unix socket support pattern being added throughout the codebase.
src/util/strencodings.cpp (3)
100-102: Function signature improvement adding validation.Changed return type from
voidtoboolto report validity of the parsed host and port, which is a better API design for validation functions.
113-117: Improved port validation logic.The function now correctly identifies and validates when:
- A port is found and non-zero (
valid = portOut != 0)- No port is specified at all (
valid = true)This change ensures addresses with zero ports are properly flagged as invalid, which is important for network address validation.
123-125: Added appropriate return value.Properly returns the validation status as determined by the logic above, completing the function signature change.
src/masternode/node.cpp (1)
157-160: Simplified socket connection logic for better maintainability.The previous approach that explicitly created a socket and then checked for null has been replaced with a single call to
ConnectDirectly()which returns astd::unique_ptr<Sock>. This change is:
- More concise and readable
- Better aligned with modern C++ practices using smart pointers
- Consistent with the socket connection refactoring across the codebase
- Properly handles connection to both TCP and Unix domain sockets
The check for connection success (
sock && sock->IsSelectable()) is also more robust than the previous approach.src/qt/optionsdialog.cpp (2)
493-499: Added Unix socket path support for proxy configuration.This change correctly handles both Unix socket paths and regular IP addresses in the proxy configuration:
- Checks if the proxy IP is a Unix socket path using
IsUnixSocketPath()- Only performs host lookup if it's not a Unix socket path
- For Unix socket paths, uses the raw text directly without resolution
- For regular IPs, resolves and creates a proper CService object
This approach correctly preserves Unix socket path information that wouldn't be valid in a traditional IP address format.
505-511: Fixed proxy comparison for Unix socket paths.The proxy reachability checks now correctly compare string representations rather than attempting to compare Proxy or CService objects directly. This allows Unix socket paths to be properly matched against the configured proxies for:
- IPv4 networks
- IPv6 networks
- Tor networks
Ensures the UI correctly reflects when a Unix socket proxy is being used for different network types.
src/test/netbase_tests.cpp (1)
111-128: Excellent edge‑case coverage – keep it upThe newly‑added assertions deliberately exercise boundary values (‑1, 0, 65535, 65536, scientific notation, embedded slashes, etc.). This regression‑style safety net is invaluable given the stricter port validation further up‑stream. No issues spotted here.
test/functional/interface_zmq.py (1)
144-145: Nice translation layer betweenipc://andunix:Replacing the ZeroMQ‑style
ipc://prefix with the node’s expectedunix:prefix keeps the Python test code and the C++ implementation decoupled. 👍src/i2p.cpp (2)
327-333: Graceful failure path already coveredGood catch converting to the new
Proxy::Connect()API and guarding against anullptr. Throwing with a detailedstrprintfkeeps error handling uniform. No further action required.
380-400: Robust private‑key sanity checksThe extra length validations in
MyDestination()prevent out‑of‑bounds reads on malformed keys. This hardens the parser against corrupted or malicious input. Well done.src/init.cpp (1)
587-592: Great to see Unix‑socket support for ‑proxyNo issues spotted. The updated help matches the implementation and keeps Windows builds inside the
#elsebranch.src/test/i2p_tests.cpp (1)
40-43: Good catch updating the storedCreateSocksignatureStoring the original functor with the new
sa_family_tparameter avoids UB when the global gets overridden. Looks correct.test/functional/feature_proxy.py (1)
98-106: Guard temp‑file creation with the capability check
socket_pathis created unconditionally, but it is only used when
self.have_unix_socketsis true. When the host kernel lacks UNIX socket
support the temp file will never be unlinked (the cleanup is itself
guarded by the same flag).Move the creation of
socket_pathinside theif self.have_unix_sockets
block (or always unlink it in afinally) to prevent orphaned files.
test/functional/interface_zmq.py
Outdated
There was a problem hiding this comment.
Temporary socket path can vanish too early and causes double‑unlink
tempfile.NamedTemporaryFile() immediately creates and deletes the file when the returned object is garbage‑collected (which happens right after .name is read because the object is not stored). Later, os.unlink(socket_path) may raise FileNotFoundError, or—worse—the node may fail to bind if the file still exists.
- socket_path = tempfile.NamedTemporaryFile().name
+ with tempfile.NamedTemporaryFile(delete=False) as tmp:
+ socket_path = tmp.name
+ # remove the placeholder file so dashd can create the AF_UNIX socket
+ os.unlink(socket_path)With this change the file does not disappear prematurely and we still clean it up explicitly at the end of the test.
📝 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.
| if unix: | |
| # Use the shortest temp path possible since paths may have as little as 92-char limit | |
| socket_path = tempfile.NamedTemporaryFile().name | |
| address = f"ipc://{socket_path}" | |
| subs = self.setup_zmq_test([(topic, address) for topic in ["hashblock", "hashtx", "rawblock", "rawtx"]]) | |
| if unix: | |
| # Use the shortest temp path possible since paths may have as little as 92-char limit | |
| with tempfile.NamedTemporaryFile(delete=False) as tmp: | |
| socket_path = tmp.name | |
| # remove the placeholder file so dashd can create the AF_UNIX socket | |
| os.unlink(socket_path) | |
| address = f"ipc://{socket_path}" |
🧰 Tools
🪛 Ruff (0.8.2)
193-193: Use a context manager for opening files
(SIM115)
src/init.cpp
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential UX improvement – surface invalid Unix‑socket paths earlier
IsUnixSocketPath() validates length and the unix: prefix, but if the file cannot be opened later (e.g. bad permissions) the user only sees a generic “connection failed” from ProxyConnect.
Consider checking access(path.c_str(), F_OK) here and emitting InitError(_("Unix socket does not exist or is not accessible: %s"), path) so mis‑spelled filenames are caught during start‑up instead of at first connect.
src/net.cpp
Outdated
There was a problem hiding this comment.
Incorrect printf placeholder for uint16_t port triggers UB / potential crash
addrConnect.GetPort() returns an integer, yet the format string uses %s (string placeholder).
Passing an int where a char* is expected invokes undefined behaviour and can seg‑fault on several platforms.
-LogPrintLevel(BCLog::PROXY, BCLog::Level::Debug, "Using proxy: %s to connect to %s:%s\n", proxy.ToString(), addrConnect.ToStringAddr(), addrConnect.GetPort());
+LogPrintLevel(BCLog::PROXY, BCLog::Level::Debug,
+ "Using proxy: %s to connect to %s:%u\n",
+ proxy.ToString(),
+ addrConnect.ToStringAddr(),
+ addrConnect.GetPort());(Use %u/%d or cast to unsigned int/int as preferred.)
test/functional/feature_proxy.py
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid leaking an open temp file – use a context manager or mkstemp
tempfile.NamedTemporaryFile() returns an open file handle.
On Windows this handle keeps the file locked, which makes binding the UNIX
domain‑socket path fail. Even on POSIX the handle stays around for the
whole test run and is only unlinked at the very end.
-# Use the shortest temp path possible since UNIX sockets may have as little as 92-char limit
-socket_path = tempfile.NamedTemporaryFile().name
+# Use the shortest temp path possible since UNIX sockets may have as little as 92‑char limit
+# We only need the pathname, not the open handle.
+fd, socket_path = tempfile.mkstemp() # returns a closed path on both Win & POSIX
+os.close(fd)This closes the descriptor immediately, eliminating cross‑platform locking
issues and avoiding the Ruff SIM115 warning.
Don’t forget to keep the later os.unlink(socket_path) cleanup.
📝 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.
| # Use the shortest temp path possible since UNIX sockets may have as little as 92-char limit | |
| socket_path = tempfile.NamedTemporaryFile().name | |
| # Use the shortest temp path possible since UNIX sockets may have as little as 92‑char limit | |
| # We only need the pathname, not the open handle. | |
| fd, socket_path = tempfile.mkstemp() # returns a closed path on both Win & POSIX | |
| os.close(fd) |
🧰 Tools
🪛 Ruff (0.8.2)
67-67: Use a context manager for opening files
(SIM115)
src/netbase.cpp
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
IsUnixSocketPath – avoid UB with reinterpret_cast to a null pointer
Taking sizeof(((sockaddr_un*)nullptr)->sun_path) relies on a
dereference of a null pointer, which is undefined behaviour even in an
unevaluated context per [expr.sizeof]/2. While most compilers tolerate
this, the strictly‑correct, portable pattern is:
constexpr size_t kSunPathLen = sizeof(sockaddr_un{}.sun_path);
if (str.size() + 1 > kSunPathLen) return false;This also avoids the C‑style cast and keeps the code self‑contained.
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 240-240: Inefficient usage of string
(stlIfStrFind)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
test/functional/interface_zmq.py (1)
191-195:⚠️ Potential issueFix temporary file handling issue
The current implementation has a potential race condition because
tempfile.NamedTemporaryFile()creates and immediately deletes the file when garbage collected. This can cause issues with socket creation or cleanup.- socket_path = tempfile.NamedTemporaryFile().name + with tempfile.NamedTemporaryFile(delete=False) as tmp: + socket_path = tmp.name + # remove the placeholder file so dashd can create the AF_UNIX socket + os.unlink(socket_path)This change ensures the file doesn't disappear prematurely and is properly cleaned up.
🧰 Tools
🪛 Ruff (0.8.2)
193-193: Use a context manager for opening files
(SIM115)
src/init.cpp (1)
1811-1821: Potential UX improvement – surface invalid Unix‑socket paths earlierThe current code validates that Unix socket paths have the correct format with the "unix:" prefix, but it doesn't check if the path exists or has the right permissions. This can lead to delayed error messages when connections are attempted rather than at startup.
Consider checking
access(path.c_str(), F_OK)here and emittingInitError(_("Unix socket does not exist or is not accessible: %s"), path)so users get immediate feedback about invalid paths.test/functional/feature_proxy.py (1)
66-67: 🛠️ Refactor suggestionFix potential file descriptor leak with temporary file handling
Using
tempfile.NamedTemporaryFile()creates an open file handle that remains open until garbage collection, which can cause issues on Windows where open files remain locked.Replace the current implementation with
tempfile.mkstemp()to avoid resource leaks:-# Use the shortest temp path possible since UNIX sockets may have as little as 92-char limit -socket_path = tempfile.NamedTemporaryFile().name +# Use the shortest temp path possible since UNIX sockets may have as little as 92-char limit +# We only need the pathname, not the open handle. +fd, socket_path = tempfile.mkstemp() # returns a closed path on both Win & POSIX +os.close(fd)This closes the descriptor immediately, avoiding cross-platform locking issues and fixing the Ruff
SIM115warning.🧰 Tools
🪛 Ruff (0.8.2)
67-67: Use a context manager for opening files
(SIM115)
src/netbase.cpp (1)
237-253: 🛠️ Refactor suggestionFix undefined behavior in UNIX socket path validation.
The current implementation of
IsUnixSocketPathusessizeof(((sockaddr_un*)nullptr)->sun_path), which is technically undefined behavior, even in an unevaluated context per C++ standard [expr.sizeof]/2. While most compilers tolerate this, it's not strictly correct.bool IsUnixSocketPath(const std::string& name) { #if HAVE_SOCKADDR_UN if (name.find(ADDR_PREFIX_UNIX) != 0) return false; // Split off "unix:" prefix std::string str{name.substr(ADDR_PREFIX_UNIX.length())}; // Path size limit is platform-dependent // see https://manpages.ubuntu.com/manpages/xenial/en/man7/unix.7.html - if (str.size() + 1 > sizeof(((sockaddr_un*)nullptr)->sun_path)) return false; + constexpr size_t kSunPathLen = sizeof(sockaddr_un{}.sun_path); + if (str.size() + 1 > kSunPathLen) return false; return true; #else return false; #endif }🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 240-240: Inefficient usage of string
(stlIfStrFind)
🧹 Nitpick comments (1)
src/netbase.cpp (1)
240-240: Consider using more efficient string prefix checking.The code uses
name.find(ADDR_PREFIX_UNIX) != 0which is less efficient than using a direct prefix comparison method likestarts_with()(C++20) or a simple prefix check.- if (name.find(ADDR_PREFIX_UNIX) != 0) return false; + if (name.compare(0, ADDR_PREFIX_UNIX.length(), ADDR_PREFIX_UNIX) != 0) return false;🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 240-240: Inefficient usage of string
(stlIfStrFind)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d942f13992a6fd22c3b3d9b8831fcad04f34bfdc and 10c7c1d.
📒 Files selected for processing (26)
configure.ac(1 hunks)doc/release-notes-6634.md(1 hunks)src/compat/compat.h(1 hunks)src/i2p.cpp(5 hunks)src/i2p.h(5 hunks)src/init.cpp(5 hunks)src/masternode/node.cpp(1 hunks)src/net.cpp(6 hunks)src/netaddress.cpp(1 hunks)src/netaddress.h(1 hunks)src/netbase.cpp(10 hunks)src/netbase.h(3 hunks)src/qt/optionsdialog.cpp(1 hunks)src/rpc/net.cpp(1 hunks)src/test/fuzz/fuzz.cpp(1 hunks)src/test/i2p_tests.cpp(5 hunks)src/test/netbase_tests.cpp(2 hunks)src/util/error.cpp(1 hunks)src/util/error.h(1 hunks)src/util/strencodings.cpp(2 hunks)src/util/strencodings.h(1 hunks)src/zmq/zmqnotificationinterface.cpp(2 hunks)src/zmq/zmqutil.h(1 hunks)test/functional/feature_proxy.py(13 hunks)test/functional/interface_zmq.py(6 hunks)test/functional/test_framework/netutil.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/fuzz/fuzz.cpp
🚧 Files skipped from review as they are similar to previous changes (17)
- src/util/error.h
- src/util/error.cpp
- src/netaddress.cpp
- src/zmq/zmqnotificationinterface.cpp
- src/masternode/node.cpp
- src/util/strencodings.h
- src/netaddress.h
- src/zmq/zmqutil.h
- src/compat/compat.h
- src/util/strencodings.cpp
- src/i2p.h
- src/rpc/net.cpp
- src/qt/optionsdialog.cpp
- configure.ac
- src/test/netbase_tests.cpp
- src/i2p.cpp
- src/net.cpp
🧰 Additional context used
🧬 Code Graph Analysis (3)
test/functional/feature_proxy.py (3)
test/functional/test_framework/socks5.py (2)
Socks5Configuration(36-43)Socks5Server(128-162)test/functional/test_framework/netutil.py (1)
test_unix_socket(159-166)test/functional/test_framework/test_node.py (1)
assert_start_raises_init_error(602-645)
src/netbase.cpp (2)
src/netbase.h (6)
CreateSockOS(278-278)m_is_unix_socket(77-81)m_is_unix_socket(83-87)m_is_unix_socket(89-93)ConnectDirectly(293-293)Socks5(331-331)src/util/sock.cpp (6)
NetworkErrorString(390-404)NetworkErrorString(390-390)NetworkErrorString(406-410)NetworkErrorString(406-406)Connect(64-67)Connect(64-64)
test/functional/interface_zmq.py (1)
test/functional/test_framework/netutil.py (1)
test_unix_socket(159-166)
🪛 Ruff (0.8.2)
test/functional/feature_proxy.py
67-67: Use a context manager for opening files
(SIM115)
test/functional/interface_zmq.py
193-193: Use a context manager for opening files
(SIM115)
test/functional/test_framework/netutil.py
162-162: Found useless expression. Either assign it to a variable or remove it.
(B018)
🪛 Cppcheck (2.10-2)
src/netbase.cpp
[performance] 240-240: Inefficient usage of string
(stlIfStrFind)
src/test/i2p_tests.cpp
[error] 45-45: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🪛 LanguageTool
doc/release-notes-6634.md
[style] ~4-~4: Consider using “formerly” to strengthen your wording.
Context: ...e now validated at startup. Values that previously worked and were considered valid can no...
(PREVIOUSLY_FORMERLY)
🔇 Additional comments (37)
test/functional/test_framework/netutil.py (1)
159-166: New utility function looks goodThe
test_unix_socket()function provides a clean way to detect UNIX socket support on various platforms, which is important for conditionally enabling UNIX socket tests.The implementation correctly handles platform differences by catching AttributeError when
socket.AF_UNIXis not available. This approach mirrors the pattern used in the existingtest_ipv6_local()function.🧰 Tools
🪛 Ruff (0.8.2)
162-162: Found useless expression. Either assign it to a variable or remove it.
(B018)
doc/release-notes-6634.md (1)
1-12: Release notes are clear and conciseThe notes effectively communicate three important changes to users:
- Enhanced port validation
- UNIX socket support for proxy connections
- UNIX socket support for ZMQ publishers
The examples provided for the UNIX socket paths are helpful, making it clear how users should format these new options.
🧰 Tools
🪛 LanguageTool
[style] ~4-~4: Consider using “formerly” to strengthen your wording.
Context: ...e now validated at startup. Values that previously worked and were considered valid can no...(PREVIOUSLY_FORMERLY)
test/functional/interface_zmq.py (5)
6-8: Import additions are appropriateThe added imports are needed for the new UNIX socket functionality -
osfor unlinking the socket file andtempfilefor creating a temporary path.
120-123: Conditional UNIX socket test is well integratedGood use of the
test_unix_socket()function to conditionally enable the UNIX socket test while providing a clear log message when skipping.
182-184: Added protocol logging is helpfulThe log message that indicates whether the test is using TCP or IPC protocol aids in debugging and understanding test execution.
144-145: Smart mapping from ZMQ's IPC prefix to Dash's UNIX prefixThe code correctly handles the conversion between ZMQ's
ipc://prefix and Dash'sunix:prefix format, ensuring compatibility between the systems.
259-260: Socket cleanup is appropriateThe
os.unlink(socket_path)call ensures that the socket file is properly removed after the test, preventing leftover files from causing issues in subsequent test runs.src/test/i2p_tests.cpp (3)
42-42: Function signature update aligns with core changesThe updated signature for the
CreateSockmock function (fromCService&tosa_family_t&) properly aligns with the broader refactoring of socket creation in the codebase to support UNIX domain sockets.
55-57: Proxy usage matches updated connection patternsThe code now correctly uses the
Proxyclass withCServiceinstances, aligning with the updated approach in the networking code that enables UNIX socket support.
134-177: Comprehensive private key validation testThe new
damaged_private_keytest is thorough and well-structured:
- It tests multiple failure cases for invalid private key formats
- It verifies specific error messages for each case
- It properly restores the original
CreateSockfunction at the endThis test will help ensure robust error handling for malformed private key files.
src/init.cpp (4)
1699-1751: Good implementation of comprehensive validation for network argumentsThis change adds thorough validation of network-related command-line arguments, ensuring they accept Unix socket paths only where appropriate based on the HAVE_SOCKADDR_UN macro. The structure using a vector of pairs with arg name and boolean flag is clean and maintainable.
1685-1697: Clear and strict port validation for network optionsThe addition of explicit port validation ensures that port numbers are valid 16-bit unsigned integers, preventing potential issues with invalid port specifications.
570-574: Good platform-specific conditional compilation for Unix socket supportThe conditional compilation based on HAVE_SOCKADDR_UN macro ensures the code is only compiled on platforms that support Unix domain sockets, maintaining compatibility across different environments.
587-591: Consistent platform-specific handling across network optionsThe consistent application of conditional compilation for Unix socket support in proxy configuration maintains code uniformity. This approach aligns with the changes to the
-onionoption and ensures proper functionality regardless of platform.test/functional/feature_proxy.py (6)
98-106: Well-structured UNIX socket proxy configurationThe conditional setup of the UNIX socket proxy configuration is well-implemented, properly setting the socket family to AF_UNIX and using the temporary file path as the socket address.
136-138: Clean proxy configuration for test nodesThe conditional addition of command-line arguments for UNIX socket proxies is well-implemented, testing both general proxy and onion-specific proxy configurations.
257-261: Complete test coverage for UNIX socket proxiesGood addition of test cases for the UNIX socket proxy functionality, ensuring that authentication and network connections work correctly over UNIX domain sockets.
348-378: Thorough verification of network info with UNIX socket proxiesThe new test cases properly verify the RPC
getnetworkinfooutput for nodes using UNIX socket proxies, including checking that proxy addresses are correctly reported with theunix:prefix without port numbers, and that network reachability flags are set correctly.
447-454: Good error handling test for excessively long pathsThe test for excessively long UNIX socket paths is valuable, especially since different platforms might have different path length limitations. The conditional error message check based on UNIX socket support is a nice touch.
457-458: Proper cleanup of temporary socket fileThe addition of explicit cleanup for the UNIX socket file is important to prevent leaving temporary files behind after tests complete.
src/netbase.cpp (9)
6-9: Correct inclusion of config.h with conditional check.The addition of the conditional inclusion of config.h aligns with best practices for handling configuration-dependent code.
27-29: Include sys/un.h conditionally for UNIX socket support.Proper conditional inclusion of the UNIX socket header based on the HAVE_SOCKADDR_UN macro ensures the code will compile correctly on platforms without UNIX socket support.
497-507: Good socket creation function with appropriate protocol selection.The function correctly handles protocol selection based on address family, using 0 for UNIX sockets and IPPROTO_TCP for others.
538-546: Correct TCP_NODELAY application only to non-UNIX sockets.The code properly applies the TCP_NODELAY option only to TCP sockets, skipping UNIX sockets where this option is not applicable.
550-550: Flexible socket creation through function pointer.Using a function pointer for socket creation allows for easy mocking in tests while maintaining the actual implementation as the default.
562-615: Well-structured socket connection function with proper error handling.The
ConnectToSocketfunction handles non-blocking connection attempts with proper timeout handling and detailed error reporting. The code effectively manages the complexity of asynchronous socket connections.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 591-591: Uninitialized variable
(uninitvar)
617-638: Clean direct connection implementation.The
ConnectDirectlyfunction provides a clear and concise implementation for creating and connecting sockets directly to a service, with appropriate error handling and logging.
640-674: Comprehensive proxy connection implementation with UNIX socket support.The
Proxy::Connectmethod properly handles both traditional TCP proxies and UNIX socket proxies, with the latter using specialized connection code that correctly extracts the path and sets up the UNIX socket address structure.
724-750: Improved proxy connection interface with proper resource management.The
ConnectThroughProxyfunction now returns astd::unique_ptr<Sock>instead of a boolean, which better represents the operation's result and allows for proper ownership of the socket resource. The error handling is also clear and appropriate.src/netbase.h (8)
34-35: Clear constant definition for UNIX socket prefix.Defining
ADDR_PREFIX_UNIXas a constant makes the code more maintainable and reduces the risk of inconsistencies when dealing with UNIX socket paths.
56-64: Well-documented UNIX socket path validation function.The function declaration includes clear documentation about its purpose, parameters, and return value.
68-75: Enhanced Proxy class with UNIX socket support.The Proxy class has been extended with appropriate constructors and member variables to support both traditional TCP proxies and UNIX socket proxies.
77-96: Clean and consistent Proxy member functions.The IsValid(), GetFamily(), ToString(), and Connect() methods correctly handle both proxy types, providing a consistent interface regardless of the underlying connection mechanism.
278-278: Updated socket creation function signature.The function now takes a sa_family_t parameter instead of a CService, allowing for more flexible socket creation based on address family.
283-283: Updated socket factory function pointer.The CreateSock function pointer signature has been updated to match the new CreateSockOS signature, maintaining consistency.
293-293: New direct connection function with clear interface.The ConnectDirectly function provides a clear interface for creating and connecting sockets directly to a service.
306-309: Improved proxy connection function signature.The ConnectThroughProxy function now returns a std::unique_ptr instead of a boolean, providing better resource management and more information about the connection result.
Additional Information
Breaking Changes
None expected.
Checklist