feat: implement Platform Ban PoSe DIP-0031 #6613
Conversation
WalkthroughThis pull request adds support for platform bans within the masternode system as specified by DIP-0031. It extends the deterministic masternode manager to reference the masternode metadata manager, enabling platform ban state updates during block processing. The masternode metadata classes gain new members and methods to track platform ban status and height, and a new Estimated code review effort4 (~90 minutes) ✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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 |
| 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:
|
|
This pull request has conflicts, please rebase. |
it's already not magic number, but a constant day_of_blocks 🫣
2 other places do not have doc yet, but both of them have associated log record: which clarify "100" clearly.
there's no race condition because mutex (cs) |
Could you suggest value? DIP doesn't mention policy to forget old platform ban. |
I force-pushed to resolve conflict with 6730. Please notice also, that logic of |
|
@coderabbitai, how well tested is this PR? Are there tests that could / or should be written for the features introduced here? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@PastaPastaPasta, let me analyze the test coverage for this Platform Ban PoSe implementation. 🏁 Script executed: Length of output: 11270 🏁 Script executed: Length of output: 9232 Based on my analysis of the codebase, this PR has moderate test coverage with significant gaps that should be addressed. Here's my assessment: Current Test Coverage ✅The PR includes a comprehensive functional test (
Critical Testing Gaps ❌However, there are significant gaps in test coverage for the complex validation logic: 1. Error Path TestingThe // These error cases need individual test coverage:
return tl::unexpected{1}; // Recently removed node
return tl::unexpected{1}; // Height slightly out of range
return tl::unexpected{10}; // Height significantly out of range
return tl::unexpected{100}; // Wrong node type (not Evo)
return tl::unexpected{100}; // Missing quorum
return tl::unexpected{100}; // Invalid signature2. Edge Case TestingMissing tests for:
3. Unit TestsNo unit tests exist for:
4. Memory ManagementThe Recommended Additional TestsI recommend adding:
The current test provides good coverage for the intended functionality, but the complex validation logic would benefit from more comprehensive error case testing to ensure robustness against malicious or malformed inputs. 🐰 🧠 Learnings used |
that's true, there're no tests for p2p punishments for other nodes |
|
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 (15)
src/evo/deterministicmns.cpp(2 hunks)src/evo/deterministicmns.h(2 hunks)src/evo/specialtxman.cpp(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)
📓 Path-based instructions (1)
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/test_framework/p2p.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/deterministicmns.cpp:1284-1287
Timestamp: 2025-06-16T17:59:55.669Z
Learning: In Dash masternode ProRegTx validation, platform ports (platformHTTPPort and platformP2PPort) are mandatory and must be non-zero, while netInfo (ipAndPort) is optional. This means that even if an empty netInfo returns 0 from GetPrimary().GetPort(), it won't cause false positives in port duplication checks since platform ports cannot be 0.
test/functional/test_framework/p2p.py (1)
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
🪛 Flake8 (7.2.0)
test/functional/test_framework/p2p.py
[error] 594-594: multiple statements on one line (def)
(E704)
[error] 595-595: multiple statements on one line (def)
(E704)
[error] 596-596: multiple statements on one line (def)
(E704)
[error] 597-597: multiple statements on one line (def)
(E704)
[error] 598-598: multiple statements on one line (def)
(E704)
✅ 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 (13)
- src/evo/deterministicmns.h
- src/masternode/meta.cpp
- test/functional/p2p_platform_ban.py
- src/net_processing.cpp
- test/functional/test_framework/messages.py
- src/evo/specialtxman.cpp
- src/protocol.h
- src/evo/deterministicmns.cpp
- src/node/chainstate.cpp
- src/llmq/dkgsession.cpp
- src/version.h
- src/masternode/meta.h
- src/protocol.cpp
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/test_framework/p2p.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/deterministicmns.cpp:1284-1287
Timestamp: 2025-06-16T17:59:55.669Z
Learning: In Dash masternode ProRegTx validation, platform ports (platformHTTPPort and platformP2PPort) are mandatory and must be non-zero, while netInfo (ipAndPort) is optional. This means that even if an empty netInfo returns 0 from GetPrimary().GetPort(), it won't cause false positives in port duplication checks since platform ports cannot be 0.
test/functional/test_framework/p2p.py (1)
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
🪛 Flake8 (7.2.0)
test/functional/test_framework/p2p.py
[error] 594-594: multiple statements on one line (def)
(E704)
[error] 595-595: multiple statements on one line (def)
(E704)
[error] 596-596: multiple statements on one line (def)
(E704)
[error] 597-597: multiple statements on one line (def)
(E704)
[error] 598-598: multiple statements on one line (def)
(E704)
🔇 Additional comments (3)
test/functional/test_framework/p2p.py (3)
68-68: LGTM: Platform ban message import added correctly.The
msg_platformbanimport aligns with the DIP-0031 implementation and follows the existing import pattern in the messages module.
103-104: LGTM: Protocol version properly updated for platform ban support.The version bump to 70238 with appropriate comment documenting the DIP-0031 feature is consistent with the protocol changes across the codebase.
158-158: LGTM: Platform ban message properly registered in MESSAGEMAP.The addition of
b"platformban": msg_platformbanfollows the existing pattern and enables proper message deserialization in the test framework.
| def on_islock(self, message): pass | ||
| def on_mnlistdiff(self, message): pass | ||
| def on_platformban(self, message): pass | ||
| def on_qdata(self, message): pass | ||
| def on_qgetdata(self, message): pass |
There was a problem hiding this comment.
Fix formatting violations by separating method definitions from pass statements.
The static analysis correctly identifies formatting violations. Each callback method should have the pass statement on a separate line per PEP 8 guidelines.
Apply this diff to fix the formatting:
- def on_islock(self, message): pass
- def on_mnlistdiff(self, message): pass
- def on_platformban(self, message): pass
+ def on_islock(self, message):
+ pass
+ def on_mnlistdiff(self, message):
+ pass
+ def on_platformban(self, message):
+ pass
def on_qdata(self, message): pass
- def on_qgetdata(self, message): pass
+ def on_qgetdata(self, message):
+ passCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 594-594: multiple statements on one line (def)
(E704)
[error] 595-595: multiple statements on one line (def)
(E704)
[error] 596-596: multiple statements on one line (def)
(E704)
[error] 597-597: multiple statements on one line (def)
(E704)
[error] 598-598: multiple statements on one line (def)
(E704)
🤖 Prompt for AI Agents
In test/functional/test_framework/p2p.py around lines 594 to 598, the method
definitions currently have the pass statement on the same line, violating PEP 8
formatting rules. To fix this, place the pass statement on its own line inside
each method, ensuring proper indentation and separation from the method
signature.
|
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: