From 6a5df01e908ad3464ecb0f0c14758f3f51e8e13a Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 29 Jun 2021 22:05:56 -0300 Subject: [PATCH 01/13] p2p: Remove BIP61 reject messages Backport of btc@fa25f43ac5692082dba3f90456c501eb08f1b75c and btc@b1b0cfecb639ce44be280c7a45a41a19e893c401 --- doc/release-notes.md | 29 +++++++ src/net.cpp | 1 - src/net_processing.cpp | 88 ++++++---------------- src/protocol.cpp | 2 - src/protocol.h | 7 -- src/validation.h | 2 - test/functional/p2p_leak.py | 5 +- test/functional/test_framework/messages.py | 32 -------- test/functional/test_framework/mininode.py | 3 - 9 files changed, 52 insertions(+), 117 deletions(-) diff --git a/doc/release-notes.md b/doc/release-notes.md index 283a76a5aa38..0e7433fcda3b 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -493,6 +493,35 @@ OpenSSL is no longer used by PIVX Core P2P and network changes ----------------------- +#### Removal of reject network messages from PIVX Core (BIP61) + +The command line option to enable BIP61 (`-enablebip61`) has been removed. + +Nodes on the network can not generally be trusted to send valid ("reject") +messages, so this should only ever be used when connected to a trusted node. +Please use the recommended alternatives if you rely on this feature: + +* Testing or debugging of implementations of the PIVX P2P network protocol + should be done by inspecting the log messages that are produced by a recent + version of PIVX Core. Bitcoin Core logs debug messages + (`-debug=`) to a stream (`-printtoconsole`) or to a file + (`-debuglogfile=`). + +* Testing the validity of a block can be achieved by specific RPCs: + - `submitblock` + +* Testing the validity of a transaction can be achieved by specific RPCs: + - `sendrawtransaction` + +* Wallets should not use the absence of "reject" messages to indicate a + transaction has propagated the network, nor should wallets use "reject" + messages to set transaction fees. Wallets should rather use fee estimation + to determine transaction fees and set replace-by-fee if desired. Thus, they + could wait until the transaction has confirmed (taking into account the fee + target they set (compare the RPC `estimatesmartfee`)) or listen for the + transaction announcement by other network peers to check for propagation. + +#### NAT-PMP Support - Added NAT-PMP port mapping support via [`libnatpmp`](https://miniupnp.tuxfamily.org/libnatpmp.html) diff --git a/src/net.cpp b/src/net.cpp index f244fd9e336a..13f537d6d538 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -422,7 +422,6 @@ bool CNode::DisconnectOldProtocol(int nVersionIn, int nVersionRequired, std::str fDisconnect = false; if (nVersionIn < nVersionRequired) { LogPrintf("%s : peer=%d using obsolete version %i; disconnecting\n", __func__, id, nVersionIn); - g_connman->PushMessage(this, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strLastCommand, REJECT_OBSOLETE, strprintf("Version must be %d or greater", ActiveProtocol()))); fDisconnect = true; } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0da0214e2769..4364d22b60b1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -101,12 +101,6 @@ int nPreferredDownload = 0; namespace { -struct CBlockReject { - unsigned char chRejectCode; - std::string strRejectReason; - uint256 hashBlock; -}; - class CNodeBlocks { @@ -201,8 +195,6 @@ struct CNodeState { bool fShouldBan; //! String name of this peer (debugging/logging purposes). const std::string name; - //! List of asynchronously-determined block rejections to notify this peer about. - std::vector rejects; //! The best known block we know this peer has announced. const CBlockIndex* pindexBestKnownBlock; //! The hash of the last unknown block this peer has announced. @@ -686,8 +678,6 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta if (state.IsInvalid(nDoS)) { if (it != mapBlockSource.end() && State(it->second)) { assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes - CBlockReject reject = {(unsigned char) state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), hash}; - State(it->second)->rejects.push_back(reject); if (nDoS > 0) { Misbehaving(it->second, nDoS); } @@ -1092,7 +1082,6 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR if (strCommand == NetMsgType::VERSION) { // Each connection can only send one version message if (pfrom->nVersion != 0) { - connman->PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strCommand, REJECT_DUPLICATE, std::string("Duplicate version message"))); LOCK(cs_main); Misbehaving(pfrom->GetId(), 1); return false; @@ -1117,8 +1106,6 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR } if (pfrom->nServicesExpected & ~nServices) { LogPrint(BCLog::NET, "peer=%d does not offer the expected services (%08x offered, %08x expected); disconnecting\n", pfrom->id, nServices, pfrom->nServicesExpected); - connman->PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strCommand, REJECT_NONSTANDARD, - strprintf("Expected to offer services %08x", pfrom->nServicesExpected))); pfrom->fDisconnect = true; return false; } @@ -1619,11 +1606,9 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR LogPrint(BCLog::MEMPOOLREJ, "%s from peer=%d %s was not accepted into the memory pool: %s\n", tx.GetHash().ToString(), pfrom->id, pfrom->cleanSubVer, FormatStateMessage(state)); - if (state.GetRejectCode() < REJECT_INTERNAL) // Never send AcceptToMemoryPool's internal codes over P2P - connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::REJECT, strCommand, state.GetRejectCode(), - state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), inv.hash)); - if (nDoS > 0) + if (nDoS > 0) { Misbehaving(pfrom->GetId(), nDoS); + } } } @@ -1721,8 +1706,6 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR int nDoS; if(state.IsInvalid(nDoS)) { assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes - connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::REJECT, strCommand, state.GetRejectCode(), - state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), inv.hash)); if(nDoS > 0) { TRY_LOCK(cs_main, lockMain); if(lockMain) Misbehaving(pfrom->GetId(), nDoS); @@ -1899,51 +1882,29 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR pfrom->fRelayTxes = true; } - - else if (strCommand == NetMsgType::REJECT) { - try { - std::string strMsg; - unsigned char ccode; - std::string strReason; - vRecv >> LIMITED_STRING(strMsg, CMessageHeader::COMMAND_SIZE) >> ccode >> LIMITED_STRING(strReason, MAX_REJECT_MESSAGE_LENGTH); - - std::ostringstream ss; - ss << strMsg << " code " << itostr(ccode) << ": " << strReason; - - if (strMsg == NetMsgType::BLOCK || strMsg == NetMsgType::TX) { - uint256 hash; - vRecv >> hash; - ss << ": hash " << hash.ToString(); - } - LogPrint(BCLog::NET, "Reject %s\n", SanitizeString(ss.str())); - } catch (const std::ios_base::failure& e) { - // Avoid feedback loops by preventing reject messages from triggering a new reject message. - LogPrint(BCLog::NET, "Unparseable reject message received\n"); - } - } else { - bool found = false; - const std::vector& allMessages = getAllNetMessageTypes(); - for (const std::string& msg : allMessages) { - if (msg == strCommand) { - found = true; - break; - } + // Tier two msg type search + bool found = false; + const std::vector& allMessages = getAllNetMessageTypes(); + for (const std::string& msg : allMessages) { + if (msg == strCommand) { + found = true; + break; } + } - if (found) { - // Check if the dispatcher can process this message first. If not, try going with the old flow. - if (!masternodeSync.MessageDispatcher(pfrom, strCommand, vRecv)) { - //probably one the extensions - mnodeman.ProcessMessage(pfrom, strCommand, vRecv); - g_budgetman.ProcessMessage(pfrom, strCommand, vRecv); - masternodePayments.ProcessMessageMasternodePayments(pfrom, strCommand, vRecv); - sporkManager.ProcessSpork(pfrom, strCommand, vRecv); - masternodeSync.ProcessMessage(pfrom, strCommand, vRecv); - } - } else { - // Ignore unknown commands for extensibility - LogPrint(BCLog::NET, "Unknown command \"%s\" from peer=%d\n", SanitizeString(strCommand), pfrom->id); + if (found) { + // Check if the dispatcher can process this message first. If not, try going with the old flow. + if (!masternodeSync.MessageDispatcher(pfrom, strCommand, vRecv)) { + //probably one the extensions + mnodeman.ProcessMessage(pfrom, strCommand, vRecv); + g_budgetman.ProcessMessage(pfrom, strCommand, vRecv); + masternodePayments.ProcessMessageMasternodePayments(pfrom, strCommand, vRecv); + sporkManager.ProcessSpork(pfrom, strCommand, vRecv); + masternodeSync.ProcessMessage(pfrom, strCommand, vRecv); } + } else { + // Ignore unknown commands for extensibility + LogPrint(BCLog::NET, "Unknown command \"%s\" from peer=%d\n", SanitizeString(strCommand), pfrom->id); } return true; @@ -2027,7 +1988,6 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic& inter if (!pfrom->vRecvGetData.empty()) fMoreWork = true; } catch (const std::ios_base::failure& e) { - connman->PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strCommand, REJECT_MALFORMED, std::string("error parsing message"))); if (strstr(e.what(), "end of data")) { // Allow exceptions from under-length message on vRecv LogPrint(BCLog::NET, "ProcessMessages(%s, %u bytes): Exception '%s' caught, normally caused by a message being shorter than its stated length\n", SanitizeString(strCommand), nMessageSize, e.what()); @@ -2111,10 +2071,6 @@ bool PeerLogicValidation::SendMessages(CNode* pto, std::atomic& interruptM CNodeState& state = *State(pto->GetId()); - for (const CBlockReject& reject : state.rejects) - connman->PushMessage(pto, msgMaker.Make(NetMsgType::REJECT, std::string(NetMsgType::BLOCK), reject.chRejectCode, reject.strRejectReason, reject.hashBlock)); - state.rejects.clear(); - if (state.fShouldBan) { state.fShouldBan = false; if (pto->fWhitelisted) diff --git a/src/protocol.cpp b/src/protocol.cpp index f40fc79aed7e..b5895155d93e 100644 --- a/src/protocol.cpp +++ b/src/protocol.cpp @@ -36,7 +36,6 @@ const char* NOTFOUND = "notfound"; const char* FILTERLOAD = "filterload"; const char* FILTERADD = "filteradd"; const char* FILTERCLEAR = "filterclear"; -const char* REJECT = "reject"; const char* SENDHEADERS = "sendheaders"; const char* SPORK = "spork"; const char* GETSPORKS = "getsporks"; @@ -97,7 +96,6 @@ const static std::string allNetMessageTypes[] = { NetMsgType::FILTERLOAD, NetMsgType::FILTERADD, NetMsgType::FILTERCLEAR, - NetMsgType::REJECT, NetMsgType::SENDHEADERS, "filtered block", // Should never occur "ix", // deprecated diff --git a/src/protocol.h b/src/protocol.h index 0c3143dc79f1..158a7b1e1f6f 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -201,13 +201,6 @@ extern const char* FILTERADD; * @see https://bitcoin.org/en/developer-reference#filterclear */ extern const char* FILTERCLEAR; -/** - * The reject message informs the receiving node that one of its previous - * messages has been rejected. - * @since protocol version 70002 as described by BIP61. - * @see https://bitcoin.org/en/developer-reference#reject - */ -extern const char* REJECT; /** * Indicates that a node prefers to receive new block announcements via a * "headers" message rather than an "inv". diff --git a/src/validation.h b/src/validation.h index 30ab92c5fed2..8d940d555a94 100644 --- a/src/validation.h +++ b/src/validation.h @@ -101,8 +101,6 @@ static const unsigned int BLOCK_DOWNLOAD_WINDOW = 1024; static const unsigned int DATABASE_WRITE_INTERVAL = 60 * 60; /** Time to wait (in seconds) between flushing chainstate to disk. */ static const unsigned int DATABASE_FLUSH_INTERVAL = 24 * 60 * 60; -/** Maximum length of reject messages. */ -static const unsigned int MAX_REJECT_MESSAGE_LENGTH = 111; /** Average delay between local address broadcasts in seconds. */ static const unsigned int AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL = 24 * 24 * 60; /** Average delay between peer address broadcasts in seconds. */ diff --git a/test/functional/p2p_leak.py b/test/functional/p2p_leak.py index 7c4e2733e0ef..f7eaaaa354ef 100755 --- a/test/functional/p2p_leak.py +++ b/test/functional/p2p_leak.py @@ -4,7 +4,7 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test message sending before handshake completion. -A node should never send anything other than VERSION/VERACK/REJECT until it's +A node should never send anything other than VERSION/VERACK until it's received a VERACK. This test connects to a node and sends it a few messages, trying to intice it @@ -70,8 +70,6 @@ def on_open(self): for i in range(banscore): self.send_message(msg_verack()) - def on_reject(self, message): pass - # Node that never sends a version. This one just sits idle and hopes to receive # any message (it shouldn't!) class CNodeNoVersionIdle(CLazyNode): @@ -84,7 +82,6 @@ def __init__(self): self.version_received = False super().__init__() - def on_reject(self, message): pass def on_verack(self, message): pass # When version is received, don't reply with a verack. Instead, see if the # node will give us a message that it shouldn't. This is not an exhaustive diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index 521e9258c230..af151776c582 100644 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -1249,38 +1249,6 @@ def serialize(self): def __repr__(self): return "msg_headers(headers=%s)" % repr(self.headers) - -class msg_reject(): - command = b"reject" - REJECT_MALFORMED = 1 - - def __init__(self): - self.message = b"" - self.code = 0 - self.reason = b"" - self.data = 0 - - def deserialize(self, f): - self.message = deser_string(f) - self.code = struct.unpack(" Date: Tue, 29 Jun 2021 22:13:28 -0300 Subject: [PATCH 02/13] net_processing: Remove duplicate, and wrong, peer misbehaving due an invalid arriving block. 1) The actual peer misbehaving check is being performed inside PeerLogicValidation::BlockChecked. 2) The CValidationState inputted to ProcessNewBlock does not return the error nor invalidity reason if the block is marked as invalid during the activate best chain process (thus why BlockChecked exists). --- src/net_processing.cpp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 4364d22b60b1..048d7fb7501f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1703,14 +1703,6 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR if (!fAccepted) { CheckBlockSpam(state, pfrom, hashBlock); } - int nDoS; - if(state.IsInvalid(nDoS)) { - assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes - if(nDoS > 0) { - TRY_LOCK(cs_main, lockMain); - if(lockMain) Misbehaving(pfrom->GetId(), nDoS); - } - } //disconnect this node if its old protocol version pfrom->DisconnectOldProtocol(pfrom->nVersion, ActiveProtocol(), strCommand); } else { From 960647445335be0b1d3a42c67faa2f0d747ddee8 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 29 Jun 2021 22:26:01 -0300 Subject: [PATCH 03/13] net_processing: Clean CheckBlockSpam code and add proper cs_main locks in State() call and mapBlockIndex access. --- src/net_processing.cpp | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 048d7fb7501f..bba26ce55f04 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -130,8 +130,7 @@ class CNodeBlocks // Compute the number of the received blocks size_t nBlocks = 0; - for(auto point : points) - { + for(auto point : points) { nBlocks += point.second; } @@ -142,8 +141,7 @@ class CNodeBlocks bool banNode = (nAvgValue >= 1.5 * maxAvg && size >= maxAvg) || (nAvgValue >= maxAvg && nBlocks >= maxSize) || (nBlocks >= maxSize * 3); - if(banNode) - { + if(banNode) { // Clear the points and ban the node points.clear(); return state.DoS(100, error("block-spam ban node for sending spam")); @@ -1036,31 +1034,35 @@ void static ProcessGetData(CNode* pfrom, CConnman* connman, const std::atomicGetId()); - if(!nodestate) { - return; - } - const auto it = mapBlockIndex.find(hashBlock); - if (it == mapBlockIndex.end()) { - return; + CNodeState* nodestate = nullptr; + int blockReceivedHeight = 0; + { + LOCK(cs_main); + nodestate = State(nodeId); + if (!nodestate) { return; } + + const auto it = mapBlockIndex.find(hashBlock); + if (it == mapBlockIndex.end()) { return; } + blockReceivedHeight = it->second->nHeight; } - nodestate->nodeBlocks.onBlockReceived(it->second->nHeight); + nodestate->nodeBlocks.onBlockReceived(blockReceivedHeight); bool nodeStatus = true; // UpdateState will return false if the node is attacking us or update the score and return true. + CValidationState state; nodeStatus = nodestate->nodeBlocks.updateState(state, nodeStatus); int nDoS = 0; if (state.IsInvalid(nDoS)) { if (nDoS > 0) { LOCK(cs_main); - Misbehaving(pfrom->GetId(), nDoS); + Misbehaving(nodeId, nDoS); } nodeStatus = false; } @@ -1701,7 +1703,7 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR bool fAccepted = true; ProcessNewBlock(state, pblock, nullptr, &fAccepted); if (!fAccepted) { - CheckBlockSpam(state, pfrom, hashBlock); + CheckBlockSpam(pfrom->GetId(), hashBlock); } //disconnect this node if its old protocol version pfrom->DisconnectOldProtocol(pfrom->nVersion, ActiveProtocol(), strCommand); From 22b17577b3134572311bca2bd25fdd49e7c83d69 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 30 Jun 2021 00:17:23 -0300 Subject: [PATCH 04/13] validation: Proper validation state set for "out of order" check in CheckBlock. --- src/test/validation_block_tests.cpp | 8 ++++---- src/validation.cpp | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index d3d332dd11ec..3d9e0063a1a7 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -143,10 +143,10 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering) if (block->vtx.size() == 1) { bool processed = ProcessNewBlock(state, block, nullptr); // Future to do: "prevblk-not-found" here is the only valid reason to not check processed flag. - if (state.GetRejectReason() == "duplicate" || - state.GetRejectReason() == "prevblk-not-found" || - state.GetRejectReason() == "bad-prevblk") continue; - ASSERT_WITH_MSG(!processed, ("Error: " + state.GetRejectReason()).c_str()); + std::string stateReason = state.GetRejectReason(); + if (stateReason == "duplicate" || stateReason == "prevblk-not-found" || + stateReason == "bad-prevblk" || stateReason == "blk-out-of-order") continue; + ASSERT_WITH_MSG(processed, ("Error: " + state.GetRejectReason()).c_str()); } } }); diff --git a/src/validation.cpp b/src/validation.cpp index 91aa087f1461..c03ba70fa593 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2759,7 +2759,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo //out of order auto mi = mapBlockIndex.find(block.hashPrevBlock); if (mi == mapBlockIndex.end()) { - return false; + return state.Error("blk-out-of-order"); } pindexPrev = mi->second; } From bd85440f0f8d846c73d1a7264f3b7482b8e4459c Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 30 Jun 2021 00:48:57 -0300 Subject: [PATCH 05/13] Refactor: get rid off the error prone CValidationState argument in ProcessNewBlock. Any block processing error is being notified by the BlockChecked signal. --- CMakeLists.txt | 1 + src/Makefile.am | 1 + src/miner.cpp | 9 +++--- src/net_processing.cpp | 5 ++-- src/rpc/mining.cpp | 38 +++++++------------------ src/test/evo_deterministicmns_tests.cpp | 15 +++++----- src/test/miner_tests.cpp | 8 ++++-- src/test/test_pivx.cpp | 3 +- src/test/util/blocksutil.cpp | 32 ++++----------------- src/test/validation_block_tests.cpp | 20 +++++++------ src/test/validation_tests.cpp | 10 ++++--- src/util/blockstatecatcher.h | 37 ++++++++++++++++++++++++ src/validation.cpp | 24 +++++++++++----- src/validation.h | 11 +++++-- 14 files changed, 117 insertions(+), 97 deletions(-) create mode 100644 src/util/blockstatecatcher.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 46c3e6baf63b..676c005186d2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -418,6 +418,7 @@ set(UTIL_SOURCES ./src/arith_uint256.cpp ./src/uint256.cpp ./src/util/threadnames.cpp + ./src/util/blockstatecatcher.h ./src/util/system.cpp ./src/utilstrencodings.cpp ./src/utilmoneystr.cpp diff --git a/src/Makefile.am b/src/Makefile.am index f433e718da77..3831b986bd96 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -285,6 +285,7 @@ BITCOIN_CORE_H = \ guiinterfaceutil.h \ uint256.h \ undo.h \ + util/blockstatecatcher.h \ util/memory.h \ util/system.h \ util/macros.h \ diff --git a/src/miner.cpp b/src/miner.cpp index 1173b7c3dbc3..a5d752024c64 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -12,7 +12,6 @@ #include "amount.h" #include "blockassembler.h" -#include "consensus/tx_verify.h" // needed in case of no ENABLE_WALLET #include "consensus/params.h" #include "masternode-sync.h" #include "net.h" @@ -20,12 +19,12 @@ #include "primitives/block.h" #include "primitives/transaction.h" #include "timedata.h" +#include "util/blockstatecatcher.h" #include "util/system.h" #include "utilmoneystr.h" #ifdef ENABLE_WALLET #include "wallet/wallet.h" #endif -#include "validationinterface.h" #include "invalid.h" #include "policy/policy.h" @@ -79,8 +78,10 @@ bool ProcessBlockFound(const std::shared_ptr& pblock, CWallet& wal reservekey->KeepKey(); // Process this block the same as if we had received it from another node - CValidationState state; - if (!ProcessNewBlock(state, pblock, nullptr)) { + BlockStateCatcher sc(pblock->GetHash()); + sc.registerEvent(); + bool res = ProcessNewBlock(pblock, nullptr); + if (!res || sc.stateErrorFound()) { return error("PIVXMiner : ProcessNewBlock, block not accepted"); } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index bba26ce55f04..5157fec10df3 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1693,7 +1693,6 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR } } else { pfrom->AddInventoryKnown(inv); - CValidationState state; if (!mapBlockIndex.count(hashBlock)) { { LOCK(cs_main); @@ -1701,8 +1700,8 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR mapBlockSource.emplace(hashBlock, pfrom->GetId()); } bool fAccepted = true; - ProcessNewBlock(state, pblock, nullptr, &fAccepted); - if (!fAccepted) { + ProcessNewBlock(pblock, nullptr, &fAccepted); + if (!fAccepted) { // future: can be moved inside BlockChecked. CheckBlockSpam(pfrom->GetId(), hashBlock); } //disconnect this node if its old protocol version diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index eac3f1f4c062..d026613c2e12 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -14,6 +14,7 @@ #include "miner.h" #include "net.h" #include "rpc/server.h" +#include "util/blockstatecatcher.h" #include "validationinterface.h" #ifdef ENABLE_WALLET #include "wallet/rpcwallet.h" @@ -34,6 +35,8 @@ UniValue generateBlocks(const Consensus::Params& consensus, { UniValue blockHashes(UniValue::VARR); + BlockStateCatcher sc(UINT256_ZERO); + sc.registerEvent(); while (nHeight < nHeightEnd && !ShutdownRequested()) { // Get available coins @@ -53,8 +56,9 @@ UniValue generateBlocks(const Consensus::Params& consensus, if (!SolveBlock(pblock, nHeight + 1)) continue; } - CValidationState state; - if (!ProcessNewBlock(state, pblock, nullptr)) + sc.setBlockHash(pblock->GetHash()); + bool res = ProcessNewBlock(pblock, nullptr); + if (!res || sc.stateErrorFound()) throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock, block not accepted"); ++nHeight; @@ -723,25 +727,6 @@ UniValue getblocktemplate(const JSONRPCRequest& request) } #endif // ENABLE_MINING_RPC -class submitblock_StateCatcher : public CValidationInterface -{ -public: - uint256 hash; - bool found; - CValidationState state; - - submitblock_StateCatcher(const uint256& hashIn) : hash(hashIn), found(false), state(){}; - -protected: - virtual void BlockChecked(const CBlock& block, const CValidationState& stateIn) - { - if (block.GetHash() != hash) - return; - found = true; - state = stateIn; - }; -}; - UniValue submitblock(const JSONRPCRequest& request) { if (request.fHelp || request.params.size() < 1 || request.params.size() > 2) @@ -788,11 +773,9 @@ UniValue submitblock(const JSONRPCRequest& request) } } - CValidationState state; - submitblock_StateCatcher sc(block.GetHash()); - RegisterValidationInterface(&sc); - bool fAccepted = ProcessNewBlock(state, blockptr, nullptr); - UnregisterValidationInterface(&sc); + BlockStateCatcher sc(block.GetHash()); + sc.registerEvent(); + bool fAccepted = ProcessNewBlock(blockptr, nullptr); if (fBlockPresent) { if (fAccepted && !sc.found) return "duplicate-inconclusive"; @@ -801,9 +784,8 @@ UniValue submitblock(const JSONRPCRequest& request) if (fAccepted) { if (!sc.found) return "inconclusive"; - state = sc.state; } - return BIP22ValidationResult(state); + return BIP22ValidationResult(sc.state); } UniValue estimatefee(const JSONRPCRequest& request) diff --git a/src/test/evo_deterministicmns_tests.cpp b/src/test/evo_deterministicmns_tests.cpp index 04878963723b..67e9cf8c5f25 100644 --- a/src/test/evo_deterministicmns_tests.cpp +++ b/src/test/evo_deterministicmns_tests.cpp @@ -401,7 +401,7 @@ BOOST_FIXTURE_TEST_CASE(dip3_protx, TestChain400Setup) CValidationState state; BOOST_CHECK(!ProcessSpecialTxsInBlock(block, &indexFake, state, true)); BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-protx-dup-owner-key"); - ProcessNewBlock(state, std::make_shared(block), nullptr); + ProcessNewBlock(std::make_shared(block), nullptr); // todo: move to check reject reason BOOST_CHECK_EQUAL(chainActive.Height(), nHeight); // bad block not connected } // Block with two ProReg txes using same operator key @@ -418,7 +418,7 @@ BOOST_FIXTURE_TEST_CASE(dip3_protx, TestChain400Setup) CValidationState state; BOOST_CHECK(!ProcessSpecialTxsInBlock(block, &indexFake, state, true)); BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-protx-dup-operator-key"); - ProcessNewBlock(state, std::make_shared(block), nullptr); + ProcessNewBlock(std::make_shared(block), nullptr); // todo: move to check reject reason BOOST_CHECK_EQUAL(chainActive.Height(), nHeight); // bad block not connected } // Block with two ProReg txes using ip address @@ -432,7 +432,7 @@ BOOST_FIXTURE_TEST_CASE(dip3_protx, TestChain400Setup) CValidationState state; BOOST_CHECK(!ProcessSpecialTxsInBlock(block, &indexFake, state, true)); BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-protx-dup-IP-address"); - ProcessNewBlock(state, std::make_shared(block), nullptr); + ProcessNewBlock(std::make_shared(block), nullptr); // todo: move to check reject reason BOOST_CHECK_EQUAL(chainActive.Height(), nHeight); // bad block not connected } @@ -501,8 +501,7 @@ BOOST_FIXTURE_TEST_CASE(dip3_protx, TestChain400Setup) GetScriptForDestination(coinbaseKey.GetPubKey().GetID()))); pblock->vtx[0] = MakeTransactionRef(invalidCoinbaseTx); pblock->hashMerkleRoot = BlockMerkleRoot(*pblock); - CValidationState state; - ProcessNewBlock(state, pblock, nullptr); + ProcessNewBlock(pblock, nullptr); // block not connected chainTip = WITH_LOCK(cs_main, return chainActive.Tip()); BOOST_CHECK(chainTip->nHeight == nHeight); @@ -610,7 +609,7 @@ BOOST_FIXTURE_TEST_CASE(dip3_protx, TestChain400Setup) CValidationState state; BOOST_CHECK(!ProcessSpecialTxsInBlock(block, &indexFake, state, true)); BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-protx-dup-addr"); - ProcessNewBlock(state, std::make_shared(block), nullptr); + ProcessNewBlock(std::make_shared(block), nullptr); // todo: move to ProcessBlockAndCheckRejectionReason. BOOST_CHECK_EQUAL(chainActive.Height(), nHeight); // bad block not connected } @@ -728,7 +727,7 @@ BOOST_FIXTURE_TEST_CASE(dip3_protx, TestChain400Setup) BOOST_CHECK_MESSAGE(!ProcessSpecialTxsInBlock(block, &indexFake, state, true), "Accepted block with duplicate operator key in ProUpReg txes"); BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-protx-dup-operator-key"); - ProcessNewBlock(state, std::make_shared(block), nullptr); + ProcessNewBlock(std::make_shared(block), nullptr); BOOST_CHECK_EQUAL(chainActive.Height(), nHeight); // bad block not connected } @@ -748,7 +747,7 @@ BOOST_FIXTURE_TEST_CASE(dip3_protx, TestChain400Setup) BOOST_CHECK_MESSAGE(!ProcessSpecialTxsInBlock(block, &indexFake, state, true), "Accepted block with duplicate operator key in ProReg+ProUpReg txes"); BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-protx-dup-operator-key"); - ProcessNewBlock(state, std::make_shared(block), nullptr); + ProcessNewBlock(std::make_shared(block), nullptr); BOOST_CHECK_EQUAL(chainActive.Height(), nHeight); // bad block not connected } diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index bf447f336402..81b9b8a608bd 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -11,6 +11,7 @@ #include "miner.h" #include "pubkey.h" #include "uint256.h" +#include "util/blockstatecatcher.h" #include "util/system.h" #include "validation.h" #include "wallet/wallet.h" @@ -213,10 +214,11 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) txFirst.emplace_back(pblock->vtx[0]); pblock->hashMerkleRoot = BlockMerkleRoot(*pblock); pblock->nNonce = blockinfo[i].nonce; - CValidationState state; - BOOST_CHECK(ProcessNewBlock(state, pblock, nullptr)); - BOOST_CHECK(state.IsValid()); + BlockStateCatcher stateCatcher(pblock->GetHash()); + stateCatcher.registerEvent(); + BOOST_CHECK(ProcessNewBlock(pblock, nullptr)); SyncWithValidationInterfaceQueue(); + BOOST_CHECK(stateCatcher.found && stateCatcher.state.IsValid()); pblock->hashPrevBlock = pblock->GetHash(); } diff --git a/src/test/test_pivx.cpp b/src/test/test_pivx.cpp index 459bcb344ae9..39dac4744be0 100644 --- a/src/test/test_pivx.cpp +++ b/src/test/test_pivx.cpp @@ -153,8 +153,7 @@ TestChainSetup::TestChainSetup(int blockCount) : TestingSetup(CBaseChainParams:: CBlock TestChainSetup::CreateAndProcessBlock(const std::vector& txns, const CScript& scriptPubKey, bool fNoMempoolTx) { CBlock block = CreateBlock(txns, scriptPubKey, fNoMempoolTx); - CValidationState state; - ProcessNewBlock(state, std::make_shared(block), nullptr); + ProcessNewBlock(std::make_shared(block), nullptr); return block; } diff --git a/src/test/util/blocksutil.cpp b/src/test/util/blocksutil.cpp index c5c88ebe700c..d1d5fe488a99 100644 --- a/src/test/util/blocksutil.cpp +++ b/src/test/util/blocksutil.cpp @@ -4,39 +4,19 @@ #include "test/util/blocksutil.h" #include "validation.h" -#include "validationinterface.h" +#include "util/blockstatecatcher.h" #include -class BlockStateCatcher : public CValidationInterface -{ -public: - uint256 hash; - bool found; - CValidationState state; - - BlockStateCatcher(const uint256& hashIn) : hash(hashIn), found(false), state(){}; - -protected: - virtual void BlockChecked(const CBlock& block, const CValidationState& stateIn) - { - if (block.GetHash() != hash) return; - found = true; - state = stateIn; - }; -}; - void ProcessBlockAndCheckRejectionReason(std::shared_ptr& pblock, const std::string& blockRejectionReason, int expectedChainHeight) { - CValidationState state; - BlockStateCatcher stateChecker(pblock->GetHash()); - RegisterValidationInterface(&stateChecker); - ProcessNewBlock(state, pblock, nullptr); - UnregisterValidationInterface(&stateChecker); - BOOST_CHECK(stateChecker.found); - state = stateChecker.state; + BlockStateCatcher stateCatcher(pblock->GetHash()); + stateCatcher.registerEvent(); + ProcessNewBlock(pblock, nullptr); + BOOST_CHECK(stateCatcher.found); + CValidationState state = stateCatcher.state; BOOST_CHECK_EQUAL(WITH_LOCK(cs_main, return chainActive.Height();), expectedChainHeight); BOOST_CHECK(!state.IsValid()); BOOST_CHECK_EQUAL(state.GetRejectReason(), blockRejectionReason); diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index 3d9e0063a1a7..c53a999ddb02 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -11,6 +11,7 @@ #include "pow.h" #include "random.h" #include "test/test_pivx.h" +#include "util/blockstatecatcher.h" #include "validation.h" #include "validationinterface.h" @@ -116,9 +117,8 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering) BuildChain(Params().GenesisBlock().GetHash(), 100, 15, 10, 500, blocks); } - CValidationState state; // Connect the genesis block and drain any outstanding events - BOOST_CHECK_MESSAGE(ProcessNewBlock(state, std::make_shared(Params().GenesisBlock()), nullptr), "Error: genesis not connected"); + BOOST_CHECK_MESSAGE(ProcessNewBlock(std::make_shared(Params().GenesisBlock()), nullptr), "Error: genesis not connected"); SyncWithValidationInterfaceQueue(); // subscribe to events (this subscriber will validate event ordering) @@ -132,21 +132,23 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering) boost::thread_group threads; for (int i = 0; i < 10; i++) { threads.create_thread([&blocks]() { - CValidationState state; for (int i = 0; i < 1000; i++) { auto block = blocks[GetRand(blocks.size() - 1)]; - ProcessNewBlock(state, block, nullptr); + ProcessNewBlock(block, nullptr); } + BlockStateCatcher sc(UINT256_ZERO); + sc.registerEvent(); // to make sure that eventually we process the full chain - do it here for (const auto& block : blocks) { if (block->vtx.size() == 1) { - bool processed = ProcessNewBlock(state, block, nullptr); + sc.setBlockHash(block->GetHash()); + bool processed = ProcessNewBlock(block, nullptr); // Future to do: "prevblk-not-found" here is the only valid reason to not check processed flag. - std::string stateReason = state.GetRejectReason(); - if (stateReason == "duplicate" || stateReason == "prevblk-not-found" || - stateReason == "bad-prevblk" || stateReason == "blk-out-of-order") continue; - ASSERT_WITH_MSG(processed, ("Error: " + state.GetRejectReason()).c_str()); + std::string stateReason = sc.state.GetRejectReason(); + if (sc.found && (stateReason == "duplicate" || stateReason == "prevblk-not-found" || + stateReason == "bad-prevblk" || stateReason == "blk-out-of-order")) continue; + ASSERT_WITH_MSG(processed, ("Error: " + sc.state.GetRejectReason()).c_str()); } } }); diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp index 8eef74aa1fa4..fb90709eab07 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -7,6 +7,7 @@ #include "primitives/transaction.h" #include "sapling/sapling_validation.h" #include "test/librust/utiltest.h" +#include "util/blockstatecatcher.h" #include "wallet/test/wallet_test_fixture.h" #include @@ -97,10 +98,11 @@ void CheckBlockZcRejection(std::shared_ptr& pblock, int nHeight, CMutabl { pblock->vtx.emplace_back(MakeTransactionRef(mtx)); BOOST_CHECK(SolveBlock(pblock, nHeight)); - CValidationState state; - BOOST_CHECK(!ProcessNewBlock(state, pblock, nullptr)); - BOOST_CHECK(!state.IsValid()); - BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-blk-with-zc"); + BlockStateCatcher stateCatcher(pblock->GetHash()); + stateCatcher.registerEvent(); + BOOST_CHECK(!ProcessNewBlock(pblock, nullptr)); + BOOST_CHECK(stateCatcher.found && !stateCatcher.state.IsValid()); + BOOST_CHECK_EQUAL(stateCatcher.state.GetRejectReason(), "bad-blk-with-zc"); } void CheckMempoolZcRejection(CMutableTransaction& mtx) diff --git a/src/util/blockstatecatcher.h b/src/util/blockstatecatcher.h new file mode 100644 index 000000000000..2db568fc7e34 --- /dev/null +++ b/src/util/blockstatecatcher.h @@ -0,0 +1,37 @@ +// Copyright (c) 2021 The PIVX developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://www.opensource.org/licenses/mit-license.php. + +#ifndef PIVX_BLOCKSTATECATCHER_H +#define PIVX_BLOCKSTATECATCHER_H + +#include "validation.h" +#include "validationinterface.h" + +/** + * Validation interface listener used to get feedback from ProcessNewBlock result. + */ +class BlockStateCatcher : public CValidationInterface +{ +public: + uint256 hash; + bool found; + CValidationState state; + bool isRegistered{false}; + + BlockStateCatcher(const uint256& hashIn) : hash(hashIn), found(false), state(){}; + ~BlockStateCatcher() { if (isRegistered) UnregisterValidationInterface(this); } + void registerEvent() { RegisterValidationInterface(this); isRegistered = true; } + void setBlockHash(const uint256& _hash) { clear(); hash = _hash; } + void clear() { hash.SetNull(); found = false; state = CValidationState(); } + bool stateErrorFound() { return found && state.IsError(); } + +protected: + virtual void BlockChecked(const CBlock& block, const CValidationState& stateIn) { + if (block.GetHash() != hash) return; + found = true; + state = stateIn; + }; +}; + +#endif //PIVX_BLOCKSTATECATCHER_H diff --git a/src/validation.cpp b/src/validation.cpp index c03ba70fa593..af15f8933e92 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -13,6 +13,7 @@ #include "addrman.h" #include "amount.h" #include "blocksignature.h" +#include "util/blockstatecatcher.h" #include "budget/budgetmanager.h" #include "chainparams.h" #include "checkpoints.h" @@ -3306,7 +3307,7 @@ static bool AcceptBlock(const CBlock& block, CValidationState& state, CBlockInde return true; } -bool ProcessNewBlock(CValidationState& state, const std::shared_ptr pblock, const FlatFilePos* dbp, bool* fAccepted) +bool ProcessNewBlock(const std::shared_ptr pblock, const FlatFilePos* dbp, bool* fAccepted) { AssertLockNotHeld(cs_main); @@ -3317,7 +3318,9 @@ bool ProcessNewBlock(CValidationState& state, const std::shared_ptrGetHash().GetHex(), FormatStateMessage(state)); } @@ -3327,11 +3330,13 @@ bool ProcessNewBlock(CValidationState& state, const std::shared_ptrnHeight; } + CValidationState state; // Only used to report errors, not invalidity - ignore it if (!ActivateBestChain(state, pblock)) return error("%s : ActivateBestChain failed", __func__); @@ -3854,6 +3859,10 @@ bool LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp) static std::multimap mapBlocksUnknownParent; int64_t nStart = GetTimeMillis(); + // Block checked event listener + BlockStateCatcher stateCatcher(UINT256_ZERO); + stateCatcher.registerEvent(); + int nLoaded = 0; try { // This takes over fileIn and calls fclose() on it in the CBufferedFile destructor @@ -3905,12 +3914,14 @@ bool LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp) // process in case the block isn't known yet if (mapBlockIndex.count(hash) == 0 || (mapBlockIndex[hash]->nStatus & BLOCK_HAVE_DATA) == 0) { - CValidationState state; std::shared_ptr block_ptr = std::make_shared(block); - if (ProcessNewBlock(state, block_ptr, dbp)) + stateCatcher.setBlockHash(block_ptr->GetHash()); + if (ProcessNewBlock(block_ptr, dbp)) { nLoaded++; - if (state.IsError()) + } + if (stateCatcher.stateErrorFound()) { break; + } } else if (hash != Params().GetConsensus().hashGenesisBlock && mapBlockIndex[hash]->nHeight % 1000 == 0) { LogPrint(BCLog::REINDEX, "Block Import: already had block %s at height %d\n", hash.ToString(), mapBlockIndex[hash]->nHeight); } @@ -3927,11 +3938,10 @@ bool LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp) if (ReadBlockFromDisk(block, it->second)) { LogPrint(BCLog::REINDEX, "%s: Processing out of order child %s of %s\n", __func__, block.GetHash().ToString(), head.ToString()); - CValidationState dummy; std::shared_ptr block_ptr = std::make_shared(block); - if (ProcessNewBlock(dummy, block_ptr, &it->second)) { + if (ProcessNewBlock(block_ptr, &it->second)) { nLoaded++; - queue.push_back(block.GetHash()); + queue.emplace_back(block.GetHash()); } } range.first++; diff --git a/src/validation.h b/src/validation.h index 8d940d555a94..9dc3d4bfb6f8 100644 --- a/src/validation.h +++ b/src/validation.h @@ -158,14 +158,19 @@ extern CBlockIndex* pindexBestHeader; * block is made active. Note that it does not, however, guarantee that the * specific block passed to it has been checked for validity! * - * @param[out] state This may be set to an Error state if any error occurred processing it, including during validation/connection/etc of otherwise unrelated blocks during reorganisation; or it may be set to an Invalid state if pblock is itself invalid (but this is not guaranteed even when the block is checked). If you want to *possibly* get feedback on whether pblock is valid, you must also install a CValidationInterface - this will have its BlockChecked method called whenever *any* block completes validation. - * @param[in] pfrom The node which we are receiving the block from; it is added to mapBlockSource and may be penalised if the block is invalid. + * If you want to *possibly* get feedback on whether pblock is valid, you must + * install a CValidationInterface (see validationinterface.h) - this will have + * its BlockChecked method called whenever *any* block completes validation. + * + * Note that we guarantee that either the proof-of-work is valid on pblock, or + * (and possibly also) BlockChecked will have been called. + * * @param[in] pblock The block we want to process. * @param[out] dbp The already known disk position of pblock, or nullptr if not yet stored. * @param[out] fAccepted Whether the block is accepted or not * @return True if state.IsValid() */ -bool ProcessNewBlock(CValidationState& state, const std::shared_ptr pblock, const FlatFilePos* dbp, bool* fAccepted = nullptr); +bool ProcessNewBlock(const std::shared_ptr pblock, const FlatFilePos* dbp, bool* fAccepted = nullptr); /** Open a block file (blk?????.dat) */ FILE* OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false); From 0d4ba29a1880c9435e61983fbb42f478087cc5b1 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 2 Jul 2021 22:33:11 -0300 Subject: [PATCH 06/13] Move block spam filter check to BlockChecked Aside from the code responsibilities division improvement, this adds the good property of checking block spam filter for blocks that were rejected during the connection process as well (before, was only called if AcceptBlock rejected the block). --- src/net_processing.cpp | 79 ++++++++++++++++++++---------------------- 1 file changed, 38 insertions(+), 41 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5157fec10df3..b896c8ef5892 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -600,8 +600,43 @@ void Misbehaving(NodeId pnode, int howmuch) EXCLUSIVE_LOCKS_REQUIRED(cs_main) LogPrintf("Misbehaving: %s (%d -> %d)\n", state->name, state->nMisbehavior - howmuch, state->nMisbehavior); } +static void CheckBlockSpam(NodeId nodeId, const uint256& hashBlock) +{ + // Block spam filtering + if (!gArgs.GetBoolArg("-blockspamfilter", DEFAULT_BLOCK_SPAM_FILTER)) { + return; + } + CNodeState* nodestate = nullptr; + int blockReceivedHeight = 0; + { + LOCK(cs_main); + nodestate = State(nodeId); + if (!nodestate) { return; } + + const auto it = mapBlockIndex.find(hashBlock); + if (it == mapBlockIndex.end()) { return; } + blockReceivedHeight = it->second->nHeight; + } + + nodestate->nodeBlocks.onBlockReceived(blockReceivedHeight); + bool nodeStatus = true; + // UpdateState will return false if the node is attacking us or update the score and return true. + CValidationState state; + nodeStatus = nodestate->nodeBlocks.updateState(state, nodeStatus); + int nDoS = 0; + if (state.IsInvalid(nDoS)) { + if (nDoS > 0) { + LOCK(cs_main); + Misbehaving(nodeId, nDoS); + } + nodeStatus = false; + } + if (!nodeStatus) { + LogPrintf("Block spam protection: %s\n", hashBlock.ToString()); + } +} ////////////////////////////////////////////////////////////////////////////// @@ -679,6 +714,9 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta if (nDoS > 0) { Misbehaving(it->second, nDoS); } + + // Spam filter + CheckBlockSpam(it->second, block.GetHash()); } } @@ -1034,44 +1072,6 @@ void static ProcessGetData(CNode* pfrom, CConnman* connman, const std::atomicsecond->nHeight; - } - - nodestate->nodeBlocks.onBlockReceived(blockReceivedHeight); - bool nodeStatus = true; - // UpdateState will return false if the node is attacking us or update the score and return true. - CValidationState state; - nodeStatus = nodestate->nodeBlocks.updateState(state, nodeStatus); - int nDoS = 0; - if (state.IsInvalid(nDoS)) { - if (nDoS > 0) { - LOCK(cs_main); - Misbehaving(nodeId, nDoS); - } - nodeStatus = false; - } - - if (!nodeStatus) { - LogPrintf("Block spam protection: %s\n", hashBlock.ToString()); - } -} - bool fRequestedSporksIDB = false; bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vRecv, int64_t nTimeReceived, CConnman* connman, std::atomic& interruptMsgProc) { @@ -1701,9 +1701,6 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR } bool fAccepted = true; ProcessNewBlock(pblock, nullptr, &fAccepted); - if (!fAccepted) { // future: can be moved inside BlockChecked. - CheckBlockSpam(pfrom->GetId(), hashBlock); - } //disconnect this node if its old protocol version pfrom->DisconnectOldProtocol(pfrom->nVersion, ActiveProtocol(), strCommand); } else { From 4f62c04b9f4552f83b25d774c33675821d365689 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 2 Jul 2021 22:45:17 -0300 Subject: [PATCH 07/13] Remove now unneeded `fAccepted` flag from ProcessNewBlock. Plus pass pblock as const reference. --- src/net_processing.cpp | 3 +-- src/validation.cpp | 3 +-- src/validation.h | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b896c8ef5892..8b68cb336861 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1699,8 +1699,7 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR MarkBlockAsReceived(hashBlock); mapBlockSource.emplace(hashBlock, pfrom->GetId()); } - bool fAccepted = true; - ProcessNewBlock(pblock, nullptr, &fAccepted); + ProcessNewBlock(pblock, nullptr); //disconnect this node if its old protocol version pfrom->DisconnectOldProtocol(pfrom->nVersion, ActiveProtocol(), strCommand); } else { diff --git a/src/validation.cpp b/src/validation.cpp index af15f8933e92..e9714c0eb9d8 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3307,7 +3307,7 @@ static bool AcceptBlock(const CBlock& block, CValidationState& state, CBlockInde return true; } -bool ProcessNewBlock(const std::shared_ptr pblock, const FlatFilePos* dbp, bool* fAccepted) +bool ProcessNewBlock(const std::shared_ptr& pblock, const FlatFilePos* dbp) { AssertLockNotHeld(cs_main); @@ -3327,7 +3327,6 @@ bool ProcessNewBlock(const std::shared_ptr pblock, const FlatFileP // Store to disk CBlockIndex* pindex = nullptr; bool ret = AcceptBlock(*pblock, state, &pindex, dbp); - if (fAccepted) *fAccepted = ret; CheckBlockIndex(); if (!ret) { GetMainSignals().BlockChecked(*pblock, state); diff --git a/src/validation.h b/src/validation.h index 9dc3d4bfb6f8..5b44ce052c3a 100644 --- a/src/validation.h +++ b/src/validation.h @@ -167,10 +167,9 @@ extern CBlockIndex* pindexBestHeader; * * @param[in] pblock The block we want to process. * @param[out] dbp The already known disk position of pblock, or nullptr if not yet stored. - * @param[out] fAccepted Whether the block is accepted or not * @return True if state.IsValid() */ -bool ProcessNewBlock(const std::shared_ptr pblock, const FlatFilePos* dbp, bool* fAccepted = nullptr); +bool ProcessNewBlock(const std::shared_ptr& pblock, const FlatFilePos* dbp); /** Open a block file (blk?????.dat) */ FILE* OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false); From 38d49eadf3366f99c1bc73d7773d622aa66a3d56 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 5 Jul 2021 21:18:40 -0300 Subject: [PATCH 08/13] Document 'DisconnectOldProtocol' call rationale in block processing. Plus minor cleanup, removing unused parameter. --- src/net.cpp | 3 +-- src/net.h | 2 +- src/net_processing.cpp | 9 ++++++--- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 13f537d6d538..85f63fa67672 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -417,14 +417,13 @@ void CNode::CloseSocketDisconnect() } } -bool CNode::DisconnectOldProtocol(int nVersionIn, int nVersionRequired, std::string strLastCommand) +bool CNode::DisconnectOldProtocol(int nVersionIn, int nVersionRequired) { fDisconnect = false; if (nVersionIn < nVersionRequired) { LogPrintf("%s : peer=%d using obsolete version %i; disconnecting\n", __func__, id, nVersionIn); fDisconnect = true; } - return fDisconnect; } diff --git a/src/net.h b/src/net.h index 480d9265179c..2659cfbc704e 100644 --- a/src/net.h +++ b/src/net.h @@ -777,7 +777,7 @@ class CNode } void CloseSocketDisconnect(); - bool DisconnectOldProtocol(int nVersionIn, int nVersionRequired, std::string strLastCommand = ""); + bool DisconnectOldProtocol(int nVersionIn, int nVersionRequired); void copyStats(CNodeStats& stats); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8b68cb336861..02a371e09e8f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1112,8 +1112,9 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR return false; } - if (pfrom->DisconnectOldProtocol(nVersion, ActiveProtocol(), strCommand)) + if (pfrom->DisconnectOldProtocol(nVersion, ActiveProtocol())) { return false; + } if (nVersion == 10300) nVersion = 300; @@ -1700,8 +1701,10 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR mapBlockSource.emplace(hashBlock, pfrom->GetId()); } ProcessNewBlock(pblock, nullptr); - //disconnect this node if its old protocol version - pfrom->DisconnectOldProtocol(pfrom->nVersion, ActiveProtocol(), strCommand); + + // Disconnect node if its running an old protocol version, + // used during upgrades, when the node is already connected. + pfrom->DisconnectOldProtocol(pfrom->nVersion, ActiveProtocol()); } else { LogPrint(BCLog::NET, "%s : Already processed block %s, skipping ProcessNewBlock()\n", __func__, pblock->GetHash().GetHex()); } From 2e1af2b25a7e4a15b5db66fb794bdb6c42b57f38 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 4 Jul 2021 10:51:09 -0300 Subject: [PATCH 09/13] Remove IsBlockHashInChain function. Only used inside IsTransactionInChain. --- src/validation.cpp | 19 +++++++++---------- src/validation.h | 1 - 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index e9714c0eb9d8..1a5df7c9e01d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2936,23 +2936,22 @@ bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& sta return true; } -bool IsBlockHashInChain(const uint256& hashBlock) -{ - if (hashBlock.IsNull() || !mapBlockIndex.count(hashBlock)) - return false; - - return chainActive.Contains(mapBlockIndex[hashBlock]); -} - bool IsTransactionInChain(const uint256& txId, int& nHeightTx, CTransactionRef& tx) { uint256 hashBlock; if (!GetTransaction(txId, tx, hashBlock, true)) return false; - if (!IsBlockHashInChain(hashBlock)) + + if (hashBlock.IsNull() || !mapBlockIndex.count(hashBlock)) { return false; + } + + CBlockIndex* pindex = mapBlockIndex[hashBlock]; + if (!chainActive.Contains(pindex)) { + return false; + } - nHeightTx = mapBlockIndex.at(hashBlock)->nHeight; + nHeightTx = pindex->nHeight; return true; } diff --git a/src/validation.h b/src/validation.h index 5b44ce052c3a..9eb83fe1e49d 100644 --- a/src/validation.h +++ b/src/validation.h @@ -255,7 +255,6 @@ void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, int nHeight, b bool IsTransactionInChain(const uint256& txId, int& nHeightTx, CTransactionRef& tx); bool IsTransactionInChain(const uint256& txId, int& nHeightTx); -bool IsBlockHashInChain(const uint256& hashBlock); /** * Check if transaction will be final in the next block to be created. From f99c5ed699c074e46928a4530815596eca3cbe3d Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 4 Jul 2021 10:55:40 -0300 Subject: [PATCH 10/13] Move-only: IsTransactionInChain functions moved inside zpivchain.cpp and made static as them are only used there. --- src/validation.cpp | 25 ------------------------- src/validation.h | 3 --- src/zpivchain.cpp | 25 +++++++++++++++++++++++++ 3 files changed, 25 insertions(+), 28 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 1a5df7c9e01d..8049f99932eb 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2936,31 +2936,6 @@ bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& sta return true; } -bool IsTransactionInChain(const uint256& txId, int& nHeightTx, CTransactionRef& tx) -{ - uint256 hashBlock; - if (!GetTransaction(txId, tx, hashBlock, true)) - return false; - - if (hashBlock.IsNull() || !mapBlockIndex.count(hashBlock)) { - return false; - } - - CBlockIndex* pindex = mapBlockIndex[hashBlock]; - if (!chainActive.Contains(pindex)) { - return false; - } - - nHeightTx = pindex->nHeight; - return true; -} - -bool IsTransactionInChain(const uint256& txId, int& nHeightTx) -{ - CTransactionRef tx; - return IsTransactionInChain(txId, nHeightTx, tx); -} - bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIndex* const pindexPrev) { const int nHeight = pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1; diff --git a/src/validation.h b/src/validation.h index 9eb83fe1e49d..61285d960452 100644 --- a/src/validation.h +++ b/src/validation.h @@ -253,9 +253,6 @@ bool CheckInputs(const CTransaction& tx, CValidationState& state, const CCoinsVi /** Apply the effects of this transaction on the UTXO set represented by view */ void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, int nHeight, bool fSkipInvalid = false); -bool IsTransactionInChain(const uint256& txId, int& nHeightTx, CTransactionRef& tx); -bool IsTransactionInChain(const uint256& txId, int& nHeightTx); - /** * Check if transaction will be final in the next block to be created. * diff --git a/src/zpivchain.cpp b/src/zpivchain.cpp index 56940b21a47f..46f194f8caa6 100644 --- a/src/zpivchain.cpp +++ b/src/zpivchain.cpp @@ -13,6 +13,31 @@ // 6 comes from OPCODE (1) + vch.size() (1) + BIGNUM size (4) #define SCRIPT_OFFSET 6 +static bool IsTransactionInChain(const uint256& txId, int& nHeightTx, CTransactionRef& tx) +{ + uint256 hashBlock; + if (!GetTransaction(txId, tx, hashBlock, true)) + return false; + + if (hashBlock.IsNull() || !mapBlockIndex.count(hashBlock)) { + return false; + } + + CBlockIndex* pindex = mapBlockIndex[hashBlock]; + if (!chainActive.Contains(pindex)) { + return false; + } + + nHeightTx = pindex->nHeight; + return true; +} + +static bool IsTransactionInChain(const uint256& txId, int& nHeightTx) +{ + CTransactionRef tx; + return IsTransactionInChain(txId, nHeightTx, tx); +} + bool IsSerialInBlockchain(const CBigNum& bnSerial, int& nHeightTx) { uint256 txHash; From c398797a61f5d657022b212dd952ef20bde0050a Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 5 Jun 2021 12:11:44 -0300 Subject: [PATCH 11/13] Refactor: Name GetDataMsg enum and replace ppszTypeName array for direct GetCommand call. The array is duplicated with the NetMsgType constants --- src/protocol.cpp | 53 ++++++++++++++++++++---------------------------- src/protocol.h | 53 ++++++++++++++++++++++++------------------------ 2 files changed, 49 insertions(+), 57 deletions(-) diff --git a/src/protocol.cpp b/src/protocol.cpp index b5895155d93e..1fdeb846588d 100644 --- a/src/protocol.cpp +++ b/src/protocol.cpp @@ -52,26 +52,6 @@ const char* SYNCSTATUSCOUNT = "ssc"; const char* GETMNLIST = "dseg"; }; // namespace NetMsgType -static const char* ppszTypeName[] = { - "ERROR", // Should never occur - NetMsgType::TX, - NetMsgType::BLOCK, - "filtered block", // Should never occur - "ix", // deprecated - "txlvote", // deprecated - NetMsgType::SPORK, - NetMsgType::MNWINNER, - "mnodescanerr", - NetMsgType::BUDGETVOTE, - NetMsgType::BUDGETPROPOSAL, - NetMsgType::FINALBUDGET, - NetMsgType::FINALBUDGETVOTE, - "mnq", - NetMsgType::MNBROADCAST, - NetMsgType::MNPING, - "dstx" // deprecated -}; - /** All known message types. Keep this in the same order as the list of * messages above and in protocol.h. */ @@ -207,25 +187,36 @@ bool operator<(const CInv& a, const CInv& b) return (a.type < b.type || (a.type == b.type && a.hash < b.hash)); } -bool CInv::IsKnownType() const -{ - return (type >= 1 && type < (int)ARRAYLEN(ppszTypeName)); -} - bool CInv::IsMasterNodeType() const{ return type > 2; } -const char* CInv::GetCommand() const +std::string CInv::GetCommand() const { - if (!IsKnownType()) { - LogPrint(BCLog::NET, "CInv::GetCommand() : type=%d unknown type", type); - return "UNKNOWN"; + std::string cmd; + switch (type) { + case MSG_TX: return cmd.append(NetMsgType::TX); + case MSG_BLOCK: return cmd.append(NetMsgType::BLOCK); + case MSG_FILTERED_BLOCK: return cmd.append(NetMsgType::MERKLEBLOCK); + case MSG_TXLOCK_REQUEST: return cmd.append("ix"); // Deprecated + case MSG_TXLOCK_VOTE: return cmd.append("txlvote"); // Deprecated + case MSG_SPORK: return cmd.append(NetMsgType::SPORK); + case MSG_MASTERNODE_WINNER: return cmd.append(NetMsgType::MNWINNER); + case MSG_MASTERNODE_SCANNING_ERROR: return cmd.append("mnodescanerr"); // Deprecated + case MSG_BUDGET_VOTE: return cmd.append(NetMsgType::BUDGETVOTE); + case MSG_BUDGET_PROPOSAL: return cmd.append(NetMsgType::BUDGETPROPOSAL); + case MSG_BUDGET_FINALIZED: return cmd.append(NetMsgType::FINALBUDGET); + case MSG_BUDGET_FINALIZED_VOTE: return cmd.append(NetMsgType::FINALBUDGETVOTE); + case MSG_MASTERNODE_QUORUM: return cmd.append("mnq"); // Unused + case MSG_MASTERNODE_ANNOUNCE: return cmd.append(NetMsgType::MNBROADCAST); + case MSG_MASTERNODE_PING: return cmd.append(NetMsgType::MNPING); + case MSG_DSTX: return cmd.append("dstx"); // Deprecated + default: + throw std::out_of_range(strprintf("%s: type=%d unknown type", __func__, type)); } - - return ppszTypeName[type]; } + std::string CInv::ToString() const { return strprintf("%s %s", GetCommand(), hash.ToString()); diff --git a/src/protocol.h b/src/protocol.h index 158a7b1e1f6f..34e8363df852 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -326,32 +326,11 @@ class CAddress : public CService unsigned int nTime; }; -/** inv message data */ -class CInv +/** getdata message types */ +enum GetDataMsg { -public: - CInv(); - CInv(int typeIn, const uint256& hashIn); - - SERIALIZE_METHODS(CInv, obj) { READWRITE(obj.type, obj.hash); } - - friend bool operator<(const CInv& a, const CInv& b); - - bool IsKnownType() const; - bool IsMasterNodeType() const; - std::string ToString() const; - - // TODO: make private (improves encapsulation) -public: - int type; - uint256 hash; - -private: - const char* GetCommand() const; -}; - -enum { - MSG_TX = 1, + UNDEFINED = 0, + MSG_TX, MSG_BLOCK, // Nodes may always request a MSG_FILTERED_BLOCK in a getdata, however, // MSG_FILTERED_BLOCK should not appear in any invs except as a part of getdata. @@ -368,7 +347,29 @@ enum { MSG_MASTERNODE_QUORUM, MSG_MASTERNODE_ANNOUNCE, MSG_MASTERNODE_PING, - MSG_DSTX + MSG_DSTX, + MSG_TYPE_MAX = MSG_DSTX +}; + +/** inv message data */ +class CInv +{ +public: + CInv(); + CInv(int typeIn, const uint256& hashIn); + + SERIALIZE_METHODS(CInv, obj) { READWRITE(obj.type, obj.hash); } + + friend bool operator<(const CInv& a, const CInv& b); + + bool IsMasterNodeType() const; + std::string GetCommand() const; + std::string ToString() const; + + // TODO: make private (improve encapsulation) +public: + int type; + uint256 hash; }; #endif // BITCOIN_PROTOCOL_H From 56a78fc65a2ffac234f7b251ef45bb244f506b73 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 19 Jul 2021 13:45:29 -0300 Subject: [PATCH 12/13] Clean few compiler warnings in blockassembler, validation, rpcwallet and evo_deterministicmns_tests files. --- src/blockassembler.cpp | 17 ++++++++--------- src/test/evo_deterministicmns_tests.cpp | 1 - src/validation.cpp | 2 -- src/wallet/rpcwallet.cpp | 2 -- 4 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/blockassembler.cpp b/src/blockassembler.cpp index f31c15bc7c13..b5a373469d14 100644 --- a/src/blockassembler.cpp +++ b/src/blockassembler.cpp @@ -31,7 +31,6 @@ #include #include -#include // Unconfirmed transactions in the memory pool often depend on other // transactions in the memory pool. When we select transactions from the @@ -269,7 +268,7 @@ bool BlockAssembler::TestPackage(uint64_t packageSize, unsigned int packageSigOp // are final. bool BlockAssembler::TestPackageFinality(const CTxMemPool::setEntries& package) { - for (const CTxMemPool::txiter it : package) { + for (const CTxMemPool::txiter& it : package) { if (!IsFinalTx(it->GetSharedTx(), nHeight)) return false; } @@ -298,7 +297,7 @@ void BlockAssembler::AddToBlock(CTxMemPool::txiter iter) void BlockAssembler::UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded, indexed_modified_transaction_set& mapModifiedTx) { - for (const CTxMemPool::txiter it : alreadyAdded) { + for (const CTxMemPool::txiter& it : alreadyAdded) { CTxMemPool::setEntries descendants; mempool.CalculateDescendants(it, descendants); // Insert all descendants (not yet in block) into the modified set @@ -457,22 +456,22 @@ void BlockAssembler::addPackageTxs() SortForBlock(ancestors, iter, sortedEntries); for (size_t i = 0; i < sortedEntries.size(); ++i) { - CTxMemPool::txiter& iter = sortedEntries[i]; - if (iter->IsShielded()) { + CTxMemPool::txiter& iterSortedEntries = sortedEntries[i]; + if (iterSortedEntries->IsShielded()) { // Don't add SHIELD transactions if in maintenance (SPORK_20) if (sporkManager.IsSporkActive(SPORK_20_SAPLING_MAINTENANCE)) { break; } // Don't add SHIELD transactions if there's no reserved space left in the block - if (nSizeShielded + iter->GetTxSize() > MAX_BLOCK_SHIELDED_TXES_SIZE) { + if (nSizeShielded + iterSortedEntries->GetTxSize() > MAX_BLOCK_SHIELDED_TXES_SIZE) { break; } // Update cumulative size of SHIELD transactions in this block - nSizeShielded += iter->GetTxSize(); + nSizeShielded += iterSortedEntries->GetTxSize(); } - AddToBlock(iter); + AddToBlock(iterSortedEntries); // Erase from the modified set, if present - mapModifiedTx.erase(iter); + mapModifiedTx.erase(iterSortedEntries); } // Update transactions that depend on each of these diff --git a/src/test/evo_deterministicmns_tests.cpp b/src/test/evo_deterministicmns_tests.cpp index 67e9cf8c5f25..8f48eb19ca1f 100644 --- a/src/test/evo_deterministicmns_tests.cpp +++ b/src/test/evo_deterministicmns_tests.cpp @@ -683,7 +683,6 @@ BOOST_FIXTURE_TEST_CASE(dip3_protx, TestChain400Setup) // ProUpReg: Try to change the voting key of a masternode that doesn't exist { - const CKey& votingKey = GetRandomKey(); auto tx = CreateProUpRegTx(utxos, GetRandHash(), GetRandomKey(), GetRandomKey(), GetRandomKey(), GenerateRandomAddress(), coinbaseKey); CValidationState state; diff --git a/src/validation.cpp b/src/validation.cpp index 8049f99932eb..1d435eaa8e5f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -23,7 +23,6 @@ #include "consensus/tx_verify.h" #include "consensus/validation.h" #include "consensus/zerocoin_verify.h" -#include "evo/deterministicmns.h" #include "evo/specialtx.h" #include "flatfile.h" #include "fs.h" @@ -1532,7 +1531,6 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd CAmount nValueOut = 0; CAmount nValueIn = 0; unsigned int nMaxBlockSigOps = MAX_BLOCK_SIGOPS_CURRENT; - uint256 hashBlock = block.GetHash(); // Sapling SaplingMerkleTree sapling_tree; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 7b683d0f23af..7583d3b4b2e6 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -14,13 +14,11 @@ #include "destination_io.h" #include "httpserver.h" #include "key_io.h" -#include "key_io.h" #include "masternode-sync.h" #include "net.h" #include "policy/feerate.h" #include "rpc/server.h" #include "sapling/sapling_operation.h" -#include "sapling/transaction_builder.h" #include "sapling/key_io_sapling.h" #include "spork.h" #include "timedata.h" From 8e9f2bcce6c3adb542a36a55d4734aa8c1329f7e Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 21 Jul 2021 17:11:51 -0300 Subject: [PATCH 13/13] Net_processing: Minimize tier two messages search time and improve code. Do not loop over every protocol message type, only walk through the tier two possible messages. --- src/net_processing.cpp | 42 ++++++++++++++++++------------------------ src/protocol.cpp | 8 +++++++- src/protocol.h | 8 ++++++-- 3 files changed, 31 insertions(+), 27 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 02a371e09e8f..8121f975153a 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -130,7 +130,7 @@ class CNodeBlocks // Compute the number of the received blocks size_t nBlocks = 0; - for(auto point : points) { + for (auto point : points) { nBlocks += point.second; } @@ -141,7 +141,7 @@ class CNodeBlocks bool banNode = (nAvgValue >= 1.5 * maxAvg && size >= maxAvg) || (nAvgValue >= maxAvg && nBlocks >= maxSize) || (nBlocks >= maxSize * 3); - if(banNode) { + if (banNode) { // Clear the points and ban the node points.clear(); return state.DoS(100, error("block-spam ban node for sending spam")); @@ -1874,29 +1874,23 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR pfrom->fRelayTxes = true; } - // Tier two msg type search - bool found = false; - const std::vector& allMessages = getAllNetMessageTypes(); - for (const std::string& msg : allMessages) { - if (msg == strCommand) { - found = true; - break; - } - } - - if (found) { - // Check if the dispatcher can process this message first. If not, try going with the old flow. - if (!masternodeSync.MessageDispatcher(pfrom, strCommand, vRecv)) { - //probably one the extensions - mnodeman.ProcessMessage(pfrom, strCommand, vRecv); - g_budgetman.ProcessMessage(pfrom, strCommand, vRecv); - masternodePayments.ProcessMessageMasternodePayments(pfrom, strCommand, vRecv); - sporkManager.ProcessSpork(pfrom, strCommand, vRecv); - masternodeSync.ProcessMessage(pfrom, strCommand, vRecv); + else { + // Tier two msg type search + const std::vector& allMessages = getTierTwoNetMessageTypes(); + if (std::find(allMessages.begin(), allMessages.end(), strCommand) != allMessages.end()) { + // Check if the dispatcher can process this message first. If not, try going with the old flow. + if (!masternodeSync.MessageDispatcher(pfrom, strCommand, vRecv)) { + // Probably one the extensions + mnodeman.ProcessMessage(pfrom, strCommand, vRecv); + g_budgetman.ProcessMessage(pfrom, strCommand, vRecv); + masternodePayments.ProcessMessageMasternodePayments(pfrom, strCommand, vRecv); + sporkManager.ProcessSpork(pfrom, strCommand, vRecv); + masternodeSync.ProcessMessage(pfrom, strCommand, vRecv); + } + } else { + // Ignore unknown commands for extensibility + LogPrint(BCLog::NET, "Unknown command \"%s\" from peer=%d\n", SanitizeString(strCommand), pfrom->id); } - } else { - // Ignore unknown commands for extensibility - LogPrint(BCLog::NET, "Unknown command \"%s\" from peer=%d\n", SanitizeString(strCommand), pfrom->id); } return true; diff --git a/src/protocol.cpp b/src/protocol.cpp index 1fdeb846588d..4a4379618f5d 100644 --- a/src/protocol.cpp +++ b/src/protocol.cpp @@ -80,7 +80,7 @@ const static std::string allNetMessageTypes[] = { "filtered block", // Should never occur "ix", // deprecated "txlvote", // deprecated - NetMsgType::SPORK, + NetMsgType::SPORK, // --- tiertwoNetMessageTypes start here --- NetMsgType::MNWINNER, "mnodescanerr", NetMsgType::BUDGETVOTE, @@ -98,6 +98,7 @@ const static std::string allNetMessageTypes[] = { NetMsgType::SYNCSTATUSCOUNT }; const static std::vector allNetMessageTypesVec(allNetMessageTypes, allNetMessageTypes + ARRAYLEN(allNetMessageTypes)); +const static std::vector tiertwoNetMessageTypesVec(std::find(allNetMessageTypesVec.begin(), allNetMessageTypesVec.end(), NetMsgType::SPORK), allNetMessageTypesVec.end()); CMessageHeader::CMessageHeader(const MessageStartChars& pchMessageStartIn) { @@ -226,3 +227,8 @@ const std::vector& getAllNetMessageTypes() { return allNetMessageTypesVec; } + +const std::vector& getTierTwoNetMessageTypes() +{ + return tiertwoNetMessageTypesVec; +} diff --git a/src/protocol.h b/src/protocol.h index 34e8363df852..41848b75adc3 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -267,6 +267,9 @@ extern const char* SYNCSTATUSCOUNT; /* Get a vector of all valid message types (see above) */ const std::vector& getAllNetMessageTypes(); +/* Get a vector of all tier two valid message types (see above) */ +const std::vector& getTierTwoNetMessageTypes(); + /** nServices flags */ enum ServiceFlags : uint64_t { // Nothing @@ -363,13 +366,14 @@ class CInv friend bool operator<(const CInv& a, const CInv& b); bool IsMasterNodeType() const; - std::string GetCommand() const; std::string ToString() const; // TODO: make private (improve encapsulation) -public: int type; uint256 hash; + +private: + std::string GetCommand() const; }; #endif // BITCOIN_PROTOCOL_H