Skip to content

fix: skip repeated authenticated Platform bans #7826

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 1 commit into from
Oct 15, 2026
Merged

fix: skip repeated authenticated Platform bans #7826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 4 additions & 5 deletions src/masternode/meta.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,11 +200,10 @@ bool CMasternodeMetaMan::SetPlatformBan(const uint256& inv_hash, PlatformBanMess

const uint256& protx_hash = ban_msg.m_protx_hash;

bool ret = GetMetaInfo(protx_hash).SetPlatformBan(true, ban_msg.m_requested_height);
if (ret) {
m_seen_platform_bans.emplace(inv_hash, std::move(ban_msg));
}
return ret;
const bool changed = GetMetaInfo(protx_hash).SetPlatformBan(true, ban_msg.m_requested_height);
// Remember authenticated messages even when newer metadata supersedes them.
m_seen_platform_bans.emplace(inv_hash, std::move(ban_msg));
return changed;
}

bool CMasternodeMetaMan::AlreadyHavePlatformBan(const uint256& inv_hash) const
Expand Down
2 changes: 1 addition & 1 deletion src/masternode/meta.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ class CMasternodeMetaInfo
if (height < m_platform_ban_updated) {
return false;
}
if (height == m_platform_ban_updated && !is_banned) {
if (height == m_platform_ban_updated && (!is_banned || m_platform_ban)) {
return false;
}
m_platform_ban = is_banned;
Expand Down
8 changes: 6 additions & 2 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3846,14 +3846,18 @@ MessageProcessingResult PeerManagerImpl::ProcessPlatformBanMessage(NodeId node,

const uint256 hash = ban_msg.GetHash();

LogPrintf("PLATFORMBAN -- hash: %s protx_hash: %s height: %d peer=%d\n", hash.ToString(), ban_msg.m_protx_hash.ToString(), ban_msg.m_requested_height, node);

// NOTE: deliberately no solicitation gate here, unlike the other GETDATA-only object types.
// PLATFORMBAN has no local ingress (no RPC, no internal producer): the originating Dash
// Platform node injects the ban by pushing the message straight to a Dash Core peer, so the
// first hop is always unsolicited by design. See p2p_platform_ban.py.
MessageProcessingResult ret{};
ret.m_to_erase = CInv{MSG_PLATFORM_BAN, hash};
if (m_mn_metaman.AlreadyHavePlatformBan(hash)) {
return ret;
}

LogPrintf("PLATFORMBAN -- hash: %s protx_hash: %s height: %d peer=%d\n", hash.ToString(),
ban_msg.m_protx_hash.ToString(), ban_msg.m_requested_height, node);

const auto list = m_dmnman.GetListAtChainTip();
auto dmn = list.GetMN(ban_msg.m_protx_hash);
Expand Down
5 changes: 5 additions & 0 deletions test/functional/p2p_platform_ban.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ def run_test(self):
wait_until_helper(lambda: p2p_node2.message_count["platformban"] > 0, timeout=10, lock=p2p_lock)
p2p_node2.message_count["platformban"] = 0

self.log.info("An already verified Platform ban skips handler work")
ban_inv_hash = hash256(msg.serialize()[:-96])[::-1].hex()
with self.mninfo[2].get_node(self).assert_debug_log([], unexpected_msgs=[f"PLATFORMBAN -- hash: {ban_inv_hash}"]):
p2p_node2.send_and_ping(msg)
Comment on lines +76 to +79

@thepastaclaw thepastaclaw Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

鈴笍 Deferred (maintainer decision; not blocking) at 20399642; see the replies below.

馃煛 Suggestion: Cover cold-cache duplicate and superseded metadata updates

This replay follows a successful first delivery that already populated the authenticated cache, so it returns before reaching either modified metadata method. It therefore covers the handler fast path but not same-height metadata idempotence or caching an authenticated ban rejected by a newer watermark鈥攂oth explicit goals of this PR. Add cold-cache scenarios by restarting the receiver with persisted banned metadata, and separately by applying a newer service update to the targeted EvoNode before restarting. Keep the signed message within the accepted height window. For each scenario, verify that the first delivery reaches the handler without forwarding inventory or changing the persisted ban state/watermark, and that the second delivery produces no new handler log. The existing protx info metaInfo fields expose the state and watermark directly.

source: gpt-6.1-sol (phase2-reviewer: general)

@PastaPastaPasta PastaPastaPasta Oct 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Declining for this PR. The two metadata changes (same-height SetPlatformBan returning false, and caching a ban superseded by a newer watermark) are each a one-line condition that is clear from the diff, and the maintainer preference here is to keep the PR minimal rather than add restart-based functional scenarios for them. The existing replay check covers the handler fast path, which is the behavior the fix is about.

馃 Posted autonomously by Claude on behalf of pasta.

@thepastaclaw thepastaclaw Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Understood; I accept keeping this PR minimal and deferring the restart-based coverage, since the existing replay test covers the handler fast path and I have not identified a correctness defect in the two metadata changes.

Noted as intentionally deferred; I will not press it further here.


assert not self.check_banned(self.mninfo[0])

mninfos_valid = self.mninfo.copy()[1:]
Expand Down
Loading