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)_
This commit is contained in:
Konstantin Akimov 2023-12-02 01:59:27 +07:00 committed by Odysseas Gabrielides
parent 18b580591c
commit 2e01672b89
2 changed files with 76 additions and 33 deletions

View File

@ -10,6 +10,7 @@
#include <llmq/signing.h> #include <llmq/signing.h>
#include <llmq/utils.h> #include <llmq/utils.h>
#include <llmq/quorums.h> #include <llmq/quorums.h>
#include <node/blockstorage.h>
#include <chain.h> #include <chain.h>
#include <chainparams.h> #include <chainparams.h>
@ -17,6 +18,7 @@
#include <versionbits.h> #include <versionbits.h>
#include <algorithm> #include <algorithm>
#include <stack>
#include <string> #include <string>
#include <vector> #include <vector>
@ -53,7 +55,7 @@ CMNHFManager::~CMNHFManager()
CMNHFManager::Signals CMNHFManager::GetSignalsStage(const CBlockIndex* const pindexPrev) CMNHFManager::Signals CMNHFManager::GetSignalsStage(const CBlockIndex* const pindexPrev)
{ {
Signals signals = GetFromCache(pindexPrev); Signals signals = GetForBlock(pindexPrev);
const int height = pindexPrev->nHeight + 1; const int height = pindexPrev->nHeight + 1;
for (auto it = signals.begin(); it != signals.end(); ) { for (auto it = signals.begin(); it != signals.end(); ) {
bool found{false}; bool found{false};
@ -99,7 +101,7 @@ bool MNHFTx::Verify(const uint256& quorumHash, const uint256& requestId, const u
return true; 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) { if (tx.nVersion != 3 || tx.nType != TRANSACTION_MNHF_SIGNAL) {
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-type"); 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"); 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) { if (!pindexQuorum) {
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-quorum-hash"); return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-quorum-hash");
} }
@ -160,8 +162,6 @@ std::optional<uint8_t> extractEHFSignal(const CTransaction& tx)
static bool extractSignals(const CBlock& block, const CBlockIndex* const pindex, std::vector<uint8_t>& new_signals, BlockValidationState& state) static bool extractSignals(const CBlock& block, const CBlockIndex* const pindex, std::vector<uint8_t>& new_signals, BlockValidationState& state)
{ {
AssertLockHeld(cs_main);
// we skip the coinbase // we skip the coinbase
for (size_t i = 1; i < block.vtx.size(); ++i) { for (size_t i = 1; i < block.vtx.size(); ++i) {
const CTransaction& tx = *block.vtx[i]; const CTransaction& tx = *block.vtx[i];
@ -190,13 +190,13 @@ static bool extractSignals(const CBlock& block, const CBlockIndex* const pindex,
return true; return true;
} }
bool CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, BlockValidationState& state) std::optional<CMNHFManager::Signals> CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, BlockValidationState& state)
{ {
try { try {
std::vector<uint8_t> new_signals; std::vector<uint8_t> new_signals;
if (!extractSignals(block, pindex, new_signals, state)) { if (!extractSignals(block, pindex, new_signals, state)) {
// state is set inside extractSignals // state is set inside extractSignals
return false; return std::nullopt;
} }
Signals signals = GetSignalsStage(pindex->pprev); Signals signals = GetSignalsStage(pindex->pprev);
if (new_signals.empty()) { if (new_signals.empty()) {
@ -204,25 +204,27 @@ bool CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pi
AddToCache(signals, pindex); AddToCache(signals, pindex);
} }
LogPrint(BCLog::EHF, "CMNHFManager::ProcessBlock: no new signals; number of known signals: %d\n", signals.size()); 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 // Extra validation of signals to be sure that it can succeed
for (const auto& versionBit : new_signals) { 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()); 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()) { 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())) { 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) { if (fJustCheck) {
// We are done, no need actually update any params // We are done, no need actually update any params
return true; return signals;
} }
for (const auto& versionBit : new_signals) { for (const auto& versionBit : new_signals) {
if (Params().IsValidMNActivation(versionBit, pindex->GetMedianTimePast())) { if (Params().IsValidMNActivation(versionBit, pindex->GetMedianTimePast())) {
@ -232,10 +234,11 @@ bool CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pi
} }
AddToCache(signals, pindex); AddToCache(signals, pindex);
return true; return signals;
} catch (const std::exception& e) { } catch (const std::exception& e) {
LogPrintf("CMNHFManager::ProcessBlock -- failed: %s\n", e.what()); 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; return true;
} }
const Signals signals = GetFromCache(pindex); const Signals signals = GetForBlock(pindex);
for (const auto& versionBit : excluded_signals) { 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()); 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()); assert(signals.find(versionBit) != signals.end());
@ -261,18 +264,48 @@ bool CMNHFManager::UndoBlock(const CBlock& block, const CBlockIndex* const pinde
return true; return true;
} }
CMNHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex* const pindex) CMNHFManager::Signals CMNHFManager::GetForBlock(const CBlockIndex* pindex)
{ {
if (pindex == nullptr) return {}; if (pindex == nullptr) return {};
std::stack<const CBlockIndex *> to_calculate;
std::optional<CMNHFManager::Signals> 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::Signals> CMNHFManager::GetFromCache(const CBlockIndex* const pindex)
{
Signals signals{};
if (pindex == nullptr) return signals;
// TODO: remove this check of phashBlock to nullptr // TODO: remove this check of phashBlock to nullptr
// This check is needed only because unit test 'versionbits_tests.cpp' // This check is needed only because unit test 'versionbits_tests.cpp'
// lets `phashBlock` to be nullptr // lets `phashBlock` to be nullptr
if (pindex->phashBlock == nullptr) return {}; if (pindex->phashBlock == nullptr) return signals;
const uint256& blockHash = pindex->GetBlockHash(); const uint256& blockHash = pindex->GetBlockHash();
Signals signals{};
{ {
LOCK(cs_cache); LOCK(cs_cache);
if (mnhfCache.get(blockHash, signals)) { if (mnhfCache.get(blockHash, signals)) {
@ -282,15 +315,16 @@ CMNHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex* const pindex
{ {
LOCK(cs_cache); LOCK(cs_cache);
if (ThresholdState::ACTIVE != v20_activation.State(pindex->pprev, Params().GetConsensus(), Consensus::DEPLOYMENT_V20)) { if (ThresholdState::ACTIVE != v20_activation.State(pindex->pprev, Params().GetConsensus(), Consensus::DEPLOYMENT_V20)) {
mnhfCache.insert(blockHash, {}); mnhfCache.insert(blockHash, signals);
return {}; return signals;
} }
} }
bool ok = m_evoDb.Read(std::make_pair(DB_SIGNALS, blockHash), signals); if (m_evoDb.Read(std::make_pair(DB_SIGNALS, blockHash), signals)) {
assert(ok); LOCK(cs_cache);
LOCK(cs_cache); mnhfCache.insert(blockHash, signals);
mnhfCache.insert(blockHash, signals); return signals;
return signals; }
return std::nullopt;
} }
void CMNHFManager::AddToCache(const Signals& signals, const CBlockIndex* const pindex) 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) void CMNHFManager::AddSignal(const CBlockIndex* const pindex, int bit)
{ {
auto signals = GetFromCache(pindex->pprev); auto signals = GetForBlock(pindex->pprev);
signals.emplace(bit, pindex->nHeight); signals.emplace(bit, pindex->nHeight);
AddToCache(signals, pindex); AddToCache(signals, pindex);
} }

View File

@ -22,7 +22,6 @@ class CBlock;
class CBlockIndex; class CBlockIndex;
class CEvoDB; class CEvoDB;
class TxValidationState; class TxValidationState;
extern RecursiveMutex cs_main;
// mnhf signal special transaction // mnhf signal special transaction
class MNHFTx class MNHFTx
@ -110,14 +109,17 @@ public:
explicit CMNHFManager(const CMNHFManager&) = delete; 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<Signals> 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 * 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 // Implements interface
@ -132,13 +134,20 @@ private:
/** /**
* This function returns list of signals available on previous block. * 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. * 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<Signals> GetFromCache(const CBlockIndex* const pindex);
}; };
std::optional<uint8_t> extractEHFSignal(const CTransaction& tx); std::optional<uint8_t> 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 #endif // BITCOIN_EVO_MNHFTX_H