wallet: Save wallet scan progress #25036

pull w0xlt wants to merge 2 commits into bitcoin:master from w0xlt:save_scan_progress changing 8 files +68 −24
  1. w0xlt commented at 10:23 pm on April 29, 2022: contributor

    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 https://github.com/bitcoin/bitcoin/issues/25010

  2. mzumsande commented at 10:32 pm on April 29, 2022: contributor
    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() ?
  3. DrahtBot added the label interfaces on Apr 29, 2022
  4. DrahtBot added the label Wallet on Apr 29, 2022
  5. w0xlt force-pushed on Apr 29, 2022
  6. w0xlt commented at 11:09 pm on April 29, 2022: contributor
    @mzumsande #25036#pullrequestreview-958358214 addressed in https://github.com/bitcoin/bitcoin/pull/25036/commits/170fb4ca5522de7057a3199e23cd7602241da1c8
  7. in src/node/interfaces.cpp:498 in 170fb4ca55 outdated 
    491+    {
492+        LOCK(cs_main);
493+        const CChain& active = Assert(m_node.chainman)->ActiveChain();
494+        const CBlockIndex *index = m_node.chainman->m_blockman.LookupBlockIndex(block_hash);
495+
496+        return active.GetLocator(index);
    aureleoules commented at 2:36 pm on May 6, 2022:
    If the block index is not found (nullptr) the locator will fallback on the tip, I’m wondering if this is intended?
    w0xlt commented at 2:57 pm on May 9, 2022:

    I kept in src/node/interfaces.cpp the same behavior defined in src/chain.cpp:CCain::GetLocator.

    As this method is only called by ScanForWalletTransactions() it will not receive any non-existent blocks. This call only exists inside the condition (chain().findBlock() and if (!block.IsNull())).

    I can change this if necessary.

  8. in src/node/interfaces.cpp:494 in 170fb4ca55 outdated 
    486@@ -487,6 +487,14 @@ class ChainImpl : public Chain
487         const CChain& active = Assert(m_node.chainman)->ActiveChain();
488         return active.GetLocator();
489     }
490+    CBlockLocator getLocator(const uint256& block_hash) override
491+    {
492+        LOCK(cs_main);
493+        const CChain& active = Assert(m_node.chainman)->ActiveChain();
494+        const CBlockIndex *index = m_node.chainman->m_blockman.LookupBlockIndex(block_hash);
    aureleoules commented at 2:37 pm on May 6, 2022:

    nit

    0        const CBlockIndex* index = m_node.chainman->m_blockman.LookupBlockIndex(block_hash);
  9. in src/wallet/wallet.h:539 in 170fb4ca55 outdated 
    535@@ -536,7 +536,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
536         //! USER_ABORT.
537         uint256 last_failed_block;
538     };
539-    ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate);
540+    ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate, const bool save_progress=false);
    aureleoules commented at 2:46 pm on May 6, 2022:

    nit

    0    ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate, const bool save_progress = false);
  10. in src/wallet/wallet.cpp:1776 in 170fb4ca55 outdated 
    1767@@ -1768,6 +1768,13 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1768             // scan succeeded, record block as most recent successfully scanned
1769             result.last_scanned_block = block_hash;
1770             result.last_scanned_height = block_height;
1771+
1772+            if (save_progress) {
1773+                CBlockLocator loc= m_chain->getLocator(block_hash);
1774+
1775+                WalletBatch batch(GetDatabase());
1776+                batch.WriteBestBlock(loc);
    aureleoules commented at 2:48 pm on May 6, 2022:
    Should WriteBestBlock be called for each block processed instead of periodically? This snippet was brought up in the original issue (https://github.com/bitcoin/bitcoin/issues/25010#issue-1217715117). https://github.com/bitcoin/bitcoin/blob/f0a834e2f10a0aa60c7cc76e9f3eb090168a32e5/src/index/base.cpp#L170-L175
    w0xlt commented at 2:59 pm on May 9, 2022:
    I initially considered that the wallet scans tend to be much faster than the indices (unless the wallet is old and hasn’t been used for a while), so maybe it would be better to save every block. I can change this if necessary.
  11. in src/wallet/wallet.cpp:1773 in 170fb4ca55 outdated 
    1767@@ -1768,6 +1768,13 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1768             // scan succeeded, record block as most recent successfully scanned
1769             result.last_scanned_block = block_hash;
1770             result.last_scanned_height = block_height;
1771+
1772+            if (save_progress) {
1773+                CBlockLocator loc= m_chain->getLocator(block_hash);
    aureleoules commented at 2:48 pm on May 6, 2022:

    nit

    0                CBlockLocator loc = m_chain->getLocator(block_hash);
  12. w0xlt force-pushed on May 10, 2022
  13. w0xlt commented at 10:20 pm on May 10, 2022: contributor

    https://github.com/bitcoin/bitcoin/commit/37f192b435b03c742aa5f065d1d2d537c4002c94 addresses #25036 (review) and #25036 (review).

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

  14. DrahtBot added the label Needs rebase on May 11, 2022
  15. w0xlt force-pushed on May 12, 2022
  16. DrahtBot removed the label Needs rebase on May 12, 2022
  17. w0xlt commented at 6:47 pm on May 12, 2022: contributor
    Rebased.
  18. furszy commented at 6:08 pm on May 26, 2022: member

    Concept ACK, code reviewed 6b879a1e.

    I think that we could simplify it a bit more in this way: https://github.com/furszy/bitcoin-core/commit/e48ec2e727e57a62e1d2d31aa974a6cdbb46e323 (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.

  19. in src/wallet/wallet.h:539 in 6b879a1e19 outdated 
    535@@ -536,7 +536,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
536         //! USER_ABORT.
537         uint256 last_failed_block;
538     };
539-    ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate);
540+    ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate, const bool save_progress = false);
    jonatack commented at 6:27 pm on May 26, 2022:

    Maybe better to avoid the default and pass explicitly? There aren’t many callers.

    0    ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate, const bool save_progress);
    w0xlt commented at 4:39 pm on June 1, 2022:
    Done. Thanks.
  20. in src/wallet/wallet.cpp:1732 in 6b879a1e19 outdated 
    1728@@ -1729,6 +1729,13 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1729     double progress_current = progress_begin;
1730     int block_height = start_height;
1731     while (!fAbortRescan && !chain().shutdownRequested()) {
1732+
    jonatack commented at 6:32 pm on May 26, 2022:
    nit, remove extra line
    w0xlt commented at 4:39 pm on June 1, 2022:
    Done. Thanks.
  21. in src/node/interfaces.cpp:492 in 6b879a1e19 outdated 
    486@@ -487,6 +487,16 @@ class ChainImpl : public Chain
487         const CChain& active = Assert(m_node.chainman)->ActiveChain();
488         return active.GetLocator();
489     }
490+    std::optional<CBlockLocator> getLocator(const uint256& block_hash) override
491+    {
492+        LOCK(cs_main);
    jonatack commented at 6:32 pm on May 26, 2022: 
    0        LOCK(::cs_main);
    w0xlt commented at 4:39 pm on June 1, 2022:
    Done. Thanks.
  22. in src/node/interfaces.cpp:498 in 6b879a1e19 outdated 
    493+        const CChain& active = Assert(m_node.chainman)->ActiveChain();
494+        const CBlockIndex* index = m_node.chainman->m_blockman.LookupBlockIndex(block_hash);
495+
496+        if (!index) return std::nullopt;
497+
498+        return active.GetLocator(index);
    jonatack commented at 6:38 pm on May 26, 2022:

    Lines 493-498 could be written more simply and avoid an unneeded temporary object and call:

    0        const CBlockIndex* index = m_node.chainman->m_blockman.LookupBlockIndex(block_hash);
1        if (!index) return std::nullopt;
2        return Assert(m_node.chainman)->ActiveChain().GetLocator(index);
    w0xlt commented at 4:39 pm on June 1, 2022:
    Done. Thanks.
  23. in src/wallet/wallet.cpp:1780 in 6b879a1e19 outdated 
    1779+                auto loc = m_chain->getLocator(block_hash);
1780+
1781+                if (loc) {
1782+                    WalletBatch batch(GetDatabase());
1783+                    batch.WriteBestBlock(loc.value());
1784+                }
    jonatack commented at 6:53 pm on May 26, 2022:

    suggestion

    0-                auto loc = m_chain->getLocator(block_hash);
1-
2-                if (loc) {
3+                if (const auto& loc{m_chain->getLocator(block_hash)}) {
    w0xlt commented at 4:39 pm on June 1, 2022:
    Done. Thanks.
  24. in src/node/interfaces.cpp:530 in 6b879a1e19 outdated 
    486@@ -487,6 +487,16 @@ class ChainImpl : public Chain
487         const CChain& active = Assert(m_node.chainman)->ActiveChain();
488         return active.GetLocator();
489     }
490+    std::optional<CBlockLocator> getLocator(const uint256& block_hash) override
    jonatack commented at 11:25 am on May 27, 2022:
    I think this child implementation member can be private. See #24150 for rationale.
    w0xlt commented at 4:41 pm on June 1, 2022:
    I didn’t quite understand the suggestion. If this method is changed to private, the wallet will not be able to access it.
    ryanofsky commented at 7:44 pm on June 8, 2022:

    re: #25036 (review)

    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.

    w0xlt commented at 2:26 am on June 9, 2022:

    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.

  25. jonatack commented at 11:27 am on May 27, 2022: contributor
    Concept ACK, agree with @furszy that test coverage would be good.
  26. w0xlt force-pushed on May 31, 2022
  27. w0xlt force-pushed on Jun 1, 2022
  28. DrahtBot commented at 3:48 pm on June 1, 2022: contributor

    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.

  29. w0xlt commented at 5:31 pm on June 1, 2022: contributor

    @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?

  30. furszy commented at 7:44 pm on June 1, 2022: member

    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.

  31. in src/interfaces/chain.h:114 in 2475146a44 outdated 
    110@@ -111,6 +111,9 @@ class Chain
111     //! Get locator for the current chain tip.
112     virtual CBlockLocator getTipLocator() = 0;
113 
114+    //! Get locator for given block hash.
    ryanofsky commented at 7:50 pm on June 8, 2022:

    In commit “wallet: Save wallet scan progress” (2475146a44ae8419c89bdaa0581ba7b86838c244)

    Comment is a little misleading because returned locator is not always for the given block. Would suggest something like: “Return a locator that refers to a block in the active chain. If specified block is not in the active chain, return locator for the latest ancestor that is in the chain.”

    Could also call the method something more explicit like getActiveChainLocator, but I think the current name is OK.

  32. in src/interfaces/chain.h:115 in 2475146a44 outdated 
    110@@ -111,6 +111,9 @@ class Chain
111     //! Get locator for the current chain tip.
112     virtual CBlockLocator getTipLocator() = 0;
113 
114+    //! Get locator for given block hash.
115+    virtual std::optional<CBlockLocator> getLocator(const uint256& block_hash) = 0;
    ryanofsky commented at 7:56 pm on June 8, 2022:

    In commit “wallet: Save wallet scan progress” (2475146a44ae8419c89bdaa0581ba7b86838c244)

    I would recommend having this method return CBlockLocator not optional<CBlockLocator>, because the CBlockLocator type is already nullable and has an IsNull() method. Returning a type with multiple levels of nullability is potentially confusing for callers, and likely to result in bugs.

  33. ryanofsky approved
  34. ryanofsky commented at 9:48 pm on June 8, 2022: contributor
    Code review ACK 2475146a44ae8419c89bdaa0581ba7b86838c244. I implemented a unit test for this in cdcda3ba964af5cbd12d01ef570fc7b25e0d64f2 (branch) which would be nice to include here.
  35. w0xlt force-pushed on Jun 9, 2022
  36. w0xlt force-pushed on Jun 9, 2022
  37. w0xlt force-pushed on Jun 9, 2022
  38. in src/node/interfaces.cpp:533 in 6a3088634e outdated 
    530         return active.GetLocator();
531     }
532+    CBlockLocator getActiveChainLocator(const uint256& block_hash) override
533+    {
534+        LOCK(::cs_main);
535+        const CBlockIndex* index = m_node.chainman->m_blockman.LookupBlockIndex(block_hash);
    ryanofsky commented at 2:28 am on June 9, 2022:

    In commit “wallet: Save wallet scan progress” (6a3088634edaf55dd458d3d5a95d38c868db971a)

    I think you need if (!index) return {}; here so there is a way to detect a block not being found, and to avoid saving the tip as the best block if a block is not found.

  39. w0xlt commented at 2:33 am on June 9, 2022: contributor
    @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.
  40. in src/wallet/wallet.cpp:1710 in 18926f9259 outdated 
    1708@@ -1709,8 +1709,8 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1709 {
1710     using Clock = std::chrono::steady_clock;
    ryanofsky commented at 2:34 am on June 9, 2022:

    In commit “wallet test: Add unit test for wallet scan save_progress option” (18926f9259ed8656645602807a944ca49a4e2138)

    This declaration is unused and looks like it needs to be deleted to avoid a fatal warning:

    0wallet/wallet.cpp:1710:11: error: unused type alias 'Clock' [-Werror,-Wunused-local-typedef]
1    using Clock = std::chrono::steady_clock;

    https://cirrus-ci.com/task/4683407052505088?logs=ci#L1338

  41. w0xlt force-pushed on Jun 9, 2022
  42. in src/node/interfaces.cpp:535 in 86698868cd outdated 
    532+    CBlockLocator getActiveChainLocator(const uint256& block_hash) override
533+    {
534+        LOCK(::cs_main);
535+        const CBlockIndex* index = m_node.chainman->m_blockman.LookupBlockIndex(block_hash);
536+        if (!index) return {};
537+        return Assert(m_node.chainman)->ActiveChain().GetLocator(index);
    ryanofsky commented at 8:42 am on June 9, 2022:

    In commit “wallet: Save wallet scan progress” (86698868cdb878140c7d6a3909d6aa6f024d8cbf)

    Assert on this line isn’t really in the ideal place because m_node.chainman is already dereferenced above. Would suggest

    0const CBlockIndex* index = chainman().m_blockman.LookupBlockIndex(block_hash);
1if (!index) return {};
2return chainman().ActiveChain().GetLocator(index);
  43. in src/wallet/wallet.cpp:1710 in 86698868cd outdated 
    1704@@ -1705,10 +1705,9 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
1705  * the main chain after to the addition of any new keys you want to detect
1706  * transactions for.
1707  */
1708-CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate)
1709+CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate, const bool save_progress)
1710 {
1711-    using Clock = std::chrono::steady_clock;
    ryanofsky commented at 8:58 am on June 9, 2022:

    In commit “wallet: Save wallet scan progress” (86698868cdb878140c7d6a3909d6aa6f024d8cbf)

    This commit doesn’t compile with this line removed. The line should be removed in the next commit (which doesn’t compile with the line added)

  44. ryanofsky approved
  45. ryanofsky commented at 8:59 am on June 9, 2022: contributor
    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
  46. w0xlt force-pushed on Jun 9, 2022
  47. ryanofsky approved
  48. ryanofsky commented at 3:38 pm on June 15, 2022: contributor
    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
  49. in src/wallet/wallet.cpp:1775 in 32e88f795b outdated 
    1769@@ -1768,6 +1770,15 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1770             // scan succeeded, record block as most recent successfully scanned
1771             result.last_scanned_block = block_hash;
1772             result.last_scanned_height = block_height;
1773+
1774+            if (save_progress && next_interval) {
1775+                CBlockLocator loc= m_chain->getActiveChainLocator(block_hash);
    furszy commented at 2:04 pm on June 16, 2022:
    nit: missing space before “=”
  50. in src/wallet/wallet.cpp:1779 in 0ef383dd05 outdated 
    1773+            if (save_progress && next_interval) {
1774+                CBlockLocator loc= m_chain->getActiveChainLocator(block_hash);
1775+
1776+                if (!loc.IsNull()) {
1777+                    WalletBatch batch(GetDatabase());
1778+                    batch.WriteBestBlock(loc);
    furszy commented at 2:45 pm on June 16, 2022:

    nit: We probably could log this for debugging purposes: WalletLogPrintf("Saving scan progress %d.\n", block_height);

    As it’s done once every 60 seconds, it will not spam the log much.

  51. furszy approved
  52. furszy commented at 2:51 pm on June 16, 2022: member

    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.

  53. wallet: Save wallet scan progress 
    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>
    a89ddfbe22
  54. wallet test: Add unit test for wallet scan save_progress option 230a2f4cc3
  55. w0xlt force-pushed on Jun 23, 2022
  56. w0xlt commented at 8:27 pm on June 23, 2022: contributor
    Force-pushed https://github.com/bitcoin/bitcoin/pull/25036/commits/230a2f4cc3fab9f66b6c24ba809ddbea77755cb7 addressing @furszy suggestions.
  57. furszy approved
  58. furszy commented at 5:45 pm on June 24, 2022: member

    re-ACK 230a2f4

    Only changes were the missing space and logging.

  59. in src/node/interfaces.cpp:519 in a89ddfbe22 outdated 
    515@@ -516,20 +516,27 @@ class ChainImpl : public Chain
516     }
517     bool haveBlockOnDisk(int height) override
518     {
519-        LOCK(cs_main);
520+        LOCK(::cs_main);
    achow101 commented at 8:46 pm on June 24, 2022:

    In a89ddfbe22b6db5beda678c9493e08fec6144122 “wallet: Save wallet scan progress”

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

    jarolrod commented at 5:19 am on June 27, 2022:
    would second to not include these unnecessary changes, git blame depth
  60. achow101 commented at 9:07 pm on June 24, 2022: member
    ACK 230a2f4cc3fab9f66b6c24ba809ddbea77755cb7
  61. ryanofsky approved
  62. ryanofsky commented at 5:48 pm on July 11, 2022: contributor
    Code review ACK 230a2f4cc3fab9f66b6c24ba809ddbea77755cb7. Only change since last review is tweaking whitespace and adding log print
  63. MarcoFalke merged this on Jul 12, 2022
  64. MarcoFalke closed this on Jul 12, 2022
  65. w0xlt deleted the branch on Jul 12, 2022
  66. sidhujag referenced this in commit 0a9ad161d8 on Jul 12, 2022
  67. bitcoin locked this on Jul 12, 2023


w0xlt mzumsande aureleoules furszy jonatack ryanofsky DrahtBot achow101 jarolrod

Labels
Wallet interfaces

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me