Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions src/evo/mnauth.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,22 @@
void CMNAuth::PushMNAUTH(CNode& peer, CConnman& connman, const CActiveMasternodeManager& mn_activeman)
{
CMNAuth mnauth;
if (mn_activeman.GetProTxHash().IsNull()) {
const uint256 pro_tx_hash{mn_activeman.GetProTxHash()};
if (pro_tx_hash.IsNull()) {
return;
}
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());
Comment on lines +27 to +34

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 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
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']

return;
}
}
Comment on lines +26 to +37

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

  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.

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 auto receivedMNAuthChallenge = peer.GetReceivedMNAuthChallenge();
if (receivedMNAuthChallenge.IsNull()) {
Expand All @@ -39,7 +52,7 @@ void CMNAuth::PushMNAUTH(CNode& peer, CConnman& connman, const CActiveMasternode
}
const uint256 signHash{::SerializeHash(std::make_tuple(mn_activeman.GetPubKey(), receivedMNAuthChallenge, peer.IsInboundConn(), nOurNodeVersion))};

mnauth.proRegTxHash = mn_activeman.GetProTxHash();
mnauth.proRegTxHash = pro_tx_hash;

// all clients uses basic BLS
mnauth.sig = mn_activeman.Sign(signHash, false);
Expand Down
75 changes: 75 additions & 0 deletions test/functional/p2p_mnauth.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
#!/usr/bin/env python3
# Copyright (c) 2026 The Dash Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.

"""
Test MNAUTH emission on the registered masternode service only.
"""

import platform

from test_framework.test_framework import (
DashTestFramework,
MasternodeInfo,
)
from test_framework.util import (
assert_equal,
p2p_port,
)


class P2PMNAUTHTest(DashTestFramework):
def add_options(self, parser):
self.add_wallet_options(parser)

def set_test_params(self):
# Disable the framework's implicit `bind=127.0.0.1` so all binds for the
# masternode are visible in this file rather than coming from the conf.
self.bind_to_localhost_only = False
self.alt_port = p2p_port(10)
self.mn_port = p2p_port(2)
# NAT-style variant requires a non-127.0.0.1 loopback bind; only Linux
# routes 127.0.0.0/8 to lo by default.
self.nat_capable = platform.system() == "Linux"

mn_args = [
f"-bind=127.0.0.1:{self.mn_port}",
f"-bind=127.0.0.1:{self.alt_port}",
f"-externalip=127.0.0.1:{self.mn_port}",
]
if self.nat_capable:
mn_args.append(f"-bind=127.0.0.2:{self.mn_port}")

self.set_dash_test_params(3, 1, extra_args=[[], [], mn_args])

def run_test(self):
masternode: MasternodeInfo = self.mninfo[0]
masternode_node = masternode.get_node(self)
connector = self.nodes[1]
use_v2transport = self.options.v2transport

expected_addr = f"127.0.0.1:{masternode.nodePort}"
alternate_addr = f"127.0.0.1:{self.alt_port}"

self.wait_until(lambda: masternode_node.masternode("status")["state"] == "READY")
assert_equal(masternode_node.masternode("status")["service"], expected_addr)

self.log.info(f"Connect to the registered masternode service over {'v2' if use_v2transport else 'v1'} and expect MNAUTH")
with connector.assert_debug_log([f"Masternode probe successful for {masternode.proTxHash}"]):
assert_equal(connector.masternode("connect", expected_addr, use_v2transport), "successfully connected")

self.log.info(f"Connect to the alternate bind over {'v2' if use_v2transport else 'v1'} and expect no MNAUTH")
with masternode_node.assert_debug_log(["Not sending MNAUTH on unexpected local service"]):
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:
Comment on lines +64 to +67

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 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']

nat_addr = f"127.0.0.2:{self.mn_port}"
self.log.info(f"Connect to a different loopback IP on the registered port over {'v2' if use_v2transport else 'v1'} (NAT-style: addrBind address differs from advertised externalip, ports match) and expect MNAUTH")
with connector.assert_debug_log([f"Masternode probe successful for {masternode.proTxHash}"]):
assert_equal(connector.masternode("connect", nat_addr, use_v2transport), "successfully connected")


if __name__ == '__main__':
P2PMNAUTHTest().main()
61 changes: 61 additions & 0 deletions test/functional/p2p_mnauth_onion.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
#!/usr/bin/env python3
# Copyright (c) 2026 The Dash Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.

"""
Test that MNAUTH is suppressed when an inbound arrives over a different network class
than the masternode's registered service. Specifically: an IPv4-registered masternode
must not emit MNAUTH on a Tor-tagged inbound, even when the local port matches — that
would deanonymize the masternode by linking its onion endpoint to its IPv4 identity.

Set up: bind the masternode's listener at its registered port with `=onion`, so that any
inbound arriving there is treated by dashd as a Tor connection (m_inbound_onion=true).
Port match is then guaranteed and only the network differs, isolating the network check.
"""

from test_framework.test_framework import (
DashTestFramework,
MasternodeInfo,
)
from test_framework.util import (
assert_equal,
p2p_port,
)


class P2PMNAUTHOnionTest(DashTestFramework):
def add_options(self, parser):
self.add_wallet_options(parser)

def set_test_params(self):
self.bind_to_localhost_only = False
self.mn_port = p2p_port(2)
self.set_dash_test_params(3, 1, extra_args=[
[],
[],
[
f"-bind=127.0.0.1:{self.mn_port}=onion",
f"-externalip=127.0.0.1:{self.mn_port}",
],
])

def run_test(self):
masternode: MasternodeInfo = self.mninfo[0]
masternode_node = masternode.get_node(self)
connector = self.nodes[1]
use_v2transport = self.options.v2transport

expected_addr = f"127.0.0.1:{masternode.nodePort}"

self.wait_until(lambda: masternode_node.masternode("status")["state"] == "READY")
assert_equal(masternode_node.masternode("status")["service"], expected_addr)

self.log.info(f"Probe the MN over {'v2' if use_v2transport else 'v1'} via its onion-tagged listener; ports match, network differs, expect no MNAUTH")
with masternode_node.assert_debug_log(["Not sending MNAUTH on unexpected local service"]):
with connector.assert_debug_log(["connection is a masternode probe but first received message is not MNAUTH"]):
assert_equal(connector.masternode("connect", expected_addr, use_v2transport), "successfully connected")


if __name__ == '__main__':
P2PMNAUTHOnionTest().main()
4 changes: 4 additions & 0 deletions test/functional/test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,10 @@
'p2p_ibd_txrelay.py',
'rpc_coinjoin.py',
'rpc_masternode.py',
'p2p_mnauth.py --v1transport',
'p2p_mnauth.py --v2transport',
'p2p_mnauth_onion.py --v1transport',
'p2p_mnauth_onion.py --v2transport',
'rpc_mnauth.py',
'rpc_verifychainlock.py',
'wallet_create_tx.py --legacy-wallet',
Expand Down
Loading