fix: restore BLS scheme after failed block validation

Author: thepastaclaw

src/test/evo_deterministicmns_tests.cpp

@@ -990,6 +990,43 @@ BOOST_AUTO_TEST_CASE(v19_activation_legacy)
     FuncV19Activation(setup);
 }
 
+BOOST_AUTO_TEST_CASE(v19_boundary_validation_failure_restores_bls_scheme)
+{
+    TestChainV19Setup setup;
+    auto& chainman = *Assert(setup.m_node.chainman.get());
+    const CScript coinbase_pk = GetScriptForRawPubKey(setup.coinbaseKey.GetPubKey());
+
+    BOOST_REQUIRE(!DeploymentActiveAt(*chainman.ActiveChain().Tip(), chainman.GetConsensus(), Consensus::DEPLOYMENT_V19));
+    BOOST_REQUIRE(DeploymentActiveAfter(chainman.ActiveChain().Tip(), chainman.GetConsensus(), Consensus::DEPLOYMENT_V19));
+    const bool previous_bls_scheme{bls::bls_legacy_scheme.load()};
+
+    CMutableTransaction bad_tx;
+    bad_tx.nVersion = 1;
+    bad_tx.vin.emplace_back(COutPoint(uint256::ONE, 0));
+    bad_tx.vout.emplace_back(1 * COIN, CScript{} << OP_TRUE);
+
+    bls::bls_legacy_scheme.store(true);
+
+    CBlock proposal_block = setup.CreateBlock({bad_tx}, coinbase_pk, chainman.ActiveChainstate());
+    {
+        LOCK(cs_main);
+        BlockValidationState state;
+        BOOST_CHECK(!TestBlockValidity(state, *Assert(setup.m_node.chainlocks), *Assert(setup.m_node.evodb), Params(),
+                                       chainman.ActiveChainstate(), proposal_block, chainman.ActiveChain().Tip(),
+                                       /*fCheckPOW=*/true, /*fCheckMerkleRoot=*/true));
+        BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-txns-inputs-missingorspent");
+    }
+    BOOST_CHECK(bls::bls_legacy_scheme.load());
+
+    CBlock connect_block = setup.CreateBlock({bad_tx}, coinbase_pk, chainman.ActiveChainstate());
+    const int height_before_invalid_block{chainman.ActiveChain().Height()};
+    (void)chainman.ProcessNewBlock(std::make_shared<const CBlock>(connect_block), /*force_processing=*/true, /*new_block=*/nullptr);
+    BOOST_CHECK_EQUAL(chainman.ActiveChain().Height(), height_before_invalid_block);
+    BOOST_CHECK(bls::bls_legacy_scheme.load());
+
+    bls::bls_legacy_scheme.store(previous_bls_scheme);
+}
+
 BOOST_AUTO_TEST_CASE(dip3_protx_legacy)
 {
     TestChainDIP3Setup setup;

src/test/validation_block_tests.cpp

@@ -362,4 +362,5 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
         rpc_thread.join();
     }
 }
+
 BOOST_AUTO_TEST_SUITE_END()

src/validation.cpp

@@ -7,6 +7,7 @@
 #include <validation.h>
 
 #include <arith_uint256.h>
+#include <bls/bls.h>
 #include <chain.h>
 #include <chainparams.h>
 #include <checkqueue.h>
@@ -140,6 +141,40 @@ CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMe
 } // namespace node
 using node::GetTransaction;
 
+namespace {
+
+void SetBLSLegacyScheme(bool legacy_scheme, const char* logging_context) noexcept
+{
+    if (legacy_scheme != bls::bls_legacy_scheme.load()) {
+        bls::bls_legacy_scheme.store(legacy_scheme);
+        LogPrintf("%s: bls_legacy_scheme=%d\n", logging_context, bls::bls_legacy_scheme.load());
+    }
+}
+
+class ScopedBLSLegacyScheme
+{
+public:
+    explicit ScopedBLSLegacyScheme(const char* logging_context) noexcept :
+        m_logging_context(logging_context),
+        m_saved_scheme(bls::bls_legacy_scheme.load())
+    {
+    }
+
+    ScopedBLSLegacyScheme(const ScopedBLSLegacyScheme&) = delete;
+    ScopedBLSLegacyScheme& operator=(const ScopedBLSLegacyScheme&) = delete;
+
+    ~ScopedBLSLegacyScheme() noexcept
+    {
+        SetBLSLegacyScheme(m_saved_scheme, m_logging_context);
+    }
+
+private:
+    const char* const m_logging_context;
+    const bool m_saved_scheme;
+};
+
+} // namespace
+
 const CBlockIndex* CChainState::FindForkInGlobalIndex(const CBlockLocator& locator) const
 {
     AssertLockHeld(cs_main);
@@ -2181,7 +2216,7 @@ static int64_t nBlocksTotal = 0;
  *  Validity checks that depend on the UTXO set are also done; ConnectBlock()
  *  can fail if those validity checks fail (among other reasons). */
 bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, CBlockIndex* pindex,
-                               CCoinsViewCache& view, bool fJustCheck)
+                               CCoinsViewCache& view, bool fJustCheck, bool* effective_bls_legacy_scheme)
 {
     int64_t nTimeStart = GetTimeMicros();
 
@@ -2193,6 +2228,8 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
 
     assert(m_chain_helper);
 
+    ScopedBLSLegacyScheme bls_scheme_guard{__func__};
+
     // Check it again in case a previous version let a bad block in
     // NOTE: We don't currently (re-)invoke ContextualCheckBlock() or
     // ContextualCheckBlockHeader() here. This means that if we add a new
@@ -2551,6 +2588,9 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
         nTime8 - nTimeStart // in microseconds (µs)
     );
 
