-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: implement Platform Ban PoSe DIP-0031 #6613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 14 commits
521a8e1
775114e
fbf89d6
2cd44f0
361991f
2dd8026
f316bf9
9b6597e
00e047d
24797be
7d39124
c739f3a
e73e620
497a04c
853e9c8
6e39350
71825d1
e0d6327
e8b01f4
4221644
c687022
c191c53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,14 +5,18 @@ | |||||||||||||||||
#ifndef BITCOIN_MASTERNODE_META_H | ||||||||||||||||||
#define BITCOIN_MASTERNODE_META_H | ||||||||||||||||||
|
||||||||||||||||||
#include <bls/bls.h> | ||||||||||||||||||
#include <saltedhasher.h> | ||||||||||||||||||
#include <serialize.h> | ||||||||||||||||||
#include <sync.h> | ||||||||||||||||||
#include <threadsafety.h> | ||||||||||||||||||
#include <uint256.h> | ||||||||||||||||||
#include <unordered_lru_cache.h> | ||||||||||||||||||
|
||||||||||||||||||
#include <atomic> | ||||||||||||||||||
#include <map> | ||||||||||||||||||
#include <memory> | ||||||||||||||||||
#include <optional> | ||||||||||||||||||
#include <vector> | ||||||||||||||||||
|
||||||||||||||||||
class CConnman; | ||||||||||||||||||
|
@@ -46,6 +50,11 @@ class CMasternodeMetaInfo | |||||||||||||||||
std::atomic<int64_t> lastOutboundAttempt{0}; | ||||||||||||||||||
std::atomic<int64_t> lastOutboundSuccess{0}; | ||||||||||||||||||
|
||||||||||||||||||
//! bool flag is node currently under platform ban by p2p message | ||||||||||||||||||
bool m_platform_ban GUARDED_BY(cs){false}; | ||||||||||||||||||
//! height at which platform ban has been applied or removed | ||||||||||||||||||
int m_platform_ban_updated GUARDED_BY(cs){0}; | ||||||||||||||||||
|
||||||||||||||||||
public: | ||||||||||||||||||
CMasternodeMetaInfo() = default; | ||||||||||||||||||
explicit CMasternodeMetaInfo(const uint256& _proTxHash) : proTxHash(_proTxHash) {} | ||||||||||||||||||
|
@@ -55,22 +64,18 @@ class CMasternodeMetaInfo | |||||||||||||||||
nMixingTxCount(ref.nMixingTxCount.load()), | ||||||||||||||||||
mapGovernanceObjectsVotedOn(ref.mapGovernanceObjectsVotedOn), | ||||||||||||||||||
lastOutboundAttempt(ref.lastOutboundAttempt.load()), | ||||||||||||||||||
lastOutboundSuccess(ref.lastOutboundSuccess.load()) | ||||||||||||||||||
lastOutboundSuccess(ref.lastOutboundSuccess.load()), | ||||||||||||||||||
m_platform_ban(ref.m_platform_ban), | ||||||||||||||||||
m_platform_ban_updated(ref.m_platform_ban_updated) | ||||||||||||||||||
{ | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
SERIALIZE_METHODS(CMasternodeMetaInfo, obj) | ||||||||||||||||||
{ | ||||||||||||||||||
LOCK(obj.cs); | ||||||||||||||||||
READWRITE( | ||||||||||||||||||
obj.proTxHash, | ||||||||||||||||||
obj.nLastDsq, | ||||||||||||||||||
obj.nMixingTxCount, | ||||||||||||||||||
obj.mapGovernanceObjectsVotedOn, | ||||||||||||||||||
obj.outboundAttemptCount, | ||||||||||||||||||
obj.lastOutboundAttempt, | ||||||||||||||||||
obj.lastOutboundSuccess | ||||||||||||||||||
); | ||||||||||||||||||
READWRITE(obj.proTxHash, obj.nLastDsq, obj.nMixingTxCount, obj.mapGovernanceObjectsVotedOn, | ||||||||||||||||||
obj.outboundAttemptCount, obj.lastOutboundAttempt, obj.lastOutboundSuccess, obj.m_platform_ban, | ||||||||||||||||||
obj.m_platform_ban_updated); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
UniValue ToJson() const; | ||||||||||||||||||
|
@@ -96,6 +101,24 @@ class CMasternodeMetaInfo | |||||||||||||||||
int64_t GetLastOutboundAttempt() const { return lastOutboundAttempt; } | ||||||||||||||||||
void SetLastOutboundSuccess(int64_t t) { lastOutboundSuccess = t; outboundAttemptCount = 0; } | ||||||||||||||||||
int64_t GetLastOutboundSuccess() const { return lastOutboundSuccess; } | ||||||||||||||||||
bool SetPlatformBan(bool is_banned, int height) | ||||||||||||||||||
{ | ||||||||||||||||||
LOCK(cs); | ||||||||||||||||||
if (height <= m_platform_ban_updated) { | ||||||||||||||||||
return false; | ||||||||||||||||||
} | ||||||||||||||||||
if (height == m_platform_ban_updated && !is_banned) { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This condition is unreachable now because we'll bail out in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: We do need to update dip-0031 accordingly imo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||||||||||||||||||
return false; | ||||||||||||||||||
} | ||||||||||||||||||
m_platform_ban = is_banned; | ||||||||||||||||||
m_platform_ban_updated = height; | ||||||||||||||||||
return true; | ||||||||||||||||||
} | ||||||||||||||||||
bool IsPlatformBanned() const | ||||||||||||||||||
{ | ||||||||||||||||||
LOCK(cs); | ||||||||||||||||||
return m_platform_ban; | ||||||||||||||||||
} | ||||||||||||||||||
}; | ||||||||||||||||||
using CMasternodeMetaInfoPtr = std::shared_ptr<CMasternodeMetaInfo>; | ||||||||||||||||||
|
||||||||||||||||||
|
@@ -150,6 +173,41 @@ class MasternodeMetaStore | |||||||||||||||||
std::string ToString() const; | ||||||||||||||||||
}; | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* Platform PoSe Ban are result in the node voting against the targeted evonode in all future DKG sessions until that targeted | ||||||||||||||||||
*evonode has been successfully banned. Platform will initiate this ban process by passing relevant information to Core using RPC. See DIP-0031 | ||||||||||||||||||
* | ||||||||||||||||||
* We use 2 main classes to manage Platform PoSe Ban | ||||||||||||||||||
* | ||||||||||||||||||
* PlatformBanMessage | ||||||||||||||||||
* CMasternodeMetaInfo - a higher-level construct which store extra (non-deterministic) information about masternodes including platform ban status | ||||||||||||||||||
**/ | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* PlatformBanMessage - low-level constructs which present p2p message PlatformBan containing the protx hash, requested | ||||||||||||||||||
* height to ban, and BLS data: quorum hash and bls signature | ||||||||||||||||||
*/ | ||||||||||||||||||
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)); | ||||||||||||||||||
UdjinM6 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
uint256 GetHash() const; | ||||||||||||||||||
}; | ||||||||||||||||||
Comment on lines
+187
to
+206
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainVerify 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 cpp Length of output: 183 Action: Add Signature Verification Logic for PlatformBanMessage It appears that the current implementation in
|
||||||||||||||||||
|
||||||||||||||||||
class CMasternodeMetaMan : public MasternodeMetaStore | ||||||||||||||||||
{ | ||||||||||||||||||
private: | ||||||||||||||||||
|
@@ -161,6 +219,12 @@ class CMasternodeMetaMan : public MasternodeMetaStore | |||||||||||||||||
|
||||||||||||||||||
std::vector<uint256> vecDirtyGovernanceObjectHashes GUARDED_BY(cs); | ||||||||||||||||||
|
||||||||||||||||||
// equal to double of expected amount of all evo nodes, see DIP-0028 | ||||||||||||||||||
// it consumes no more than 1Mb of RAM but will cover extreme cases | ||||||||||||||||||
static constexpr size_t SeenBanInventorySize = 900; | ||||||||||||||||||
mutable unordered_lru_cache<uint256, PlatformBanMessage, StaticSaltedHasher> m_seen_platform_bans GUARDED_BY(cs){ | ||||||||||||||||||
SeenBanInventorySize}; | ||||||||||||||||||
|
||||||||||||||||||
public: | ||||||||||||||||||
explicit CMasternodeMetaMan(); | ||||||||||||||||||
~CMasternodeMetaMan(); | ||||||||||||||||||
|
@@ -181,6 +245,10 @@ class CMasternodeMetaMan : public MasternodeMetaStore | |||||||||||||||||
void RemoveGovernanceObject(const uint256& nGovernanceObjectHash); | ||||||||||||||||||
|
||||||||||||||||||
std::vector<uint256> GetAndClearDirtyGovernanceObjectHashes(); | ||||||||||||||||||
|
||||||||||||||||||
bool AlreadyHavePlatformBan(const uint256& inv_hash) const; | ||||||||||||||||||
std::optional<PlatformBanMessage> GetPlatformBan(const uint256& inv_hash) const; | ||||||||||||||||||
void RememberPlatformBan(const uint256& inv_hash, PlatformBanMessage&& msg); | ||||||||||||||||||
}; | ||||||||||||||||||
Comment on lines
+246
to
249
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Const-correctness and API tweak for RememberPlatformBan Take the message by const reference (no need for rvalue) and document that it refreshes existing entries. Apply: - void RememberPlatformBan(const uint256& inv_hash, PlatformBanMessage&& msg);
+ void RememberPlatformBan(const uint256& inv_hash, const PlatformBanMessage& msg); 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||
|
||||||||||||||||||
#endif // BITCOIN_MASTERNODE_META_H |
Uh oh!
There was an error while loading. Please reload this page.