Merge #425: peg-in re-checking cleanup and fixes

abb7b01 Add some additional comments about BitcoindRPCCheck infra (Gregory Sanders)
62c56f4 don't set CorruptionPossible for peg-in transaction failure (Gregory Sanders)
5d7cd6b parameterize the rpc re-checking of peg-in validation failed blocks (Gregory Sanders)
ba8d498 bitcoind rpc check should take main locks for mapBlockIndex, check for key (Gregory Sanders)

Author: instagibbs

qa/rpc-tests/feature_fedpeg.py

@@ -97,6 +97,7 @@ def setup_network(self, split=False):
                 '-peginconfirmationdepth=10',
                 '-mainchainrpchost=127.0.0.1',
                 '-mainchainrpcport=%s' % rpc_port(n),
+                '-recheckpeginblockinterval=15', # Long enough to allow failure and repair before timeout
             ]
             if not self.options.parent_bitcoin:
                 args.extend([
@@ -319,12 +320,13 @@ def run_test(self):
         self.nodes[1] = start_node(1, self.options.tmpdir, self.extra_args[1], binary=self.binary, chain=self.parent_chain, cookie_auth=True)
         parent2 = self.nodes[1]
         connect_nodes_bi(self.nodes, 0, 1)
-        time.sleep(5)
 
         # Don't make a block, race condition when pegin-invalid block
         # is awaiting further validation, nodes reject subsequent blocks
         # even ones they create
+        print("Now waiting for node to re-evaluate peg-in witness failed block... should take a few seconds")
         self.sync_all()
+        print("Completed!\n")
         print("Now send funds out in two stages, partial, and full")
         some_btc_addr = get_new_unconfidential_address(parent)
         bal_1 = sidechain.getwalletinfo()["balance"]["bitcoin"]

src/init.cpp

@@ -520,6 +520,7 @@ std::string HelpMessage(HelpMessageMode mode)
         strUsage += HelpMessageOpt("-parentscriptprefix", strprintf(_("The byte prefix, in decimal, of the parent chain's base58 script address. (default: %d)"), 196));
         strUsage += HelpMessageOpt("-con_parent_chain_signblockscript", _("Whether parent chain uses pow or signed blocks. If the parent chain uses signed blocks, the challenge (scriptPubKey) script. If not, an empty string. (default: empty script [ie parent uses pow])"));
         strUsage += HelpMessageOpt("-con_parent_pegged_asset=<hex>", _("Asset ID (hex) for pegged asset for when parent chain has CA. (default: 0x00)"));
+        strUsage += HelpMessageOpt("-recheckpeginblockinterval", strprintf(_("The interval in seconds at which a peg-in witness failing block is re-evaluated in case of intermittant peg-in witness failure. 0 means never. (default: %u)"), 120));
     }
     strUsage += HelpMessageOpt("-validatepegin", strprintf(_("Validate peg-in claims. An RPC connection will be attempted to the trusted bitcoind using the `mainchain*` settings below. All functionaries must run this enabled. (default: %u)"), DEFAULT_VALIDATE_PEGIN));
     strUsage += HelpMessageOpt("-mainchainrpchost=<addr>", strprintf("The address which the daemon will try to connect to the trusted bitcoind to validate peg-ins, if enabled. (default: cookie auth)"));
@@ -1691,7 +1692,10 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
     SetRPCWarmupFinished();
 
     CScheduler::Function f2 = boost::bind(&BitcoindRPCCheck, false);
-    scheduler.scheduleEvery(f2, 120);
+    unsigned int check_rpc_every = GetArg("-recheckpeginblockinterval", 120);
+    if (check_rpc_every) {
+        scheduler.scheduleEvery(f2, check_rpc_every);
+    }
 
     uiInterface.InitMessage(_("Awaiting bitcoind RPC warmup"));
 

src/validation.cpp

@@ -1870,7 +1870,7 @@ bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoins
             if (tx.vin[i].m_is_pegin) {
                 // Check existence and validity of pegin witness
                 if (tx.wit.vtxinwit.size() <= i || !IsValidPeginWitness(tx.wit.vtxinwit[i].m_pegin_witness, prevout)) {
-                    return state.DoS(0, false, REJECT_PEGIN, "bad-pegin-witness", true);
+                    return state.DoS(0, false, REJECT_PEGIN, "bad-pegin-witness");
                 }
                 std::pair<uint256, COutPoint> pegin = std::make_pair(uint256(tx.wit.vtxinwit[i].m_pegin_witness.stack[2]), prevout);
                 if (inputs.IsWithdrawSpent(pegin)) {
@@ -2212,6 +2212,13 @@ void ThreadScriptCheck() {
     scriptcheckqueue.Thread();
 }
 
+/* This function has two major purposes:
+ * 1) Checks that the RPC connection to the parent chain node
+ * can be attained, and is returning back reasonable answers.
+ * 2) Re-evaluates a list of blocks that have been deemed "bad"
+ * from the perspective of peg-in witness validation. Blocks are
+ * added to this queue in ConnectTip based on the error code returned.
+ */
 bool BitcoindRPCCheck(const bool init)
 {
     //First, we can clear out any blocks thatsomehow are now deemed valid
@@ -2220,9 +2227,12 @@ bool BitcoindRPCCheck(const bool init)
     pblocktree->ReadInvalidBlockQueue(vblocksToReconsider);
     std::vector<uint256> vblocksToReconsiderAgain;
     BOOST_FOREACH(uint256& blockhash, vblocksToReconsider) {
-        CBlockIndex* pblockindex = mapBlockIndex[blockhash];
-        if ((pblockindex->nStatus & BLOCK_FAILED_MASK)) {
-            vblocksToReconsiderAgain.push_back(blockhash);
+        LOCK(cs_main);
+        if (mapBlockIndex.count(blockhash)) {
+            CBlockIndex* pblockindex = mapBlockIndex[blockhash];
+            if ((pblockindex->nStatus & BLOCK_FAILED_MASK)) {
+                vblocksToReconsiderAgain.push_back(blockhash);
+            }
         }
     }
     vblocksToReconsider = vblocksToReconsiderAgain;
@@ -2312,11 +2322,14 @@ bool BitcoindRPCCheck(const bool init)
 
         //Now to clear out now-valid blocks
         BOOST_FOREACH(const uint256& blockhash, vblocksToReconsider) {
-            CBlockIndex* pblockindex = mapBlockIndex[blockhash];
+            LOCK(cs_main);
+            if (mapBlockIndex.count(blockhash)) {
+                CBlockIndex* pblockindex = mapBlockIndex[blockhash];
 
-            //Marked as invalid still, put back into queue
-            if((pblockindex->nStatus & BLOCK_FAILED_MASK)) {
-                vblocksToReconsiderAgain.push_back(blockhash);
+                //Marked as invalid still, put back into queue
+                if((pblockindex->nStatus & BLOCK_FAILED_MASK)) {
+                    vblocksToReconsiderAgain.push_back(blockhash);
+                }
             }
         }
 
@@ -3224,8 +3237,10 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams,
         if (!rv) {
             if (state.IsInvalid()) {
                 InvalidBlockFound(pindexNew, state);
-                //Possibly result of RPC to bitcoind failure
-                //or unseen Bitcoin blocks.
+                // Possibly result of RPC to bitcoind failure
+                // or unseen Bitcoin blocks.
+                // These blocks are later re-evaluated at an interval
+                // set by `-recheckpeginblockinterval`.
                 if (state.GetRejectCode() == REJECT_PEGIN) {
                     //Write queue of invalid blocks that
                     //must be cleared to continue operation