fix: restore BLS scheme after failed block validation

src/test/bls_tests.cpp

@@ -6,10 +6,16 @@
 #include <bls/bls_batchverifier.h>
 #include <util/helpers.h>
 
+#include <chainparams.h>
 #include <clientversion.h>
+#include <consensus/validation.h>
+#include <deploymentstatus.h>
 #include <random.h>
+#include <script/standard.h>
 #include <streams.h>
+#include <test/util/setup_common.h>
 #include <util/strencodings.h>
+#include <validation.h>
 
 #include <boost/test/unit_test.hpp>
 
@@ -594,4 +600,60 @@ BOOST_AUTO_TEST_CASE(test_get_hash_consistency)
     BOOST_CHECK(hash1 == hash2);
 }
 
+namespace {
+struct TestChainV19Setup : public TestChainSetup {
+    TestChainV19Setup() :
+        TestChainSetup(494, CBaseChainParams::REGTEST,
+                       {"-testactivationheight=v19@500", "-testactivationheight=v20@500", "-testactivationheight=mn_rr@500"})
+    {
+        const CScript coinbase_pk = GetScriptForRawPubKey(coinbaseKey.GetPubKey());
+        // Mine up to the block just before V19 activation
+        for (int i = 0; i < 5; ++i) {
+            CreateAndProcessBlock({}, coinbase_pk);
+        }
+        assert(DeploymentActiveAfter(m_node.chainman->ActiveChain().Tip(), m_node.chainman->GetConsensus(), Consensus::DEPLOYMENT_V19) &&
+               !DeploymentActiveAt(*m_node.chainman->ActiveChain().Tip(), m_node.chainman->GetConsensus(), Consensus::DEPLOYMENT_V19));
+    }
+};
+} // namespace
+
+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));
+    struct ScopedBLSLegacySchemeRestore {
+        explicit ScopedBLSLegacySchemeRestore(bool saved_scheme) : m_saved_scheme(saved_scheme) {}
+        ~ScopedBLSLegacySchemeRestore() { bls::bls_legacy_scheme.store(m_saved_scheme); }
+        bool m_saved_scheme;
+    } bls_scheme_restore{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());
+}
+
 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,36 @@ CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMe
 } // namespace node
 using node::GetTransaction;
 
+namespace {
+
+//! Restores bls::bls_legacy_scheme on scope exit unless Commit() is called.
+//! ConnectBlock switches the scheme while validating a block across the V19
+//! boundary; this keeps a failed or dry-run validation from leaking that
+//! change into the process-wide flag.
+class ScopedBLSLegacyScheme
+{
+public:
+    ScopedBLSLegacyScheme() noexcept : m_saved(bls::bls_legacy_scheme.load()) {}
+    ~ScopedBLSLegacyScheme() noexcept
+    {
+        if (!m_committed && m_saved != bls::bls_legacy_scheme.load()) {
+            bls::bls_legacy_scheme.store(m_saved);
+            LogPrintf("ScopedBLSLegacyScheme: reverted bls_legacy_scheme=%d\n", m_saved);
+        }
+    }
+
+    ScopedBLSLegacyScheme(const ScopedBLSLegacyScheme&) = delete;
+    ScopedBLSLegacyScheme& operator=(const ScopedBLSLegacyScheme&) = delete;
+
+    void Commit() noexcept { m_committed = true; }
+
+private:
+    const bool m_saved;
+    bool m_committed{false};
+};
+
+} // namespace
+
 const CBlockIndex* CChainState::FindForkInGlobalIndex(const CBlockLocator& locator) const
 {
     AssertLockHeld(cs_main);
@@ -2193,6 +2224,9 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
 
     assert(m_chain_helper);
 
+    // Roll back any BLS scheme switch below unless we fully connect the block.
+    ScopedBLSLegacyScheme bls_scheme_guard;
+
     // 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
@@ -2534,6 +2568,9 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
     view.SetBestBlock(pindex->GetBlockHash());
     m_evoDb.WriteBestBlock(pindex->GetBlockHash());
 
+    // Block is committed: keep the scheme it switched to (fJustCheck dry runs returned above).
+    bls_scheme_guard.Commit();
+
     if (mnlist_updates_opt.has_value()) {
         const auto& mnlu = mnlist_updates_opt.value();
         GetMainSignals().NotifyMasternodeListChanged(false, mnlu.old_list, mnlu.diff);
@@ -2991,6 +3028,11 @@ bool CChainState::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew
     if (m_mempool) AssertLockHeld(m_mempool->cs);
 
     assert(pindexNew->pprev == m_chain.Tip());
+    // ConnectBlock commits its BLS-scheme switch on success, but the tip isn't
+    // advanced until m_chain.SetTip() below. If a step in between (e.g.
+    // FlushStateToDisk) fails, restore the scheme so it stays in sync with the
+    // unchanged tip.
+    ScopedBLSLegacyScheme bls_scheme_guard;
     // Read block from disk.
     int64_t nTime1 = GetTimeMicros();
     std::shared_ptr<const CBlock> pthisBlock;
@@ -3045,6 +3087,7 @@ bool CChainState::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew
     }
     // Update m_chain & related variables.
     m_chain.SetTip(*pindexNew);
+    bls_scheme_guard.Commit();
     UpdateTip(pindexNew);
 
     int64_t nTime6 = GetTimeMicros(); nTimePostConnect += nTime6 - nTime5; nTimeTotal += nTime6 - nTime1;
@@ -4338,11 +4381,6 @@ 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();
-
     uint256 hash = block.GetHash();
     if (chainlocks.HasConflictingChainLock(pindexPrev->nHeight + 1, hash)) {
         LogPrintf("ERROR: %s: conflicting with chainlock\n", __func__);
@@ -4371,12 +4409,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 +4481,9 @@ bool CVerifyDB::VerifyDB(
         return true;
     }
 
+    // ConnectBlock below switches the scheme but the tip isn't advanced here; restore it on return.
+    ScopedBLSLegacyScheme bls_scheme_guard;
+
     // begin tx and let it rollback
     auto dbTx = evoDb.BeginTransaction();
 
@@ -4538,6 +4573,9 @@ bool CVerifyDB::VerifyDB(
 
     // check level 4: try reconnecting blocks
     if (nCheckLevel >= 4 && !skipped_l3_checks) {
+        // Each successful ConnectBlock leaves the effective scheme set globally so
+        // subsequent iterations see it. The outer bls_scheme_guard restores the
+        // original scheme when VerifyDB returns (the chain tip isn't advanced here).
         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) {