From 2e01672b8921bd253c13128f5c4510905563305a Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Sat, 2 Dec 2023 01:59:27 +0700 Subject: [PATCH] fix: assertion in CMNHFManager due to missing data in evoDB (#5736) Prior required changes: bitcoin/bitcoin#19438 from https://github.com/dashpay/dash/pull/5740 ## Issue being fixed or feature implemented Assertion failure: ``` assertion: ok file: evo/mnhftx.cpp, line: 287 function: AbstractEHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex*) No debug information available for stacktrace. You should add debug information and then run: dash-qt -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaadwiyltnawxc5e3ifzxgzlsoruw63ramzqws3dvojstucraebqxg43foj2gs33ohiqg62ykeaqgm2lmmu5cazlwn4xw23timz2hqltdobycyidmnfxgkoragi4docraebthk3tdoruw63r2ebawe43uojqwg5cfjbde2ylomftwk4r2hjjwsz3omfwhgicdjvheqrsnmfxgcz3foi5dur3fordhe33ninqwg2dffbrw63ttoqqegqtmn5rwwslomrsxqkrjbyrelhyaaaaaaaedgkiaaaaaaaadsm4qaaaaaaaa7gpyqaaaaaaaa2njraaaaaaaadkl3caaaaaaaabhxznqaaaaaaadqa2eaaaaaaaax33twaaaaaaabwaihqaaaaaaac7yooqbaaaaaahba45qcaaaaaacwkz2aeaaaaaaeitgeaiaaaaaaaa= dash-qt: evo/mnhftx.cpp:287: AbstractEHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex*): Assertion `ok' failed. ``` This can happen in case if Dash Core has been update from v19 (or earlier v20.alphaX) to v20.0.0 after v20 activation without re-indexing ## What was done? `CMNHFManager` is visiting missing blocks recursively until reach first v20 block or first block actually saved in evoDb. Without changes from bitcoin/bitcoin#19438 there's an other issue: ``` 2023-11-27T11:12:10Z POTENTIAL DEADLOCK DETECTED Previous lock order was: (2) 'cs_main' in llmq/instantsend.cpp:459 (in thread 'isman') (1) 'cs_llmq_vbc' in llmq/utils.cpp:711 (in thread 'isman') Current lock order is: 'cs_dip3list' in qt/masternodelist.cpp:135 (TRY) (in thread 'main') (1) 'cs_llmq_vbc' in llmq/utils.cpp:719 (in thread 'main') (2) 'cs_main' in node/blockstorage.cpp:77 (in thread 'main') Assertion failed: detected inconsistent lock order for 'cs_main' in node/blockstorage.cpp:77 (in thread 'main'), details in debug log. 2023-11-27T11:12:10Z Posix Signal: Aborted ``` ## How Has This Been Tested? Run unit/functional test; run dash-qt on my local backup of problematic storage (succeed without error); reindex testnet. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ --- src/evo/mnhftx.cpp | 86 ++++++++++++++++++++++++++++++++-------------- src/evo/mnhftx.h | 23 +++++++++---- 2 files changed, 76 insertions(+), 33 deletions(-) diff --git a/src/evo/mnhftx.cpp b/src/evo/mnhftx.cpp index 8f648aea45..46c4868a85 100644 --- a/src/evo/mnhftx.cpp +++ b/src/evo/mnhftx.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -17,6 +18,7 @@ #include #include +#include #include #include @@ -53,7 +55,7 @@ CMNHFManager::~CMNHFManager() CMNHFManager::Signals CMNHFManager::GetSignalsStage(const CBlockIndex* const pindexPrev) { - Signals signals = GetFromCache(pindexPrev); + Signals signals = GetForBlock(pindexPrev); const int height = pindexPrev->nHeight + 1; for (auto it = signals.begin(); it != signals.end(); ) { bool found{false}; @@ -99,7 +101,7 @@ bool MNHFTx::Verify(const uint256& quorumHash, const uint256& requestId, const u return true; } -bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state) { if (tx.nVersion != 3 || tx.nType != TRANSACTION_MNHF_SIGNAL) { return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-type"); @@ -114,7 +116,7 @@ bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValida return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-version"); } - const CBlockIndex* pindexQuorum = g_chainman.m_blockman.LookupBlockIndex(mnhfTx.signal.quorumHash); + const CBlockIndex* pindexQuorum = WITH_LOCK(::cs_main, return g_chainman.m_blockman.LookupBlockIndex(mnhfTx.signal.quorumHash)); if (!pindexQuorum) { return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-quorum-hash"); } @@ -160,8 +162,6 @@ std::optional extractEHFSignal(const CTransaction& tx) static bool extractSignals(const CBlock& block, const CBlockIndex* const pindex, std::vector& new_signals, BlockValidationState& state) { - AssertLockHeld(cs_main); - // we skip the coinbase for (size_t i = 1; i < block.vtx.size(); ++i) { const CTransaction& tx = *block.vtx[i]; @@ -190,13 +190,13 @@ static bool extractSignals(const CBlock& block, const CBlockIndex* const pindex, return true; } -bool CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, BlockValidationState& state) +std::optional CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, BlockValidationState& state) { try { std::vector new_signals; if (!extractSignals(block, pindex, new_signals, state)) { // state is set inside extractSignals - return false; + return std::nullopt; } Signals signals = GetSignalsStage(pindex->pprev); if (new_signals.empty()) { @@ -204,25 +204,27 @@ bool CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pi AddToCache(signals, pindex); } LogPrint(BCLog::EHF, "CMNHFManager::ProcessBlock: no new signals; number of known signals: %d\n", signals.size()); - return true; + return signals; } - int mined_height = pindex->nHeight; + const int mined_height = pindex->nHeight; // Extra validation of signals to be sure that it can succeed for (const auto& versionBit : new_signals) { LogPrintf("CMNHFManager::ProcessBlock: add mnhf bit=%d block:%s number of known signals:%lld\n", versionBit, pindex->GetBlockHash().ToString(), signals.size()); if (signals.find(versionBit) != signals.end()) { - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-mnhf-duplicate"); + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-mnhf-duplicate"); + return std::nullopt; } if (!Params().IsValidMNActivation(versionBit, pindex->GetMedianTimePast())) { - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-mnhf-non-mn-fork"); + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-mnhf-non-mn-fork"); + return std::nullopt; } } if (fJustCheck) { // We are done, no need actually update any params - return true; + return signals; } for (const auto& versionBit : new_signals) { if (Params().IsValidMNActivation(versionBit, pindex->GetMedianTimePast())) { @@ -232,10 +234,11 @@ bool CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pi } AddToCache(signals, pindex); - return true; + return signals; } catch (const std::exception& e) { LogPrintf("CMNHFManager::ProcessBlock -- failed: %s\n", e.what()); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "failed-proc-mnhf-inblock"); + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "failed-proc-mnhf-inblock"); + return std::nullopt; } } @@ -251,7 +254,7 @@ bool CMNHFManager::UndoBlock(const CBlock& block, const CBlockIndex* const pinde return true; } - const Signals signals = GetFromCache(pindex); + const Signals signals = GetForBlock(pindex); for (const auto& versionBit : excluded_signals) { LogPrintf("%s: exclude mnhf bit=%d block:%s number of known signals:%lld\n", __func__, versionBit, pindex->GetBlockHash().ToString(), signals.size()); assert(signals.find(versionBit) != signals.end()); @@ -261,18 +264,48 @@ bool CMNHFManager::UndoBlock(const CBlock& block, const CBlockIndex* const pinde return true; } -CMNHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex* const pindex) +CMNHFManager::Signals CMNHFManager::GetForBlock(const CBlockIndex* pindex) { if (pindex == nullptr) return {}; + std::stack to_calculate; + + std::optional signalsTmp; + while (!(signalsTmp = GetFromCache(pindex)).has_value()) { + to_calculate.push(pindex); + pindex = pindex->pprev; + } + + const Consensus::Params& consensusParams{Params().GetConsensus()}; + while (!to_calculate.empty()) { + CBlock block; + if (!ReadBlockFromDisk(block, pindex, consensusParams)) { + throw std::runtime_error("failed-getehfforblock-read"); + } + BlockValidationState state; + signalsTmp = ProcessBlock(block, pindex, false, state); + if (!signalsTmp.has_value()) { + LogPrintf("%s: process block failed due to %s\n", __func__, state.ToString()); + throw std::runtime_error("failed-getehfforblock-construct"); + } + + to_calculate.pop(); + } + return *signalsTmp; +} + +std::optional CMNHFManager::GetFromCache(const CBlockIndex* const pindex) +{ + Signals signals{}; + if (pindex == nullptr) return signals; + // TODO: remove this check of phashBlock to nullptr // This check is needed only because unit test 'versionbits_tests.cpp' // lets `phashBlock` to be nullptr - if (pindex->phashBlock == nullptr) return {}; + if (pindex->phashBlock == nullptr) return signals; const uint256& blockHash = pindex->GetBlockHash(); - Signals signals{}; { LOCK(cs_cache); if (mnhfCache.get(blockHash, signals)) { @@ -282,15 +315,16 @@ CMNHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex* const pindex { LOCK(cs_cache); if (ThresholdState::ACTIVE != v20_activation.State(pindex->pprev, Params().GetConsensus(), Consensus::DEPLOYMENT_V20)) { - mnhfCache.insert(blockHash, {}); - return {}; + mnhfCache.insert(blockHash, signals); + return signals; } } - bool ok = m_evoDb.Read(std::make_pair(DB_SIGNALS, blockHash), signals); - assert(ok); - LOCK(cs_cache); - mnhfCache.insert(blockHash, signals); - return signals; + if (m_evoDb.Read(std::make_pair(DB_SIGNALS, blockHash), signals)) { + LOCK(cs_cache); + mnhfCache.insert(blockHash, signals); + return signals; + } + return std::nullopt; } void CMNHFManager::AddToCache(const Signals& signals, const CBlockIndex* const pindex) @@ -310,7 +344,7 @@ void CMNHFManager::AddToCache(const Signals& signals, const CBlockIndex* const p void CMNHFManager::AddSignal(const CBlockIndex* const pindex, int bit) { - auto signals = GetFromCache(pindex->pprev); + auto signals = GetForBlock(pindex->pprev); signals.emplace(bit, pindex->nHeight); AddToCache(signals, pindex); } diff --git a/src/evo/mnhftx.h b/src/evo/mnhftx.h index aae8c17412..73983a5540 100644 --- a/src/evo/mnhftx.h +++ b/src/evo/mnhftx.h @@ -22,7 +22,6 @@ class CBlock; class CBlockIndex; class CEvoDB; class TxValidationState; -extern RecursiveMutex cs_main; // mnhf signal special transaction class MNHFTx @@ -110,14 +109,17 @@ public: explicit CMNHFManager(const CMNHFManager&) = delete; /** - * Every new block should be processed when Tip() is updated by calling of CMNHFManager::ProcessBlock + * Every new block should be processed when Tip() is updated by calling of CMNHFManager::ProcessBlock. + * This function actually does only validate EHF transaction for this block and update internal caches/evodb state */ - bool ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + std::optional ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, BlockValidationState& state); /** * Every undo block should be processed when Tip() is updated by calling of CMNHFManager::UndoBlock + * This function actually does nothing at the moment, because status of ancester block is already know. + * Altough it should be still called to do some sanity checks */ - bool UndoBlock(const CBlock& block, const CBlockIndex* const pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool UndoBlock(const CBlock& block, const CBlockIndex* const pindex); // Implements interface @@ -132,13 +134,20 @@ private: /** * This function returns list of signals available on previous block. + * if the signals for previous block is not available in cache it would read blocks from disk + * until state won't be recovered. * NOTE: that some signals could expired between blocks. - * validate them by */ - Signals GetFromCache(const CBlockIndex* const pindex); + Signals GetForBlock(const CBlockIndex* const pindex); + + /** + * This function access to in-memory cache or to evo db but does not calculate anything + * NOTE: that some signals could expired between blocks. + */ + std::optional GetFromCache(const CBlockIndex* const pindex); }; std::optional extractEHFSignal(const CTransaction& tx); -bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main); +bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state); #endif // BITCOIN_EVO_MNHFTX_H