Granular invalidateblock and RewindBlockIndex

sipa commented at 1:27 am on February 14, 2019: member

This PR makes a number of improvements to the InvalidateBlock (invalidateblock RPC) and RewindBlockIndex functions, primarily around breaking up their long-term cs_main holding. In addition:

They’re made safely interruptible (bitcoind can be shutdown, and no progress in either will be lost, though if incomplete, invalidateblock won’t continue after restart and will need to be called again)
The validation queue is prevented from overflowing (meaning invalidateblock on a very old block will not drive bitcoind OOM) (see #14289).
invalidateblock won’t bother to move transactions back into the mempool after 10 blocks (optimization).

This is not an optimal solution, as we’re relying on the scheduler call sites to make sure the scheduler doesn’t overflow. Ideally, the scheduler would guarantee this directly, but that needs a few further changes (moving the signal emissions out of cs_main) to prevent deadlocks.

I have manually tested the invalidateblock changes (including interrupting, and running with -checkblockindex and -checkmempool), but haven’t tried the rewinding (which is probably becoming increasingly unnecessary, as very few pre-0.13.1 nodes remain that would care to upgrade).

fanquake added the label Validation on Feb 14, 2019

sipa force-pushed on Feb 14, 2019

in src/test/txindex_tests.cpp:79 in 756568b928 outdated

70@@ -70,6 +71,8 @@ BOOST_FIXTURE_TEST_CASE(txindex_initial_sync, TestChain100Setup)
71     }
72 
73     txindex.Stop(); // Stop thread before calling destructor
74+    LOCK(cs_main);
75+    txindex_ptr.reset(nullptr);
76 }

MarcoFalke commented at 3:10 am on February 14, 2019:

Thanks for the txindex test fix. This should be a separate pull request, since the issue is already present (intermittent)

sipa commented at 3:11 am on February 14, 2019:

Oh, I was wondering how it could be related…

DrahtBot commented at 4:44 am on February 14, 2019: 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:

#15494 (rpc: Return whether the block was invalidated on invalidateblock by promag)
#15305 ([validation] Crash if disconnecting a block fails by sdaftuar)
#14193 (validation: Add missing mempool locks by MarcoFalke)
#13582 (Extract AppInitLoadBlockIndex from AppInitMain by Empact)

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.

laanwj added this to the milestone 0.18.0 on Feb 14, 2019

sipa force-pushed on Feb 14, 2019

MarcoFalke referenced this in commit f9d50e83e2 on Feb 16, 2019

in src/validation.cpp:2805 in 119cd3df2a outdated

2818-        }
2819-    }
2820+        DisconnectedBlockTransactions disconnectpool;
2821+        bool ret = DisconnectTip(state, chainparams, &disconnectpool);
2822+        // DisconnectTip will add transactions to disconnectpool; try to add these
2823+        // back to the mempool.

promag commented at 3:35 pm on February 18, 2019:

Commit 119cd3df2a31eb9e70863c1863c194a45d43e5c1

Add or remove depending on ret.

sipa commented at 9:32 pm on February 24, 2019:

I don’t know what you’re referring to? Is the logic wrong or are you suggested an improvement to the comments?

in src/validation.cpp:2806 in 119cd3df2a outdated

2819-    }
2820+        DisconnectedBlockTransactions disconnectpool;
2821+        bool ret = DisconnectTip(state, chainparams, &disconnectpool);
2822+        // DisconnectTip will add transactions to disconnectpool; try to add these
2823+        // back to the mempool.
2824+        UpdateMempoolForReorg(disconnectpool, ret);

promag commented at 3:36 pm on February 18, 2019:

Commit 119cd3df2a31eb9e70863c1863c194a45d43e5c1

nit, could add /* fAddToMempool = */ ret.

sipa commented at 9:33 pm on February 24, 2019:

Done.

in src/validation.cpp:2809 in 119cd3df2a outdated

2801-    CBlockIndex *invalid_walk_tip = chainActive.Tip();
2802 
2803-    DisconnectedBlockTransactions disconnectpool;
2804-    while (chainActive.Contains(pindex)) {
2805+    while (true) {
2806+        LOCK(cs_main);

promag commented at 3:46 pm on February 18, 2019:

Commit 119cd3df2a31eb9e70863c1863c194a45d43e5c1

Should relax current thread between unlock/lock on each cycle?

promag commented at 3:53 pm on February 18, 2019:

Commit ebe6eebab3b3a42da70ec99c1f30568ae0abb16f

Maybe calling ShutdownRequested is enough.

sipa commented at 7:31 pm on February 24, 2019:

Should relax current thread between unlock/lock on each cycle?

I don’t understand.

in src/validation.cpp:2798 in ebe6eebab3 outdated

2788@@ -2789,9 +2789,12 @@ bool PreciousBlock(CValidationState& state, const CChainParams& params, CBlockIn
2789 
2790 bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex *pindex)
2791 {
2792+    CBlockIndex* to_mark_failed = pindex;

promag commented at 3:56 pm on February 18, 2019:

Commit ebe6eebab3b3a42da70ec99c1f30568ae0abb16f

Could bring invalid_walk_tip to this scope instead?

sipa commented at 7:33 pm on February 24, 2019:

To which scope?

in src/validation.cpp:2810 in 119cd3df2a outdated

2826+        assert(invalid_walk_tip->pprev == chainActive.Tip());
2827 
2828-    // Now mark the blocks we just disconnected as descendants invalid
2829-    // (note this may not be all descendants).
2830-    while (pindex_was_in_chain && invalid_walk_tip != pindex) {
2831+        // We first disconnect and then mark the blocks as invalid.

promag commented at 3:59 pm on February 18, 2019:

Commit 119cd3df2a31eb9e70863c1863c194a45d43e5c1

Should be singular and then mark the block as invalid.?

sipa commented at 9:33 pm on February 24, 2019:

Improved the comments.

in src/validation.cpp:2830 in ebe6eebab3 outdated

2825+        // just child of failed)
2826+        to_mark_failed = invalid_walk_tip;
2827     }
2828 
2829     {
2830         // Mark the block itself as invalid.

promag commented at 4:01 pm on February 18, 2019:

Commit ebe6eebab3b3a42da70ec99c1f30568ae0abb16f

Update comment? I mean, can be a different block?

sipa commented at 9:34 pm on February 24, 2019:

Clarified the comment.

in src/validation.cpp:2798 in 1b9518cc7c outdated

2794@@ -2795,6 +2795,11 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c
2795     while (true) {
2796         if (ShutdownRequested()) break;
2797 
2798+        // Make sure the validation callbacks don't overflow

promag commented at 4:07 pm on February 18, 2019:

Commit 1b9518cc7c48750559a39fad4a5056d6a029ee8a

Care to elaborate? Looks like this code could be in a function or something?

sipa commented at 9:34 pm on February 24, 2019:

I’ve created a small function, and improved the comments (“overflow” wasn’t really correct and certainly confusing).

in src/validation.cpp:2815 in 3af503b4a8 outdated

2811@@ -2811,7 +2812,7 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c
2812         bool ret = DisconnectTip(state, chainparams, &disconnectpool);
2813         // DisconnectTip will add transactions to disconnectpool; try to add these
2814         // back to the mempool.
2815-        UpdateMempoolForReorg(disconnectpool, ret);
2816+        UpdateMempoolForReorg(disconnectpool, (++disconnected <= 10) && ret);

promag commented at 4:12 pm on February 18, 2019:

Commit 3af503b4a86f3743c8660f15c0a62de0289b7d94

Care to elaborate this optimization here? Looks like it could be:

0bool ret = DisconnectTip(state, chainparams, ++disconnected <= 10 ? &disconnectpool : nullptr);
1...
2if (disconnected <= 10) UpdateMempoolForReorg(disconnectpool, ret)

sipa commented at 7:49 pm on February 24, 2019:

No, we still need to call UpdateMempoolForReorg, even if we don’t want to add the disconnected transactions back to the mempool.

in src/rpc/blockchain.cpp:1589 in 78122f3d7e outdated

1582@@ -1583,15 +1583,15 @@ static UniValue invalidateblock(const JSONRPCRequest& request)
1583     uint256 hash(ParseHashV(request.params[0], "blockhash"));
1584     CValidationState state;
1585 
1586+    CBlockIndex* pblockindex;
1587     {
1588         LOCK(cs_main);
1589         CBlockIndex* pblockindex = LookupBlockIndex(hash);

MarcoFalke commented at 4:31 pm on February 18, 2019:

Commit 78122f3d7ed059b00bdaed585:

I think this is invalid cpp and wouldn’t compile

promag commented at 4:40 pm on February 18, 2019:

You mean should just be pblockindex = LookupBlockIndex(hash);?

MarcoFalke commented at 4:47 pm on February 18, 2019:

Yeah, I guess it compiles, but it might do the wrong thing.

sipa commented at 9:35 pm on February 24, 2019:

FIxed. This was fixed, but in a later commit.

in src/rpc/blockchain.cpp:1589 in 119cd3df2a outdated

1585@@ -1586,7 +1586,7 @@ static UniValue invalidateblock(const JSONRPCRequest& request)
1586     CBlockIndex* pblockindex;
1587     {
1588         LOCK(cs_main);
1589-        CBlockIndex* pblockindex = LookupBlockIndex(hash);
1590+        pblockindex = LookupBlockIndex(hash);

promag commented at 4:54 pm on February 18, 2019:

This should be in previous commit.

sipa commented at 9:35 pm on February 24, 2019:

Done.

in src/validation.cpp:4227 in 8994e394c0 outdated

4232+            nHeight++;
4233         }
4234-        nHeight++;
4235-    }
4236 
4237+        // Immediately erase post-segwit blocks without witness not in the main chain.

Sjors commented at 3:31 pm on February 22, 2019:

This bit seems to be new behaviour. Can it safely be moved into a separate commit? (I guess not because you wouldn’t get to these branches when walking down from the tip)

ryanofsky commented at 4:57 pm on February 22, 2019:

In commit “Release cs_main during RewindBlockIndex operation” (8994e394c0b4e76616c10acdd309d0225eabad9e)

Could you add some rationale in this comment saying why this is done here? Previous behavior of erasing in one place instead of two places seems simpler. Is this an optimization? Is it needed for correctness for some reason now that cs_main is released?

sipa commented at 9:39 pm on February 24, 2019:

I’ve split that commit into 4 separate ones. I think the flow of changes should be a lot clearer now.

sipa commented at 9:41 pm on February 24, 2019:

I’ve moved to to a separate step instead of lumping it together with the nHeight calculation loop.

There’s no need for it to be done while holding the lock. The reason it’s separate is because the disconnect/erase of active blocks should be done simultaneously (so that things get erased even when interrupted, and erasing can’t be done before disconnecting), but for non-mainchain blocks we need a separate loop still.

I’ve added some comments to elaborate, but I’ll leave it to you to comment further or marking it as resolved.

in src/validation.cpp:4211 in 8994e394c0 outdated

4207+}
4208+
4209 bool CChainState::RewindBlockIndex(const CChainParams& params)
4210 {
4211-    LOCK(cs_main);
4212+    CBlockIndex *tip;

Sjors commented at 3:39 pm on February 22, 2019:

Maybe explain the big picture here: first we walk from genesis to the tip to look for the first block with missing data, i.e. the first block after SegWit activation without BLOCK_OPT_WITNESS. Then we walk from the tip down to this, winding back and deleting each block.

sipa commented at 9:40 pm on February 24, 2019:

I’ve improved the comments, let me know if it’s clear now.

in src/validation.cpp:4291 in 8994e394c0 outdated

4281+            }
4282+
4283+            // Reduce validity flag and have-data flags.
4284+            // We do this after actual disconnecting, otherwise we'll end up writing the lack of data
4285+            // to disk before writing the chainstate, resulting in a failure to continue if interrupted.
4286+            // Note: If we encounter an insufficiently validated block that

Sjors commented at 3:47 pm on February 22, 2019:

This note seems out of place now; it used to be above

if (IsWitnessEnabled(pindexIter->pprev, params.GetConsensus()) && !(pindexIter->nStatus & BLOCK_OPT_WITNESS) && !chainActive.Contains(pindexIter))

Sjors commented at 10:42 pm on February 22, 2019: member

Tested 3af503b on macOS 10.14.3 by calling invalidateblock 0000000000000000001c8018d9cb3b742ef25114f27563e3fc4a1902167f9893 (SegWit activation at height 481,824) from inside the QT console, without setting -dbcache.

On master memory usage exceeds 10 GB by block 558,000. It also required a force quit to abort.

With this PR memory stays well below 1 GB and I’m able to gracefully exit during this. It then marks the most recent block as invalid, so when you restart it doesn’t suddenly start syncing to the tip again. For a user upgrading from pre-segwit, would they have to manually restart the process by calling reconsiderblock [...]?

Rewinding to SegWit activation took 7 hours on a one year old iMac. At this point it’s probably faster to just wipe the chain and sync from scratch, but that’s not for this PR. QT sync window won’t update during this, but I can live with that (something to solve for the next soft fork…).

Abstract EraseBlockData out of RewindBlockIndex

Note that the former 'else' branch in RewindBlockIndex is now
dealt with more naturally inside the EraseBlockData call (by
checking whether the parent needs to be re-added as candidate
after deleting a child).

9d6dcc52c6

Move erasure of non-active blocks to a separate loop in RewindBlockIndex

This lets us simplify the iteration to just walking back in the chain,
rather than looping over all of mapBlockIndex.

32b2696ab4

sipa force-pushed on Feb 24, 2019

Merge the disconnection and erasing loops in RewindBlockIndex 1d342875c2

sipa force-pushed on Feb 25, 2019

Release cs_main during RewindBlockIndex operation 436f7d735f

Call RewindBlockIndex without cs_main held 880ce7d46b

Make RewindBlockIndex interruptible 241b2c74ac

Call InvalidateBlock without cs_main held 9b1ff5c742

sipa force-pushed on Feb 25, 2019

in src/validation.cpp:4205 in 9d6dcc52c6 outdated

4200+            ++ret.first;
4201+        }
4202+    }
4203+    // Mark parent as eligible for main chain again
4204+    if (index->pprev && index->pprev->IsValid(BLOCK_VALID_TRANSACTIONS) && index->pprev->HaveTxsDownloaded()) {
4205+        setBlockIndexCandidates.insert(index->pprev);

Sjors commented at 12:38 pm on February 25, 2019:

IIUC this now happens every block rather than only at the end. I assume there’s no other thread potentially messing with setBlockIndexCandidates in a way that we care about?

sipa commented at 4:02 pm on February 25, 2019:

This happens with cs_main held.

Sjors commented at 7:39 pm on February 25, 2019:

Ah I see, the loop over mapBlockIndex that calls EraseBlockData is entirely inside LOCK(cs_main). So that’s indeed the same as before 9d6dcc52c6cb0cdcda220fddccaabb0ffd40068d.

sipa commented at 7:41 pm on February 25, 2019:

Oh, that too, but that doesn’t matter. The entire operation in this function could be called with a different lock per invocation.

Sjors approved

Sjors commented at 12:53 pm on February 25, 2019: member

tACK ac64328

Note that QT doesn’t update the block count (etc) as this unwinding happens, although it does update the mempool count. I can live with that for now.

in src/validation.cpp:4215 in 32b2696ab4 outdated

4208@@ -4209,9 +4209,18 @@ void CChainState::EraseBlockData(CBlockIndex* index)
4209 bool CChainState::RewindBlockIndex(const CChainParams& params)
4210 {
4211     LOCK(cs_main);
4212-
4213     // Note that during -reindex-chainstate we are called with an empty chainActive!
4214 
4215+    // First erase all post-segwit blocks without witness not in the main chain,
4216+    // as this can we done without costly DisconnectTip calls. Active

MarcoFalke commented at 9:58 pm on February 26, 2019:

in commit 32b2696ab4b079db736074b57bbc24deaee0b3d9

I don’t quite understand the comment, nor why the for loop was moved up.

Is it because the loop is cheaper to do before the DisconnectTip calls than after?
Is it because the DisconnectTip calls are cheaper after the loop ran?
…

MarcoFalke commented at 10:20 pm on February 26, 2019:

I see it is needed for the next commit

in src/validation.cpp:4277 in 436f7d735f outdated

4312+            // rewind all the way.  Blocks remaining on chainActive at this point
4313+            // must not have their validity reduced.
4314+            EraseBlockData(tip);
4315 
4316+            tip = tip->pprev;
4317+        }

MarcoFalke commented at 10:28 pm on February 26, 2019:

in commit 436f7d735f1c37e77d42ff59d4cbb1bd76d5fcfb

style-nit: FlushStateToDisk will lock cs_main anyway, so I don’t see a need for limiting the scope of this cs_main in the first place and adding unnecessary indentation.

MarcoFalke commented at 11:01 pm on February 26, 2019:

Ah, I see this is required in a later commit

MarcoFalke approved

MarcoFalke commented at 11:04 pm on February 26, 2019: member

utACK ac64328b96fbe7475d2b12a9a0c2ba4787267204

I left some feedback in earlier commits, but then it solved itself after reading the later commits. Review felt like reading a story that tells itself. Would review again 10/10

promag commented at 12:01 pm on February 27, 2019: member

Tested:

concurrent invalidateblock calls
shutdown during invalidateblock

After these tests I felt that invalidateblock could return whether the invalidation finished or not, or other meaningful data (other PR).

However when testing concurrent invalidateblock and reconsiderblock, got the following assertion on start:

0...
12019-02-27T11:53:54Z Checking all blk files are present...
22019-02-27T11:53:54Z Opening LevelDB in /Users/joao/Library/Application Support/Bitcoin/regtest/chainstate
32019-02-27T11:53:54Z Opened LevelDB successfully
42019-02-27T11:53:54Z Using obfuscation key for /Users/joao/Library/Application Support/Bitcoin/regtest/chainstate: 326eeb3c93ca8734
5Assertion failed: (!setBlockIndexCandidates.empty()), function PruneBlockIndexCandidates, file validation.cpp, line 2534.

To reproduce apply the patch

 0--- a/src/validation.cpp
 1+++ b/src/validation.cpp
 2@@ -2840,6 +2840,8 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c
 3         to_mark_failed = invalid_walk_tip;
 4     }
 5
 6+    MilliSleep(10000);
 7+
 8     {
 9         // Mark pindex itself as invalid, regardless of whether it was in the main chain or not.
10         LOCK(cs_main);

Then:

invalidateblock <old block hash>
during the above sleep call reconsiderblock <previous tip>
restart bitcoind.

promag commented at 12:19 pm on February 27, 2019: member

Conceptually, should InvalidateBlock interrupt/error if it disconnects the same tip twice? Or should invalidateblock and reconsiderblock be exclusive? Something to think and probably tackle in a different PR.

Sjors commented at 1:43 pm on February 27, 2019: member

I think invalidateblock and reconsiderblock should be exclusive (in a later PR). invalidateblock can be interrupted by exiting bitcoind (thanks to this PR), which is good enough UX for something as rare as that. I’ve only used invalidateblock for testing rollback behavior; the only other use case I can think of is a DIY soft fork :-) Well, and recovering from CVE-2018-17144 if it had been exploited.

sipa force-pushed on Feb 28, 2019

sipa commented at 10:10 pm on February 28, 2019: member

@promag I think I found what was causing the issue you observed when running reconsiderblock and invalidateblock simultaneously. The final step in the invalidation process would unconditionally mark the last disconnected block as invalid - even if reconsiderblock had made it the active block again at the same time.

Would you mind trying again? Another useful thing to test is to put a sleep inside the first loop in invalidblock (before the cs_main grab), to test the effect of a reconsiderblock in the middle of that loop.

Release cs_main during InvalidateBlock iterations 9bb32eb571

Prevent callback overruns in InvalidateBlock and RewindBlockIndex 9ce9c37004

Optimization: don't add txn back to mempool after 10 invalidates 8d220417cd

sipa force-pushed on Feb 28, 2019

promag commented at 10:18 pm on February 28, 2019: member

@sipa I did that, sleeps all over the place, and that case sounded like scratching the chain. I’ll repeat the tests.

sipa commented at 10:21 pm on February 28, 2019: member

@promag An alternative is only sleeping when “disconnected==1” (which would be in the middle for any invalidate that’s longer than 1 deep, but not sleep all the time).

It’s possible that the resulting effect is a reconsider and invalidate that start fighting with eachother and disconnect/connect the same blocks over and over again. It’s not too hard to detect this scenario inside invalidateblock and bail out, but I’d rather not overload the change here further with such edge cases.

in src/validation.cpp:2832 in 9bb32eb571 outdated

2854-    pindex->nStatus |= BLOCK_FAILED_VALID;
2855-    setDirtyBlockIndex.insert(pindex);
2856-    setBlockIndexCandidates.erase(pindex);
2857-    m_failed_blocks.insert(pindex);
2858+    {
2859+        // Mark pindex (or the last disconnected block) as invalid, regardless of whether it was in the main chain or not.

Sjors commented at 11:46 am on March 1, 2019:

Nit, move this comment below the interference check, replace “something is interfering” with “some other thread may have interfered before we reestablished the cs_main lock”.

in src/validation.cpp:2848 in 9bb32eb571 outdated

2879-            setBlockIndexCandidates.insert(it->second);
2880+        // The resulting new best tip may not be in setBlockIndexCandidates anymore, so
2881+        // add it again.
2882+        BlockMap::iterator it = mapBlockIndex.begin();
2883+        while (it != mapBlockIndex.end()) {
2884+            if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->HaveTxsDownloaded() && !setBlockIndexCandidates.value_comp()(it->second, chainActive.Tip())) {

Sjors commented at 11:54 am on March 1, 2019:

Unchanged behavior, but I’m confused why we only reconsider candidate blocks with IsValid(BLOCK_VALID_TRANSACTIONS) && HaveTxsDownloaded(), rather than any candidate with just more proof of work that’s wasn’t marked as invalid. Maybe the answer is: because such block candidates would never have been removed from setBlockIndexCandidates in the first place?

sipa commented at 6:05 pm on March 1, 2019:

That’s the definition of setBlockIndexCandidates: all blocks with valid downloaded transactions (with optionally the ones that are less cumulative work than the chain tip removed).

Sjors commented at 7:05 pm on March 1, 2019:

I think I got confused with setBlockIndexHeaderCandidates from #13937.

Sjors approved

Sjors commented at 11:58 am on March 1, 2019: member

utACK 8d22041. Several improvements in 9bb32eb571a846b66ed3bac493f55cee11a3a1b9 Release cs_main during InvalidateBlock iterations since my previous tACK. It’s more readable, e.g thanks to the additional variable to_mark_failed.

In the event this PR needs further changes (hopefully not) consider splitting that commit; moving LOCK(cs_main); into the loop and adding if (ShutdownRequested()) break; vs. everything else.

I would also prefer dealing with edge cases with concurrent RPC calls in a later PR. We can point out in the release note that it’s currently not 100% safe to do that.

gmaxwell commented at 2:28 pm on March 2, 2019: contributor

ACK

in src/validation.cpp:2848 in 8d220417cd outdated

2869+    {
2870+        // Mark pindex (or the last disconnected block) as invalid, regardless of whether it was in the main chain or not.
2871+        LOCK(cs_main);
2872+        if (chainActive.Contains(to_mark_failed)) {
2873+            // If the to-be-marked invalid block is in the active chain, something is interfering and we can't proceed.
2874+            return false;

promag commented at 4:36 pm on March 3, 2019:

Just noting that return value is not used.

in src/validation.cpp:2851 in 8d220417cd outdated

2875+        }
2876 
2877-    // DisconnectTip will add transactions to disconnectpool; try to add these
2878-    // back to the mempool.
2879-    UpdateMempoolForReorg(disconnectpool, true);
2880+        to_mark_failed->nStatus |= BLOCK_FAILED_VALID;

promag commented at 4:39 pm on March 3, 2019:

Couldn’t this be inside the loop above, in line 2810?

0if (ShutdownRequested() || !chainActive.Contains(pindex)) {
1    ...
2    return
3}

which would avoid the unlock+lock.

sipa commented at 9:04 pm on March 3, 2019:

This whole lock scope could indeed be merged into the loop above, but I think it’s clearer when it’s separate (the loop only deals with disconnecting the main chain if it were to contain pindex, the section afterwards deals with cleanups that are necessary regardless of whether pindex was in the main chain).

Given that we’re already locking/unlocking for each disconnected block, I don’t think getting rid of one extra lock/unlock matter much.

promag commented at 9:07 pm on March 3, 2019:

Ok, makes sense.

Make last disconnected block BLOCK_FAILED_VALID, even when aborted 519b0bc5dc

sipa commented at 9:13 pm on March 3, 2019: member

I added an extra commit that solves the situation where if you used invalidateblock, had a cache flush before it completed, then had your node crash, and then restarted with -checkblockindex would assert with a state corruption.

I’ve now tested a few more invalidateblock scenarios (with -checkblockindex and -checkmempool enabled), including interrupting it in the middle, killing/crashing it, and running reconsiderblock in parallel. They all seem to work fine. The RewindBlockIndex changes are untested still.

laanwj added the label Needs backport on Mar 4, 2019

gmaxwell commented at 5:51 pm on March 4, 2019: contributor

re-ACK

sipa commented at 9:57 pm on March 6, 2019: member

I think this is ready.

MarcoFalke removed the label Needs backport on Mar 6, 2019

laanwj merged this on Mar 7, 2019

laanwj closed this on Mar 7, 2019

laanwj referenced this in commit 0fd3632868 on Mar 7, 2019

laanwj referenced this in commit 3db0cc3947 on Mar 7, 2019

jasonbcox referenced this in commit c0c79f53e5 on Aug 21, 2020

PastaPastaPasta referenced this in commit b6e9ba864f on Jun 27, 2021

PastaPastaPasta referenced this in commit b132016d9e on Jun 28, 2021

PastaPastaPasta referenced this in commit 502f7cc322 on Jun 29, 2021

PastaPastaPasta referenced this in commit b159f3f33a on Jul 1, 2021

PastaPastaPasta referenced this in commit c9eba77fb9 on Jul 1, 2021

vijaydasmp referenced this in commit ab7883c5fc on Sep 5, 2021

Munkybooty referenced this in commit 0e411b63fc on Sep 5, 2021

vijaydasmp referenced this in commit d6f3fad809 on Sep 6, 2021

UdjinM6 referenced this in commit 6f0646a262 on Oct 29, 2021

pravblockc referenced this in commit fda1d429af on Nov 18, 2021

DrahtBot locked this on Dec 16, 2021

Granular invalidateblock and RewindBlockIndex #15402

Conflicts