Merge ElementsProject/elements#1030: [forward port] When validation is waiting for parent chain daemon, "stall"

b2e1cc38de Regression test for pegin validation issues during sync (Glenn Willen)
d5042b41c8 Finish removing 'recheckpeginblockinterval'; move MainchainRPCCheck (Glenn Willen)
313f73d5b2 When validation is waiting for parent chain daemon, "stall". (Andrew Poelstra)

Pull request description:

  Forward-port of #1022

ACKs for top commit:
  gwillen:
    utACK b2e1cc38de, verified that it contains only the requested changes from da11d7b6fd.

Tree-SHA512: 971b6a137efdc54f84995b17595346bf3d3ebd5a04eef1676225c664923e0b52393550bc51ea366c4d3010aa35a9de16da0643bb5bfb40c7eb788a180c99b1a0

src/init.cpp

@@ -627,7 +627,6 @@ void SetupServerArgs(NodeContext& node)
     argsman.AddArg("-mainchainrpccookiefile=<file>", "The bitcoind cookie auth path which the daemon will use to connect to the trusted mainchain daemon to validate peg-ins. (default: `<datadir>/regtest/.cookie`)", ArgsManager::ALLOW_ANY, OptionsCategory::ELEMENTS);
     argsman.AddArg("-mainchainrpctimeout=<n>", strprintf("Timeout in seconds during mainchain RPC requests, or 0 for no timeout. (default: %d)", DEFAULT_HTTP_CLIENT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::ELEMENTS);
     argsman.AddArg("-peginconfirmationdepth=<n>", strprintf("Pegin claims must be this deep to be considered valid. (default: %d)", DEFAULT_PEGIN_CONFIRMATION_DEPTH), ArgsManager::ALLOW_ANY, OptionsCategory::ELEMENTS);
-    argsman.AddArg("-recheckpeginblockinterval=<n>", strprintf("The interval in seconds at which a peg-in witness failing block is re-evaluated in case of intermittent peg-in witness failure. 0 means never. (default: %u)", 120), ArgsManager::ALLOW_ANY, OptionsCategory::ELEMENTS);
     argsman.AddArg("-parentpubkeyprefix", strprintf("The byte prefix, in decimal, of the parent chain's base58 pubkey address. (default: %d)", 111), ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
     argsman.AddArg("-parentscriptprefix", strprintf("The byte prefix, in decimal, of the parent chain's base58 script address. (default: %d)", 196), ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
     argsman.AddArg("-parent_bech32_hrp", strprintf("The human-readable part of the parent chain's bech32 encoding. (default: %s)", "bc"), ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
@@ -1299,6 +1298,61 @@ bool AppInitLockDataDirectory()
     return true;
 }
 
+/* This function checks that the RPC connection to the parent chain node
+ * can be attained, and is returning back reasonable answers.
+ */
+bool MainchainRPCCheck()
+{
+    // Check for working and valid rpc
+    // Retry until a non-RPC_IN_WARMUP result
+    while (true) {
+        try {
+            // The first thing we have to check is the version of the node.
+            UniValue params(UniValue::VARR);
+            UniValue reply = CallMainChainRPC("getnetworkinfo", params);
+            UniValue error = reply["error"];
+            if (!error.isNull()) {
+                // On the first call, it's possible to node is still in
+                // warmup; in that case, just wait and retry.
+                if (error["code"].get_int() == RPC_IN_WARMUP) {
+                    UninterruptibleSleep(std::chrono::milliseconds{1000});
+                    continue;
+                }
+                else {
+                    LogPrintf("ERROR: Mainchain daemon RPC check returned 'error' response.\n");
+                    return false;
+                }
+            }
+            UniValue result = reply["result"];
+            if (!result.isObject() || !result.get_obj()["version"].isNum() ||
+                    result.get_obj()["version"].get_int() < MIN_MAINCHAIN_NODE_VERSION) {
+                LogPrintf("ERROR: Parent chain daemon too old; need Bitcoin Core version 0.16.3 or newer.\n");
+                return false;
+            }
+
+            // Then check the genesis block to correspond to parent chain.
+            params.push_back(UniValue(0));
+            reply = CallMainChainRPC("getblockhash", params);
+            error = reply["error"];
+            if (!error.isNull()) {
+                LogPrintf("ERROR: Mainchain daemon RPC check returned 'error' response.\n");
+                return false;
+            }
+            result = reply["result"];
+            if (!result.isStr() || result.get_str() != Params().ParentGenesisBlockHash().GetHex()) {
+                LogPrintf("ERROR: Invalid parent genesis block hash response via RPC. Contacting wrong parent daemon?\n");
+                return false;
+            }
+        } catch (const std::runtime_error& re) {
+            LogPrintf("ERROR: Failure connecting to mainchain daemon RPC: %s\n", std::string(re.what()));
+            return false;
+        }
+
+        // Success
+        return true;
+    }
+}
+
 bool AppInitInterfaces(NodeContext& node)
 {
     node.chain = interfaces::MakeChain(node);
@@ -2079,29 +2133,34 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
     // ELEMENTS:
     if (gArgs.GetBoolArg("-validatepegin", Params().GetConsensus().has_parent_chain)) {
         uiInterface.InitMessage(_("Awaiting mainchain RPC warmup").translated);
-    }
-    if (!MainchainRPCCheck(true)) { //Initial check only
-        const std::string err_msg = "ERROR: elements is set to verify pegins but cannot get a valid response from the mainchain daemon. Please check debug.log for more information.\n\nIf you haven't setup a bitcoind please get the latest stable version from https://bitcoincore.org/en/download/ or if you do not need to validate pegins set in your elements configuration validatepegin=0";
-        // We fail immediately if this node has RPC server enabled
-        if (gArgs.GetBoolArg("-server", false)) {
-            InitError(Untranslated(err_msg));
-            return false;
-        } else {
-            // Or gently warn the user, and continue
-            InitError(Untranslated(err_msg));
-            gArgs.SoftSetArg("-validatepegin", "0");
+        if (!MainchainRPCCheck()) {
+            const std::string err_msg = "ERROR: elements is set to verify pegins but cannot get a valid response from the mainchain daemon. Please check debug.log for more information.\n\nIf you haven't setup a bitcoind please get the latest stable version from https://bitcoincore.org/en/download/ or if you do not need to validate pegins set in your elements configuration validatepegin=0";
+            // We fail immediately if this node has RPC server enabled
+            if (gArgs.GetBoolArg("-server", false)) {
+                InitError(Untranslated(err_msg));
+                return false;
+            } else {
+                // Or gently warn the user, and continue
+                InitError(Untranslated(err_msg));
+                gArgs.SoftSetArg("-validatepegin", "0");
+            }
         }
     }
 
-    // Start the lightweight block re-evaluation scheduler thread
-    CScheduler::Function reevaluationLoop = [&node]{ node.reverification_scheduler->serviceQueue(); };
-    threadGroup.create_thread(std::bind(&TraceThread<CScheduler::Function>, "reevaluation_scheduler", reevaluationLoop));
-
-    CScheduler::Function f2 = std::bind(&MainchainRPCCheck, false);
-    unsigned int check_rpc_every = gArgs.GetArg("-recheckpeginblockinterval", 120);
-    if (check_rpc_every) {
-        node.reverification_scheduler->scheduleEvery(f2, std::chrono::seconds(check_rpc_every));
-    }
+    // Call ActivateBestChain every 30 seconds. This is almost always a
+    // harmless no-op. It is necessary in the unusual case where:
+    // (1) Our connection to bitcoind is lost, and
+    // (2) we build up a queue of blocks to validate in the meantime, and then
+    // (3) our connection to bitcoind is restored, but
+    // (4) nothing after that causes ActivateBestChain to be called, including
+    //     no further blocks arriving for us to validate.
+    // Unfortunately, this unusual case happens in the functional test suite.
+    node.reverification_scheduler->scheduleEvery([]{
+        BlockValidationState state;
+        if (!ActivateBestChain(state, Params())) {
+            LogPrintf("Failed to periodically activate best chain (%s)\n", state.ToString());
+        }
+    }, std::chrono::seconds{30});
 
     uiInterface.InitMessage(_("Done loading").translated);
 

src/mainchainrpc.cpp

@@ -153,18 +153,24 @@ UniValue CallMainChainRPC(const std::string& strMethod, const UniValue& params)
 
 bool IsConfirmedBitcoinBlock(const uint256& hash, const int nMinConfirmationDepth, const int nbTxs)
 {
+    LogPrintf("Checking for confirmed bitcoin block with hash %s, mindepth %d, nbtxs %d\n", hash.ToString().c_str(), nMinConfirmationDepth, nbTxs);
     try {
         UniValue params(UniValue::VARR);
         params.push_back(hash.GetHex());
         UniValue reply = CallMainChainRPC("getblockheader", params);
-        if (!find_value(reply, "error").isNull())
+        if (!find_value(reply, "error").isNull()) {
+            LogPrintf("ERROR: Got error reply from bitcoind getblockheader.\n");
             return false;
+        }
         UniValue result = find_value(reply, "result");
-        if (!result.isObject())
+        if (!result.isObject()) {
+            LogPrintf("ERROR: bitcoind getblockheader result was malformed (not object).\n");
             return false;
+        }
 
         UniValue confirmations = find_value(result.get_obj(), "confirmations");
         if (!confirmations.isNum() || confirmations.get_int64() < nMinConfirmationDepth) {
+            LogPrintf("Insufficient confirmations (got %s).\n", confirmations.write());
             return false;
         }
 

src/pegins.cpp

@@ -240,7 +240,11 @@ bool CheckParentProofOfWork(uint256 hash, unsigned int nBits, const Consensus::P
     return true;
 }
 
-bool IsValidPeginWitness(const CScriptWitness& pegin_witness, const std::vector<std::pair<CScript, CScript>>& fedpegscripts, const COutPoint& prevout, std::string& err_msg, bool check_depth) {
+bool IsValidPeginWitness(const CScriptWitness& pegin_witness, const std::vector<std::pair<CScript, CScript>>& fedpegscripts, const COutPoint& prevout, std::string& err_msg, bool check_depth, bool* depth_failed) {
+    if (depth_failed) {
+        *depth_failed = false;
+    }
+
     // 0) Return false if !consensus.has_parent_chain
     if (!Params().GetConsensus().has_parent_chain) {
         err_msg = "Parent chain is not enabled on this network.";
@@ -372,6 +376,9 @@ bool IsValidPeginWitness(const CScriptWitness& pegin_witness, const std::vector<
         LogPrintf("Required depth: %d\n", required_depth);
         if (!IsConfirmedBitcoinBlock(block_hash, required_depth, num_txs)) {
             err_msg = "Needs more confirmations.";
+            if (depth_failed) {
+                *depth_failed = true;
+            }
             return false;
         }
     }

src/pegins.h

@@ -23,7 +23,7 @@ bool GetAmountFromParentChainPegin(CAmount& amount, const CTransaction& txBTC, u
 /** Check whether a parent chain block hash satisfies the proof-of-work requirement specified by nBits */
 bool CheckParentProofOfWork(uint256 hash, unsigned int nBits, const Consensus::Params&);
 /** Checks pegin witness for validity */
-bool IsValidPeginWitness(const CScriptWitness& pegin_witness, const std::vector<std::pair<CScript, CScript>>& fedpegscripts, const COutPoint& prevout, std::string& err_msg, bool check_depth);
+bool IsValidPeginWitness(const CScriptWitness& pegin_witness, const std::vector<std::pair<CScript, CScript>>& fedpegscripts, const COutPoint& prevout, std::string& err_msg, bool check_depth, bool* depth_failed = nullptr);
 
 /* Consensus-critical. Matching against telescoped multisig used on Liquid v1:
  * Pseudo-structure:

src/rpc/rawtransaction_util.cpp

@@ -545,8 +545,9 @@ bool ValidateTransactionPeginInputs(const CMutableTransaction& mtx, std::map<int
             continue;
         }
         // Report warning about immature peg-in though
-        if(txin.m_is_pegin && !IsValidPeginWitness(mtx.witness.vtxinwit[i].m_pegin_witness, fedpegscripts, txin.prevout, err, true)) {
-            CHECK_NONFATAL(err == "Needs more confirmations.");
+        bool depth_failed = false;
+        if(txin.m_is_pegin && !IsValidPeginWitness(mtx.witness.vtxinwit[i].m_pegin_witness, fedpegscripts, txin.prevout, err, true, &depth_failed)) {
+            CHECK_NONFATAL(depth_failed);
             immature_pegin = true;
         }
     }

src/txdb.cpp

@@ -33,7 +33,7 @@ static const char DB_LAST_BLOCK = 'l';
 
 // ELEMENTS:
 static const char DB_PEGIN_FLAG = 'w';
-static const char DB_INVALID_BLOCK_Q = 'q';
+// static const char DB_INVALID_BLOCK_Q = 'q';  // No longer used, but avoid reuse.
 static const char DB_PAK = 'p';
 
 namespace {
@@ -269,14 +269,6 @@ bool CBlockTreeDB::ReadFlag(const std::string &name, bool &fValue) {
     return true;
 }
 
-// ELEMENTS:
-bool CBlockTreeDB::ReadInvalidBlockQueue(std::vector<uint256> &vBlocks) {
-    return Read(std::make_pair(DB_INVALID_BLOCK_Q, uint256S("0")), vBlocks);//FIXME: why uint 56 and not ""
-}
-bool CBlockTreeDB::WriteInvalidBlockQueue(const std::vector<uint256> &vBlocks) {
-    return Write(std::make_pair(DB_INVALID_BLOCK_Q, uint256S("0")), vBlocks);
-}
-
 bool CBlockTreeDB::ReadPAKList(std::vector<std::vector<unsigned char> >& offline_list, std::vector<std::vector<unsigned char> >& online_list, bool& reject)
 {
         return Read(std::make_pair(DB_PAK, uint256S("1")), offline_list) && Read(std::make_pair(DB_PAK, uint256S("2")), online_list) && Read(std::make_pair(DB_PAK, uint256S("3")), reject);

src/txdb.h

@@ -109,8 +109,6 @@ public:
     bool ReadFlag(const std::string &name, bool &fValue);
     bool LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function<CBlockIndex*(const uint256&)> insertBlockIndex);
     // ELEMENTS:
-    bool ReadInvalidBlockQueue(std::vector<uint256> &vBlocks);
-    bool WriteInvalidBlockQueue(const std::vector<uint256> &vBlocks);
     bool ReadPAKList(std::vector<std::vector<unsigned char> >& offline_list, std::vector<std::vector<unsigned char> >& online_list, bool& reject);
     bool WritePAKList(const std::vector<std::vector<unsigned char> >& offline_list, const std::vector<std::vector<unsigned char> >& online_list, bool reject);
 };

src/validation.cpp

@@ -2083,6 +2083,29 @@ static int64_t nTimeCallbacks = 0;
 static int64_t nTimeTotal = 0;
 static int64_t nBlocksTotal = 0;
 
+bool CheckPeginRipeness(const CBlock& block, const std::vector<std::pair<CScript, CScript>>& fedpegscripts) {
+    for (unsigned int i = 0; i < block.vtx.size(); i++) {
+        const CTransaction &tx = *(block.vtx[i]);
+
+        if (!tx.IsCoinBase()) {
+            for (unsigned int i = 0; i < tx.vin.size(); ++i) {
+                if (tx.vin[i].m_is_pegin) {
+                    std::string err;
+                    bool depth_failed = false;
+                    if ((tx.witness.vtxinwit.size() <= i) || !IsValidPeginWitness(tx.witness.vtxinwit[i].m_pegin_witness, fedpegscripts, tx.vin[i].prevout, err, true, &depth_failed)) {
+                        if (depth_failed) {
+                            return false;  // Pegins not ripe.
+                        } else {
+                            return true;  // Some other failure; details later.
+                        }
+                    }
+                }
+            }
+        }
+    }
+    return true;
+}
+
 /** Apply the effects of this block (with given index) on the UTXO set represented by coins.
  *  Validity checks that depend on the UTXO set are also done; ConnectBlock()
  *  can fail if those validity checks fail (among other reasons). */
@@ -2820,7 +2843,7 @@ public:
  *
  * The block is added to connectTrace if connection succeeds.
  */
-bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions &disconnectpool)
+bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions &disconnectpool, bool& fStall)
 {
     AssertLockHeld(cs_main);
     AssertLockHeld(m_mempool.cs);
@@ -2838,6 +2861,14 @@ bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& ch
         pthisBlock = pblock;
     }
     const CBlock& blockConnecting = *pthisBlock;
+
+    const auto& fedpegscripts = GetValidFedpegScripts(pindexNew, chainparams.GetConsensus(), false /* nextblock_validation */);
+    if (!CheckPeginRipeness(blockConnecting, fedpegscripts)) {
+        LogPrintf("STALLING further progress in ConnectTip while waiting for parent chain daemon to catch up! Chain will not grow until this is remedied!\n");
+        fStall = true;
+        return true;
+    }
+
     // Apply the block atomically to the chain state.
     int64_t nTime2 = GetTimeMicros(); nTimeReadFromDisk += nTime2 - nTime1;
     int64_t nTime3;
@@ -2854,29 +2885,6 @@ bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& ch
         if (!rv) {
             if (state.IsInvalid()) {
                 InvalidBlockFound(pindexNew, state);
-
-                // ELEMENTS:
-                // Possibly result of RPC to mainchain bitcoind failure
-                // or unseen Bitcoin blocks.
-                // These blocks are later re-evaluated at an interval
-                // set by `-recheckpeginblockinterval`.
-                if (state.GetRejectReason() == "bad-pegin-witness") {
-                    //Write queue of invalid blocks that
-                    //must be cleared to continue operation
-                    std::vector<uint256> vinvalidBlocks;
-                    pblocktree->ReadInvalidBlockQueue(vinvalidBlocks);
-                    bool blockAlreadyInvalid = false;
-                    for (uint256& hash : vinvalidBlocks) {
-                        if (hash == blockConnecting.GetHash()) {
-                            blockAlreadyInvalid = true;
-                            break;
-                        }
-                    }
-                    if (!blockAlreadyInvalid) {
-                        vinvalidBlocks.push_back(blockConnecting.GetHash());
-                        pblocktree->WriteInvalidBlockQueue(vinvalidBlocks);
-                    }
-                }
             }
             return error("%s: ConnectBlock %s failed, %s", __func__, pindexNew->GetBlockHash().ToString(), state.ToString());
         }
@@ -2988,7 +2996,7 @@ void CChainState::PruneBlockIndexCandidates() {
  *
  * @returns true unless a system error occurred
  */
-bool CChainState::ActivateBestChainStep(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace)
+bool CChainState::ActivateBestChainStep(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace, bool& fStall)
 {
     AssertLockHeld(cs_main);
     AssertLockHeld(m_mempool.cs);
@@ -3033,7 +3041,7 @@ bool CChainState::ActivateBestChainStep(BlockValidationState& state, const CChai
 
         // Connect new blocks.
         for (CBlockIndex *pindexConnect : reverse_iterate(vpindexToConnect)) {
-            if (!ConnectTip(state, chainparams, pindexConnect, pindexConnect == pindexMostWork ? pblock : std::shared_ptr<const CBlock>(), connectTrace, disconnectpool)) {
+            if (!ConnectTip(state, chainparams, pindexConnect, pindexConnect == pindexMostWork ? pblock : std::shared_ptr<const CBlock>(), connectTrace, disconnectpool, fStall)) {
                 if (state.IsInvalid()) {
                     // The block violates a consensus rule.
                     if (state.GetResult() != BlockValidationResult::BLOCK_MUTATED) {
@@ -3051,6 +3059,12 @@ bool CChainState::ActivateBestChainStep(BlockValidationState& state, const CChai
                     return false;
                 }
             } else {
+                if (fStall) {
+                    // We didn't make progress because the parent chain is not
+                    // synced enough to check pegins. Try again later.
+                    fContinue = false;
+                    break;
+                }
                 PruneBlockIndexCandidates();
                 if (!pindexOldTip || m_chain.Tip()->nChainWork > pindexOldTip->nChainWork) {
                     // We're in a better position than we were. Return temporarily to release the lock.
@@ -3130,6 +3144,8 @@ bool CChainState::ActivateBestChain(BlockValidationState &state, const CChainPar
     CBlockIndex *pindexMostWork = nullptr;
     CBlockIndex *pindexNewTip = nullptr;
     int nStopAtHeight = gArgs.GetArg("-stopatheight", DEFAULT_STOPATHEIGHT);
+    bool fStall = false;
+
     do {
         // Block until the validation queue drains. This should largely
         // never happen in normal operation, however may happen during
@@ -3160,7 +3176,7 @@ bool CChainState::ActivateBestChain(BlockValidationState &state, const CChainPar
 
                 bool fInvalidFound = false;
                 std::shared_ptr<const CBlock> nullBlockPtr;
-                if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace)) {
+                if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace, fStall)) {
                     // A system error occurred
                     return false;
                 }
@@ -3176,6 +3192,11 @@ bool CChainState::ActivateBestChain(BlockValidationState &state, const CChainPar
                     assert(trace.pblock && trace.pindex);
                     GetMainSignals().BlockConnected(trace.pblock, trace.pindex);
                 }
+
+                if (fStall) {
+                    // Stuck waiting for parent chain daemon, twiddle our thumbs for awhile.
+                    break;
+                }
             } while (!m_chain.Tip() || (starting_tip && CBlockIndexWorkComparator()(m_chain.Tip(), starting_tip)));
             if (!blocks_connected) return true;
 
@@ -3194,6 +3215,11 @@ bool CChainState::ActivateBestChain(BlockValidationState &state, const CChainPar
         }
         // When we reach this point, we switched to a new tip (stored in pindexNewTip).
 
+        if (fStall) {
+            // Stuck waiting for parent chain daemon, twiddle our thumbs for awhile.
+            break;
+        }
+
         if (nStopAtHeight && pindexNewTip && pindexNewTip->nHeight >= nStopAtHeight) StartShutdown();
 
         // We check shutdown only after giving ActivateBestChainStep a chance to run once so that we
@@ -5709,131 +5735,3 @@ void ChainstateManager::MaybeRebalanceCaches()
         }
     }
 }
-
-// ELEMENTS:
-/* 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 MainchainRPCCheck(const bool init)
-{
-    // First, we can clear out any blocks thatsomehow are now deemed valid
-    // eg reconsiderblock rpc call manually
-    std::vector<uint256> vblocksToReconsider;
-    pblocktree->ReadInvalidBlockQueue(vblocksToReconsider);
-    std::vector<uint256> vblocksToReconsiderAgain;
-    for(uint256& blockhash : vblocksToReconsider) {
-        LOCK(cs_main);
-        ChainstateManager& chainman = g_chainman;
-        if (chainman.BlockIndex().count(blockhash)) {
-            CBlockIndex* pblockindex = chainman.BlockIndex()[blockhash];
-            if ((pblockindex->nStatus & BLOCK_FAILED_MASK)) {
-                vblocksToReconsiderAgain.push_back(blockhash);
-            }
-        }
-    }
-    vblocksToReconsider = vblocksToReconsiderAgain;
-    vblocksToReconsiderAgain.clear();
-    pblocktree->WriteInvalidBlockQueue(vblocksToReconsider);
-
-    // Next, check for working and valid rpc
-    if (gArgs.GetBoolArg("-validatepegin", Params().GetConsensus().has_parent_chain)) {
-        // During init try until a non-RPC_IN_WARMUP result
-        while (true) {
-            try {
-                // The first thing we have to check is the version of the node.
-                UniValue params(UniValue::VARR);
-                UniValue reply = CallMainChainRPC("getnetworkinfo", params);
-                UniValue error = reply["error"];
-                if (!error.isNull()) {
-                    // On the first call, it's possible to node is still in
-                    // warmup; in that case, just wait and retry.
-                    // If this is not the initial call, just report failure.
-                    if (init && error["code"].get_int() == RPC_IN_WARMUP) {
-                        UninterruptibleSleep(std::chrono::milliseconds{1000});
-                        continue;
-                    }
-                    else {
-                        LogPrintf("ERROR: Mainchain daemon RPC check returned 'error' response.\n");
-                        return false;
-                    }
-                }
-                UniValue result = reply["result"];
-                if (!result.isObject() || !result.get_obj()["version"].isNum() ||
-                        result.get_obj()["version"].get_int() < MIN_MAINCHAIN_NODE_VERSION) {
-                    LogPrintf("ERROR: Parent chain daemon too old; need Bitcoin Core version 0.16.3 or newer.\n");
-                    return false;
-                }
-
-                // Then check the genesis block to correspond to parent chain.
-                params.push_back(UniValue(0));
-                reply = CallMainChainRPC("getblockhash", params);
-                error = reply["error"];
-                if (!error.isNull()) {
-                    LogPrintf("ERROR: Mainchain daemon RPC check returned 'error' response.\n");
-                    return false;
-                }
-                result = reply["result"];
-                if (!result.isStr() || result.get_str() != Params().ParentGenesisBlockHash().GetHex()) {
-                    LogPrintf("ERROR: Invalid parent genesis block hash response via RPC. Contacting wrong parent daemon?\n");
-                    return false;
-                }
-            } catch (const std::runtime_error& re) {
-                LogPrintf("ERROR: Failure connecting to mainchain daemon RPC: %s\n", std::string(re.what()));
-                return false;
-            }
-            // Success
-            break;
-        }
-    }
-
-    //Sanity startup check won't reconsider queued blocks
-    if (init) {
-       return true;
-    }
-
-    // Getting this far means we either aren't validating pegins(so let's make sure that's why
-    // it failed previously) or we successfully connected to bitcoind
-    // Time to reconsider blocks
-    if (vblocksToReconsider.size() > 0) {
-        BlockValidationState state;
-        for(const uint256& blockhash : vblocksToReconsider) {
-            {
-                LOCK(cs_main);
-                ChainstateManager& chainman = g_chainman;
-                if (chainman.BlockIndex().count(blockhash) == 0)
-                    continue;
-                CBlockIndex* pblockindex = chainman.BlockIndex()[blockhash];
-                ResetBlockFailureFlags(pblockindex);
-            }
-        }
-
-        //All blocks are now being reconsidered
-        ActivateBestChain(state, Params());
-        //This simply checks for DB errors
-        if (!state.IsValid()) {
-            //Something scary?
-        }
-
-        //Now to clear out now-valid blocks
-        for(const uint256& blockhash : vblocksToReconsider) {
-            LOCK(cs_main);
-            ChainstateManager& chainman = g_chainman;
-            if (chainman.BlockIndex().count(blockhash)) {
-                CBlockIndex* pblockindex = chainman.BlockIndex()[blockhash];
-
-                //Marked as invalid still, put back into queue
-                if((pblockindex->nStatus & BLOCK_FAILED_MASK)) {
-                    vblocksToReconsiderAgain.push_back(blockhash);
-                }
-            }
-        }
-
-        //Write back remaining blocks
-        pblocktree->WriteInvalidBlockQueue(vblocksToReconsiderAgain);
-    }
-    return true;
-}

src/validation.h

@@ -719,8 +719,8 @@ public:
     std::string ToString() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
 private:
-    bool ActivateBestChainStep(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool.cs);
-    bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool.cs);
+    bool ActivateBestChainStep(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace, bool& fStall) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool.cs);
+    bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool, bool& fStall) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool.cs);
 
     void InvalidBlockFound(CBlockIndex *pindex, const BlockValidationState &state) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
     CBlockIndex* FindMostWorkChain() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
@@ -983,8 +983,4 @@ inline bool IsBlockPruned(const CBlockIndex* pblockindex)
     return (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
 }
 
-// ELEMENTS:
-/** Check if bitcoind connection via RPC is correctly working*/
-bool MainchainRPCCheck(bool init);
-
 #endif // BITCOIN_VALIDATION_H

test/functional/feature_fedpeg.py

@@ -120,13 +120,13 @@ class FedPegTest(BitcoinTestFramework):
                 '-peginconfirmationdepth=10',
                 '-mainchainrpchost=127.0.0.1',
                 '-mainchainrpcport=%s' % rpc_port(n),
-                '-recheckpeginblockinterval=15', # Long enough to allow failure and repair before timeout
                 '-parentgenesisblockhash=%s' % self.parentgenesisblockhash,
                 '-parentpubkeyprefix=111',
                 '-parentscriptprefix=196',
                 '-parent_bech32_hrp=bcrt',
                 # Turn of consistency checks that can cause assert when parent node stops
                 # and a peg-in transaction fails this belt-and-suspenders check.
+                # NOTE: This can cause spurious problems in regtest, and should be dealt with in a better way.
                 '-checkmempool=0',
             ]
             if not self.options.parent_bitcoin:
@@ -541,9 +541,12 @@ class FedPegTest(BitcoinTestFramework):
         self.start_node(1)
         self.connect_nodes(0, 1)
 
-        # Don't make a block, race condition when pegin-invalid block
-        # is awaiting further validation, nodes reject subsequent blocks
-        # even ones they create
+        # Make a bunch of blocks while catching up, as a regression test for
+        # https://github.com/ElementsProject/elements/issues/891 (sporadic
+        # failures when catching up after loss of parent daemon connectivity.)
+        print("Generating some blocks, to stress-test handling of parent daemon reconnection")
+        sidechain.generate(10)
+
         print("Now waiting for node to re-evaluate peg-in witness failed block... should take a few seconds")
         for node_group in self.node_groups:
             self.sync_all(node_group)