Merge #1022: When validation is waiting for parent chain daemon, "stall".

e5ba4e8 Regression test for pegin validation issues during sync (Glenn Willen)
7f79f72 Finish removing 'recheckpeginblockinterval'; move MainchainRPCCheck (Glenn Willen)
4ba4e07 When validation is waiting for parent chain daemon, "stall". (Glenn Willen)

Pull request description:

  Currently, if -validatepegin is given, and block validation can't proceed
  because the parent chain is not synced, we mark the block invalid and put
  it in a queue to be "revalidated" later. Unfortunately, marking a block
  invalid has downstream consequences, in particular causing descendant blocks
  to be marked invalid, which are not currently fixed by the queue.

  Instead, we'll use a different strategy: if the mainchain daemon isn't
  sufficiently synced to validate a block, we will "stall" connecting that
  block to the chain, and have ActivateBestChain simply keep the tip at the
  previous block until we're ready.

  We can still download and validate (partly) blocks past this point while
  we're waiting. They will be connected once the parent chain daemon catches
  up.

Top commit has no ACKs.

Tree-SHA512: 0aa8d22460af062509f431e0f742297733d0e20b3fc1babca35f25a07c5243c7665dd0ba5fb439e3782c842a264dacd14cee6da49f8df3d1a26f5391c4742538

Author: apoelstra

doc/man/elements-qt.1

@@ -854,12 +854,6 @@ Entries in the PAK list. Order of entries matter.
 .IP
 Pegin claims must be this deep to be considered valid. (default: 8)
 .HP
-\fB\-recheckpeginblockinterval=\fR<n>
-.IP
-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: 120)
-.HP
 \fB\-validatepegin\fR
 .IP
 Validate peg\-in claims. An RPC connection will be attempted to the

doc/man/elementsd.1

@@ -828,12 +828,6 @@ Entries in the PAK list. Order of entries matter.
 .IP
 Pegin claims must be this deep to be considered valid. (default: 8)
 .HP
-\fB\-recheckpeginblockinterval=\fR<n>
-.IP
-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: 120)
-.HP
 \fB\-validatepegin\fR
 .IP
 Validate peg\-in claims. An RPC connection will be attempted to the

src/init.cpp

@@ -582,7 +582,6 @@ void SetupServerArgs()
     gArgs.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`)", false, OptionsCategory::ELEMENTS);
     gArgs.AddArg("-mainchainrpctimeout=<n>", strprintf("Timeout in seconds during mainchain RPC requests, or 0 for no timeout. (default: %d)", DEFAULT_HTTP_CLIENT_TIMEOUT), false, OptionsCategory::ELEMENTS);
     gArgs.AddArg("-peginconfirmationdepth=<n>", strprintf("Pegin claims must be this deep to be considered valid. (default: %d)", DEFAULT_PEGIN_CONFIRMATION_DEPTH), false, OptionsCategory::ELEMENTS);
-    gArgs.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), false, OptionsCategory::ELEMENTS);
     gArgs.AddArg("-parentpubkeyprefix", strprintf("The byte prefix, in decimal, of the parent chain's base58 pubkey address. (default: %d)", 111), false, OptionsCategory::CHAINPARAMS);
     gArgs.AddArg("-parentscriptprefix", strprintf("The byte prefix, in decimal, of the parent chain's base58 script address. (default: %d)", 196), false, OptionsCategory::CHAINPARAMS);
     gArgs.AddArg("-parent_bech32_hrp", strprintf("The human-readable part of the parent chain's bech32 encoding. (default: %s)", "bc"), false, OptionsCategory::CHAINPARAMS);
@@ -1279,6 +1278,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) {
+                    MilliSleep(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 AppInitMain(InitInterfaces& interfaces)
 {
     const CChainParams& chainparams = Params();
@@ -1879,29 +1933,34 @@ bool AppInitMain(InitInterfaces& interfaces)
     // ELEMENTS:
     if (gArgs.GetBoolArg("-validatepegin", Params().GetConsensus().has_parent_chain)) {
         uiInterface.InitMessage(_("Awaiting mainchain RPC warmup"));
-    }
-    if (!MainchainRPCCheck(true)) { //Initial check only
-        const std::string err_msg = "ERROR: elements is set to verify pegins but cannot get 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(err_msg);
-            return false;
-        } else {
-            // Or gently warn the user, and continue
-            InitWarning(err_msg);
-            gArgs.SoftSetArg("-validatepegin", "0");
+        if (!MainchainRPCCheck()) {
+            const std::string err_msg = "ERROR: elements is set to verify pegins but cannot get 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(err_msg);
+                return false;
+            } else {
+                // Or gently warn the user, and continue
+                InitWarning(err_msg);
+                gArgs.SoftSetArg("-validatepegin", "0");
+            }
         }
     }
 
-    // Start the lightweight block re-evaluation scheduler thread
-    CScheduler::Function reevaluationLoop = std::bind(&CScheduler::serviceQueue, &reverification_scheduler);
-    threadGroup.create_thread(std::bind(&TraceThread<CScheduler::Function>, "reevaluation_scheduler", reevaluationLoop));
-
-    CScheduler::Function f2 = boost::bind(&MainchainRPCCheck, false);
-    unsigned int check_rpc_every = gArgs.GetArg("-recheckpeginblockinterval", 120) * 1000;
-    if (check_rpc_every) {
-        reverification_scheduler.scheduleEvery(f2, 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.
+    reverification_scheduler.scheduleEvery([]{
+        CValidationState state;
+        if (!ActivateBestChain(state, Params())) {
+            LogPrintf("Failed to periodically activate best chain (%s)\n", FormatStateMessage(state));
+        }
+    }, 30 * 1000);
 
     uiInterface.InitMessage(_("Done loading"));
 

src/mainchainrpc.cpp

@@ -152,18 +152,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

@@ -19,7 +19,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);
 // Constructs unblinded output to be used in amount and scriptpubkey checks during pegin
 CTxOut GetPeginOutputFromWitness(const CScriptWitness& pegin_witness);
 

src/rpc/rawtransaction.cpp

@@ -1060,8 +1060,9 @@ UniValue SignTransaction(interfaces::Chain& chain, CMutableTransaction& mtx, con
             continue;
         }
         // Report warning about immature peg-in though
-        if(txin.m_is_pegin && !IsValidPeginWitness(txConst.witness.vtxinwit[i].m_pegin_witness, fedpegscripts, txin.prevout, err, true)) {
-            assert(err == "Needs more confirmations.");
+        bool depth_failed = false;
+        if(txin.m_is_pegin && !IsValidPeginWitness(txConst.witness.vtxinwit[i].m_pegin_witness, fedpegscripts, txin.prevout, err, true, &depth_failed)) {
+            assert(depth_failed);
             immature_pegin = true;
         }
 

src/txdb.cpp

@@ -34,7 +34,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 {
@@ -272,14 +272,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

@@ -99,8 +99,6 @@ class CBlockTreeDB : public CDBWrapper
     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);
 };