fix: Guard mnauth by local masternode service#4974
Conversation
|
I'm pretty sure I'd like some tests on this one please :) |
|
I can test it on testnet, just get me a build. |
You should be able to download it from CI build artefacts e.g. https://gitlab.com/dashpay/dash/-/jobs/2893007730/artifacts/browse/build-ci/dashcore-linux64/src/dashd for |
|
404 on that link, I am guessing because the build failed, I will check back later. |
oh, sorry, my bad |
|
What about nodes that uses local addresses to bind? Local IP that is translated 1:1 to public? There are certain DMZ setup or other cases when nodes are running like that. EDIT: of course externalip is set to public ip |
|
My test will be on a node whose P2P is port forwarded into the VM, so I assume the |
|
Just for clarity, I'm asking for functional tests |
|
I tested this fix first with my Masternode configured for TOR, I then checked on a node with and |
PastaPastaPasta
force-pushed
the
mnauth_same_addr_only
branch
from
bc12135 to
eddd6bc
Compare
|
@PastaPastaPasta @UdjinM6 Are we planning to merge this at some point? It's still assigned to the 18.2 milestone |
|
I'm not sure... I'd really like to see a functional test on this, but if we let it sit this long w/o adding a test we may have to wait a very long time before Udjin or I decide to write the test. That makes me inclined to be okay merging it. Any thoughts Udjin? |
UdjinM6
force-pushed
the
mnauth_same_addr_only
branch
from
eddd6bc to
46ee326
Compare
|
I'm not sure how to make it testable cause local addresses aren't passed via EDIT: @kxcd could you test the new version on your nodes pls? |
UdjinM6
force-pushed
the
mnauth_same_addr_only
branch
from
46ee326 to
4ddda42
Compare
|
This pull request has conflicts, please rebase. |
|
Is it still relevant? Can it be merged without tests? |
❌ Backport Verification - Not ApplicableThis PR is not a Bitcoin backport - it contains Dash-specific functionality and should not be processed by the backport verification system. Original Bitcoin commit: (none - Dash-specific change) Analysis:
Recommendation: Note: If this PR was incorrectly labeled as a backport, please remove any backport-related labels and process it as a native Dash enhancement. |
UdjinM6
force-pushed
the
mnauth_same_addr_only
branch
from
4ddda42 to
4de3119
Compare
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
UdjinM6
force-pushed
the
mnauth_same_addr_only
branch
from
4de3119 to
e8e8767
Compare
UdjinM6
changed the title
mnauth to peers that are connected to the address registered in DML
Co-authored-by: OpenAI Codex <codex@openai.com>
Co-authored-by: OpenAI Codex <codex@openai.com>
UdjinM6
force-pushed
the
mnauth_same_addr_only
branch
from
e8e8767 to
20c6139
Compare
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis pull request caches the masternode proTx hash and adds an inbound peer service-address validation in CMNAuth::PushMNAUTH: if an inbound peer's bound service differs from mn_activeman.GetService(), the code logs and returns without sending MNAUTH. Tests were added: p2p_mnauth.py probes registered and alternate binds (including optional 127.0.0.2 on Linux) to assert MNAUTH is sent only for the expected service; p2p_mnauth_onion.py verifies suppression for onion-tagged listeners. The test runner default list was updated to include these tests with v1/v2 transport variants. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean mnauth service guard implementation. The new check correctly compares the accepted socket local endpoint (peer.addrBind) against the active masternode registered service, properly handling multi-homed inbound connections. Functional test covers both the expected-service and alternate-bind paths for v1/v2 transport.
Reviewed commit: 20c6139
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
MNList now supports multiple core p2p addresses, we should acknowledge that here and use that.
|
✅ Review complete (commit dcf382e) |
That's true for MNList but masternodes themselves are single-address today - see |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, focused PR adding an inbound-only guard so masternodes only emit MNAUTH on the registered service. The convergent finding from both Claude and Codex is the main concern: the new exact-match comparison between peer.addrBind (kernel getsockname() on the accepted socket) and mn_activeman.GetService() (advertised service, often derived from -externalip) silently suppresses MNAUTH for legitimate inbound peers on NAT/port-forwarded masternode deployments. The new functional test only covers loopback where the two values coincide, so it would not catch this regression.
Reviewed commit: 20c6139
Fresh dispatcher run for this queue item. A same-SHA review already existed, so this records the fresh verification without duplicating inline threads.
Code Review
Reviewed commit: 20c6139
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/evo/mnauth.cpp`:
- [BLOCKING] lines 26-36: Strict addrBind == GetService() check breaks MNAUTH for NAT/port-forwarded masternodes
`mn_activeman.GetService()` is the masternode's *advertised* service, populated by `CActiveMasternodeManager::GetLocalAddress()` (src/active/masternode.cpp:217), which preferentially returns the address from `GetLocal()` — typically the value of `-externalip` or another discovered public address. By contrast, `peer.addrBind` is filled from `getsockname()` on the accepted socket (`GetBindAddress` in src/net.cpp:359, used at src/net.cpp:1867), i.e. the concrete local endpoint the kernel reports for the inbound connection.
For a typical VPS where the public IP is bound directly to the NIC, these coincide and the new guard is a no-op. But for masternodes behind NAT or port forwarding (e.g. `-bind=0.0.0.0:9999 -externalip=PUBLIC_IP:9999` while the host's real interface IP is a LAN/private address), the two values diverge for legitimate inbound probes to the registered service. The new check then silently skips MNAUTH on those connections, the remote side logs `connection is a masternode probe but first received message is not MNAUTH`, and InstantSend/CoinJoin/quorum probing breaks for that masternode. `RequireRoutableExternalIP()` on mainnet only forces externalip to be routable; it does not require it to equal the locally bound interface, so this is a real regression for an existing deployment pattern.
The new functional test (test/functional/p2p_mnauth.py) sets `-externalip=127.0.0.1:{mn_port}` so that the advertised service equals the loopback bind, making the test pass even though the NAT case it doesn't model is exactly the case this guard breaks. Consider relaxing the check (e.g. compare ports and accept any locally bound address, or fall back to checking `peer.addrBind.GetPort() == expected_service.GetPort()` when the addresses differ) and add a test variant where externalip differs from the bind address.
In `test/functional/p2p_mnauth.py`:
- [SUGGESTION] lines 84-94: Test depends on implicit framework-default bind alongside explicit -bind
The masternode is started with `extra_args=["-bind=127.0.0.1:{alt_port}", "-externalip=127.0.0.1:{mn_port}"]` and the test then expects to connect successfully to both `mn_port` and `alt_port`. The first connection only works because the framework's default `-bind=127.0.0.1:p2p_port(2)` is also active; that coupling is invisible from reading the test. A future change to `bind_to_localhost_only` defaults or to how `extra_confs`/`extra_args` are merged could silently leave the masternode listening on `alt_port` only, at which point the first `connect(expected_addr)` would fail with a hard-to-diagnose connection error rather than the MNAUTH-related assertion the test is designed to catch. Make the test self-contained by either passing both binds explicitly via `-bind=` or asserting (e.g. via `getnetworkinfo`/`localaddresses`) that both endpoints are listening before exercising the probes.
The strict `expected_service != connected_service` equality check broke MNAUTH on legitimate inbound for NAT/port-forwarded masternodes, where the advertised service (from `-externalip` / `GetLocal()`) is the public IP but `peer.addrBind` (from `getsockname()`) is the LAN/NIC interface IP. A pure port-only relaxation would in turn leak the masternode's identity over Tor: a hidden service forwarding to a local port matching the registered port would receive an MNAUTH naming the IPv4-registered ProRegTx, linking the .onion to the IPv4 endpoint. Replace the equality check with two conditions that must both hold: the local listening port must match the registered service port (so NAT setups work), and the inbound's `ConnectedThroughNetwork()` must match the registered service's network class (so an IPv4-registered masternode never emits MNAUTH on a Tor-tagged inbound, and vice versa). Tests: - `p2p_mnauth.py`: pass all binds explicitly via `-bind=` so the test no longer depends on the framework's implicit `bind=127.0.0.1` conf entry; on Linux, add a NAT-style sub-test that connects to `127.0.0.2:mn_port` (different loopback address, same port as the advertised service) and expects MNAUTH. - `p2p_mnauth_onion.py` (new): tag the masternode's listener at the registered port with `=onion` so port match is guaranteed and only the network class differs, isolating the network check; verify MNAUTH is suppressed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Refinement is a sensible tradeoff: it fixes the NAT regression introduced by the prior strict-equality check while preserving the onion-privacy guard. One legitimate gap remains — same-port multi-IPv4-bind deployments are still indistinguishable to the guard since ConnectedThroughNetwork() reflects the remote peer's net class, not the local listener. Two minor nitpicks on style and test robustness.
Reviewed commit: dcf382e
🟡 1 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/evo/mnauth.cpp`:
- [SUGGESTION] lines 26-37: Same-port multi-bind IPv4 listeners are still indistinguishable; `ConnectedThroughNetwork()` reflects remote net class, not the local listener
For non-onion inbound peers, `CNode::ConnectedThroughNetwork()` returns `addr.GetNetClass()` (src/net.cpp:584-586) — the *remote* peer's net class — not the local listener's. The guard therefore reduces `peer.addrBind` to `(port, peer-net-class)`, which has two consequences:
1. A masternode started with multiple IPv4 binds on the same port (e.g. `-bind=A:9999 -bind=B:9999 -externalip=A:9999`) will still emit MNAUTH on inbounds that arrived on bind B, because port matches and both endpoints are NET_IPV4. The original 'node bound to multiple addresses' confusion is unresolved for the same-port multi-homing case, which is also currently untested.
2. Edge cases like a dual-stack IPv6 listener accepting an IPv4-mapped peer, or a public-IPv4 MN receiving an inbound from a private/LAN peer (`NET_UNROUTABLE`), can flip the network comparison even though the local listener is unchanged.
This is not a regression — strict address equality would re-break the NAT case the PR is designed to fix, and the consensus-critical privacy concern (onion deanonymization) is correctly handled because `m_inbound_onion` is set on the listener side. But the partial fix is worth documenting in a code comment so future readers don't expect the guard to enforce listener-address identity, and the same-port multi-bind case could be covered by an additional test or an explicit address-family comparison against the local bind.
| if (peer.IsInboundConn()) { | ||
| const CService expected_service{mn_activeman.GetService()}; | ||
| const CService connected_service{static_cast<const CService&>(peer.addrBind)}; | ||
| if (expected_service.GetPort() != connected_service.GetPort() || | ||
| expected_service.GetNetwork() != peer.ConnectedThroughNetwork()) { | ||
| LogPrint(BCLog::NET_NETCONN, /* Continued */ | ||
| "CMNAuth::%s -- Not sending MNAUTH on unexpected local service, expected=%s, connected=%s, " | ||
| "peer=%d\n", | ||
| __func__, expected_service.ToStringAddrPort(), connected_service.ToStringAddrPort(), peer.GetId()); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Same-port multi-bind IPv4 listeners are still indistinguishable; ConnectedThroughNetwork() reflects remote net class, not the local listener
For non-onion inbound peers, CNode::ConnectedThroughNetwork() returns addr.GetNetClass() (src/net.cpp:584-586) — the remote peer's net class — not the local listener's. The guard therefore reduces peer.addrBind to (port, peer-net-class), which has two consequences:
- A masternode started with multiple IPv4 binds on the same port (e.g.
-bind=A:9999 -bind=B:9999 -externalip=A:9999) will still emit MNAUTH on inbounds that arrived on bind B, because port matches and both endpoints are NET_IPV4. The original 'node bound to multiple addresses' confusion is unresolved for the same-port multi-homing case, which is also currently untested. - Edge cases like a dual-stack IPv6 listener accepting an IPv4-mapped peer, or a public-IPv4 MN receiving an inbound from a private/LAN peer (
NET_UNROUTABLE), can flip the network comparison even though the local listener is unchanged.
This is not a regression — strict address equality would re-break the NAT case the PR is designed to fix, and the consensus-critical privacy concern (onion deanonymization) is correctly handled because m_inbound_onion is set on the listener side. But the partial fix is worth documenting in a code comment so future readers don't expect the guard to enforce listener-address identity, and the same-port multi-bind case could be covered by an additional test or an explicit address-family comparison against the local bind.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/evo/mnauth.cpp`:
- [SUGGESTION] lines 26-37: Same-port multi-bind IPv4 listeners are still indistinguishable; `ConnectedThroughNetwork()` reflects remote net class, not the local listener
For non-onion inbound peers, `CNode::ConnectedThroughNetwork()` returns `addr.GetNetClass()` (src/net.cpp:584-586) — the *remote* peer's net class — not the local listener's. The guard therefore reduces `peer.addrBind` to `(port, peer-net-class)`, which has two consequences:
1. A masternode started with multiple IPv4 binds on the same port (e.g. `-bind=A:9999 -bind=B:9999 -externalip=A:9999`) will still emit MNAUTH on inbounds that arrived on bind B, because port matches and both endpoints are NET_IPV4. The original 'node bound to multiple addresses' confusion is unresolved for the same-port multi-homing case, which is also currently untested.
2. Edge cases like a dual-stack IPv6 listener accepting an IPv4-mapped peer, or a public-IPv4 MN receiving an inbound from a private/LAN peer (`NET_UNROUTABLE`), can flip the network comparison even though the local listener is unchanged.
This is not a regression — strict address equality would re-break the NAT case the PR is designed to fix, and the consensus-critical privacy concern (onion deanonymization) is correctly handled because `m_inbound_onion` is set on the listener side. But the partial fix is worth documenting in a code comment so future readers don't expect the guard to enforce listener-address identity, and the same-port multi-bind case could be covered by an additional test or an explicit address-family comparison against the local bind.
| const CService expected_service{mn_activeman.GetService()}; | ||
| const CService connected_service{static_cast<const CService&>(peer.addrBind)}; | ||
| if (expected_service.GetPort() != connected_service.GetPort() || | ||
| expected_service.GetNetwork() != peer.ConnectedThroughNetwork()) { | ||
| LogPrint(BCLog::NET_NETCONN, /* Continued */ | ||
| "CMNAuth::%s -- Not sending MNAUTH on unexpected local service, expected=%s, connected=%s, " | ||
| "peer=%d\n", | ||
| __func__, expected_service.ToStringAddrPort(), connected_service.ToStringAddrPort(), peer.GetId()); |
There was a problem hiding this comment.
💬 Nitpick: Redundant static_cast<const CService&> and connected_service local
CAddress publicly inherits from CService (src/protocol.h:398), so peer.addrBind.GetPort() and peer.addrBind.ToStringAddrPort() already invoke the CService members directly. The intermediate connected_service and the explicit slicing cast add noise without changing behavior.
💡 Suggested change
| const CService expected_service{mn_activeman.GetService()}; | |
| const CService connected_service{static_cast<const CService&>(peer.addrBind)}; | |
| if (expected_service.GetPort() != connected_service.GetPort() || | |
| expected_service.GetNetwork() != peer.ConnectedThroughNetwork()) { | |
| LogPrint(BCLog::NET_NETCONN, /* Continued */ | |
| "CMNAuth::%s -- Not sending MNAUTH on unexpected local service, expected=%s, connected=%s, " | |
| "peer=%d\n", | |
| __func__, expected_service.ToStringAddrPort(), connected_service.ToStringAddrPort(), peer.GetId()); | |
| if (peer.IsInboundConn()) { | |
| const CService expected_service{mn_activeman.GetService()}; | |
| if (expected_service.GetPort() != peer.addrBind.GetPort() || | |
| expected_service.GetNetwork() != peer.ConnectedThroughNetwork()) { | |
| LogPrint(BCLog::NET_NETCONN, /* Continued */ | |
| "CMNAuth::%s -- Not sending MNAUTH on unexpected local service, expected=%s, connected=%s, " | |
| "peer=%d\n", | |
| __func__, expected_service.ToStringAddrPort(), peer.addrBind.ToStringAddrPort(), peer.GetId()); | |
| return; | |
| } | |
| } |
source: ['claude']
| with connector.assert_debug_log(["connection is a masternode probe but first received message is not MNAUTH"]): | ||
| assert_equal(connector.masternode("connect", alternate_addr, use_v2transport), "successfully connected") | ||
|
|
||
| if self.nat_capable: |
There was a problem hiding this comment.
💬 Nitpick: Negative cases assert only on log strings
Both new tests verify suppression by matching log lines (Not sending MNAUTH on unexpected local service, connection is a masternode probe but first received message is not MNAUTH). These strings are coupled to log formatting that may be reworded later. A more durable assertion would also confirm that getpeerinfo on the connector for the alt-port (or onion-tagged) peer never reports verified_proregtx_hash as set — that catches the regression class even if the log lines are renamed, and rules out a hypothetical path where MNAUTH is suppressed but the peer is authenticated by some other route.
source: ['claude']
8 participants
A node can be bound to multiple addresses. Send
mnauthto nodes connected to the right address only to avoid confusion.Thanks @kxcd 👍