Skip to content

fix: assertion in CMNHFManager due to missing data in evoDB #5736

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 2 commits into from
Dec 1, 2023
Merged

fix: assertion in CMNHFManager due to missing data in evoDB #5736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8a7f5c
fix: assertion in CMNHFManager::GetFromCache due to missing data in e…
knst Nov 24, 2023
9fc5e64
fix: no need locking cs_main for EHFManager
knst Nov 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 60 additions & 26 deletions src/evo/mnhftx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@
#include <llmq/signing.h>
#include <llmq/utils.h>
#include <llmq/quorums.h>
#include <node/blockstorage.h>

#include <chain.h>
#include <chainparams.h>
#include <validation.h>
#include <versionbits.h>

#include <algorithm>
#include <stack>
#include <string>
#include <vector>

Expand Down Expand Up @@ -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};
Expand Down Expand Up @@ -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");
Expand All @@ -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");
}
Expand Down Expand Up @@ -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)
{
AssertLockHeld(cs_main);

// we skip the coinbase
for (size_t i = 1; i < block.vtx.size(); ++i) {
const CTransaction& tx = *block.vtx[i];
Expand Down Expand Up @@ -190,39 +190,41 @@ 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::Signals> CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, BlockValidationState& state)
{
try {
std::vector<uint8_t> 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()) {
if (!fJustCheck) {
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())) {
Expand All @@ -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;
}
}

Expand All @@ -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());
Expand All @@ -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<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
// 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)) {
Expand All @@ -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)
Expand All @@ -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);
}
Expand Down
23 changes: 16 additions & 7 deletions src/evo/mnhftx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ class CBlock;
class CBlockIndex;
class CEvoDB;
class TxValidationState;
extern RecursiveMutex cs_main;

// mnhf signal special transaction
class MNHFTx
Expand Down Expand Up @@ -110,14 +109,17 @@ class CMNHFManager : public AbstractEHFManager
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
* 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
Expand All @@ -132,13 +134,20 @@ class CMNHFManager : public AbstractEHFManager

/**
* 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<Signals> GetFromCache(const CBlockIndex* const pindex);
};

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