fix(net): enforce per-proTxHash uniqueness for verified masternode inbounds - #7522
fix(net): enforce per-proTxHash uniqueness for verified masternode inbounds#7522PastaPastaPasta wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
|
✅ Final review complete — no blockers (commit 49120ec) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR closes a real DoS gap where a single masternode operator key could stack unlimited limit-exempt, eviction-immune inbound sockets by sending MNAUTH on two simultaneous inbound connections; the ResolveDuplicateConnection logic and defense-in-depth dedup in GetEvictionCandidates/CreateNodeFromAcceptedSocket are correct and thoroughly tested for the resolution matrix and eviction path. The main defect is that the permanent commit stack is not independently buildable: the first commit (7516edc) calls CMNAuth::ResolveDuplicateConnection before that API exists (introduced in the next commit), breaking git bisect and make check at that checkout, and two later commits (35b8cfb, 6ac8645) are pure fixups of mistakes made earlier in the same unmerged stack that should be folded into the commits they amend before merge. Two smaller issues remain: the new admission-side proTxHash-dedup logic in CreateNodeFromAcceptedSocket has no direct regression test, and the mnauth.h comments overstate the invariant as 'at most one verified connection' when the code actually allows one verified inbound plus one verified outbound (via the probe mechanism) to coexist.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(completed); claude/general=claude-sonnet-5(completed); verifier=claude/final-verifier=claude-sonnet-5(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (completed)
🟡 3 suggestion(s) | 💬 1 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/test/evo_mnauth_tests.cpp`:
- [SUGGESTION] src/test/evo_mnauth_tests.cpp:97-98: First commit in the permanent stack does not compile on its own
Commit 7516edce8cd ('test: add regression for verified MN inbound proTx uniqueness') adds src/test/evo_mnauth_tests.cpp and wires it into src/Makefile.test.include, but the test calls `CMNAuth::ResolveDuplicateConnection` and uses `CMNAuth::DuplicateResolution` -- confirmed absent from src/evo/mnauth.h at that commit (`git show 7516edce8cd:src/evo/mnauth.h | grep ResolveDuplicateConnection` returns nothing). These symbols are only introduced by the following commit, 04d17d28325 ('fix: enforce per-proTxHash uniqueness for verified MN inbounds'). Checking out 7516edce8cd in isolation and running `make check` fails to compile, which breaks `git bisect` for anyone landing between the two commits and violates the general expectation that every commit in a permanent history builds and passes on its own. Reorder the fix commit ahead of the test commit, or squash the test-adding and fix commits together so every retained commit is self-contained and buildable.
- [SUGGESTION] src/test/evo_mnauth_tests.cpp:1-261: Later commits are same-stack fixups that should be folded into the commits they amend
Commit 35b8cfb8d87 ('test: strengthen U023 regression coverage and tidy review nits') legitimately adds new coverage (the resolution matrix, the repeated-attempt loop, verified-outbound eviction case), but its diff also bundles three narrow self-corrections of the immediately preceding commit in the same stack: it removes the unused `AttemptToEvictConnectionPublic` test accessor added by 7516edce8cd, fixes the out-of-alphabetical-order Makefile.test.include entry that 7516edce8cd introduced, and replaces a stray non-ASCII em dash in src/net.cpp introduced by 04d17d28325. Commit 6ac8645834d ('docs: drop internal audit finding IDs from comments') then only strips leftover 'U023'/'V0NN' internal labels from comments written by the two prior commits in this same PR, with no independent contribution. Verified via `git show` on both commits: neither touches anything outside files/lines the stack itself introduced. None of these have standalone value in permanent git log/blame history; fold the accessor removal, Makefile-ordering fix, and em-dash fix into the commits that introduced them (7516edce8cd and 04d17d28325 respectively), and fold the label cleanup into whichever commit last wrote those comments, leaving only the net-new test coverage as its own commit.
In `src/net.cpp`:
- [SUGGESTION] src/net.cpp:1902-1915: Admission-side proTxHash dedup in CreateNodeFromAcceptedSocket has no direct regression test
This PR replaces a per-socket `nVerifiedInboundMasternodes++` counter with a `Uint256HashSet` that counts unique verified inbound proRegTxHashes, closing the same duplicate-exemption gap on the admission path that ResolveDuplicateConnection closes on the MNAUTH path. But the added test suite (src/test/evo_mnauth_tests.cpp) only exercises `ResolveDuplicateConnection` directly and the real `CConnman::GetEvictionCandidates` via `GetEvictionCandidatesPublic()` -- confirmed with a grep of the test file that no test touches this function or its counting logic. If this dedup were reverted to the old per-socket increment, the full test suite would still pass while the defense-in-depth exemption-stacking bug it's meant to catch would silently return. Extract the effective-inbound-count calculation into a small testable helper, or add a CConnman-level test proving N inbound sockets sharing one proRegTxHash yield only one limit-exempt slot while distinct identities each get their own.
| const auto res = CMNAuth::ResolveDuplicateConnection(/*victim_is_active_mn=*/true, my_protx, their, | ||
| /*new_is_inbound=*/true, /*existing_is_inbound=*/true); |
There was a problem hiding this comment.
🟡 Suggestion: First commit in the permanent stack does not compile on its own
Commit 7516edc ('test: add regression for verified MN inbound proTx uniqueness') adds src/test/evo_mnauth_tests.cpp and wires it into src/Makefile.test.include, but the test calls CMNAuth::ResolveDuplicateConnection and uses CMNAuth::DuplicateResolution -- confirmed absent from src/evo/mnauth.h at that commit (git show 7516edce8cd:src/evo/mnauth.h | grep ResolveDuplicateConnection returns nothing). These symbols are only introduced by the following commit, 04d17d2 ('fix: enforce per-proTxHash uniqueness for verified MN inbounds'). Checking out 7516edc in isolation and running make check fails to compile, which breaks git bisect for anyone landing between the two commits and violates the general expectation that every commit in a permanent history builds and passes on its own. Reorder the fix commit ahead of the test commit, or squash the test-adding and fix commits together so every retained commit is self-contained and buildable.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — First commit in the permanent stack does not compile on its own no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // 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. | ||
|
|
||
| #include <arith_uint256.h> | ||
| #include <evo/mnauth.h> | ||
| #include <llmq/utils.h> | ||
| #include <net.h> | ||
| #include <node/connection_types.h> | ||
| #include <test/util/net.h> | ||
| #include <test/util/setup_common.h> | ||
| #include <uint256.h> | ||
| #include <util/time.h> | ||
|
|
||
| #include <boost/test/unit_test.hpp> | ||
|
|
||
| #include <memory> | ||
| #include <set> | ||
| #include <vector> | ||
|
|
||
| BOOST_FIXTURE_TEST_SUITE(evo_mnauth_tests, BasicTestingSetup) | ||
|
|
||
| namespace { | ||
|
|
||
| CService ip(uint32_t i) | ||
| { | ||
| struct in_addr s; | ||
| s.s_addr = i; | ||
| return CService(CNetAddr(s), Params().GetDefaultPort()); | ||
| } | ||
|
|
||
| uint256 ProTx(uint32_t i) { return ArithToUint256(arith_uint256(i)); } | ||
|
|
||
| CNode* MakeNode(NodeId id, uint32_t addr_host, ConnectionType conn_type) | ||
| { | ||
| CAddress addr{ip(addr_host), NODE_NONE}; | ||
| auto* node = new CNode{id, | ||
| /*sock=*/nullptr, | ||
| addr, | ||
| /*nKeyedNetGroupIn=*/0, | ||
| /*nLocalHostNonceIn=*/0, | ||
| CAddress(), | ||
| /*addrNameIn=*/"", | ||
| conn_type, | ||
| /*inbound_onion=*/false}; | ||
| node->fSuccessfullyConnected = true; | ||
| return node; | ||
| } | ||
|
|
||
| CNode* MakeInboundNode(NodeId id, uint32_t addr_host) | ||
| { | ||
| return MakeNode(id, addr_host, ConnectionType::INBOUND); | ||
| } | ||
|
|
||
| //! Find a counterparty proTxHash for which we are the deterministic outbound side (or not, | ||
| //! when want_self is false). Both orientations must be reachable for the tests below to be | ||
| //! meaningful, so callers assert on the result. | ||
| uint256 FindPeerForOrientation(const uint256& my_protx, bool want_self) | ||
| { | ||
| for (uint32_t i = 1; i < 4096; ++i) { | ||
| const uint256 their = ProTx(i); | ||
| if (their == my_protx) continue; | ||
| const bool self_is_outbound = llmq::utils::DeterministicOutboundConnection(my_protx, their) == my_protx; | ||
| if (self_is_outbound == want_self) return their; | ||
| } | ||
| return uint256(); | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| /** | ||
| * The core invariant: a verified inbound connection is exempt from the inbound | ||
| * connection limit and immune to eviction, so at most one such connection may exist per | ||
| * proRegTxHash. Pre-fix, when the local node was an active masternode and both the existing | ||
| * and the newly authenticated peers were inbound, neither branch of the dedup logic set | ||
| * fDisconnect: the deterministic-outbound branch only flagged a probe (a deferred, conditional | ||
| * hint) and the other branch tested !IsInboundConn() on both peers, which was false for both. | ||
| * Both sockets stayed verified, so an attacker holding one operator key could stack unlimited | ||
| * limit-exempt, eviction-immune inbound slots. | ||
| * | ||
| * This asserts uniqueness for *both* deterministic-outbound orientations, since a peer stacking | ||
| * connections is free to pick victims for which it is the deterministic outbound. | ||
| */ | ||
| BOOST_AUTO_TEST_CASE(dual_inbound_mnauth_always_disconnects_exactly_one) | ||
| { | ||
| const uint256 my_protx = ProTx(0x1000); | ||
|
|
||
| const uint256 their_we_outbound = FindPeerForOrientation(my_protx, /*want_self=*/true); | ||
| const uint256 their_they_outbound = FindPeerForOrientation(my_protx, /*want_self=*/false); | ||
| // Guard against the search silently covering only one branch. | ||
| BOOST_REQUIRE(!their_we_outbound.IsNull()); | ||
| BOOST_REQUIRE(!their_they_outbound.IsNull()); | ||
| BOOST_REQUIRE(llmq::utils::DeterministicOutboundConnection(my_protx, their_we_outbound) == my_protx); | ||
| BOOST_REQUIRE(llmq::utils::DeterministicOutboundConnection(my_protx, their_they_outbound) == their_they_outbound); | ||
|
|
||
| for (const auto& their : {their_we_outbound, their_they_outbound}) { | ||
| const auto res = CMNAuth::ResolveDuplicateConnection(/*victim_is_active_mn=*/true, my_protx, their, | ||
| /*new_is_inbound=*/true, /*existing_is_inbound=*/true); | ||
| // Exactly one of the two sockets must go, and it must go now: a probe flag is not a | ||
| // disconnect (CMasternodeUtils::DoMaintenance acts on it only after PROBE_WAIT_INTERVAL | ||
| // and only once outbound capacity is exhausted), which is precisely why the pre-fix | ||
| // behaviour was exploitable. | ||
| BOOST_CHECK_MESSAGE(res.disconnect_new != res.disconnect_existing, | ||
| "dual inbound MNAUTH must disconnect exactly one peer"); | ||
| BOOST_CHECK_MESSAGE(!res.mark_new_probe && !res.mark_existing_probe, | ||
| "dual inbound MNAUTH must not settle for a deferred probe flag"); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Non-masternode victims kept the pre-existing behaviour of dropping the new connection. | ||
| */ | ||
| BOOST_AUTO_TEST_CASE(dual_inbound_mnauth_non_mn_victim_disconnects_new) | ||
| { | ||
| const auto res = CMNAuth::ResolveDuplicateConnection(/*victim_is_active_mn=*/false, /*my_protx=*/uint256(), | ||
| /*their_protx=*/ProTx(2), /*new_is_inbound=*/true, | ||
| /*existing_is_inbound=*/true); | ||
| BOOST_CHECK(res.disconnect_new); | ||
| BOOST_CHECK(!res.disconnect_existing); | ||
| BOOST_CHECK(!res.mark_new_probe); | ||
| BOOST_CHECK(!res.mark_existing_probe); | ||
| } | ||
|
|
||
| /** | ||
| * Whole resolution matrix. The security property is only about inbound sockets, so the mixed | ||
| * inbound/outbound cases must keep exactly one connection alive without ever tearing down both | ||
| * legs (that would partition the masternode mesh, a worse outcome than the DoS being fixed). | ||
| */ | ||
| BOOST_AUTO_TEST_CASE(duplicate_resolution_matrix_never_drops_both_legs) | ||
| { | ||
| const uint256 my_protx = ProTx(0x2000); | ||
| const uint256 their_we_outbound = FindPeerForOrientation(my_protx, /*want_self=*/true); | ||
| const uint256 their_they_outbound = FindPeerForOrientation(my_protx, /*want_self=*/false); | ||
| BOOST_REQUIRE(!their_we_outbound.IsNull()); | ||
| BOOST_REQUIRE(!their_they_outbound.IsNull()); | ||
|
|
||
| for (const auto& their : {their_we_outbound, their_they_outbound}) { | ||
| for (const bool new_inbound : {false, true}) { | ||
| for (const bool existing_inbound : {false, true}) { | ||
| const auto res = CMNAuth::ResolveDuplicateConnection(/*victim_is_active_mn=*/true, my_protx, their, | ||
| new_inbound, existing_inbound); | ||
| // Never drop both legs. | ||
| BOOST_CHECK_MESSAGE(!(res.disconnect_new && res.disconnect_existing), | ||
| "resolution must never disconnect both connections"); | ||
| // A connection is never both dropped and flagged as a probe. | ||
| BOOST_CHECK(!(res.disconnect_new && res.mark_new_probe)); | ||
| BOOST_CHECK(!(res.disconnect_existing && res.mark_existing_probe)); | ||
|
|
||
| if (new_inbound && existing_inbound) { | ||
| // The dual-inbound case: one must be dropped outright. | ||
| BOOST_CHECK(res.disconnect_new != res.disconnect_existing); | ||
| } else { | ||
| // Mixed or dual-outbound: the duplicate must still be resolved somehow, | ||
| // either by a disconnect or by the legacy probe hint. | ||
| BOOST_CHECK(res.disconnect_new || res.disconnect_existing || res.mark_new_probe || | ||
| res.mark_existing_probe); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Drive the resolver the way CMNAuth::ProcessMessage does — once per already-verified peer | ||
| * carrying the same proRegTxHash — and track how many verified inbound sockets survive. Pre-fix | ||
| * this converged on N (every duplicate stayed verified); it must now stay pinned at one no | ||
| * matter how many connections the attacker opens. | ||
| */ | ||
| BOOST_AUTO_TEST_CASE(repeated_inbound_mnauth_never_exceeds_one_verified) | ||
| { | ||
| const uint256 my_protx = ProTx(0x3000); | ||
|
|
||
| for (const bool we_are_outbound : {true, false}) { | ||
| const uint256 their = FindPeerForOrientation(my_protx, we_are_outbound); | ||
| BOOST_REQUIRE(!their.IsNull()); | ||
|
|
||
| // Verified inbound sockets currently held for `their`. | ||
| int live = 0; | ||
| for (int attempt = 0; attempt < 16; ++attempt) { | ||
| bool new_survives = true; | ||
| // ProcessMessage runs the resolver against every existing verified peer. | ||
| for (int existing = 0; existing < live; ++existing) { | ||
| const auto res = CMNAuth::ResolveDuplicateConnection(/*victim_is_active_mn=*/true, my_protx, their, | ||
| /*new_is_inbound=*/true, | ||
| /*existing_is_inbound=*/true); | ||
| if (res.disconnect_existing) --live; | ||
| if (res.disconnect_new) { | ||
| new_survives = false; | ||
| break; // ProcessMessage stops iterating once the new peer is dropped | ||
| } | ||
| } | ||
| if (new_survives) ++live; | ||
| BOOST_CHECK_MESSAGE(live <= 1, "verified inbound sockets for one proRegTxHash must never exceed 1"); | ||
| } | ||
| BOOST_CHECK_EQUAL(live, 1); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Defence in depth at the eviction layer, exercising the real CConnman::GetEvictionCandidates: | ||
| * many inbound sockets all advertising the same verified proRegTxHash must not all be | ||
| * eviction-protected, while distinct identities and outbound connections stay protected. | ||
| */ | ||
| BOOST_AUTO_TEST_CASE(verified_inbound_eviction_protection_is_unique_per_protx) | ||
| { | ||
| auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); | ||
| CConnman::Options options; | ||
| options.nMaxConnections = DEFAULT_MAX_PEER_CONNECTIONS; | ||
| options.m_max_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS; | ||
| options.m_active_masternode = true; | ||
| connman->Init(options); | ||
|
|
||
| const uint256 attacker_protx = ProTx(0xaaaa); | ||
| const uint256 honest_protx = ProTx(0xbbbb); | ||
|
|
||
| constexpr int duplicate_inbound = 8; | ||
| NodeId id{0}; | ||
| std::set<NodeId> attacker_ids; | ||
| for (int i = 0; i < duplicate_inbound; ++i) { | ||
| CNode* node = MakeInboundNode(id, 0x0a000001 + i); | ||
| node->nTimeFirstMessageReceived = 1s; | ||
| node->fFirstMessageIsMNAUTH = true; | ||
| node->SetVerifiedProRegTxHash(attacker_protx); | ||
| connman->AddTestNode(*node); | ||
| attacker_ids.insert(id++); | ||
| } | ||
|
|
||
| // A distinct legitimate masternode identity must remain protected. | ||
| CNode* honest = MakeInboundNode(id++, 0x0b000001); | ||
| honest->nTimeFirstMessageReceived = 1s; | ||
| honest->fFirstMessageIsMNAUTH = true; | ||
| honest->SetVerifiedProRegTxHash(honest_protx); | ||
| connman->AddTestNode(*honest); | ||
|
|
||
| // Our own outbound connection to the very same identity must never be an eviction | ||
| // candidate: the per-proTxHash cap applies to inbound sockets only. | ||
| CNode* outbound = MakeNode(id++, 0x0c000001, ConnectionType::OUTBOUND_FULL_RELAY); | ||
| outbound->nTimeFirstMessageReceived = 1s; | ||
| outbound->fFirstMessageIsMNAUTH = true; | ||
| outbound->SetVerifiedProRegTxHash(attacker_protx); | ||
| connman->AddTestNode(*outbound); | ||
|
|
||
| // CNode captures m_connected at construction; step past INBOUND_EVICTION_PROTECTION_TIME so | ||
| // only verified-proTxHash protection remains in play. | ||
| SetMockTime(GetTime<std::chrono::seconds>() + 1h); | ||
|
|
||
| std::set<NodeId> candidate_ids; | ||
| for (const auto& c : connman->GetEvictionCandidatesPublic()) { | ||
| candidate_ids.insert(c.id); | ||
| } | ||
|
|
||
| BOOST_CHECK_MESSAGE(candidate_ids.count(honest->GetId()) == 0, "a distinct verified MN must stay protected"); | ||
| BOOST_CHECK_MESSAGE(candidate_ids.count(outbound->GetId()) == 0, "verified outbound must never be evictable"); | ||
|
|
||
| int attacker_candidates = 0; | ||
| for (const NodeId attacker_id : attacker_ids) { | ||
| if (candidate_ids.count(attacker_id)) ++attacker_candidates; | ||
| } | ||
| // Exactly one of the attacker's duplicate inbound sockets keeps the protection; the rest | ||
| // must be evictable. Pre-fix all 8 were protected. | ||
| BOOST_CHECK_EQUAL(attacker_candidates, duplicate_inbound - 1); |
There was a problem hiding this comment.
🟡 Suggestion: Later commits are same-stack fixups that should be folded into the commits they amend
Commit 35b8cfb ('test: strengthen U023 regression coverage and tidy review nits') legitimately adds new coverage (the resolution matrix, the repeated-attempt loop, verified-outbound eviction case), but its diff also bundles three narrow self-corrections of the immediately preceding commit in the same stack: it removes the unused AttemptToEvictConnectionPublic test accessor added by 7516edc, fixes the out-of-alphabetical-order Makefile.test.include entry that 7516edc introduced, and replaces a stray non-ASCII em dash in src/net.cpp introduced by 04d17d2. Commit 6ac8645 ('docs: drop internal audit finding IDs from comments') then only strips leftover 'U023'/'V0NN' internal labels from comments written by the two prior commits in this same PR, with no independent contribution. Verified via git show on both commits: neither touches anything outside files/lines the stack itself introduced. None of these have standalone value in permanent git log/blame history; fold the accessor removal, Makefile-ordering fix, and em-dash fix into the commits that introduced them (7516edc and 04d17d2 respectively), and fold the label cleanup into whichever commit last wrote those comments, leaving only the net-new test coverage as its own commit.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — Later commits are same-stack fixups that should be folded into the commits they amend no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| { | ||
| READ_LOCK(m_nodes_mutex); | ||
| // Count unique verified inbound proRegTxHashes. Duplicates of the same identity must not | ||
| // inflate the limit exemption; only one exempt slot per masternode is assumed safe. | ||
| Uint256HashSet verified_inbound_protx; | ||
| for (const CNode* pnode : m_nodes) { | ||
| if (pnode->IsInboundConn()) { | ||
| nInbound++; | ||
| if (!pnode->GetVerifiedProRegTxHash().IsNull()) { | ||
| nVerifiedInboundMasternodes++; | ||
| if (const uint256 verified_protx = pnode->GetVerifiedProRegTxHash(); !verified_protx.IsNull()) { | ||
| verified_inbound_protx.insert(verified_protx); | ||
| } | ||
| } | ||
| } | ||
| nVerifiedInboundMasternodes = static_cast<int>(verified_inbound_protx.size()); |
There was a problem hiding this comment.
🟡 Suggestion: Admission-side proTxHash dedup in CreateNodeFromAcceptedSocket has no direct regression test
This PR replaces a per-socket nVerifiedInboundMasternodes++ counter with a Uint256HashSet that counts unique verified inbound proRegTxHashes, closing the same duplicate-exemption gap on the admission path that ResolveDuplicateConnection closes on the MNAUTH path. But the added test suite (src/test/evo_mnauth_tests.cpp) only exercises ResolveDuplicateConnection directly and the real CConnman::GetEvictionCandidates via GetEvictionCandidatesPublic() -- confirmed with a grep of the test file that no test touches this function or its counting logic. If this dedup were reverted to the old per-socket increment, the full test suite would still pass while the defense-in-depth exemption-stacking bug it's meant to catch would silently return. Extract the effective-inbound-count calculation into a small testable helper, or add a CConnman-level test proving N inbound sockets sharing one proRegTxHash yield only one limit-exempt slot while distinct identities each get their own.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Admission-side proTxHash dedup in CreateNodeFromAcceptedSocket has no direct regression test no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| @@ -54,6 +58,30 @@ class CMNAuth | |||
| READWRITE(obj.proRegTxHash, obj.sig); | |||
| } | |||
|
|
|||
| /** | |||
| * Decision for an already-verified peer that claims the same proRegTxHash as a newly authenticated peer. | |||
| * Exactly one of the two connections may remain verified; dual-inbound must not both stay open. | |||
| */ | |||
There was a problem hiding this comment.
💬 Nitpick: Class/struct comments overstate the invariant as total uniqueness, not inbound-only uniqueness
The class comment says 'a given proRegTxHash can only retain a single verified connection at a time' and the DuplicateResolution comment says 'exactly one of the two connections may remain verified.' But tracing ResolveDuplicateConnection in src/evo/mnauth.cpp: in the mixed-direction case (one side inbound, other outbound) it can return mark_existing_probe or mark_new_probe instead of a disconnect. CMNAuth::ProcessMessage only sets fDisconnect when disconnect_new/disconnect_existing is returned -- when a probe flag is set instead, the peer proceeds to peer.SetVerifiedProRegTxHash(...), so both the probe-marked leg and the newly-verified leg remain simultaneously verified (one inbound, one outbound) until CMasternodeUtils::DoMaintenance later drops the probe, which can be deferred while outbound capacity is available. The actual invariant enforced is 'at most one verified INBOUND connection per proRegTxHash' (which is what matters for the limit-exemption/eviction-immunity DoS this PR fixes), not 'at most one verified connection total.' Update the comments to describe the inbound-only invariant so future readers/callers don't assume two verified connections (one in each direction) can never coexist.
source: ['codex']
Verified inbound masternode peers are exempt from the inbound connection limit and are protected from eviction, so at most one may exist per proRegTxHash. When the local node was an active masternode and both the already-verified peer and the newly authenticated peer were inbound, neither branch of the duplicate handling resolved the collision: the deterministic-outbound branch only set m_masternode_probe_connection (a deferred hint acted on later, and only once outbound capacity is tight), and the other branch tested !IsInboundConn() on both peers, which was false for both. Both sockets stayed verified, letting a single operator key stack unbounded limit-exempt, eviction-immune inbound slots. Hoist the dual-inbound case so the new leg is dropped outright, and express the outcome as an enum so each resolution is a single value rather than four independent booleans. Mixed-direction and dual-outbound behaviour is unchanged.
6ac8645 to
49120ec
Compare
2 participants
Issue being fixed or feature implemented
In
CMNAuth::ProcessMessage, the duplicate-identity handler only resolved a collision when at least one of the two sockets was outbound. When the local node is an active masternode and both the already-verified peer and the newly authenticated peer are inbound, one branch merely flagged the old connection as a probe — a deferred hint acted on only afterPROBE_WAIT_INTERVAL, and only once outbound capacity is tight — and the other branch tested!IsInboundConn()on both peers, which was false for both, so nothing happened at all.That matters because verified inbound masternode peers are unconditionally protected in
GetEvictionCandidatesand are subtracted from the inbound limit inCreateNodeFromAcceptedSocket. Leaving both sockets verified therefore hands a single operator key an unbounded supply of connection slots that are both limit-exempt and eviction-immune.MNAUTH is signed over a per-connection random challenge against the operator BLS key of a masternode in the tip list, so replay is not possible; the attacker must hold a valid operator key, and the victim must be an active masternode. The unbounded case applies to victims where the deterministic outbound direction does not resolve in the victim's favour — roughly half of masternodes for a given attacker key — while the other half is rate-limited by the probe interval.
What was done?
Hoist the dual-inbound case so the new leg is disconnected outright, and express the outcome as a
DuplicateActionenum instead of four independent booleans (the previous struct could represent 16 states, only 5 of which were valid). Mixed-direction and dual-outbound behaviour is unchanged.An earlier revision of this PR also deduplicated by proRegTxHash in
GetEvictionCandidatesand inCreateNodeFromAcceptedSocketas defence in depth. Both have been dropped as redundant, and the reasoning is worth recording:ForEachNode, which filters onFullyConnectedOnly(fSuccessfullyConnected && !fDisconnect), and MNAUTH is rejected before VERACK. So a verified node can only hide from the scan by already being flaggedfDisconnect. Message processing is single-threaded, so there is no window in which two verified inbound legs coexist.GetEvictionCandidatesalready skipsfDisconnectnodes, so the dedup there guarded a state that cannot occur — while making which duplicate survives depend onm_nodesiteration order.ProcessMessagereturns beforeSetVerifiedProRegTxHashwhen the new leg is dropped, so a rejected duplicate counts towardnInboundbut never toward the exemption. It fails safe.Fixing this once, in the place where the "at most one verified inbound per proRegTxHash" invariant belongs, is preferable to three partial enforcements.
How Has This Been Tested?
src/test/evo_mnauth_tests.cppdrivesResolveDuplicateConnectiondirectly: the dual-inbound case for both deterministic-outbound orientations, the non-masternode victim case, the full resolution matrix (asserting the exact action for every direction combination), and a loop that drives the resolver the wayProcessMessagedoes and asserts the number of surviving verified inbound sockets never exceeds one. Pre-fix that count converged on N.Built locally and
test_dash --run_test=evo_mnauth_testspasses (4 cases). Full validation is delegated to CI.Breaking Changes
None.
Checklist: