Remove dead CheckForkWarningConditionsOnNewFork

MarcoFalke commented at 5:46 am on September 7, 2020: member

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).

MarcoFalke force-pushed on Sep 7, 2020

fanquake added the label Validation on Sep 7, 2020

MarcoFalke added the label Refactoring on Sep 7, 2020

laanwj commented at 8:36 am on September 8, 2020: member

Concept ACK on removing complicated warning mechanisms if they don’t work at all, and apparently doesn’t have any tests either.

DrahtBot commented at 1:48 pm on September 19, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20158 (tree-wide: De-globalize ChainstateManager by dongcarl)

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.

in src/validation.cpp:2826 in fae0f2c792 outdated

2745@@ -2803,11 +2746,7 @@ bool CChainState::ActivateBestChainStep(BlockValidationState& state, const CChai
2746     }
2747     m_mempool.check(&CoinsTip());
2748 
2749-    // Callbacks/notifications for a new best chain.
2750-    if (fInvalidFound)
2751-        CheckForkWarningConditionsOnNewFork(vpindexToConnect.back());

dongcarl commented at 4:54 pm on October 20, 2020:

Nit: Since we’re touching this, perhaps a comment could be added to vpindexToConnect w/re how it’s ordered for future clarity?

MarcoFalke commented at 9:48 am on October 28, 2020:

Thanks. Added new commit with comment

dongcarl commented at 4:55 pm on October 20, 2020: member

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:

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index 58af8744d9..f52d8e87b5 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -1340,19 +1340,23 @@ static void CheckForkWarningConditions() EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 5 
 6     // If our best fork is no longer within 72 blocks (+/- 12 hours if no one mines it)
 7     // of our head, drop it
 8-    if (pindexBestForkTip && ::ChainActive().Height() - pindexBestForkTip->nHeight >= 72)
 9+    if (pindexBestForkTip && ::ChainActive().Height() - pindexBestForkTip->nHeight >= 72) {
10         pindexBestForkTip = nullptr;
11+        assert(false);
12+    }
13 
14     if (pindexBestForkTip || (pindexBestInvalid && pindexBestInvalid->nChainWork > ::ChainActive().Tip()->nChainWork + (GetBlockProof(*::ChainActive().Tip()) * 6)))
15     {
16         if (!GetfLargeWorkForkFound() && pindexBestForkBase)
17         {
18+            assert(false);
19             std::string warning = std::string("'Warning: Large-work fork detected, forking after block ") +
20                 pindexBestForkBase->phashBlock->ToString() + std::string("'");
21             AlertNotify(warning);
22         }
23         if (pindexBestForkTip && pindexBestForkBase)
24         {
25+            assert(false);
26             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__,
27                    pindexBestForkBase->nHeight, pindexBestForkBase->phashBlock->ToString(),
28                    pindexBestForkTip->nHeight, pindexBestForkTip->phashBlock->ToString());
29@@ -1397,6 +1401,7 @@ static void CheckForkWarningConditionsOnNewFork(CBlockIndex* pindexNewForkTip) E
30             pindexNewForkTip->nChainWork - pfork->nChainWork > (GetBlockProof(*pfork) * 7) &&
31             ::ChainActive().Height() - pindexNewForkTip->nHeight < 72)
32     {
33+        assert(false);
34         pindexBestForkTip = pindexNewForkTip;
35         pindexBestForkBase = pfork;
36     }
37diff --git a/src/warnings.cpp b/src/warnings.cpp
38index 501bf7e637..c0a266613c 100644
39--- a/src/warnings.cpp
40+++ b/src/warnings.cpp
41@@ -26,6 +26,7 @@ void SetMiscWarning(const bilingual_str& warning)
42 void SetfLargeWorkForkFound(bool flag)
43 {
44     LOCK(g_warnings_mutex);
45+    assert(!flag);
46     fLargeWorkForkFound = flag;
47 }

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

MarcoFalke commented at 11:27 am on October 27, 2020: member

The system has been introduced in #2658 , where it was tested to be working. So it might have been working at one point, but might be broken after commit 4e0eed88acdd41826868c151373068bfad18b84d (not verified, just a guess). Also the bugfix in commit a2f678d3552aaaaa8d6f480cf5f2fbc849f376c3 is presumably incomplete.

MarcoFalke commented at 11:28 am on October 27, 2020: member

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.

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.

MarcoFalke added the label Review club on Oct 27, 2020

practicalswift commented at 7:05 am on October 28, 2020: contributor

Concept ACK

in src/validation.cpp:2695 in fad05ee544 outdated

2691@@ -2749,7 +2692,7 @@ bool CChainState::ActivateBestChainStep(BlockValidationState& state, const CChai
2692         fBlocksDisconnected = true;
2693     }
2694 
2695-    // Build list of new blocks to connect.
2696+    // Build list of new blocks to connect (in reverse order).

dongcarl commented at 2:41 pm on October 30, 2020:

Nit, feel free to ignore.

0    // Build list of new blocks to connect (in descending height order).

MarcoFalke commented at 4:12 pm on October 30, 2020:

Picked

MarcoFalke force-pushed on Oct 30, 2020

dongcarl commented at 4:44 pm on October 30, 2020: member

re-ACK fae6cefdb2b708b0d5f329cecd804fa9315b043b

in src/warnings.cpp:26 in fae6cefdb2 outdated

22@@ -23,18 +23,6 @@ void SetMiscWarning(const bilingual_str& warning)
23     g_misc_warnings = warning;
24 }
25 
26-void SetfLargeWorkForkFound(bool flag)

jnewbery commented at 10:41 am on November 3, 2020:

You can also remove the fLargeWorkForkFound bool above and the if (fLargeWorkForkFound) check below (the bool only ever gets initialized to false and never updated).

MarcoFalke commented at 1:03 pm on November 3, 2020:

thx, removed

in src/validation.cpp:1340 in fae6cefdb2 outdated

1341-    if (pindexBestForkTip && ::ChainActive().Height() - pindexBestForkTip->nHeight >= 72)
1342-        pindexBestForkTip = nullptr;
1343-
1344-    if (pindexBestForkTip || (pindexBestInvalid && pindexBestInvalid->nChainWork > ::ChainActive().Tip()->nChainWork + (GetBlockProof(*::ChainActive().Tip()) * 6)))
1345+    if (pindexBestInvalid && pindexBestInvalid->nChainWork > ::ChainActive().Tip()->nChainWork + (GetBlockProof(*::ChainActive().Tip()) * 6))
1346     {

jnewbery commented at 12:02 pm on November 3, 2020:

Since you’re already touching the line above, consider joining this with that line to comply with current code style guide. Same for else statement below.

MarcoFalke commented at 1:03 pm on November 3, 2020:

thx, style fixed up

jnewbery commented at 12:32 pm on November 3, 2020: member

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.

Remove dead CheckForkWarningConditionsOnNewFork fa62304c97

doc: Clarify that vpindexToConnect is in reverse order

Also, style-fixups of touched code

fa7eed5be7

MarcoFalke force-pushed on Nov 3, 2020

jnewbery commented at 2:33 pm on November 3, 2020: member

utACK fa7eed5be704ccdbdce5c9aedb953dd9c8b30446

fjahr commented at 10:59 pm on November 4, 2020: member

Code review ACK fa7eed5be704ccdbdce5c9aedb953dd9c8b30446

I agree that this code is broken as described and removing and looking for a better approach seems the right way to go.

glozow commented at 1:02 pm on November 5, 2020: member

utACK https://github.com/bitcoin/bitcoin/commit/fa7eed5be704ccdbdce5c9aedb953dd9c8b30446 I see that it’s dead code

MarcoFalke merged this on Nov 18, 2020

MarcoFalke closed this on Nov 18, 2020

MarcoFalke deleted the branch on Nov 18, 2020

sidhujag referenced this in commit 4548e94d91 on Nov 18, 2020

DrahtBot locked this on Feb 15, 2022

Remove dead CheckForkWarningConditionsOnNewFork #19905

Conflicts