diff options
author | MarcoFalke <falke.marco@gmail.com> | 2020-11-18 11:23:50 +0100 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2020-11-18 11:23:56 +0100 |
commit | a64ff1c4d32c0458660426e4f40b45b8b7c13a87 (patch) | |
tree | 97d10b5c1a8ceb7527a441bcb1bac8017708afcb /src/validation.cpp | |
parent | 54532f46c4e257cde682d492de72cde261998d3f (diff) | |
parent | fa7eed5be704ccdbdce5c9aedb953dd9c8b30446 (diff) | |
download | bitcoin-a64ff1c4d32c0458660426e4f40b45b8b7c13a87.tar.xz |
Merge #19905: Remove dead CheckForkWarningConditionsOnNewFork
fa7eed5be704ccdbdce5c9aedb953dd9c8b30446 doc: Clarify that vpindexToConnect is in reverse order (MarcoFalke)
fa62304c9760f0de9838e56150008816e7a9bacb Remove dead CheckForkWarningConditionsOnNewFork (MarcoFalke)
Pull request description:
The function has several code and logic bugs, which prevent it from working at all:
* `vpindexToConnect.back()` is passed to `CheckForkWarningConditionsOnNewFork`, which is the earliest connected block (least work block), *not* the new fork tip
* `ActivateBestChainStep` will never try to connect a block that descends from an invalid block, so the invalid fork will only ever be of height 1, never hitting the 7 block minimum condition
Instead of dragging the dead and wrong code around through every change in validation, remove it. In the future it could make sense to add a fork detection somewhere outside of the `ActivateBestChainStep` logic (maybe net_processing).
ACKs for top commit:
jnewbery:
utACK fa7eed5be704ccdbdce5c9aedb953dd9c8b30446
fjahr:
Code review ACK fa7eed5be704ccdbdce5c9aedb953dd9c8b30446
glozow:
utACK https://github.com/bitcoin/bitcoin/commit/fa7eed5be704ccdbdce5c9aedb953dd9c8b30446 I see that it's dead code
Tree-SHA512: 815bdbac7c1eb5b7594b0866a2dbd3c7619797afaadb03a5269fb96739ffb83b05b8e4f7c1e68d48d7886132dd0b12c14c3fb4ee0e72de1074726050ed203e1a
Diffstat (limited to 'src/validation.cpp')
-rw-r--r-- | src/validation.cpp | 87 |
1 files changed, 12 insertions, 75 deletions
diff --git a/src/validation.cpp b/src/validation.cpp index feb7502a0f..8c87c53ac7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1312,8 +1312,6 @@ bool CChainState::IsInitialBlockDownload() const return false; } -static CBlockIndex *pindexBestForkTip = nullptr, *pindexBestForkBase = nullptr; - static void AlertNotify(const std::string& strMessage) { uiInterface.NotifyAlertChanged(); @@ -1339,73 +1337,16 @@ static void CheckForkWarningConditions() EXCLUSIVE_LOCKS_REQUIRED(cs_main) AssertLockHeld(cs_main); // Before we get past initial download, we cannot reliably alert about forks // (we assume we don't get stuck on a fork before finishing our initial sync) - if (::ChainstateActive().IsInitialBlockDownload()) + if (::ChainstateActive().IsInitialBlockDownload()) { return; - - // If our best fork is no longer within 72 blocks (+/- 12 hours if no one mines it) - // of our head, drop it - if (pindexBestForkTip && ::ChainActive().Height() - pindexBestForkTip->nHeight >= 72) - pindexBestForkTip = nullptr; - - if (pindexBestForkTip || (pindexBestInvalid && pindexBestInvalid->nChainWork > ::ChainActive().Tip()->nChainWork + (GetBlockProof(*::ChainActive().Tip()) * 6))) - { - if (!GetfLargeWorkForkFound() && pindexBestForkBase) - { - std::string warning = std::string("'Warning: Large-work fork detected, forking after block ") + - pindexBestForkBase->phashBlock->ToString() + std::string("'"); - AlertNotify(warning); - } - if (pindexBestForkTip && pindexBestForkBase) - { - LogPrintf("%s: Warning: Large valid fork found\n forking the chain at height %d (%s)\n lasting to height %d (%s).\nChain state database corruption likely.\n", __func__, - pindexBestForkBase->nHeight, pindexBestForkBase->phashBlock->ToString(), - pindexBestForkTip->nHeight, pindexBestForkTip->phashBlock->ToString()); - SetfLargeWorkForkFound(true); - } - else - { - LogPrintf("%s: Warning: Found invalid chain at least ~6 blocks longer than our best chain.\nChain state database corruption likely.\n", __func__); - SetfLargeWorkInvalidChainFound(true); - } } - else - { - SetfLargeWorkForkFound(false); - SetfLargeWorkInvalidChainFound(false); - } -} -static void CheckForkWarningConditionsOnNewFork(CBlockIndex* pindexNewForkTip) EXCLUSIVE_LOCKS_REQUIRED(cs_main) -{ - AssertLockHeld(cs_main); - // If we are on a fork that is sufficiently large, set a warning flag - CBlockIndex* pfork = pindexNewForkTip; - CBlockIndex* plonger = ::ChainActive().Tip(); - while (pfork && pfork != plonger) - { - while (plonger && plonger->nHeight > pfork->nHeight) - plonger = plonger->pprev; - if (pfork == plonger) - break; - pfork = pfork->pprev; - } - - // We define a condition where we should warn the user about as a fork of at least 7 blocks - // with a tip within 72 blocks (+/- 12 hours if no one mines it) of ours - // We use 7 blocks rather arbitrarily as it represents just under 10% of sustained network - // hash rate operating on the fork. - // or a chain that is entirely longer than ours and invalid (note that this should be detected by both) - // We define it this way because it allows us to only store the highest fork tip (+ base) which meets - // the 7-block condition and from this always have the most-likely-to-cause-warning fork - if (pfork && (!pindexBestForkTip || pindexNewForkTip->nHeight > pindexBestForkTip->nHeight) && - pindexNewForkTip->nChainWork - pfork->nChainWork > (GetBlockProof(*pfork) * 7) && - ::ChainActive().Height() - pindexNewForkTip->nHeight < 72) - { - pindexBestForkTip = pindexNewForkTip; - pindexBestForkBase = pfork; + if (pindexBestInvalid && pindexBestInvalid->nChainWork > ::ChainActive().Tip()->nChainWork + (GetBlockProof(*::ChainActive().Tip()) * 6)) { + LogPrintf("%s: Warning: Found invalid chain at least ~6 blocks longer than our best chain.\nChain state database corruption likely.\n", __func__); + SetfLargeWorkInvalidChainFound(true); + } else { + SetfLargeWorkInvalidChainFound(false); } - - CheckForkWarningConditions(); } // Called both upon regular invalid block discovery *and* InvalidateBlock @@ -2746,8 +2687,8 @@ bool CChainState::ActivateBestChainStep(BlockValidationState& state, const CChai AssertLockHeld(cs_main); AssertLockHeld(m_mempool.cs); - const CBlockIndex *pindexOldTip = m_chain.Tip(); - const CBlockIndex *pindexFork = m_chain.FindFork(pindexMostWork); + const CBlockIndex* pindexOldTip = m_chain.Tip(); + const CBlockIndex* pindexFork = m_chain.FindFork(pindexMostWork); // Disconnect active blocks which are no longer in the best chain. bool fBlocksDisconnected = false; @@ -2767,7 +2708,7 @@ bool CChainState::ActivateBestChainStep(BlockValidationState& state, const CChai fBlocksDisconnected = true; } - // Build list of new blocks to connect. + // Build list of new blocks to connect (in descending height order). std::vector<CBlockIndex*> vpindexToConnect; bool fContinue = true; int nHeight = pindexFork ? pindexFork->nHeight : -1; @@ -2777,7 +2718,7 @@ bool CChainState::ActivateBestChainStep(BlockValidationState& state, const CChai int nTargetHeight = std::min(nHeight + 32, pindexMostWork->nHeight); vpindexToConnect.clear(); vpindexToConnect.reserve(nTargetHeight - nHeight); - CBlockIndex *pindexIter = pindexMostWork->GetAncestor(nTargetHeight); + CBlockIndex* pindexIter = pindexMostWork->GetAncestor(nTargetHeight); while (pindexIter && pindexIter->nHeight != nHeight) { vpindexToConnect.push_back(pindexIter); pindexIter = pindexIter->pprev; @@ -2785,7 +2726,7 @@ bool CChainState::ActivateBestChainStep(BlockValidationState& state, const CChai nHeight = nTargetHeight; // Connect new blocks. - for (CBlockIndex *pindexConnect : reverse_iterate(vpindexToConnect)) { + for (CBlockIndex* pindexConnect : reverse_iterate(vpindexToConnect)) { if (!ConnectTip(state, chainparams, pindexConnect, pindexConnect == pindexMostWork ? pblock : std::shared_ptr<const CBlock>(), connectTrace, disconnectpool)) { if (state.IsInvalid()) { // The block violates a consensus rule. @@ -2821,11 +2762,7 @@ bool CChainState::ActivateBestChainStep(BlockValidationState& state, const CChai } m_mempool.check(&CoinsTip()); - // Callbacks/notifications for a new best chain. - if (fInvalidFound) - CheckForkWarningConditionsOnNewFork(vpindexToConnect.back()); - else - CheckForkWarningConditions(); + CheckForkWarningConditions(); return true; } |