Decouple peer-processing-logic from block-connection-logic + BIP61 removal.

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

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=<category>`) to a stream (`-printtoconsole`) or to a file
+  (`-debuglogfile=<debug.log>`).
+
+* 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)
 
 

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 \

src/blockassembler.cpp

@@ -31,7 +31,6 @@
 
 #include <algorithm>
 #include <boost/thread.hpp>
-#include <queue>
 
 // 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

src/miner.cpp

@@ -12,20 +12,19 @@
 
 #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"
 #include "policy/feerate.h"
 #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<const CBlock>& 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");
     }
 

src/net.cpp

@@ -417,15 +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);
-        g_connman->PushMessage(this, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strLastCommand, REJECT_OBSOLETE, strprintf("Version must be %d or greater", ActiveProtocol())));
         fDisconnect = true;
     }
-
     return fDisconnect;
 }
 

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);