Skip to content

Commit

Permalink
Always mark conflicting blocks with BLOCK_CONFLICT_CHAINLOCK flag (#3924
Browse files Browse the repository at this point in the history 
)

* More accurate handling of the BLOCK_CONFLICT_CHAINLOCK flag

* Update test/functional/feature_llmq_chainlocks.py

Co-authored-by: thephez <thephez@users.noreply.github.com>

* tests: make sure that previous tip on the reorged node is marked conflicting after chainlock

* Apply suggestions from code review

Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>

Co-authored-by: thephez <thephez@users.noreply.github.com>
Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>
  • Loading branch information
@UdjinM6 @thephez @xdustinface
3 people authored Jan 16, 2021
1 parent 4a6eee2 commit 482ba4f
Show file tree
Hide file tree
Showing 6 changed files with 139 additions and 36 deletions.
35 changes: 11 additions & 24 deletions src/llmq/quorums_chainlocks.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -510,22 +510,28 @@ void CChainLocksHandler::EnforceBestChainLock()
}

bool activateNeeded;
CValidationState state;
const auto &params = Params();
{
LOCK(cs_main);

// Go backwards through the chain referenced by clsig until we find a block that is part of the main chain.
// For each of these blocks, check if there are children that are NOT part of the chain referenced by clsig
// and invalidate each of them.
// and mark all of them as conflicting.
while (pindex && !chainActive.Contains(pindex)) {
// Invalidate all blocks that have the same prevBlockHash but are not equal to blockHash
// Mark all blocks that have the same prevBlockHash but are not equal to blockHash as conflicting
auto itp = mapPrevBlockIndex.equal_range(pindex->pprev->GetBlockHash());
for (auto jt = itp.first; jt != itp.second; ++jt) {
if (jt->second == pindex) {
continue;
}
LogPrintf("CChainLocksHandler::%s -- CLSIG (%s) invalidates block %s\n",
if (!MarkConflictingBlock(state, params, jt->second)) {
LogPrintf("CChainLocksHandler::%s -- MarkConflictingBlock failed: %s\n", __func__, FormatStateMessage(state));
// This should not have happened and we are in a state were it's not safe to continue anymore
assert(false);
}
LogPrintf("CChainLocksHandler::%s -- CLSIG (%s) marked block %s as conflicting\n",
__func__, clsig.ToString(), jt->second->GetBlockHash().ToString());
DoInvalidateBlock(jt->second);
}

pindex = pindex->pprev;
Expand All @@ -541,8 +547,7 @@ void CChainLocksHandler::EnforceBestChainLock()
activateNeeded = chainActive.Tip()->GetAncestor(currentBestChainLockBlockIndex->nHeight) != currentBestChainLockBlockIndex;
}

CValidationState state;
if (activateNeeded && !ActivateBestChain(state, Params())) {
if (activateNeeded && !ActivateBestChain(state, params)) {
LogPrintf("CChainLocksHandler::%s -- ActivateBestChain failed: %s\n", __func__, FormatStateMessage(state));
}

Expand Down Expand Up @@ -587,24 +592,6 @@ void CChainLocksHandler::HandleNewRecoveredSig(const llmq::CRecoveredSig& recove
ProcessNewChainLock(-1, clsig, ::SerializeHash(clsig));
}

// WARNING, do not hold cs while calling this method as we'll otherwise run into a deadlock
void CChainLocksHandler::DoInvalidateBlock(const CBlockIndex* pindex)
{
LOCK(cs_main);

auto& params = Params();

// get the non-const pointer
CBlockIndex* pindex2 = LookupBlockIndex(pindex->GetBlockHash());

CValidationState state;
if (!InvalidateBlock(state, params, pindex2)) {
LogPrintf("CChainLocksHandler::%s -- InvalidateBlock failed: %s\n", __func__, FormatStateMessage(state));
// This should not have happened and we are in a state were it's not safe to continue anymore
assert(false);
}
}

bool CChainLocksHandler::HasChainLock(int nHeight, const uint256& blockHash)
{
LOCK(cs);
Expand Down
2 changes: 0 additions & 2 deletions src/llmq/quorums_chainlocks.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,6 @@ class CChainLocksHandler : public CRecoveredSigsListener
bool InternalHasChainLock(int nHeight, const uint256& blockHash);
bool InternalHasConflictingChainLock(int nHeight, const uint256& blockHash);

static void DoInvalidateBlock(const CBlockIndex* pindex);

BlockTxs::mapped_type GetBlockTxs(const uint256& blockHash);

void Cleanup();
Expand Down
3 changes: 3 additions & 0 deletions src/rpc/blockchain.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1592,6 +1592,9 @@ UniValue getchaintips(const JSONRPCRequest& request)
} else if (block->nStatus & BLOCK_FAILED_MASK) {
// This block or one of its ancestors is invalid.
status = "invalid";
} else if (block->nStatus & BLOCK_CONFLICT_CHAINLOCK) {
// This block or one of its ancestors is conflicting with ChainLocks.
status = "conflicting";
} else if (block->nChainTx == 0) {
// This block cannot be connected because full block data for it or one of its parents is missing.
status = "headers-only";
Expand Down
119 changes: 111 additions & 8 deletions src/validation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ class CChainState {
// Manual block validity manipulation:
bool PreciousBlock(CValidationState& state, const CChainParams& params, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main);
bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
bool MarkConflictingBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
bool ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

bool ReplayBlocks(const CChainParams& params, CCoinsView* view);
Expand Down Expand Up @@ -1385,6 +1386,21 @@ void static InvalidChainFound(CBlockIndex* pindexNew)
CheckForkWarningConditions();
}

void static ConflictingChainFound(CBlockIndex* pindexNew)
{
statsClient.inc("warnings.ConflictingChainFound", 1.0f);

LogPrintf("%s: conflicting block=%s height=%d log2_work=%.8f date=%s\n", __func__,
pindexNew->GetBlockHash().ToString(), pindexNew->nHeight,
log(pindexNew->nChainWork.getdouble())/log(2.0), FormatISO8601DateTime(pindexNew->GetBlockTime()));
CBlockIndex *tip = chainActive.Tip();
assert (tip);
LogPrintf("%s: current best=%s height=%d log2_work=%.8f date=%s\n", __func__,
tip->GetBlockHash().ToString(), chainActive.Height(), log(tip->nChainWork.getdouble())/log(2.0),
FormatISO8601DateTime(tip->GetBlockTime()));
CheckForkWarningConditions();
}

void CChainState::InvalidBlockFound(CBlockIndex *pindex, const CValidationState &state) {
statsClient.inc("warnings.InvalidBlockFound", 1.0f);
if (!state.CorruptionPossible()) {
Expand Down Expand Up @@ -2909,16 +2925,20 @@ CBlockIndex* CChainState::FindMostWorkChain() {
// for the most work chain if we come across them; we can't switch
// to a chain unless we have all the non-active-chain parent blocks.
bool fFailedChain = pindexTest->nStatus & BLOCK_FAILED_MASK;
bool fConflictingChain = pindexTest->nStatus & BLOCK_CONFLICT_CHAINLOCK;
bool fMissingData = !(pindexTest->nStatus & BLOCK_HAVE_DATA);
if (fFailedChain || fMissingData) {
// Candidate chain is not usable (either invalid or missing data)
if (fFailedChain || fMissingData || fConflictingChain) {
// Candidate chain is not usable (either invalid or conflicting or missing data)
if (fFailedChain && (pindexBestInvalid == nullptr || pindexNew->nChainWork > pindexBestInvalid->nChainWork))
pindexBestInvalid = pindexNew;
CBlockIndex *pindexFailed = pindexNew;
// Remove the entire chain from the set.
while (pindexTest != pindexFailed) {
if (fFailedChain) {
pindexFailed->nStatus |= BLOCK_FAILED_CHILD;
} else if (fConflictingChain) {
// We don't need data for conflciting blocks
pindexFailed->nStatus |= BLOCK_CONFLICT_CHAINLOCK;
} else if (fMissingData) {
// If we're missing data, then add back to mapBlocksUnlinked,
// so that if the block arrives in the future we can try adding
Expand Down Expand Up @@ -3191,7 +3211,7 @@ bool CChainState::PreciousBlock(CValidationState& state, const CChainParams& par
// call preciousblock 2**31-1 times on the same set of tips...
nBlockReverseSequenceId--;
}
if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && pindex->nChainTx) {
if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && !(pindex->nStatus & BLOCK_CONFLICT_CHAINLOCK) && pindex->nChainTx) {
setBlockIndexCandidates.insert(pindex);
PruneBlockIndexCandidates();
}
Expand Down Expand Up @@ -3262,7 +3282,7 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c
// add it again.
BlockMap::iterator it = mapBlockIndex.begin();
while (it != mapBlockIndex.end()) {
if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->nChainTx && !setBlockIndexCandidates.value_comp()(it->second, chainActive.Tip())) {
if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && !(it->second->nStatus & BLOCK_CONFLICT_CHAINLOCK) && it->second->nChainTx && !setBlockIndexCandidates.value_comp()(it->second, chainActive.Tip())) {
setBlockIndexCandidates.insert(it->second);
}
it++;
Expand All @@ -3283,6 +3303,77 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C
return g_chainstate.InvalidateBlock(state, chainparams, pindex);
}

bool CChainState::MarkConflictingBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex *pindex)
{
AssertLockHeld(cs_main);

// We first disconnect backwards and then mark the blocks as conflicting.

bool pindex_was_in_chain = false;
CBlockIndex *conflicting_walk_tip = chainActive.Tip();

if (pindex == pindexBestHeader) {
pindexBestHeader = pindexBestHeader->pprev;
}

DisconnectedBlockTransactions disconnectpool;
while (chainActive.Contains(pindex)) {
const CBlockIndex* pindexOldTip = chainActive.Tip();
pindex_was_in_chain = true;
// ActivateBestChain considers blocks already in chainActive
// unconditionally valid already, so force disconnect away from it.
if (!DisconnectTip(state, chainparams, &disconnectpool)) {
// It's probably hopeless to try to make the mempool consistent
// here if DisconnectTip failed, but we can try.
UpdateMempoolForReorg(disconnectpool, false);
return false;
}
if (pindexOldTip == pindexBestHeader) {
pindexBestHeader = pindexBestHeader->pprev;
}
}

// Now mark the blocks we just disconnected as descendants conflicting
// (note this may not be all descendants).
while (pindex_was_in_chain && conflicting_walk_tip != pindex) {
conflicting_walk_tip->nStatus |= BLOCK_CONFLICT_CHAINLOCK;
setBlockIndexCandidates.erase(conflicting_walk_tip);
conflicting_walk_tip = conflicting_walk_tip->pprev;
}

// Mark the block itself as conflicting.
pindex->nStatus |= BLOCK_CONFLICT_CHAINLOCK;
setBlockIndexCandidates.erase(pindex);

// DisconnectTip will add transactions to disconnectpool; try to add these
// back to the mempool.
UpdateMempoolForReorg(disconnectpool, true);

// The resulting new best tip may not be in setBlockIndexCandidates anymore, so
// add it again.
BlockMap::iterator it = mapBlockIndex.begin();
while (it != mapBlockIndex.end()) {
if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && !(it->second->nStatus & BLOCK_CONFLICT_CHAINLOCK) && it->second->nChainTx && !setBlockIndexCandidates.value_comp()(it->second, chainActive.Tip())) {
setBlockIndexCandidates.insert(it->second);
}
it++;
}

ConflictingChainFound(pindex);
GetMainSignals().SynchronousUpdatedBlockTip(chainActive.Tip(), nullptr, IsInitialBlockDownload());
GetMainSignals().UpdatedBlockTip(chainActive.Tip(), nullptr, IsInitialBlockDownload());

// Only notify about a new block tip if the active chain was modified.
if (pindex_was_in_chain) {
uiInterface.NotifyBlockTip(IsInitialBlockDownload(), pindex->pprev);
}
return true;
}

bool MarkConflictingBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex *pindex) {
return g_chainstate.MarkConflictingBlock(state, chainparams, pindex);
}

bool CChainState::ResetBlockFailureFlags(CBlockIndex *pindex) {
AssertLockHeld(cs_main);

Expand All @@ -3294,7 +3385,7 @@ bool CChainState::ResetBlockFailureFlags(CBlockIndex *pindex) {
if (!it->second->IsValid() && it->second->GetAncestor(nHeight) == pindex) {
it->second->nStatus &= ~BLOCK_FAILED_MASK;
setDirtyBlockIndex.insert(it->second);
if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->nChainTx && setBlockIndexCandidates.value_comp()(chainActive.Tip(), it->second)) {
if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && !(it->second->nStatus & BLOCK_CONFLICT_CHAINLOCK) && it->second->nChainTx && setBlockIndexCandidates.value_comp()(chainActive.Tip(), it->second)) {
setBlockIndexCandidates.insert(it->second);
}
if (it->second == pindexBestInvalid) {
Expand Down Expand Up @@ -3323,6 +3414,7 @@ bool ResetBlockFailureFlags(CBlockIndex *pindex) {

CBlockIndex* CChainState::AddToBlockIndex(const CBlockHeader& block, enum BlockStatus nStatus)
{
assert(!(nStatus & BLOCK_FAILED_MASK)); // no failed blocks alowed
AssertLockHeld(cs_main);

// Check for duplicate
Expand Down Expand Up @@ -3394,7 +3486,9 @@ void CChainState::ReceivedBlockTransactions(const CBlock& block, CBlockIndex* pi
pindex->nSequenceId = nBlockSequenceId++;
}
if (chainActive.Tip() == nullptr || !setBlockIndexCandidates.value_comp()(pindex, chainActive.Tip())) {
setBlockIndexCandidates.insert(pindex);
if (!(pindex->nStatus & BLOCK_CONFLICT_CHAINLOCK)) {
setBlockIndexCandidates.insert(pindex);
}
}
std::pair<std::multimap<CBlockIndex*, CBlockIndex*>::iterator, std::multimap<CBlockIndex*, CBlockIndex*>::iterator> range = mapBlocksUnlinked.equal_range(pindex);
while (range.first != range.second) {
Expand Down Expand Up @@ -3725,6 +3819,8 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState&
*ppindex = pindex;
if (pindex->nStatus & BLOCK_FAILED_MASK)
return state.Invalid(error("%s: block %s is marked invalid", __func__, hash.ToString()), 0, "duplicate");
if (pindex->nStatus & BLOCK_CONFLICT_CHAINLOCK)
return state.Invalid(error("%s: block %s is marked conflicting", __func__, hash.ToString()), 0, "duplicate");
return true;
}

Expand Down Expand Up @@ -4260,7 +4356,7 @@ bool CChainState::LoadBlockIndex(const Consensus::Params& consensus_params, CBlo
pindex->nStatus |= BLOCK_FAILED_CHILD;
setDirtyBlockIndex.insert(pindex);
}
if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && (pindex->nChainTx || pindex->pprev == nullptr))
if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && !(pindex->nStatus & BLOCK_CONFLICT_CHAINLOCK) && (pindex->nChainTx || pindex->pprev == nullptr))
setBlockIndexCandidates.insert(pindex);
if (pindex->nStatus & BLOCK_FAILED_MASK && (!pindexBestInvalid || pindex->nChainWork > pindexBestInvalid->nChainWork))
pindexBestInvalid = pindex;
Expand Down Expand Up @@ -4865,6 +4961,7 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
size_t nNodes = 0;
int nHeight = 0;
CBlockIndex* pindexFirstInvalid = nullptr; // Oldest ancestor of pindex which is invalid.
CBlockIndex* pindexFirstConflicing = nullptr; // Oldest ancestor of pindex which has BLOCK_CONFLICT_CHAINLOCK.
CBlockIndex* pindexFirstMissing = nullptr; // Oldest ancestor of pindex which does not have BLOCK_HAVE_DATA.
CBlockIndex* pindexFirstNeverProcessed = nullptr; // Oldest ancestor of pindex for which nTx == 0.
CBlockIndex* pindexFirstNotTreeValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_TREE (regardless of being valid or not).
Expand All @@ -4874,6 +4971,7 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
while (pindex != nullptr) {
nNodes++;
if (pindexFirstInvalid == nullptr && pindex->nStatus & BLOCK_FAILED_VALID) pindexFirstInvalid = pindex;
if (pindexFirstConflicing == nullptr && pindex->nStatus & BLOCK_CONFLICT_CHAINLOCK) pindexFirstConflicing = pindex;
if (pindexFirstMissing == nullptr && !(pindex->nStatus & BLOCK_HAVE_DATA)) pindexFirstMissing = pindex;
if (pindexFirstNeverProcessed == nullptr && pindex->nTx == 0) pindexFirstNeverProcessed = pindex;
if (pindex->pprev != nullptr && pindexFirstNotTreeValid == nullptr && (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TREE) pindexFirstNotTreeValid = pindex;
Expand Down Expand Up @@ -4914,8 +5012,12 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
// Checks for not-invalid blocks.
assert((pindex->nStatus & BLOCK_FAILED_MASK) == 0); // The failed mask cannot be set for blocks without invalid parents.
}
if (pindexFirstConflicing == nullptr) {
// Checks for not-conflciting blocks.
assert((pindex->nStatus & BLOCK_CONFLICT_CHAINLOCK) == 0); // The conflicting mask cannot be set for blocks without conflicting parents.
}
if (!CBlockIndexWorkComparator()(pindex, chainActive.Tip()) && pindexFirstNeverProcessed == nullptr) {
if (pindexFirstInvalid == nullptr) {
if (pindexFirstInvalid == nullptr && pindexFirstConflicing == nullptr) {
// If this block sorts at least as good as the current tip and
// is valid and we have all data for its parents, it must be in
// setBlockIndexCandidates. chainActive.Tip() must also be there
Expand Down Expand Up @@ -4981,6 +5083,7 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
// We are going to either move to a parent or a sibling of pindex.
// If pindex was the first with a certain property, unset the corresponding variable.
if (pindex == pindexFirstInvalid) pindexFirstInvalid = nullptr;
if (pindex == pindexFirstConflicing) pindexFirstConflicing = nullptr;
if (pindex == pindexFirstMissing) pindexFirstMissing = nullptr;
if (pindex == pindexFirstNeverProcessed) pindexFirstNeverProcessed = nullptr;
if (pindex == pindexFirstNotTreeValid) pindexFirstNotTreeValid = nullptr;
Expand Down
3 changes: 3 additions & 0 deletions src/validation.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,9 @@ bool PreciousBlock(CValidationState& state, const CChainParams& params, CBlockIn
/** Mark a block as invalid. */
bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

/** Mark a block as conflicting. */
bool MarkConflictingBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

/** Remove invalidity status from a block and its descendants. */
bool ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

Expand Down
13 changes: 11 additions & 2 deletions test/functional/feature_llmq_chainlocks.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ def run_test(self):

self.log.info("Isolate node, mine on both parts of the network, and reconnect")
isolate_node(self.nodes[0])
self.nodes[0].generate(5)
bad_tip = self.nodes[0].generate(5)[-1]
self.nodes[1].generatetoaddress(1, node0_mining_addr)
good_tip = self.nodes[1].getbestblockhash()
self.wait_for_chainlocked_block(self.nodes[1], good_tip)
Expand All @@ -76,9 +76,18 @@ def run_test(self):
assert(self.nodes[0].getblock(self.nodes[0].getbestblockhash())["previousblockhash"] == good_tip)
assert(self.nodes[1].getblock(self.nodes[1].getbestblockhash())["previousblockhash"] == good_tip)

self.log.info("The tip mined while this node was isolated should be marked conflicting now")
found = False
for tip in self.nodes[0].getchaintips(2):
if tip["hash"] == bad_tip:
assert(tip["status"] == "conflicting")
found = True
break
assert(found)

self.log.info("Keep node connected and let it try to reorg the chain")
good_tip = self.nodes[0].getbestblockhash()
self.log.info("Restart it so that it forgets all the chainlocks from the past")
self.log.info("Restart it so that it forgets all the chainlock messages from the past")
self.stop_node(0)
self.start_node(0)
connect_nodes(self.nodes[0], 1)
Expand Down

0 comments on commit 482ba4f

Please sign in to comment.