diff --git a/src/llmq/quorums_chainlocks.cpp b/src/llmq/quorums_chainlocks.cpp index 778d58dc218a..5183c3d3860e 100644 --- a/src/llmq/quorums_chainlocks.cpp +++ b/src/llmq/quorums_chainlocks.cpp @@ -510,22 +510,28 @@ void CChainLocksHandler::EnforceBestChainLock() } bool activateNeeded; + CValidationState state; + const auto ¶ms = 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; @@ -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)); } @@ -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); diff --git a/src/llmq/quorums_chainlocks.h b/src/llmq/quorums_chainlocks.h index 32faa8a37db3..ce186ba452d3 100644 --- a/src/llmq/quorums_chainlocks.h +++ b/src/llmq/quorums_chainlocks.h @@ -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(); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 3cfbf5fc31fa..724026676fea 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -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"; diff --git a/src/validation.cpp b/src/validation.cpp index afa48619857d..7485d73f9a76 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -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); @@ -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()) { @@ -2909,9 +2925,10 @@ 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; @@ -2919,6 +2936,9 @@ CBlockIndex* CChainState::FindMostWorkChain() { 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 @@ -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(); } @@ -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++; @@ -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); @@ -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) { @@ -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 @@ -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::iterator, std::multimap::iterator> range = mapBlocksUnlinked.equal_range(pindex); while (range.first != range.second) { @@ -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; } @@ -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; @@ -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). @@ -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; @@ -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 @@ -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; diff --git a/src/validation.h b/src/validation.h index a9373e6e7bb9..813782aaca2e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -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); diff --git a/test/functional/feature_llmq_chainlocks.py b/test/functional/feature_llmq_chainlocks.py index 2a3a28866253..d1b197720046 100755 --- a/test/functional/feature_llmq_chainlocks.py +++ b/test/functional/feature_llmq_chainlocks.py @@ -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) @@ -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)