+    if (effective_bls_legacy_scheme) {
+        *effective_bls_legacy_scheme = bls::bls_legacy_scheme.load();
+    }
     return true;
 }
 
@@ -3011,11 +3051,12 @@ bool CChainState::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew
     // When adding aggregate statistics in the future, keep in mind that
     // nBlocksTotal may be zero until the ConnectBlock() call below.
     LogPrint(BCLog::BENCHMARK, "  - Load block from disk: %.2fms\n", (nTime2 - nTime1) * MILLI);
+    bool effective_bls_legacy_scheme{bls::bls_legacy_scheme.load()};
     {
         auto dbTx = m_evoDb.BeginTransaction();
 
         CCoinsViewCache view(&CoinsTip());
-        bool rv = ConnectBlock(blockConnecting, state, pindexNew, view);
+        bool rv = ConnectBlock(blockConnecting, state, pindexNew, view, /*fJustCheck=*/false, &effective_bls_legacy_scheme);
         GetMainSignals().BlockChecked(blockConnecting, state);
         if (!rv) {
             if (state.IsInvalid())
@@ -3045,6 +3086,7 @@ bool CChainState::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew
     }
     // Update m_chain & related variables.
     m_chain.SetTip(*pindexNew);
+    SetBLSLegacyScheme(effective_bls_legacy_scheme, __func__);
     UpdateTip(pindexNew);
 
     int64_t nTime6 = GetTimeMicros(); nTimePostConnect += nTime6 - nTime5; nTimeTotal += nTime6 - nTime1;
@@ -4338,10 +4380,7 @@ bool TestBlockValidity(BlockValidationState& state,
     AssertLockHeld(cs_main);
     assert(pindexPrev && pindexPrev == chainstate.m_chain.Tip());
 
-    // TODO: instead restoring bls_legacy_scheme better to keep it unchanged
-    // Moreover, current implementation is working incorrect if current function
-    // will return value too early due to error: old value won't be restored
-    auto bls_legacy_scheme = bls::bls_legacy_scheme.load();
+    ScopedBLSLegacyScheme bls_scheme_guard{__func__};
 
     uint256 hash = block.GetHash();
     if (chainlocks.HasConflictingChainLock(pindexPrev->nHeight + 1, hash)) {
@@ -4371,12 +4410,6 @@ bool TestBlockValidity(BlockValidationState& state,
 
     assert(state.IsValid());
 
-    // we could switch to another scheme while testing, switch back to the original one
-    if (bls_legacy_scheme != bls::bls_legacy_scheme.load()) {
-        bls::bls_legacy_scheme.store(bls_legacy_scheme);
-        LogPrintf("%s: bls_legacy_scheme=%d\n", __func__, bls::bls_legacy_scheme.load());
-    }
-
     return true;
 }
 
@@ -4449,6 +4482,8 @@ bool CVerifyDB::VerifyDB(
         return true;
     }
 
+    ScopedBLSLegacyScheme bls_scheme_guard{__func__};
+
     // begin tx and let it rollback
     auto dbTx = evoDb.BeginTransaction();
 
@@ -4538,6 +4573,7 @@ bool CVerifyDB::VerifyDB(
 
     // check level 4: try reconnecting blocks
     if (nCheckLevel >= 4 && !skipped_l3_checks) {
+        bool effective_bls_legacy_scheme{bls::bls_legacy_scheme.load()};
         while (pindex != chainstate.m_chain.Tip()) {
             const int percentageDone = std::max(1, std::min(99, 100 - (int)(((double)(chainstate.m_chain.Height() - pindex->nHeight)) / (double)nCheckDepth * 50)));
             if (reportDone < percentageDone / 10) {
@@ -4550,8 +4586,9 @@ bool CVerifyDB::VerifyDB(
             CBlock block;
             if (!ReadBlockFromDisk(block, pindex, consensus_params))
                 return error("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString());
-            if (!chainstate.ConnectBlock(block, state, pindex, coins))
+            if (!chainstate.ConnectBlock(block, state, pindex, coins, /*fJustCheck=*/false, &effective_bls_legacy_scheme))
                 return error("VerifyDB(): *** found unconnectable block at %d, hash=%s (%s)", pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString());
+            SetBLSLegacyScheme(effective_bls_legacy_scheme, __func__);
             if (ShutdownRequested()) return true;
         }
     }

src/validation.h

@@ -709,7 +709,7 @@ class CChainState
 
     // Block (dis)connection on a given view:
     DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
-    bool ConnectBlock(const CBlock& block, BlockValidationState& state, CBlockIndex* pindex, CCoinsViewCache& view, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool ConnectBlock(const CBlock& block, BlockValidationState& state, CBlockIndex* pindex, CCoinsViewCache& view, bool fJustCheck = false, bool* effective_bls_legacy_scheme = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     // Apply the effects of a block disconnection on the UTXO set.
     bool DisconnectTip(BlockValidationState& state, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);