Regtest reorg does not clear databases

[zathras-crypto]

Noting...

1. Send some send & trade transactions in a regtest scenario
2. Use mscrpc 1 & mscrpc 7 to see txlist and tradelist respectively
3. invalidate block 1
4. Use mscrpc 1 & mscrpc 7 again to verify databases were not cleared

[dexX7]

5. Mine one block

^--- This is the missing step.

During the block disconnect the reorgRecoveryMode flag is set, and when then connecting a new block, the reorg handling is initiated. Maybe we should change the title to: "handle reorgs sooner" or so?

This should visualize what's going on:

diff --git a/src/omnicore/omnicore.cpp b/src/omnicore/omnicore.cpp
index e031690..ee0006e 100644
--- a/src/omnicore/omnicore.cpp
+++ b/src/omnicore/omnicore.cpp
@@ -2615,6 +2615,8 @@ int mastercore_save_state( CBlockIndex const *pBlockIndex )
  */
 static void clear_all_state()
 {
+    PrintToConsole("%s(): at GetHeight()=%d\n", __func__, GetHeight());
+
     LOCK2(cs_tally, cs_pending);

     // Memory based storage
@@ -4064,6 +4066,8 @@ int mastercore_handler_block_end(int nBlockNow, CBlockIndex const * pBlockIndex,

 int mastercore_handler_disc_begin(int nBlockNow, CBlockIndex const * pBlockIndex)
 {
+    PrintToConsole("%s(): nBlockNow=%d\n", __func__, nBlockNow);
+
     LOCK(cs_tally);

     reorgRecoveryMode = 1;

[zathras-crypto]

5. Mine one block

Oh man I feel like an idiot - thanks dude :) Tested and can confirm...

[zathras-crypto]

Closing (non) issue

[dexX7]

Well, you shouldn't feel like an idiot! :) It's not intuitive, and something I noticed also.

It probably makes sense to move the reorg logic into the disconnect handlers, but this is something we should test heavily. :)

[zathras-crypto]

Hehe it just seems so obvious once you pointed it out :) Outside of testing you wouldn't really see an invalidated block without a new one to replace it...

[dexX7]

There is something I was wondering about: are we able to deal with 1 block deep reorgs properly?

While the SP database associates blocks with block hashes, the others use height as relevant identifier.

1 - 1 + 1 = 1

See where I'm going?

[zathras-crypto]

See where I'm going?

Let's look at what happens when say block 301234 gets orphaned and replaced with a new block 301234 (a 1 block deep reorg). the 'old' 301234 gets disconnected, reorgRecoveryMaxHeight gets set to 301234. Then the new block 301234 gets connected, mastercore_handler_block_begin runs in recovery mode and calls:

p_txlistdb->isMPinBlockRange(pBlockIndex->nHeight, reorgRecoveryMaxHeight, true); // inclusive
t_tradelistdb->deleteAboveBlock(pBlockIndex->nHeight - 1); // deleteAboveBlock functions are non-inclusive (>blocknum not >=blocknum)
s_stolistdb->deleteAboveBlock(pBlockIndex->nHeight - 1);

The txlistdb will delete anything in blocks 301234 to 301234 inclusive.
The tradelistdb will delete anything above block 301233.
The stolistdb will delete anything above block 301233.

Then the new block 301234 would be processed.

This seems to indicate that a 1 block deep reorg should be safe, but we could always put together a scenario in regtest to test it out?