feat: implement Platform Ban PoSe DIP-0031 #6613
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces Platform PoSe ban (DIP-0031) end-to-end: new P2P message PLATFORMBAN / MSG_PLATFORM_BAN and protocol version bump; net_processing adds ProcessPlatformBanMessage to validate, penalize, remember, and relay bans with llmq quorum/recovered-sig checks; masternode metadata (CMasternodeMetaInfo/CMasternodeMetaMan) gains platform-ban state, storage, and serialization version bump; CDeterministicMNManager is wired to CMasternodeMetaMan and attempts to clear bans on provider-update-service transactions during block processing; llmq/DKG connection checks consider platform-ban status; test framework and a new p2p_platform_ban functional test added. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/evo/deterministicmns.cpp (1)
843-847: Platform ban status management for masternodes.This code properly integrates platform ban functionality with the ProUpServTx processing by attempting to revive platform ban status when a service update transaction is processed. The error handling correctly logs failures without disrupting the main transaction processing flow.
However, consider enhancing the log message to provide more details about why the platform ban status couldn't be updated (whether the metadata wasn't found or the SetPlatformBan call failed).
- LogPrintf("CDeterministicMNManager::%s -- MN %s is not Platform revived at height %d\n", __func__, - opt_proTx->proTxHash.ToString(), nHeight); + LogPrintf("CDeterministicMNManager::%s -- MN %s is not Platform revived at height %d: %s\n", __func__, + opt_proTx->proTxHash.ToString(), nHeight, + meta_info ? "SetPlatformBan failed" : "metadata not found");test/functional/p2p_platform_ban.py (1)
1-84: New test file properly implements platform ban validationThe test file correctly implements the platform ban functionality validation in a P2P context, with proper test setup, message creation, signing, and verification of the ban/unban process.
There are a few minor issues that could be addressed:
-from test_framework.util import p2p_port, wait_until_helper +from test_framework.util import wait_until_helper-for i in range(2): +for _ in range(2):-self.log.info(f"Platform ban message is created and signed") +self.log.info("Platform ban message is created and signed")🧰 Tools
🪛 Ruff (0.8.2)
12-12:
test_framework.util.p2p_portimported but unusedRemove unused import:
test_framework.util.p2p_port(F401)
40-40: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
63-63: f-string without any placeholders
Remove extraneous
fprefix(F541)
test/functional/test_framework/p2p.py (3)
162-162: Clarify future usage
The message typeqsendrecsigscurrently has no associated handler and is set toNone. Confirm if this is intentional or if a handler is scheduled for future development.
594-596: Add callback logic or leave a note
These newly introduced callbacks (on_islock,on_mnlistdiff,on_platformban) are empty. Consider adding logging or a TODO comment for each to highlight expected functionality or testing.
598-598: Implementqgetdatacallback
The newly reintroducedon_qgetdatais also empty. If needed, define the intended behavior or add a TODO note for clarity and future reference.src/masternode/meta.h (2)
100-112: Validate ban logic
TheSetPlatformBanmethod enforces increasing ban heights and disallows unbanning if the heights match. This is logical, but ensure all call sites handle thefalsereturn case properly.+// Example: Check return value to confirm updates are applied +if (!metaInfo->SetPlatformBan(true, newHeight)) { + // handle or log unsuccessful update +}
182-205: PlatformBanMessage class introduction
This class effectively captures the required ban data, including the BLS signature. Ensure test coverage for serialization logic and hashing.Add tests to confirm:
- Proper serialization paths (with and without
SER_GETHASH).- Hash consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/evo/deterministicmns.cpp(2 hunks)src/evo/deterministicmns.h(2 hunks)src/llmq/dkgsession.cpp(1 hunks)src/masternode/meta.cpp(3 hunks)src/masternode/meta.h(8 hunks)src/net_processing.cpp(6 hunks)src/node/chainstate.cpp(1 hunks)src/protocol.cpp(4 hunks)src/protocol.h(2 hunks)src/version.h(3 hunks)test/functional/p2p_platform_ban.py(1 hunks)test/functional/test_framework/messages.py(1 hunks)test/functional/test_framework/p2p.py(3 hunks)test/functional/test_runner.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
test/functional/test_framework/p2p.py (1)
test/functional/test_framework/messages.py (3) (3)
msg_platformban(2348-2380)msg_qdata(2451-2492)msg_qgetdata(2419-2448)
🪛 Ruff (0.8.2)
test/functional/p2p_platform_ban.py
12-12: test_framework.util.p2p_port imported but unused
Remove unused import: test_framework.util.p2p_port
(F401)
40-40: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
63-63: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (39)
test/functional/test_runner.py (1)
146-146: New test script added for platform ban functionality.This addition integrates the
p2p_platform_ban.pytest script into the test suite, correctly placed among tests that run in less than 60 seconds and annotated with the required dependency note.src/evo/deterministicmns.cpp (1)
13-13: Added masternode metadata header.Appropriate include for the new platform ban functionality.
src/protocol.h (2)
301-301: Added new platform ban message type.This addition correctly declares the new PLATFORMBAN message type in the NetMsgType namespace.
528-528: Added platform ban message enum value.The enum value is appropriately defined with a descriptive comment referencing DIP-0031. This provides context for the purpose of this new message type.
src/version.h (3)
14-14: Incremented protocol version.Protocol version increment is appropriate for introducing a breaking change with the new platform ban functionality.
23-23: Updated minimum masternode protocol version.This ensures all masternodes will support the new platform ban functionality, aligning with the PR objectives.
64-65: Added platform ban version constant.Good practice to define a specific version constant for the new feature with a clear explanatory comment.
src/node/chainstate.cpp (1)
216-216: Addedmn_metamanparameter to CDeterministicMNManager initializationThis change incorporates masternode metadata management by adding
mn_metamanas a parameter when initializingCDeterministicMNManager. This is a critical integration point for the new platform ban functionality, allowing the deterministic masternode manager to access metadata about masternodes, including their platform ban status.src/llmq/dkgsession.cpp (1)
496-504: Platform ban check implemented in DKG session verificationAdded a check for platform-banned masternodes during the DKG connection verification process. If a masternode is platform banned, it will be marked with
badConnection=trueand excluded from participating in quorum formations. This change also improves efficiency by storing the metadata object reference inmeta_infoinstead of repeatedly callingGetMetaInfo().This implementation ensures that banned platform nodes cannot participate in DKG sessions, which is a core requirement of DIP-0031.
test/functional/test_framework/messages.py (1)
2348-2381: Added new msg_platformban class for P2P message handlingThis class implements the new PLATFORMBAN message type for peer-to-peer communication, following the same pattern as other message types in the framework. It includes proper serialization, deserialization, and hash calculation functionality, which are essential for testing platform ban propagation.
The implementation correctly includes:
- Serialization of protx_hash, requested_height, quorum_hash, and signature
- Deserialization from network format
- A
calc_sha256()method that follows the expected message format- A clear string representation for debugging
src/evo/deterministicmns.h (3)
34-34: Forward declaration for new dependency looks goodAdding forward declaration for
CMasternodeMetaManclass is appropriate since it's used as a reference parameter in theCDeterministicMNManagerconstructor.
569-569: New dependency member variable properly definedThe new private member variable
m_mn_metamanis correctly defined as a reference, ensuring the lifetime management is handled appropriately.
577-581: Constructor correctly updated with dependency injectionThe constructor signature has been updated to include the new
CMasternodeMetaManparameter, and the initialization list correctly initializes the new member variable. This follows the dependency injection pattern, which improves testability and follows SOLID principles.src/protocol.cpp (4)
93-93: New PLATFORMBAN message type correctly definedThe new message type is correctly defined using the
MAKE_MSGmacro with the appropriate command string.
174-175: PLATFORMBAN message type added to allNetMessageTypes arrayThe new message type is correctly added to the comprehensive list of all network message types.
195-195: PLATFORMBAN message type added to blocks-relay-only policy exceptionsThe new message type is correctly added to the
netMessageTypesViolateBlocksOnlyarray, ensuring it's subject to the blocks-relay-only policy.
302-302: Added case for MSG_PLATFORM_BAN in GetCommandInternalThe new message type is correctly mapped in the
GetCommandInternalmethod, enabling proper command string lookups for platform ban messages.src/masternode/meta.cpp (3)
46-47: Added platform ban information to JSON outputThe
ToJsonmethod now correctly includes platform ban status and update height in the JSON representation.
132-151: Platform ban tracking methods implementedThree new methods are correctly implemented to manage platform bans:
AlreadyHavePlatformBan: checks if a platform ban is already trackedGetPlatformBan: retrieves a platform ban message by hashRememberPlatformBan: adds a new platform ban to the tracking setThese methods are properly synchronized with the
csmutex to ensure thread safety.
162-162: Added GetHash method to PlatformBanMessageThe
GetHashmethod uses the standard serialization hash function to generate a unique identifier for platform ban messages, which is essential for their storage and lookup.src/net_processing.cpp (9)
41-41: Added appropriate header for enumeration value conversionThe inclusion of
util/underlying.hprovides theToUnderlyingfunction, which is used later for converting enum class values to their underlying integer type. This is a clean approach for handling enum values in logging and messaging.
2263-2264: LGTM: Added platform ban support to AlreadyHave inventory systemThis properly extends the AlreadyHave function to check if a platform ban message is already known, integrating it with the existing masternode metadata manager. This is necessary for the proper functioning of the inventory system with the new message type.
2887-2893: LGTM: Proper handling of platform ban requests in ProcessGetDataThis code correctly handles GETDATA requests for platform ban messages, checking if they exist in the masternode metadata manager and pushing the message to the peer if found. This follows the same pattern as other specialized message types in this function.
3522-3529: Proper implementation of early-return checks for platform ban messagesThe method correctly validates that the message type matches before processing and checks that the blockchain is sufficiently synced. The early return pattern for validation checks is consistent with other message handling code in this file.
3531-3547: Thorough validation of incoming platform ban messagesThe code properly deserializes the message and performs important validation on the specified masternode:
- Checks if the masternode exists in the current tip list
- Ensures the masternode is of the correct type (Evo)
This prevents misuse of the platform ban mechanism for nodes that shouldn't be subject to it.
3549-3559: Proper validation of platform ban height constraintsThe code verifies that the ban height is within reasonable bounds (within a day's worth of blocks from the current tip), rejecting messages that are too old or implausibly in the future. This helps prevent replay attacks and ensures bans are applied in a relevant timeframe.
I appreciate the differentiated error handling with more severe penalties (10) for significantly out-of-range heights versus mild penalties (1) for borderline cases.
3561-3577: Robust signature verification for platform ban messagesThe implementation properly:
- Retrieves the correct LLMQ type from consensus parameters
- Verifies the quorum exists for the provided hash
- Constructs the request ID and message hash for signature verification
- Verifies the signature against the quorum
This cryptographic validation is critical to ensure only authorized bans are processed. The use of
SerializeHashfor message data and the explicit prefix for request IDs follows best practices.
3579-3588: LGTM: Effective platform ban application and propagationAfter successful validation, the code:
- Updates the masternode's metadata to mark it as banned
- Records the ban message for future reference
- Relays the ban to other nodes via the inventory system
The conditional check on
SetPlatformBanensures we only relay the message if the ban state actually changed, avoiding unnecessary network traffic.
5354-5354: LGTM: Proper integration with message processing flowThe call to
ProcessPlatformBanMessageis correctly placed among other specialized message processing functions, maintaining the organizational structure of the codebase while integrating the new functionality.test/functional/test_framework/p2p.py (2)
68-68: Acknowledge new message import
Addingmsg_platformbanhere appears consistent with the new platform ban message type.
158-159:❓ Verification inconclusive
Check message map consistency
Includingplatformbanandqdataentries inMESSAGEMAPseems correct. Ensure that downstream code fully handles these new message types (deserialization, validation, etc.).Run a quick search to confirm both message types are properly tested:
🏁 Script executed:
#!/bin/bash rg -A 5 "msg_platformban|msg_qdata"Length of output: 4009
Action: Verify Message Map and Downstream Handling for New Message Types
- The mapping for both
platformbanandqdatainMESSAGEMAP(intest/functional/test_framework/p2p.py) is consistent.- The
platformbanmessage is exercised in tests (e.g. intest/functional/p2p_platform_ban.py) and its implementation is confirmed intest/functional/test_framework/messages.py.- The
qdatamessage is defined intest/functional/test_framework/messages.pyand included in the message map. However, there’s no evidence of dedicated tests forqdata—so please verify that its deserialization, validation, and error handling are fully exercised downstream (or consider adding tests if they are lacking).src/masternode/meta.h (8)
8-8: Confirm necessity of new BLS include
Including<bls/bls.h>is appropriate for managing the newCBLSSignatureusage. Confirm that all dependent build files or linking steps are updated.
51-52: Initialize platform ban state
The addition ofm_platform_banandm_platform_ban_heightwith concurrency guards is a well-structured approach for maintaining thread-safe ban state.
63-65: Copy constructor updates
Copying the platform ban fields ensures the ban state is preserved in all meta copies. Good practice for consistent object behavior.
72-74: Serialize/deserialize ban fields
Properly extendingREADWRITEto includem_platform_banandm_platform_ban_heightis correct. Ensure forward/backward compatibility if older code loads or stores this data.
113-117: Thread-safe accessor
IsPlatformBanned()is straightforward and leverages the recursive mutex for concurrency. Looks good.
172-181: High-level documentation
These new explanation comments about Platform PoSe Ban and DIP-0031 provide clarity on ban functionality. Good inclusion for maintainers.
239-241: Platform ban storage management
MethodsAlreadyHavePlatformBan,GetPlatformBan, andRememberPlatformBanfacilitate ban caching. Confirm the concurrency model inm_seen_platform_bansis well-tested to prevent race conditions with updates.
17-17:✅ Verification successful
Ensure consistent use of
<optional>
Including<optional>suggests new code relies on optional types. Double-check that no older compilers are impacted and that usage is consistent across the codebase.
🏁 Script executed:
#!/bin/bash rg -A 3 "std::optional"Length of output: 65770
Optional usage is consistent across the codebase.
The grep output confirms that the use of
std::optionalis widespread and consistent, aligning with the recent migration to C++17 and its header inclusion. There is no evidence that adding#include <optional>insrc/masternode/meta.hintroduces issues on supported compilers.
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/evo/deterministicmns.cpp(2 hunks)src/evo/deterministicmns.h(2 hunks)src/llmq/dkgsession.cpp(1 hunks)src/masternode/meta.cpp(3 hunks)src/masternode/meta.h(8 hunks)src/net_processing.cpp(6 hunks)src/node/chainstate.cpp(1 hunks)src/protocol.cpp(4 hunks)src/protocol.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/protocol.h
- src/evo/deterministicmns.cpp
- src/llmq/dkgsession.cpp
- src/protocol.cpp
- src/node/chainstate.cpp
- src/masternode/meta.cpp
- src/evo/deterministicmns.h
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/net_processing.cpp (1)
src/util/underlying.h (1)
ToUnderlying(10-10)
🔇 Additional comments (22)
src/net_processing.cpp (14)
41-41: Appropriate addition of required headerThe addition of
util/underlying.his necessary for the use of theToUnderlyingfunction which will be used in the platform ban message processing logic.
2263-2265: Well-integrated condition for platform ban handlingThis addition to the
AlreadyHavefunction properly integrates platform ban messages into the existing inventory system. The implementation correctly delegates to the masternode metadata manager to check if we already have a specific platform ban.
2887-2893: Properly implemented platform ban message retrievalThis addition to the
ProcessGetDatafunction handles the retrieval and sending of platform ban messages when requested by peers. The implementation follows the established pattern for other message types and properly uses the masternode metadata system.
3522-3527: Good initial validation for Platform Ban messagesThe function correctly checks if the message type is appropriate and if the node is synced before processing the ban message. Blocking the processing of such messages during initial blockchain sync helps prevent potential issues during the sync process.
3528-3532: Well-structured message deserialization and loggingThe implementation properly deserializes the platform ban message and logs useful information about it, which will help with debugging and monitoring.
3533-3540: Proper masternode validationThe code correctly validates that the masternode exists in the deterministic masternode list, with appropriate error handling if it doesn't. The small penalty for recent removals is an appropriate approach to handle edge cases during network updates.
3541-3547: Effective type validation with appropriate penaltyThis section effectively validates that the target is an evolution node (Evo) and not a regular masternode. The significant penalty (100) for attempting to ban a node of incorrect type is appropriate for preventing misuse.
3548-3559: Robust height validation logicThe height validation checks ensure the platform ban message refers to a relevant and recent block height, with the day_of_blocks variable providing a clear time window. The tiered penalty approach (10 for significant deviation vs 1 for edge cases) is a good balance.
3560-3567: Comprehensive quorum validationThe code properly validates the quorum information by checking that the specified quorum exists for the platform ban message. The clear error message with detailed information will assist in debugging issues.
3568-3573: Secure message signing setupThe implementation establishes a secure process for signature verification by constructing a well-defined request ID and message hash. The use of a prefixed identifier helps prevent signature reuse attacks across different message types.
3574-3578: Robust signature verificationThe code properly verifies the signature using the LLMQ verification system, checking it against the active chain for the specified height. The detailed error logging will be valuable for troubleshooting.
3579-3587: Effective state update and message propagationOnce validation is complete, the code properly updates the masternode metadata to record the platform ban status, remembers the platform ban message for future reference, and relays it to other nodes. This ensures proper propagation of valid ban messages throughout the network.
5355-5355: Well-positioned message processing callThe addition of
ProcessPlatformBanMessageto the message processing chain is placed at the right location, along with other similar message types. The processing result is also handled correctly using theProcessPeerMsgRetfunction.
3564-3564: Proper enum conversion using ToUnderlyingThe use of
ToUnderlyinghere is appropriate for converting the enum value to its integer representation for logging purposes. This follows best practices for type-safe enum handling.src/masternode/meta.h (8)
8-8: Appropriate inclusion of necessary headers.The addition of
<bls/bls.h>is needed for theCBLSSignaturetype used in the newPlatformBanMessageclass, and<optional>supports the new return type inGetPlatformBan.Also applies to: 17-17
51-52: Thread-safe implementation of platform ban properties.The new member variables for platform ban state are correctly declared with proper mutex protection and reasonable default values. The naming convention is consistent with the codebase style.
63-65: Copy constructor properly handles new ban fields.The constructor correctly initializes the new platform ban state from the reference object, maintaining consistency with existing implementation patterns.
72-74: Serialization updated to include new platform ban fields.The SERIALIZE_METHODS are properly updated to include the new platform ban state in the serialization process, ensuring persistence of this data.
100-117: Well-designed platform ban state management methods.The new
SetPlatformBanandIsPlatformBannedmethods are properly implemented with:
- Thread safety through mutex locking
- Logic to prevent overriding a ban with an older ban height
- Prevention of unsetting a ban at the same height
- Clear return value indicating operation success
This provides a robust interface for managing platform ban state.
172-204: Well-documented PlatformBanMessage class implementation.The new class is properly structured with:
- Clear documentation explaining its purpose and relation to DIP-0031
- Appropriate member variables for ban message data
- Proper serialization handling with special case for hash calculation
- Declaration of the GetHash method
The implementation follows good practices for serialization by excluding the signature when computing the hash.
216-216: Thread-safe tracking of platform ban messages.The new member variable
m_seen_platform_bansis properly declared as a map with mutex protection, enabling the class to track and deduplicate platform ban messages.
239-241: Complete API for platform ban message management.The three new methods provide a comprehensive interface for:
- Checking if a platform ban message has already been seen
- Retrieving a platform ban message by hash
- Recording new platform ban messages
The return of an optional value from
GetPlatformBanis a modern and safe approach.
| class PlatformBanMessage | ||
| { | ||
| public: | ||
| uint256 m_protx_hash; | ||
| int32_t m_requested_height{0}; | ||
| uint256 m_quorum_hash; | ||
| CBLSSignature m_signature; | ||
|
|
||
| PlatformBanMessage() = default; | ||
|
|
||
| SERIALIZE_METHODS(PlatformBanMessage, obj) | ||
| { | ||
| READWRITE(obj.m_protx_hash, obj.m_requested_height, obj.m_quorum_hash); | ||
| if (!(s.GetType() & SER_GETHASH)) { | ||
| READWRITE(CBLSSignatureVersionWrapper(const_cast<CBLSSignature&>(obj.m_signature), false)); | ||
| } | ||
| } | ||
|
|
||
| uint256 GetHash() const; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify signature validation for platform ban messages.
The PlatformBanMessage class handles serialization correctly, but ensure that signature validation is implemented in the corresponding cpp file. The class would benefit from a method to verify the signature against the hashed message data.
🏁 Script executed:
#!/bin/bash
# Check if there's signature validation logic for PlatformBanMessages
echo "Searching for PlatformBanMessage signature validation..."
rg -A 10 "bool.*PlatformBanMessage.*[Vv]alidat" --type cppLength of output: 183
Action: Add Signature Verification Logic for PlatformBanMessage
It appears that the current implementation in src/masternode/meta.h only handles serialization and hash generation (via GetHash()), but no signature validation method is present in the corresponding source file. To ensure robust security for platform ban messages, please add a dedicated method (for example, bool ValidateSignature() const) in the corresponding CPP file (likely src/masternode/meta.cpp) that verifies the signature (m_signature) against the message hash.
- File to update:
src/masternode/meta.cpp(or the appropriate implementation file forPlatformBanMessage) - Action required: Implement signature validation using the hashed message data (obtained by
GetHash()). - Recommendation: Confirm that the cryptographic routine used to validate
CBLSSignatureis correctly integrated.
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
test/functional/p2p_platform_ban.py (11)
2-2: Update copyright year to reflect current year.The copyright year is set to 2025, which is in the future. This should be corrected to the current year (2024).
-# Copyright (c) 2025 The Dash Core developers +# Copyright (c) 2024 The Dash Core developers
16-19: Consider implementing message handling methods in PlatformBanInterface.The
PlatformBanInterfaceclass extendsP2PInterfacebut doesn't implement any additional functionality. Consider adding message handling methods likeon_platformban()to process incoming platform ban messages.class PlatformBanInterface(P2PInterface): def __init__(self): super().__init__() + + def on_platformban(self, message): + """Handle incoming platform ban messages.""" + self.message_count["platformban"] = self.message_count.get("platformban", 0) + 1
43-43: Remove unused variable assignment.The
self.mempool_sizevariable is set but never used elsewhere in the test. Consider removing it if not needed.- self.mempool_size = 0
52-53: Add a comment explaining the request ID generation logic.The request ID generation combines several elements but lacks documentation explaining the rationale. Consider adding a comment to clarify the purpose and format of this identifier.
+ # Generate request ID by combining the ban message intent ("PlatformPoSeBan"), target proTxHash, and requested height request_id_buf = ser_string(b"PlatformPoSeBan") + ser_uint256(msg.protx_hash) + struct.pack("<I", msg.requested_height) request_id = hash256(request_id_buf)[::-1].hex()
60-61: Add comment explaining the use of platform signing.The
use_platformsign=Trueparameter suggests there are different signing mechanisms. Consider adding a comment to explain why platform signing is used specifically for this test case.+ # Use platform signing specifically for platform ban messages recsig = self.get_recovered_sig(request_id, msg_hash, llmq_type=106, use_platformsign=True) msg.sig = bytearray.fromhex(recsig["sig"])
66-67: Extract timeout value to a constant and add assertion message.The timeout value is hardcoded. Consider extracting it to a constant and adding an assertion message to improve test readability and maintainability.
+ PLATFORMBAN_TIMEOUT = 10 - wait_until_helper(lambda: p2p_node2.message_count["platformban"] > 0, timeout=10, lock=p2p_lock) + wait_until_helper(lambda: p2p_node2.message_count["platformban"] > 0, timeout=PLATFORMBAN_TIMEOUT, + lock=p2p_lock, err_msg="Timed out waiting for platformban message") p2p_node2.message_count["platformban"] = 0
70-70: Add descriptive assertion message.Add a descriptive message to the assertion to make test failures more informative.
- assert not self.check_banned(self.mninfo[0]) + assert not self.check_banned(self.mninfo[0]), "Masternode should not be banned after a single platform ban message"
77-77: Add descriptive assertion message.Add a descriptive message to the assertion to make test failures more informative.
- assert self.check_banned(self.mninfo[0]) + assert self.check_banned(self.mninfo[0]), "Masternode should be banned after receiving platform ban message with valid quorum"
80-80: Add descriptive assertion message.Add a descriptive message to the assertion to make test failures more informative.
- assert not self.check_banned(self.mninfo[0]) + assert not self.check_banned(self.mninfo[0]), "Masternode ban should be cleared after service update"
21-81: Add docstrings to classes and methods.The test class and methods lack documentation. Consider adding docstrings to explain the purpose of the test and its methods.
class PlatformBanMessagesTest(DashTestFramework): + """ + Test for the platform ban functionality as defined in DIP-0031. + Verifies that platform ban messages are correctly processed, leading to masternode bans, + and that the bans can be cleared through service updates. + """ def set_test_params(self): self.set_dash_test_params(1, 0, [[]], evo_count=3) def skip_test_if_missing_module(self): self.skip_if_no_wallet() def check_banned(self, mn): + """Check if a masternode is currently banned based on its PoSeBanHeight.""" info = self.nodes[0].protx('info', mn.proTxHash) return info['state']['PoSeBanHeight'] != -1 def run_test(self): + """ + Main test method: + 1. Set up test environment with sporks and masternodes + 2. Create and sign a platform ban message + 3. Verify the ban is not applied after a single message + 4. Mine additional quorums and send the message again + 5. Verify the ban is applied + 6. Update the masternode service and verify the ban is cleared + """
1-84: The test implements the platform ban functionality well but could be more robust.Overall, the test provides good coverage of the platform ban functionality described in DIP-0031. It tests message creation, transmission, ban application, and ban clearing. The implementation appears correct, but some improvements could make the test more robust:
Consider testing additional edge cases like:
- Invalid signatures
- Messages from non-quorum members
- Attempts to ban non-existent masternodes
- Repeated ban messages
The test could benefit from more explicit validation of the ban mechanism's implementation details, such as:
- Verifying the correct ban reason is recorded
- Checking ban persistence across node restarts
- Testing the effect of bans on masternode payments
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/evo/deterministicmns.cpp(2 hunks)src/evo/deterministicmns.h(2 hunks)src/llmq/dkgsession.cpp(1 hunks)src/masternode/meta.cpp(2 hunks)src/masternode/meta.h(8 hunks)src/net_processing.cpp(6 hunks)src/node/chainstate.cpp(1 hunks)src/protocol.cpp(4 hunks)src/protocol.h(2 hunks)src/version.h(3 hunks)test/functional/p2p_platform_ban.py(1 hunks)test/functional/test_framework/messages.py(1 hunks)test/functional/test_framework/p2p.py(4 hunks)test/functional/test_runner.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/functional/test_runner.py
🚧 Files skipped from review as they are similar to previous changes (12)
- src/protocol.h
- src/llmq/dkgsession.cpp
- src/evo/deterministicmns.cpp
- src/masternode/meta.cpp
- src/protocol.cpp
- src/version.h
- src/node/chainstate.cpp
- test/functional/test_framework/p2p.py
- test/functional/test_framework/messages.py
- src/evo/deterministicmns.h
- src/net_processing.cpp
- src/masternode/meta.h
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
|
tests are failing |
yes, it is indeed failing due to more strict validations during mining quorum, introduced by #6645 |
There was a problem hiding this comment.
Pull Request Overview
This PR implements DIP-0031 by adding support for a new platform ban P2P message and associated functionality. Key changes include:
- Introducing the new platformban message in the P2P layer and updating protocol/version definitions.
- Adding functional tests and test framework support for platform ban.
- Updating masternode metadata and deterministic masternode manager to incorporate platform ban status.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/functional/test_runner.py | Added p2p_platform_ban.py to the list of functional tests |
| test/functional/test_framework/p2p.py | Registered new msg_platformban and added related handler callbacks |
| test/functional/test_framework/messages.py | Introduced msg_platformban with serialization support |
| test/functional/p2p_platform_ban.py | Implemented a functional test for the platform ban message |
| src/version.h | Bumped protocol and masternode protocol versions |
| src/protocol.h & src/protocol.cpp | Registered new platformban message type in the protocol |
| src/net_processing.cpp | Added processing logic for PLATFORMBAN messages and integrated them |
| src/masternode/meta.h & src/masternode/meta.cpp | Extended masternode metadata to track platform ban information |
| src/llmq/dkgsession.cpp | Updated connection verification to reject Platform PoSe banned nodes |
| src/evo/deterministicmns.h & src/evo/deterministicmns.cpp | Updated deterministic MN manager to include platform ban checks |
src/net_processing.cpp
Outdated
| PeerMsgRet PeerManagerImpl::ProcessPlatformBanMessage(CNode& pfrom, std::string_view msg_type, CDataStream& vRecv) | ||
| { |
There was a problem hiding this comment.
[nitpick] Consider adding a brief docstring or inline comment for ProcessPlatformBanMessage to clarify its validation logic and the meaning of returned error codes, aiding future maintainers.
| PeerMsgRet PeerManagerImpl::ProcessPlatformBanMessage(CNode& pfrom, std::string_view msg_type, CDataStream& vRecv) | |
| { | |
| /** | |
| * Process a PLATFORMBAN message received from a peer. | |
| * | |
| * @param pfrom The peer sending the message. | |
| * @param msg_type The type of the message (should be PLATFORMBAN). | |
| * @param vRecv The serialized message data. | |
| * @return A PeerMsgRet object indicating the result of processing: | |
| * - An empty object if the message is invalid or ignored. | |
| * - An error code if the message is malformed or violates protocol rules. | |
| * | |
| * Validation logic: | |
| * - The function does nothing if the node is not synchronized with the blockchain. | |
| * - The message is deserialized and logged for further processing. | |
| */ | |
| PeerMsgRet PeerManagerImpl::ProcessPlatformBanMessage(CNode& pfrom, std::string_view msg_type, CDataStream& vRecv) |
Copilot uses AI. Check for mistakes.
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6613.b94d7255. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6613.b94d7255. The image should be on dockerhub soon. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Consider:
|
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/net_processing.cpp (3)
727-729: Correct signature and attributes for ProcessPlatformBanMessageNice: [[nodiscard]] and the locking annotations are consistent with the surrounding handlers.
3553-3560: Avoid magic number 576: use a named constant for the windowThe 576-block window should be a named constexpr to document intent and ease maintenance.
Introduce a global constant alongside other constants near the top of this file:
(outside this hunk; shown for clarity)
C++ snippet to add near other static constexpr globals (e.g., after NODE_NETWORK_LIMITED_MIN_BLOCKS):
"""
// Number of blocks in ~24 hours (DIP-0031 Platform Ban window)
static constexpr int PLATFORM_BAN_WINDOW_BLOCKS = 576;
"""Then replace the literal here:
- constexpr int day_of_blocks = 576; + // one-day window (see PLATFORM_BAN_WINDOW_BLOCKS) + constexpr int day_of_blocks = PLATFORM_BAN_WINDOW_BLOCKS; @@ - if (tipHeight < ban_msg.m_requested_height || tipHeight - day_of_blocks > ban_msg.m_requested_height) { + if (tipHeight < ban_msg.m_requested_height || tipHeight - PLATFORM_BAN_WINDOW_BLOCKS > ban_msg.m_requested_height) { @@ - if (tipHeight + 5 < ban_msg.m_requested_height || tipHeight - day_of_blocks - 5 > ban_msg.m_requested_height) { + if (tipHeight + 5 < ban_msg.m_requested_height || tipHeight - PLATFORM_BAN_WINDOW_BLOCKS - 5 > ban_msg.m_requested_height) {
3539-3595: Erase in-flight request when receiving PLATFORMBAN to avoid stale GETDATA stateWhen we successfully process a PLATFORMBAN object, we should clear any in-flight GETDATA request and g_already_asked_for entry for that object, like we do for CLSIG. Otherwise, the requester bookkeeping can linger until expiry.
Apply within this function’s success path:
@@ - MessageProcessingResult ret{}; + MessageProcessingResult ret{}; @@ - if (meta_info->SetPlatformBan(true, ban_msg.m_requested_height)) { + if (meta_info->SetPlatformBan(true, ban_msg.m_requested_height)) { + // Clear in-flight request tracking for this object (if any) + WITH_LOCK(::cs_main, EraseObjectRequest(node, CInv{MSG_PLATFORM_BAN, hash})); LogPrintf("PLATFORMBAN -- forward message to other nodes\n"); m_mn_metaman.RememberPlatformBan(hash, std::move(ban_msg)); CInv platform_ban_inv{MSG_PLATFORM_BAN, hash}; RelayInv(platform_ban_inv); } - return {}; + return ret;
🧹 Nitpick comments (4)
src/net_processing.cpp (4)
3567-3574: Optional: Cross-check quorum_hash corresponds to the signing quorumYou ensure the provided quorum_hash exists, and you validate the recovered sig against the selected signing quorum (via VerifyRecoveredSig). However, the code does not explicitly assert that ban_msg.m_quorum_hash matches the quorum selected for (request_id, m_requested_height). Adding this consistency check would aid diagnosability and prevent misleading logs.
Example (if SelectQuorumForSigning is available in headers):
C++ snippet (illustrative):
"""
const auto llmq_params_opt = Params().GetLLMQ(llmq_type);
assert(llmq_params_opt.has_value());
auto selected = llmq::SelectQuorumForSigning(llmq_params_opt.value(),
m_chainman.ActiveChainstate().m_chain, *m_llmq_ctx->qman, request_id, ban_msg.m_requested_height, /signOffset=/0);
if (!selected || selected->qc->quorumHash != ban_msg.m_quorum_hash) {
LogPrintf("PLATFORMBAN -- hash: %s quorum hash mismatch: provided=%s selected=%s\n",
hash.ToString(), ban_msg.m_quorum_hash.ToString(), selected ? selected->qc->quorumHash.ToString() : "null");
ret.m_error = MisbehavingError{100};
return ret;
}
"""
3537-3546: Add message to MisbehavingError for better diagnosticsYou log details separately, but Misbehaving(...) can carry a message via ret.m_error.message to make the penalty context visible in standard misbehavior logs.
Example:
- ret.m_error = MisbehavingError{1}; + ret.m_error = MisbehavingError{1, "unknown protx_hash for PLATFORMBAN"};
3575-3579: Prefer constexpr string_view for the requestId prefixMinor: using a constexpr string_view avoids a dynamic std::string construction on each call.
- const std::string PLATFORM_BAN_REQUESTID_PREFIX = "PlatformPoSeBan"; + static constexpr std::string_view PLATFORM_BAN_REQUESTID_PREFIX = "PlatformPoSeBan";
3556-3560: Clarify comment about acceptable height rangeThe comment says “inside the range [TipHeight - 576 - 5, TipHeight + 5]” but this branch is the “unexpected height” path. Consider rewording to explicitly describe:
- accepted range: [tip - window, tip]
- tolerated-but-penalized range: (tip, tip + 5] U [tip - window - 5, tip - window)
This reduces cognitive load for future readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/net_processing.cpp(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/net_processing.cpp
🧬 Code Graph Analysis (1)
src/net_processing.cpp (2)
src/chainparams.cpp (2)
Params(1354-1357)Params(1354-1354)src/llmq/quorums.cpp (2)
VerifyRecoveredSig(1271-1285)VerifyRecoveredSig(1271-1273)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (5)
src/net_processing.cpp (5)
40-40: Include for ToUnderlying is appropriateIncluding util/underlying.h is correct for ToUnderlying usage later in logging.
2275-2277: Integrate MSG_PLATFORM_BAN into AlreadyHave inventory pathWiring AlreadyHave to m_mn_metaman.AlreadyHavePlatformBan looks right and keeps inventory flow consistent.
2899-2905: Serving GETDATA for PLATFORMBAN is correctly implementedReturning the serialized PLATFORMBAN object via NetMsgType::PLATFORMBAN mirrors other object handlers.
5361-5361: PLATFORMBAN handler is wired into the message dispatch correctlyInvocation via PostProcessMessage keeps penalties, relay, and erase semantics consistent with other subsystems.
3575-3585: VerifyRecoveredSig defaultsignOffsetcovers omitted argument
TheVerifyRecoveredSigdeclaration insrc/llmq/quorums.hincludes a default value for the trailingsignOffsetparameter, so calling it with seven arguments (omittingsignOffset) is valid and will compile as intended. No changes needed.
no comments, thank you
There's already named constant constexpr int day_of_blocks = 576;Independently, same literal 576 is used to clean "day old quorum data", 576 is used as CDeterministicMNList::DISK_SNAPSHOT_PERIOD for CCreditPool::DISK_SNAPSHOT_PERIOD as a marker to create a snapshot. But all of them are independent and there should not be extra dependencies between various components just because "human-friendly" 1 day constant. I'd prefer to keep as it is
There are relevant logs for each MisbehavingError case:
Wording is unified with DIP-0031 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/net_processing.cpp (2)
727-729: LGTM: handler declaration and attributesThe signature and [[nodiscard]] with lock annotations fit the surrounding style and previous feedback.
3537-3541: Nice: clear in-flight request tracking for PLATFORMBANErasing the in-flight request upon handling the object mirrors CLSIG handling and prevents stale tracking.
🧹 Nitpick comments (3)
src/net_processing.cpp (3)
3555-3567: Centralize the 576-block window as a top-level named constantUsing a file-level constexpr near other protocol constants improves consistency and makes it easier to tweak or parametrize per-network if needed.
@@ - static constexpr int PLATFORM_BAN_WINDOW_BLOCKS = 576; + // Use the global constant defined alongside other relay constants + // static constexpr int PLATFORM_BAN_WINDOW_BLOCKS is removed here @@ - if (tipHeight < ban_msg.m_requested_height || tipHeight - PLATFORM_BAN_WINDOW_BLOCKS > ban_msg.m_requested_height) { + if (tipHeight < ban_msg.m_requested_height || tipHeight - PLATFORM_BAN_WINDOW_BLOCKS > ban_msg.m_requested_height) { @@ - if (tipHeight + 5 < ban_msg.m_requested_height || tipHeight - PLATFORM_BAN_WINDOW_BLOCKS - 5 > ban_msg.m_requested_height) { + if (tipHeight + 5 < ban_msg.m_requested_height || tipHeight - PLATFORM_BAN_WINDOW_BLOCKS - 5 > ban_msg.m_requested_height) {Outside this hunk (add near other top-level constants, around Lines 160-190):
/** Minimum blocks required to signal NODE_NETWORK_LIMITED */ static const unsigned int NODE_NETWORK_LIMITED_MIN_BLOCKS = 288; +/** Number of blocks in a day (24h / 2.5m) for Platform Ban window */ +static constexpr int PLATFORM_BAN_WINDOW_BLOCKS = 576;
3542-3544: Avoid copying the deterministic MN listGetListAtChainTip likely returns a reference; binding to auto copies the entire list. Use const auto& to avoid an expensive copy.
- const auto list = Assert(m_dmnman)->GetListAtChainTip(); + const auto& list = Assert(m_dmnman)->GetListAtChainTip();
3577-3581: Minor: make the request-id prefix a static constexprAvoids re-allocating a std::string per call and signals intent.
- const std::string PLATFORM_BAN_REQUESTID_PREFIX = "PlatformPoSeBan"; + static constexpr char PLATFORM_BAN_REQUESTID_PREFIX[] = "PlatformPoSeBan";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/net_processing.cpp(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/net_processing.cpp
🧬 Code Graph Analysis (1)
src/net_processing.cpp (2)
src/chainparams.cpp (2)
Params(1354-1357)Params(1354-1354)src/llmq/quorums.cpp (2)
VerifyRecoveredSig(1271-1285)VerifyRecoveredSig(1271-1273)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
🔇 Additional comments (5)
src/net_processing.cpp (5)
40-40: LGTM: include for ToUnderlyingAdding util/underlying.h is appropriate for ToUnderlying enum logging below.
2275-2277: Inventory integration for PLATFORM_BAN looks correctAlreadyHave now consults m_mn_metaman.AlreadyHavePlatformBan(inv.hash), aligning the object with the inventory system.
2899-2905: GETDATA serving for PLATFORMBAN is consistentServing PlatformBanMessage via NetMsgType::PLATFORMBAN mirrors existing patterns for other Dash objects.
5363-5364: LGTM: integrated into main message dispatchPLATFORMBAN is processed via PostProcessMessage like other subsystems, preserving central misbehavior handling and relay flows.
3582-3587: Verified defaultsignOffsetparameter inVerifyRecoveredSig
The declaration insrc/llmq/quorums.h(lines 323–325) includes… CBLSSignature& sig, int signOffset = SIGN_HEIGHT_OFFSET );and the definition in
src/llmq/quorums.cpp(line 1271) matches. Calls inchainlock.cpp(line 374) andnet_processing.cpp(line 3582) omit the final argument, correctly using the default. No overload is needed and there are no ABI or build concerns.
It replaces direct call of EraseObjectRequest Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/net_processing.cpp (1)
3555-3567: De-magic 576 and 5; prefer file-level constexprs used consistentlyYou’ve improved readability by naming the 576 window locally, but multiple places may need it and the +/−5 slack should be named too. Define file-level constexprs alongside other constants to avoid duplication and clarify intent.
- Add near other constants (e.g., around NODE_NETWORK_LIMITED_MIN_BLOCKS):
+/** Number of blocks in a day (24 hours / 2.5 minutes) for Platform Ban window */ +static constexpr int PLATFORM_BAN_WINDOW_BLOCKS = 576; +/** Height slack in blocks for PLATFORMBAN height checks */ +static constexpr int PLATFORM_BAN_HEIGHT_SLACK = 5;
- Then simplify this function by using them and removing the local constant:
- static constexpr int PLATFORM_BAN_WINDOW_BLOCKS = 576; int tipHeight = WITH_LOCK(cs_main, return m_chainman.ActiveChainstate().m_chain.Height()); if (tipHeight < ban_msg.m_requested_height || tipHeight - PLATFORM_BAN_WINDOW_BLOCKS > ban_msg.m_requested_height) { // m_requested_height is inside the range [TipHeight - PLATFORM_BAN_WINDOW_BLOCKS - 5, TipHeight + 5] LogPrintf("PLATFORMBAN -- hash: %s protx_hash: %s unexpected height: %d tip: %d\n", hash.ToString(), ban_msg.m_protx_hash.ToString(), ban_msg.m_requested_height, tipHeight); - if (tipHeight + 5 < ban_msg.m_requested_height || tipHeight - PLATFORM_BAN_WINDOW_BLOCKS - 5 > ban_msg.m_requested_height) { + if (tipHeight + PLATFORM_BAN_HEIGHT_SLACK < ban_msg.m_requested_height || + tipHeight - PLATFORM_BAN_WINDOW_BLOCKS - PLATFORM_BAN_HEIGHT_SLACK > ban_msg.m_requested_height) { // m_requested_height is outside the range [TipHeight - PLATFORM_BAN_WINDOW_BLOCKS - 5, TipHeight + 5] ret.m_error = MisbehavingError{10}; return ret; } ret.m_error = MisbehavingError{1}; return ret; }
🧹 Nitpick comments (1)
src/net_processing.cpp (1)
3570-3587: Consider enforcing quorum hash consistency with the quorum used for signature verificationYou ensure ban_msg.m_quorum_hash refers to an existing quorum, but VerifyRecoveredSig selects a quorum internally (based on request_id and height) without consulting the provided quorum_hash. This allows messages to carry an arbitrary quorum_hash. Integrity would be stronger if we assert the quorum actually used for verification matches ban_msg.m_quorum_hash.
If the API allows, select the expected quorum for (llmq_type, request_id, ban_msg.m_requested_height) and compare hashes before accepting:
// Pseudocode – adjust to available public API auto expected = llmq::SelectQuorumForSigning(Params().GetLLMQ(llmq_type).value(), m_chainman.ActiveChainstate().m_chain, *m_llmq_ctx->qman, request_id, ban_msg.m_requested_height, /*signOffset*/ 0); if (!expected || expected->qc->quorumHash != ban_msg.m_quorum_hash) { LogPrintf("PLATFORMBAN -- hash: %s quorum hash mismatch (msg=%s, expected=%s)\n", hash.ToString(), ban_msg.m_quorum_hash.ToString(), expected ? expected->qc->quorumHash.ToString() : "null"); ret.m_error = MisbehavingError{100}; return ret; }If SelectQuorumForSigning is not exposed, consider extending the LLMQ API to retrieve the expected quorum deterministically for this purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/net_processing.cpp(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/net_processing.cpp
🧬 Code Graph Analysis (1)
src/net_processing.cpp (2)
src/chainparams.cpp (2)
Params(1354-1357)Params(1354-1354)src/llmq/quorums.cpp (2)
VerifyRecoveredSig(1271-1285)VerifyRecoveredSig(1271-1273)
🔇 Additional comments (5)
src/net_processing.cpp (5)
40-40: Include for ToUnderlying is appropriateThe new include is required for ToUnderlying() usage in logs. No issues.
727-729: Correct signature and attributes for ProcessPlatformBanMessageGood job adding [[nodiscard]] and the locking annotations. The NodeId-based signature fits the existing PostProcessMessage flow.
2275-2277: AlreadyHave(MSG_PLATFORM_BAN) correctly delegates to meta managerThis integrates cleanly with the INV/request path.
2899-2905: GETDATA handler for MSG_PLATFORM_BAN is correctly wiredResponds with NetMsgType::PLATFORMBAN when available; matches the pattern used elsewhere.
5364-5364: Good integration into the extension dispatch pathRouting PLATFORMBAN through PostProcessMessage keeps inventory/GETDATA book-keeping consistent. Note: this relies on ProcessPlatformBanMessage returning ret (not {}), see earlier comment.
Would you like me to scan the repo for any other success paths returning {} in message handlers to ensure we’re not missing similar m_to_erase losses?
Now for RelayInv too Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
src/masternode/meta.h
Outdated
| if (height <= m_platform_ban_updated) { | ||
| return false; | ||
| } | ||
| if (height == m_platform_ban_updated && !is_banned) { |
There was a problem hiding this comment.
This condition is unreachable now because we'll bail out in if (height <= m_platform_ban_updated) { above (it was changed from if (height < m_platform_ban_height) { in c739f3a#diff-4b932ee0759e8f52676ccdd2f94fb3a39ee0652695edfa3822ecb3dcb7ef9f30R107)
There was a problem hiding this comment.
This is still confusing to me tbh... So this condition is basically "evonode CAN be banned at the height it was unbanned but it CAN NOT be unbanned at the height it was banned", right? We need a test for this edge case I guess.
EDIT:
In other words, if we process a block with protxupserv and then we receive platformban for the same height - we mark the node as bad. But if we receive platformban from the future (and mark the node as bad) first and we process a block with protxupserv for the same height later - we keep the node as bad.
We do need to update dip-0031 accordingly imo.
There was a problem hiding this comment.
I am not sure why exactly I implemented this one like that, I had plan to update DIP-0031 accordingly implementation, but now I can't remember any more what is exactly not the same as DIP-0031 so far as half year passed since I implemented it.
I definitely want to prevent flipping ban/unban status. I added one more commit which change logic a bit. Could you have a look to it?
| p2p_node2.send_message(msg) | ||
| wait_until_helper(lambda: p2p_node2.message_count["platformban"] > 0, timeout=10, lock=p2p_lock) | ||
| p2p_node2.message_count["platformban"] = 0 | ||
|
|
|
|
||
| self.dynamically_evo_update_service(evo_info_0) | ||
| assert not self.check_banned(self.mninfo[0]) | ||
|
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/net_processing.cpp (2)
727-729: Add a brief contract comment and document misbehavior codes for ProcessPlatformBanMessageThe signature and lock annotations look good. Please add a short Doxygen-style comment enumerating:
- When it returns an empty result (ignored paths).
- What each MisbehavingError score means (1, 10, 100).
- That ret.m_to_erase is always set on successful deserialization to clear in-flight GETDATA.
This will save future spelunking.
3569-3588: Ensure Platform PoSeBan signature binds to the deterministically-selected quorumThe current code in
src/net_processing.cpp(lines 3569–3588) looks up whatever quorum hash is provided in the ban message and then callsVerifyInsecureon its public key. This allows any quorum (even one not deterministically tied to the ban’s(request_id, height)) to vouch for it. To harden this:
- Use the existing
llmq::VerifyRecoveredSighelper, which internally selects the correct quorum for(llmqType, request_id, height)and verifies the signature against that quorum’s key.- Optionally, assert that the caller-provided
m_quorum_hashmatches the hash of the quorum thatVerifyRecoveredSig(orSelectQuorumForSigning) picks.- This change both restores the stronger binding pattern used elsewhere (e.g. in RPC/quorums and ChainLocks) and prevents an attacker from substituting a different valid quorum.
Required changes in
src/net_processing.cpp:• Remove the direct
GetQuorum(llmq_type, ban_msg.m_quorum_hash)+VerifyInsecureblock.
• Replace with a call tollmq::VerifyRecoveredSigand handle itsVerifyRecSigStatus.
• (Optional) Usellmq::SelectQuorumForSigningto fetch the expected quorum for(request_id, height)and compare itsqc->quorumHashtoban_msg.m_quorum_hash.Proposed diff:
- Consensus::LLMQType llmq_type = Params().GetConsensus().llmqTypePlatform; - auto quorum = m_llmq_ctx->qman->GetQuorum(llmq_type, ban_msg.m_quorum_hash); - if (!quorum) { - LogPrintf("PLATFORMBAN -- hash: %s protx_hash: %s missing quorum_hash: %s llmq_type: %d\n", - hash.ToString(), ban_msg.m_protx_hash.ToString(), - ban_msg.m_quorum_hash.ToString(), ToUnderlying(llmq_type)); - ret.m_error = MisbehavingError{100}; - return ret; - } - - const std::string PLATFORM_BAN_REQUESTID_PREFIX = "PlatformPoSeBan"; - const auto data = std::make_pair(ban_msg.m_protx_hash, ban_msg.m_requested_height); - const uint256 request_id = ::SerializeHash(std::make_pair(PLATFORM_BAN_REQUESTID_PREFIX, data)); - const uint256 msg_hash = ::SerializeHash(data); - - uint256 signHash = llmq::BuildSignHash(llmq_type, quorum->qc->quorumHash, request_id, msg_hash); - - if (!ban_msg.m_signature.VerifyInsecure(quorum->qc->quorumPublicKey, signHash)) { - LogPrintf("PLATFORMBAN -- hash: %s protx_hash: %s request_id: %s msg_hash: %s sig validation failed\n", - hash.ToString(), ban_msg.m_protx_hash.ToString(), - request_id.ToString(), msg_hash.ToString()); - ret.m_error = MisbehavingError{100}; - return ret; - } + const Consensus::LLMQType llmq_type = Params().GetConsensus().llmqTypePlatform; + const std::string PLATFORM_BAN_REQUESTID_PREFIX = "PlatformPoSeBan"; + const auto data = std::make_pair(ban_msg.m_protx_hash, ban_msg.m_requested_height); + const uint256 request_id = ::SerializeHash(std::make_pair(PLATFORM_BAN_REQUESTID_PREFIX, data)); + const uint256 msg_hash = ::SerializeHash(data); + + // VerifyRecoveredSig selects the correct quorum for (llmq_type, request_id, height) + const auto recsig_status = llmq::VerifyRecoveredSig( + llmq_type, + m_chainman.ActiveChainstate().m_chain, + *m_llmq_ctx->qman, + ban_msg.m_requested_height, + request_id, + msg_hash, + ban_msg.m_signature); + if (recsig_status != llmq::VerifyRecSigStatus::Valid) { + LogPrintf("PLATFORMBAN -- hash: %s protx_hash: %s request_id: %s msg_hash: %s recsig validation failed: %d\n", + hash.ToString(), ban_msg.m_protx_hash.ToString(), + request_id.ToString(), msg_hash.ToString(), + ToUnderlying(recsig_status)); + ret.m_error = MisbehavingError{100}; + return ret; + } + + // Optional: enforce caller-provided quorum_hash matches the one selected + // auto expected = llmq::SelectQuorumForSigning( + // Params().GetConsensus().llmqParamsByType.at(llmq_type), + // m_chainman.ActiveChainstate().m_chain, + // *m_llmq_ctx->qman, + // request_id, + // ban_msg.m_requested_height); + // if (!expected || expected->qc->quorumHash != ban_msg.m_quorum_hash) { + // LogPrintf("PLATFORMBAN -- hash: %s quorum_hash mismatch: provided=%s expected=%s\n", + // hash.ToString(), + // ban_msg.m_quorum_hash.ToString(), + // expected ? expected->qc->quorumHash.ToString() : "<none>"); + // ret.m_error = MisbehavingError{100}; + // return ret; + // }Please apply these changes to ensure PoSeBan signatures are cryptographically bound to the correct quorum.
🧹 Nitpick comments (4)
src/net_processing.cpp (4)
3539-3547: Short-circuit duplicates before expensive checksAfter computing hash and setting ret.m_to_erase, bail out early if we already have this PLATFORMBAN. This avoids unnecessary DMN lookups, height window checks, and BLS verification on duplicates.
Apply this diff:
MessageProcessingResult ret{}; ret.m_to_erase = CInv{MSG_PLATFORM_BAN, hash}; + // Fast-path: drop duplicates after clearing in-flight tracking + if (m_mn_metaman.AlreadyHavePlatformBan(hash)) { + return ret; + }
3555-3567: Height-window tolerance: fix comment placement/wording and avoid magic number 5
- The inline comment “inside the range …” is placed just before we evaluate the “outside” condition, which is misleading.
- Replace the literal 5 with a named constant for clarity and future-proofing.
- Prefer a single file-level PLATFORM_BAN_WINDOW_BLOCKS instead of a function-local copy to keep semantics consistent across the codebase.
Apply this diff to correct the misleading comment and use a tolerance constant:
- static constexpr int PLATFORM_BAN_WINDOW_BLOCKS = 576; + // Use the global PLATFORM_BAN_WINDOW_BLOCKS for the window and PLATFORM_BAN_TOLERANCE_BLOCKS for edge tolerance. int tipHeight = WITH_LOCK(cs_main, return m_chainman.ActiveChainstate().m_chain.Height()); - if (tipHeight < ban_msg.m_requested_height || tipHeight - PLATFORM_BAN_WINDOW_BLOCKS > ban_msg.m_requested_height) { - // m_requested_height is inside the range [TipHeight - PLATFORM_BAN_WINDOW_BLOCKS - 5, TipHeight + 5] + if (tipHeight < ban_msg.m_requested_height || tipHeight - PLATFORM_BAN_WINDOW_BLOCKS > ban_msg.m_requested_height) { LogPrintf("PLATFORMBAN -- hash: %s protx_hash: %s unexpected height: %d tip: %d\n", hash.ToString(), ban_msg.m_protx_hash.ToString(), ban_msg.m_requested_height, tipHeight); - if (tipHeight + 5 < ban_msg.m_requested_height || tipHeight - PLATFORM_BAN_WINDOW_BLOCKS - 5 > ban_msg.m_requested_height) { - // m_requested_height is outside the range [TipHeight - PLATFORM_BAN_WINDOW_BLOCKS - 5, TipHeight + 5] + if (tipHeight + PLATFORM_BAN_TOLERANCE_BLOCKS < ban_msg.m_requested_height || tipHeight - PLATFORM_BAN_WINDOW_BLOCKS - PLATFORM_BAN_TOLERANCE_BLOCKS > ban_msg.m_requested_height) { + // m_requested_height is outside the range [TipHeight - PLATFORM_BAN_WINDOW_BLOCKS - PLATFORM_BAN_TOLERANCE_BLOCKS, TipHeight + PLATFORM_BAN_TOLERANCE_BLOCKS] ret.m_error = MisbehavingError{10}; return ret; } ret.m_error = MisbehavingError{1}; return ret; }Add these file-level constants alongside other static constexprs near the top (around other P2P constants):
// Number of blocks in a day (24h / 2.5 min) for Platform Ban height window static constexpr int PLATFORM_BAN_WINDOW_BLOCKS = 576; // Tolerance to allow slight edge drift on requested height checks static constexpr int PLATFORM_BAN_TOLERANCE_BLOCKS = 5;If you keep tolerance local, at least rename it and constexpr it once to avoid repeated literals.
3577-3581: Avoid allocating std::string for a static prefixUse a compile-time constant to prevent a heap allocation on every call.
Apply this diff:
- const std::string PLATFORM_BAN_REQUESTID_PREFIX = "PlatformPoSeBan"; + static constexpr char PLATFORM_BAN_REQUESTID_PREFIX[] = "PlatformPoSeBan";
3523-3599: Tests: add negative/boundary coverage for PLATFORMBANThe handler is non-trivial and uses 3 different misbehavior scores and a height window with tolerance. Please add functional/unit tests for:
- Height boundary edges: exact tip−PLATFORM_BAN_WINDOW_BLOCKS, tip−PLATFORM_BAN_WINDOW_BLOCKS−1, tip+0, tip+1, and at ±PLATFORM_BAN_TOLERANCE_BLOCKS.
- Non-existent proTxHash (expects score=1).
- Non-Evo type (score=100).
- Missing quorum hash (score=100).
- Quorum/signature mismatch cases (score=100), including wrong quorum_hash for the selected quorum.
- Duplicate messages (ensure short-circuit path returns with m_to_erase set and no relay).
I can draft these if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/masternode/meta.h(7 hunks)src/net_processing.cpp(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/masternode/meta.h
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/net_processing.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (5)
src/net_processing.cpp (5)
40-40: Include <util/underlying.h> is appropriateNeeded for ToUnderlying(llmq_type) cast in logging. No issues.
2276-2277: Inv-filtering hook for MSG_PLATFORM_BAN looks correctDelegation to m_mn_metaman.AlreadyHavePlatformBan(inv.hash) aligns with the inventory gating strategy used elsewhere.
2900-2905: GETDATA handler for PLATFORMBAN integrates cleanlyServing NetMsgType::PLATFORMBAN if present is consistent with other object handlers. No concerns.
3590-3596: Relay only when state changes (good) and log onceOnly relaying when SetPlatformBan returns true is the right behavior. The logging and RememberPlatformBan call are in the correct order, and ret.m_inventory is populated for PostProcessMessage. LGTM.
5363-5364: Correctly integrated into ProcessMessage dispatchReturning the non-empty result ensures PostProcessMessage clears in-flight GETDATA and relays inv as needed. No issues.
| return false; | ||
| } | ||
| if (height == m_platform_ban_updated && !is_banned) { | ||
| if (height < m_platform_ban_updated) { |
There was a problem hiding this comment.
no flipping ban/unban if it's the same height
but without = it will be flipping for the same height...
Issue being fixed or feature implemented
It implements DIP-0031
What was done?
Implemented p2p message in inventory system and functional tests for it.
How Has This Been Tested?
See
test/functional/p2p_platform_ban.pyBreaking Changes
It bumps protocol version
MIN_MASTERNODE_PROTO_VERSIONfor MN to 70236.Checklist: