Synchronize wallet import and chain reindexing #883

dsaveliev · 2019-04-03T13:41:25Z

Fixes #810
Depends on #881

There is a race condition between import thread and wallets sync.
It's possible for the wallet to process VOTE transaction meanwhile ThreadImport didn't import the corresponding validator yet.
In order to avoid such problems during reindexing, I tried to sync these two processes.

In addition, I turned off validator state loading from backup since reindexing must restore it from scratch.
This change depends on #881, so the test will be red until the PR is merged.

Signed-off-by: Dmitry Saveliev dima@thirdhash.com

frolosofsky · 2019-04-03T14:00:32Z

The problem in the current test is that it relies (a bit) on finalization state which was restored from disk. We have finalization state serialization (see finalization::StateRepository::RestoreFromDisk) and I think it must not be called when fReindex. It would be great if we fix that right in this PR.

test/functional/feature_reindex_commits.py

src/init.cpp

src/wallet/wallet.cpp

test/functional/feature_reindex_commits.py

AM5800 · 2019-04-04T07:40:07Z

@dsaveliev I would appreciate a little bit less force-pushes and rebases. I briefly took a look yesterday evening and now this is very different, so I have to start from scratch.

test/functional/feature_reindex_commits.py

test/functional/test_framework/test_framework.py

test/functional/feature_reindex_commits.py

AM5800 · 2019-04-04T08:13:09Z

Concept ACK
Would like to see it in action with #881

scravy · 2019-04-04T10:07:44Z

Travis, twice:

feature_reindex_commits.py                     | ✖ Failed  | 18 s

dsaveliev · 2019-04-04T10:10:08Z

By design, waiting for #881

frolosofsky

I humbly ask you to ensure finalizer operating well after reindex.

test/functional/feature_reindex_commits.py

src/esperanza/walletextension.cpp

my concerns have been addressed

Signed-off-by: Dmitry Saveliev <dima@thirdhash.com>

thothd

ACK 1b18c37

Gnappuraz

utACK a419ad9

frolosofsky

ACK 1b18c37

frolosofsky · 2019-04-08T06:21:14Z

src/init.cpp

    if (gArgs.GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) {
        LoadMempool();
        fDumpMempoolLater = !fRequestShutdown;
    }
+
+    // Since the import process can restore the node's ValidatorState,


I think, mentioning of ValidatorState misses a huge step in explanation of this code purpose. I'd rephrase this comment in a way to describe how WalletExtention works when reindex is enabled. What do you think?

frolosofsky · 2019-04-08T06:34:33Z

src/init.cpp

@@ -1663,7 +1672,8 @@ bool AppInitMain()
                // UNIT-E TODO: Snapshot must start working once we can trust commits
                // (commits merkle root added to the header and FROM_COMMITS is dropped).
                // Check #836 for details.
-                if (!snapshot::IsISDEnabled()) {
+                // In the case of reindex, don't restore finalization's state, since it will be built from scratch.
+                if (!snapshot::IsISDEnabled() && !fReindex) {


I know it is not a main purpose of this PR, but since it fixes things in reindex, I guess we can do it bit better right here. I suggest reseting finalization state database in case of reindex just like CBlockIndex database does it:

pblocktree.reset(new CBlockTreeDB(nBlockTreeDBCache, false, fReset));

Where fReset=fReindex is passed as fWipe parameter to the DB.

You can configure StateDBParams.wipe from finalization::StateDB::New by using ArgsManager dependency.

frolosofsky · 2019-04-08T06:40:44Z

test/functional/feature_reindex_commits.py

+            proposer=self.proposer,
+            finalizer=self.finalizer,
+            count=10)
+        assert_equal(len(votes), 10)


I don't see a reason to generate so many epochs. This test takes ~35 seconds on my machine and huge portion of the time is unnecessary block generation. I just played with the test: changing epoch_length=5 and generating 2 epochs instead of 10 decreases test time to 10 seconds.

kostyantyn · 2019-04-08T09:01:29Z

test/functional/feature_reindex_commits.py

+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""
+Test running united with -reindex options and with finalization transactions


nit: we changed the process name to ~~unite-e~~unit-e

united -> unit-e

dsaveliev added the bug A problem of existing functionality label Apr 3, 2019

dsaveliev added this to the 0.1 milestone Apr 3, 2019

dsaveliev self-assigned this Apr 3, 2019

dsaveliev requested review from kostyantyn, AM5800, Gnappuraz, thothd and a team April 3, 2019 13:41

frolosofsky reviewed Apr 3, 2019

View reviewed changes

test/functional/feature_reindex_commits.py Outdated Show resolved Hide resolved

dsaveliev force-pushed the fix-reindex-process branch from 58c7b50 to 3814df2 Compare April 3, 2019 14:07

dsaveliev mentioned this pull request Apr 3, 2019

Allow finalizer to bootstrap from deposit rescan #881

Merged

scravy reviewed Apr 3, 2019

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

test/functional/feature_reindex_commits.py Outdated Show resolved Hide resolved

dsaveliev force-pushed the fix-reindex-process branch 2 times, most recently from 8193843 to 51ed508 Compare April 3, 2019 14:48

scravy reviewed Apr 3, 2019

View reviewed changes

test/functional/feature_reindex_commits.py Outdated Show resolved Hide resolved

test/functional/feature_reindex_commits.py Outdated Show resolved Hide resolved

dsaveliev force-pushed the fix-reindex-process branch 4 times, most recently from d6d06b6 to cb1e076 Compare April 3, 2019 22:57

AM5800 reviewed Apr 4, 2019

View reviewed changes

test/functional/feature_reindex_commits.py Outdated Show resolved Hide resolved

AM5800 reviewed Apr 4, 2019

View reviewed changes

test/functional/feature_reindex_commits.py Outdated Show resolved Hide resolved

AM5800 reviewed Apr 4, 2019

View reviewed changes

test/functional/feature_reindex_commits.py Outdated Show resolved Hide resolved

AM5800 reviewed Apr 4, 2019

View reviewed changes

test/functional/test_framework/test_framework.py Outdated Show resolved Hide resolved

AM5800 reviewed Apr 4, 2019

View reviewed changes

test/functional/test_framework/test_framework.py Outdated Show resolved Hide resolved

AM5800 reviewed Apr 4, 2019

View reviewed changes

test/functional/feature_reindex_commits.py Outdated Show resolved Hide resolved

AM5800 reviewed Apr 4, 2019

View reviewed changes

test/functional/feature_reindex_commits.py Outdated Show resolved Hide resolved

dsaveliev force-pushed the fix-reindex-process branch from cb1e076 to cd71b71 Compare April 4, 2019 09:18

dsaveliev force-pushed the fix-reindex-process branch from cd71b71 to 0afe0c6 Compare April 4, 2019 09:19

dsaveliev mentioned this pull request Apr 4, 2019

Enable snapshot and finalizer commits integration #852

Merged

1 task

dsaveliev force-pushed the fix-reindex-process branch 4 times, most recently from d81bb05 to 0ff0c65 Compare April 4, 2019 15:51

scravy mentioned this pull request Apr 4, 2019

After fast sync wallet is not in the same state as after pruning #896

Open

2 tasks

frolosofsky previously requested changes Apr 5, 2019

View reviewed changes

AM5800 reviewed Apr 5, 2019

View reviewed changes

src/esperanza/walletextension.cpp Show resolved Hide resolved

dsaveliev force-pushed the fix-reindex-process branch from adecf55 to ecf5a09 Compare April 5, 2019 08:35

AM5800 reviewed Apr 5, 2019

View reviewed changes

src/esperanza/walletextension.cpp Show resolved Hide resolved

dsaveliev force-pushed the fix-reindex-process branch 2 times, most recently from a31476b to e308ffb Compare April 5, 2019 12:53

dsaveliev added 5 commits April 5, 2019 16:23

Synchronize wallet import and chain reindexing

42f7351

Signed-off-by: Dmitry Saveliev <dima@thirdhash.com>

Correctly process validator's state during import

11ea740

Signed-off-by: Dmitry Saveliev <dima@thirdhash.com>

Test improvements

4b4dc51

Signed-off-by: Dmitry Saveliev <dima@thirdhash.com>

Fix a race condition between ThreadImport and the callbacks

594676c

Signed-off-by: Dmitry Saveliev <dima@thirdhash.com>

Rebase on master

a419ad9

Signed-off-by: Dmitry Saveliev <dima@thirdhash.com>

dsaveliev force-pushed the fix-reindex-process branch from e308ffb to a419ad9 Compare April 5, 2019 14:26

Waiting for fImporting instead of fReindex

1b18c37

Signed-off-by: Dmitry Saveliev <dima@thirdhash.com>

thothd approved these changes Apr 7, 2019

View reviewed changes

Gnappuraz approved these changes Apr 7, 2019

View reviewed changes

dsaveliev merged commit c754f47 into dtr-org:master Apr 8, 2019

dsaveliev deleted the fix-reindex-process branch April 8, 2019 06:28

frolosofsky approved these changes Apr 8, 2019

View reviewed changes

kostyantyn reviewed Apr 8, 2019

View reviewed changes

dsaveliev mentioned this pull request Apr 9, 2019

Improvements for reindex feature #921

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synchronize wallet import and chain reindexing #883

Synchronize wallet import and chain reindexing #883

dsaveliev commented Apr 3, 2019

frolosofsky commented Apr 3, 2019

AM5800 commented Apr 4, 2019

AM5800 commented Apr 4, 2019

scravy commented Apr 4, 2019

dsaveliev commented Apr 4, 2019

frolosofsky left a comment •

edited

Loading

thothd left a comment

Gnappuraz left a comment

frolosofsky left a comment

frolosofsky Apr 8, 2019 •

edited

Loading

frolosofsky Apr 8, 2019

frolosofsky Apr 8, 2019

kostyantyn Apr 8, 2019 •

edited

Loading

thothd Apr 8, 2019

Synchronize wallet import and chain reindexing #883

Synchronize wallet import and chain reindexing #883

Conversation

dsaveliev commented Apr 3, 2019

frolosofsky commented Apr 3, 2019

AM5800 commented Apr 4, 2019

AM5800 commented Apr 4, 2019

scravy commented Apr 4, 2019

dsaveliev commented Apr 4, 2019

frolosofsky left a comment • edited Loading

Choose a reason for hiding this comment

thothd left a comment

Choose a reason for hiding this comment

Gnappuraz left a comment

Choose a reason for hiding this comment

frolosofsky left a comment

Choose a reason for hiding this comment

frolosofsky Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

frolosofsky Apr 8, 2019

Choose a reason for hiding this comment

frolosofsky Apr 8, 2019

Choose a reason for hiding this comment

kostyantyn Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

thothd Apr 8, 2019

Choose a reason for hiding this comment

frolosofsky left a comment •

edited

Loading

frolosofsky Apr 8, 2019 •

edited

Loading

kostyantyn Apr 8, 2019 •

edited

Loading