When validation is waiting for parent chain daemon, "stall".

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.

Author: gwillen

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

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/txdb.cpp

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

src/validation.cpp

@@ -47,6 +47,7 @@
 
 #include <future>
 #include <sstream>
+#include <string>
 
 #include <boost/algorithm/string/replace.hpp>
 #include <boost/thread.hpp>
@@ -193,8 +194,8 @@ class CChainState {
     void UnloadBlockIndex();
 
 private:
-    bool ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
-    bool ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions &disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace, bool& fStall) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions &disconnectpool, bool& fStall) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     CBlockIndex* AddToBlockIndex(const CBlockHeader& block) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
     /** Create a new block index entry for a given block hash */
@@ -1936,6 +1937,35 @@ 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;
+                    // IsValidPeginWitness technically checks more than just "ripeness". As a result,
+                    // if we get a validly-signed block which contains an ill-formed pegin witness,
+                    // this test will fail, and it will be treated as "unripe" rather than invalid,
+                    // resulting in a stall rather than rejecting the block. This is reasonable, because 
+                    // in this case (invalid block that is validly-signed by the federation), something
+                    // very strange has happened anyway.
+                    if ((tx.witness.vtxinwit.size() <= i) || !IsValidPeginWitness(tx.witness.vtxinwit[i].m_pegin_witness, fedpegscripts, tx.vin[i].prevout, err, true)) {
+                        // This is not the ideal way to check this, but we already do this in one other place.
+                        if (!strcmp(err.c_str(), "Needs more confirmations.")) {
+                            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). */
@@ -2601,7 +2631,7 @@ class ConnectTrace {
  *
  * The block is added to connectTrace if connection succeeds.
  */
-bool CChainState::ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions &disconnectpool)
+bool CChainState::ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions &disconnectpool, bool& fStall)
 {
     assert(pindexNew->pprev == chainActive.Tip());
     // Read block from disk.
@@ -2616,6 +2646,14 @@ bool CChainState::ConnectTip(CValidationState& state, const CChainParams& chainp
         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;
@@ -2632,29 +2670,6 @@ bool CChainState::ConnectTip(CValidationState& state, const CChainParams& chainp
         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.GetRejectCode() == REJECT_PEGIN) {
-                    //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(), FormatStateMessage(state));
         }
@@ -2762,7 +2777,7 @@ void CChainState::PruneBlockIndexCandidates() {
  * Try to make some progress towards making pindexMostWork the active block.
  * pblock is either nullptr or a pointer to a CBlock corresponding to pindexMostWork.
  */
-bool CChainState::ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace)
+bool CChainState::ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace, bool& fStall)
 {
     AssertLockHeld(cs_main);
 
@@ -2801,7 +2816,7 @@ bool CChainState::ActivateBestChainStep(CValidationState& state, const CChainPar
 
         // 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.CorruptionPossible()) {
@@ -2819,6 +2834,12 @@ bool CChainState::ActivateBestChainStep(CValidationState& state, const CChainPar
                     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 || chainActive.Tip()->nChainWork > pindexOldTip->nChainWork) {
                     // We're in a better position than we were. Return temporarily to release the lock.
@@ -2899,6 +2920,8 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
     CBlockIndex *pindexMostWork = nullptr;
     CBlockIndex *pindexNewTip = nullptr;
     int nStopAtHeight = gArgs.GetArg("-stopatheight", DEFAULT_STOPATHEIGHT);
+    bool fStall = false;
+
     do {
         boost::this_thread::interruption_point();
 
@@ -2930,7 +2953,7 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
 
                 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))
                     return false;
                 blocks_connected = true;
 
@@ -2944,6 +2967,11 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
                     assert(trace.pblock && trace.pindex);
                     GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs);
                 }
+
+                if (fStall) {
+                    // Stuck waiting for parent chain daemon, twiddle our thumbs for awhile.
+                    break;
+                }
             } while (!chainActive.Tip() || (starting_tip && CBlockIndexWorkComparator()(chainActive.Tip(), starting_tip)));
             if (!blocks_connected) return true;
 
@@ -2962,6 +2990,11 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
         }
         // 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
@@ -5306,34 +5339,12 @@ class CMainCleanup
 } instance_of_cmaincleanup;
 
 // ELEMENTS:
-/* This function has two major purposes:
- * 1) Checks that the RPC connection to the parent chain node
+/* This function 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);
-        if (mapBlockIndex.count(blockhash)) {
-            CBlockIndex* pblockindex = mapBlockIndex[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
+    // 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) {
@@ -5384,48 +5395,5 @@ bool MainchainRPCCheck(const bool init)
         }
     }
 
-    //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) {
-        CValidationState state;
-        for(const uint256& blockhash : vblocksToReconsider) {
-            {
-                LOCK(cs_main);
-                if (mapBlockIndex.count(blockhash) == 0)
-                    continue;
-                CBlockIndex* pblockindex = mapBlockIndex[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);
-            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);
-                }
-            }
-        }
-
-        //Write back remaining blocks
-        pblocktree->WriteInvalidBlockQueue(vblocksToReconsiderAgain);
-    }
     return true;
 }