wallet: Save wallet scan progress #25036

w0xlt · 2022-04-29T22:23:09Z

Currently, the wallet scan progress is not saved.
If it is interrupted, it will be necessary to start from scratch on the next load.
This PR changes this and the progress is saved right after checking a block.

Close #25010

mzumsande

I had thought about this too a bit:
ScanForWalletTransactions() can also be invoked from rescanblockchain RPC with a start_height and stop_height - I think we wouldn't want to save the progress in this case, e.g. if a user chooses to run rescanblockchain for some historic range, the best block should stay at the tip. So maybe only save progress if the function is invoked from AttachChain() ?

w0xlt · 2022-04-29T23:09:47Z

@mzumsande #25036 (review) addressed in 170fb4c

src/node/interfaces.cpp

src/wallet/wallet.h

src/wallet/wallet.cpp

w0xlt · 2022-05-10T22:20:57Z

37f192b addresses #25036 (comment) and #25036 (comment).

. src/interfaces/chain.h:getLocator() returns std::optional<CBlockLocator> instead of CBlockLocator
. The best block is written in wallet DB every 60 seconds instead of every block.

w0xlt · 2022-05-12T18:47:12Z

Rebased.

furszy

Concept ACK, code reviewed 6b879a1.

I think that we could simplify it a bit more in this way: furszy@e48ec2e (squash it directly if you like it, no need to add any co-authorship nor mention).

Side from that, might worth to think on ways to test this new functionality.
A quick idea could be make INTERVAL_TIME customizable and set it to 0, mock the wallet database and check how many times a BESTBLOCK key is written during the scan process. Then compare the obtained value with the number of blocks in the test chain.

jonatack

Concept ACK, agree with @furszy that test coverage would be good.

src/wallet/wallet.h

src/wallet/wallet.cpp

src/node/interfaces.cpp

src/wallet/wallet.cpp

jonatack · 2022-05-27T11:25:42Z

src/node/interfaces.cpp

        const CChain& active = Assert(m_node.chainman)->ActiveChain();
        return active.GetLocator();
    }
+    std::optional<CBlockLocator> getLocator(const uint256& block_hash) override


I think this child implementation member can be private. See #24150 for rationale.

I didn't quite understand the suggestion. If this method is changed to private, the wallet will not be able to access it.

re: #25036 (comment)

I didn't quite understand the suggestion. If this method is changed to private, the wallet will not be able to access it.

I'm neutral on the suggestion (don't see concrete benefits), but the wallet can still call the method even if it is private because it's virtual and the overridden method is public.

If you want to take implement suggestion of making it private, for the sake of consistency I would suggest making all the methods private not this just one method.

for the sake of consistency I would suggest making all the methods private

So I think it would be better to do this in a follow-up PR since it involves other unrelated methods.

DrahtBot · 2022-06-01T15:48:20Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25489 (wallet: change ScanForWalletTransactions to use Ticks(Dur2 d) by w0xlt)

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.

w0xlt · 2022-06-01T17:31:30Z

@furszy @jonatack I've added you as co-authors due to the suggested improvements.

I can add a test in a new commit or in a new PR.
I'm not quite sure how to create this test yet. I suppose you are referring to a unit test and not a functional one, right?

furszy · 2022-06-01T19:44:15Z

cool @w0xlt. It wasn't needed but appreciated 👍🏼 .

yeah, an unit test so the wallet db can be mocked and write calls intercepted. It should be simpler to implement than a functional test. We could chat tomorrow via IRC if you want as well.

ryanofsky

Code review ACK 2475146. I implemented a unit test for this in cdcda3b (branch) which would be nice to include here.

ryanofsky · 2022-06-08T19:44:58Z

src/node/interfaces.cpp

        const CChain& active = Assert(m_node.chainman)->ActiveChain();
        return active.GetLocator();
    }
+    std::optional<CBlockLocator> getLocator(const uint256& block_hash) override


re: #25036 (comment)

I didn't quite understand the suggestion. If this method is changed to private, the wallet will not be able to access it.

I'm neutral on the suggestion (don't see concrete benefits), but the wallet can still call the method even if it is private because it's virtual and the overridden method is public.

If you want to take implement suggestion of making it private, for the sake of consistency I would suggest making all the methods private not this just one method.

src/interfaces/chain.h

src/node/interfaces.cpp

w0xlt · 2022-06-09T02:33:38Z

@ryanofsky Thanks for suggestions. I initially pushed a test that changed the time interval to 0s (as suggested by @furszy) but I think adding to WalletRescanReserver the ability to simulate a time is also a very good option as it doesn't change the signature of the rescan method with a new parameter.
I added your commit.

src/wallet/wallet.cpp

ryanofsky

Code review ACK 089f3ccc8769c1d1d096f5c45f4a29e5cce85557, but the first commit doesn't compile and it would be good to fix that. Only suggested changes since last review

src/node/interfaces.cpp

src/wallet/wallet.cpp

ryanofsky

Code review ACK 0ef383dd058a783bf0912c8562597823e2dbb29b. Only changes since last review: cleaning up the wonky Assert() and moving a change to the second commit so the first commit compiles

furszy

Code review ACK 0ef383dd

Verified that the added test saves the progress for the two scanned blocks (100 and 101).

Left two non-blocking nits.

src/wallet/wallet.cpp

Currently, the wallet scan progress is not saved. If it is interrupted, it will be necessary to start from scratch on the next load. With this change, progress is saved every 60 seconds. Co-authored-by: furszy <matiasfurszyfer@protonmail.com> Co-authored-by: Jon Atack <jon@atack.com> Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

w0xlt · 2022-06-23T20:27:54Z

Force-pushed 230a2f4 addressing @furszy suggestions.

furszy

re-ACK 230a2f4

Only changes were the missing space and logging.

achow101

ACK 230a2f4

achow101 · 2022-06-24T20:46:54Z

src/node/interfaces.cpp

    bool haveBlockOnDisk(int height) override
    {
-        LOCK(cs_main);
+        LOCK(::cs_main);


In a89ddfb "wallet: Save wallet scan progress"

nit: Unrelated change. Also in getTipLocator, findLocatorFork, GuessVerificationProgress and havePruned.

would second to not include these unnecessary changes, git blame depth

ryanofsky

Code review ACK 230a2f4. Only change since last review is tweaking whitespace and adding log print

230a2f4 wallet test: Add unit test for wallet scan save_progress option (Ryan Ofsky) a89ddfb wallet: Save wallet scan progress (w0xlt) Pull request description: Currently, the wallet scan progress is not saved. If it is interrupted, it will be necessary to start from scratch on the next load. This PR changes this and the progress is saved right after checking a block. Close bitcoin#25010 ACKs for top commit: furszy: re-ACK 230a2f4 achow101: ACK 230a2f4 ryanofsky: Code review ACK 230a2f4. Only change since last review is tweaking whitespace and adding log print Tree-SHA512: 1a9dec207ed22b3443fb06a4daf967637bc02bcaf71c070b7dc33605d0cab959551e4014c9e92293a63f54c5cbcc98bb9f8844a8c60bc32a1482b1c4130fab32

mzumsande reviewed Apr 29, 2022

View reviewed changes

DrahtBot added interfaces Wallet labels Apr 29, 2022

w0xlt force-pushed the save_scan_progress branch from 5d84920 to 170fb4c Compare April 29, 2022 23:07

aureleoules reviewed May 6, 2022

View reviewed changes

src/node/interfaces.cpp Outdated Show resolved Hide resolved

src/node/interfaces.cpp Outdated Show resolved Hide resolved

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

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

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

w0xlt force-pushed the save_scan_progress branch from 170fb4c to 37f192b Compare May 10, 2022 22:15

DrahtBot added the Needs rebase label May 11, 2022

w0xlt force-pushed the save_scan_progress branch from 37f192b to 6b879a1 Compare May 12, 2022 15:46

DrahtBot removed the Needs rebase label May 12, 2022

furszy reviewed May 26, 2022

View reviewed changes

jonatack reviewed May 27, 2022

View reviewed changes

w0xlt force-pushed the save_scan_progress branch 2 times, most recently from 514d17d to 2475146 Compare June 1, 2022 00:23

DrahtBot mentioned this pull request Jun 1, 2022

CChainState -> Chainstate #24513

Merged

ryanofsky approved these changes Jun 8, 2022

View reviewed changes

w0xlt force-pushed the save_scan_progress branch 3 times, most recently from 2475146 to 18926f9 Compare June 9, 2022 02:20

ryanofsky reviewed Jun 9, 2022

View reviewed changes

src/node/interfaces.cpp Outdated Show resolved Hide resolved

ryanofsky reviewed Jun 9, 2022

View reviewed changes

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

w0xlt force-pushed the save_scan_progress branch from 18926f9 to 089f3cc Compare June 9, 2022 02:44

ryanofsky approved these changes Jun 9, 2022

View reviewed changes

src/node/interfaces.cpp Outdated Show resolved Hide resolved

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

w0xlt force-pushed the save_scan_progress branch from 089f3cc to 0ef383d Compare June 9, 2022 22:18

ryanofsky approved these changes Jun 15, 2022

View reviewed changes

furszy approved these changes Jun 16, 2022

View reviewed changes

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

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

w0xlt and others added 2 commits June 23, 2022 17:13

wallet test: Add unit test for wallet scan save_progress option

230a2f4

w0xlt force-pushed the save_scan_progress branch from 0ef383d to 230a2f4 Compare June 23, 2022 20:23

furszy approved these changes Jun 24, 2022

View reviewed changes

achow101 reviewed Jun 24, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 29, 2022

wallet: change ScanForWalletTransactions to use Ticks(Dur2 d) #25489

Merged

ryanofsky approved these changes Jul 11, 2022

View reviewed changes

maflcko merged commit 7ba0850 into bitcoin:master Jul 12, 2022

w0xlt deleted the save_scan_progress branch July 12, 2022 13:02

bitcoin locked and limited conversation to collaborators Jul 12, 2023

wallet: Save wallet scan progress #25036

wallet: Save wallet scan progress #25036

Uh oh!

Conversation

w0xlt commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt commented Apr 29, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

w0xlt commented May 10, 2022

Uh oh!

w0xlt commented May 12, 2022

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 8, 2022

Choose a reason for hiding this comment

Uh oh!

w0xlt Jun 9, 2022

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

w0xlt commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Jun 1, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

w0xlt commented Jun 9, 2022

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt commented Apr 29, 2022 •

edited

Loading

mzumsande left a comment •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack May 27, 2022 •

edited

Loading

DrahtBot commented Jun 1, 2022 •

edited

Loading

w0xlt commented Jun 1, 2022 •

edited

Loading

furszy left a comment •

edited

Loading