Remove dead CheckForkWarningConditionsOnNewFork#19905
Remove dead CheckForkWarningConditionsOnNewFork#19905maflcko merged 2 commits intobitcoin:masterfrom
Conversation
maflcko
force-pushed
the
2009-noFork
branch
2 times, most recently
from
fabfc65 to
fae0f2c
Compare
|
Concept ACK on removing complicated warning mechanisms if they don't work at all, and apparently doesn't have any tests either. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
dongcarl
left a comment
dongcarl
left a comment
There was a problem hiding this comment.
Code Review ACK fae0f2c792a5096279dd4ff8da46fdbe43ca013a
I think you're right on both accounts, and the change looks correct.
I used the following patch on HEAD^ to demonstrate correctness to myself:
diff --git a/src/validation.cpp b/src/validation.cpp
index 58af8744d9..f52d8e87b5 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1340,19 +1340,23 @@ static void CheckForkWarningConditions() EXCLUSIVE_LOCKS_REQUIRED(cs_main)
// 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)
+ if (pindexBestForkTip && ::ChainActive().Height() - pindexBestForkTip->nHeight >= 72) {
pindexBestForkTip = nullptr;
+ assert(false);
+ }
if (pindexBestForkTip || (pindexBestInvalid && pindexBestInvalid->nChainWork > ::ChainActive().Tip()->nChainWork + (GetBlockProof(*::ChainActive().Tip()) * 6)))
{
if (!GetfLargeWorkForkFound() && pindexBestForkBase)
{
+ assert(false);
std::string warning = std::string("'Warning: Large-work fork detected, forking after block ") +
pindexBestForkBase->phashBlock->ToString() + std::string("'");
AlertNotify(warning);
}
if (pindexBestForkTip && pindexBestForkBase)
{
+ assert(false);
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());
@@ -1397,6 +1401,7 @@ static void CheckForkWarningConditionsOnNewFork(CBlockIndex* pindexNewForkTip) E
pindexNewForkTip->nChainWork - pfork->nChainWork > (GetBlockProof(*pfork) * 7) &&
::ChainActive().Height() - pindexNewForkTip->nHeight < 72)
{
+ assert(false);
pindexBestForkTip = pindexNewForkTip;
pindexBestForkBase = pfork;
}
diff --git a/src/warnings.cpp b/src/warnings.cpp
index 501bf7e637..c0a266613c 100644
--- a/src/warnings.cpp
+++ b/src/warnings.cpp
@@ -26,6 +26,7 @@ void SetMiscWarning(const bilingual_str& warning)
void SetfLargeWorkForkFound(bool flag)
{
LOCK(g_warnings_mutex);
+ assert(!flag);
fLargeWorkForkFound = flag;
}For the future, I do wonder if it might be better to instead have GetWarnings reach into validation.cpp and perform the comparison between the active tip and pindexBestInvalid when it's needed.
This does 2 things:
- validation will no longer depend on
warnings.cpp - We will no longer have to reason about when to update
fLargeWorkInvalidChainFound
Sounds good. To keep the changes here delete-only, I am happy to address the suggestion in a follow-up or volunteer for review if someone beats me to it. |
|
Concept ACK |
|
re-ACK fae6cefdb2b708b0d5f329cecd804fa9315b043b |
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
I agree with the analysis here that CheckForkWarningConditionsOnNewFork() can only be called with a CBlockIndex which is for a block that is on the valid chain or the child of a block on the valid chain. This code is therefore dead and should be removed.
Also, style-fixups of touched code
|
utACK fa7eed5 |
|
Code review ACK fa7eed5 I agree that this code is broken as described and removing and looking for a better approach seems the right way to go. |
fa7eed5 doc: Clarify that vpindexToConnect is in reverse order (MarcoFalke) fa62304 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 fa7eed5 fjahr: Code review ACK fa7eed5 glozow: utACK bitcoin@fa7eed5 I see that it's dead code Tree-SHA512: 815bdbac7c1eb5b7594b0866a2dbd3c7619797afaadb03a5269fb96739ffb83b05b8e4f7c1e68d48d7886132dd0b12c14c3fb4ee0e72de1074726050ed203e1a
9 participants
The function has several code and logic bugs, which prevent it from working at all:
vpindexToConnect.back()is passed toCheckForkWarningConditionsOnNewFork, which is the earliest connected block (least work block), not the new fork tipActivateBestChainStepwill 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 conditionInstead 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
ActivateBestChainSteplogic (maybe net_processing).