Wallet passive startup #15719

pull ryanofsky wants to merge 5 commits into bitcoin:master from ryanofsky:pr/pool changing 14 files +331 −267
  1. ryanofsky commented at 9:35 PM on April 1, 2019: contributor

    Move wallet startup code closer to a simple model where the wallet attaches to the chain with a single chain.handleNotifications() call, and just starts passively receiving blocks and mempool notifications from the last update point, instead having to actively rescan blocks and request a mempool snapshot, and deal with the tip changing, and deal with early or stale notifications.

    Also, stop locking the cs_wallet mutex and registering for validationinterface notifications before the rescan. This was new behavior since 6a72f26968cf931c985d8d4797b6264274cabd06 #16426 and is not ideal because it stops other wallets and rpcs and the gui from receiving new notifications until after the scan completes.

    This change is a half-step towards implementing multiwallet scans (https://github.com/bitcoin/bitcoin/issues/11756), since it provides needed locator and birthday timestamp information to the Chain interface, and it rationalizes locking and event ordering in the startup code. The second half of implementing parallel rescans requires moving the ScanForWalletTransactions implementation (which this PR does not touch) from the wallet to the node.

  2. DrahtBot commented at 10:01 PM on April 1, 2019: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #23581 (Move BlockManager to node/blockstorage by MarcoFalke)
    • #23417 (wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101)
    • #22341 (rpc: add getxpub by Sjors)
    • #21603 (log: Mitigate disk filling attacks by rate limiting LogPrintf by dergoegge)
    • #20205 (wallet: Properly support a wallet id by achow101)
    • #18554 (wallet: ensure wallet files are not reused across chains by rodentrabies)
    • #16673 (Relog configuration args on debug.log rotation by LarryRuane)

    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.

  3. promag commented at 10:09 PM on April 1, 2019: member

    How will snapshot_fn work with IPC?

  4. ryanofsky commented at 12:37 AM on April 2, 2019: contributor
  5. in src/interfaces/chain.cpp:362 in 10f92ce5c4 outdated
     358 | @@ -358,22 +359,38 @@ class ChainImpl : public Chain
     359 |      {
     360 |          ::uiInterface.ShowProgress(title, progress, resume_possible);
     361 |      }
     362 | -    std::unique_ptr<Handler> handleNotifications(Notifications& notifications) override
     363 | +    std::unique_ptr<Handler> handleNotifications(Notifications& notifications, SnapshotFn snapshot_fn) override
     364 |      {
     365 | -        return MakeUnique<NotificationsHandlerImpl>(*this, notifications);
     366 | +        AssertLockNotHeld(::cs_main);
     367 | +        AssertLockNotHeld(::mempool.cs);
    


    practicalswift commented at 7:43 AM on April 2, 2019:

    Could use LOCKS_EXCLUDED annotations for those?


    ryanofsky commented at 3:09 PM on April 2, 2019:

    re: #15719 (review)

    Could use LOCKS_EXCLUDED annotations for those?

    Added annotations. Effect is kind of limited since this is an override and these mutexes aren't accessible where this is called in the wallet, but it still seems better to have these annotations than not.

    Longer term, after more changes like this and #15632, which reduce Chain::Lock usage, wallet code should stop holding these locks altogether.

  6. laanwj added the label Mempool on Apr 2, 2019
  7. promag commented at 12:24 PM on April 2, 2019: member

    @ryanofsky ok thanks, will look into that.

  8. in src/interfaces/chain.cpp:369 in 10f92ce5c4 outdated
     369 | +        // Declare packaged task to run in notification thread, and to first
     370 | +        // send the snapshot before enabling subsequent notifications.
     371 | +        std::vector<CTransactionRef> snapshot;
     372 | +        std::packaged_task<std::unique_ptr<Handler>()> task([&] {
     373 | +            if (snapshot_fn) snapshot_fn(std::move(snapshot));
     374 | +            return MakeUnique<NotificationsHandlerImpl>(*this, notifications);
    


    promag commented at 1:05 PM on April 2, 2019:

    Mempool can change after the snapshot but before registering the interface and so those changes will be missed.


    ryanofsky commented at 3:09 PM on April 2, 2019:

    re: #15719 (review)

    Mempool can change after the snapshot but before registering the interface and so those changes will be missed.

    This isn't true, and the comment below, "Hold locks while scheduling the task so notifications about added and removed transactions after the snapshot arrive after the snapshot" is specifically referring to this case.

    Let me know if I should make it more clear, but as long as the locks are held, transactions can't be added / removed by other threads, and as long as CallFunctionInValidationInterfaceQueue is called before transactions are added/removed, the handler will be registered before the queued notifications signal it.


    promag commented at 3:50 PM on April 2, 2019:

    But the queue can be non empty when CallFunctionInValidationInterfaceQueue is called?


    ryanofsky commented at 4:10 PM on April 2, 2019:

    re: #15719 (review)

    But the queue can be non empty when CallFunctionInValidationInterfaceQueue is called?

    I think you are referring to a different problem now (problem of old notifications being received when they shouldn't be, which is different from the problem of new notifications not being received when they should be). The problem of old notifications being received when they shouldn't be is what happens without this PR in the current code:

    https://github.com/bitcoin/bitcoin/blob/2c364fde423e74b4e03ebcff4582a9db7a6c4e4b/src/interfaces/chain.h#L273-L281

    The problem is fixed by this PR by not constructing NotificationsHandlerImpl until after snapshot_fn is called. The comment there "to first send the snapshot before enabling subsequent notifications" is about this.


    promag commented at 5:12 PM on April 2, 2019:

    AddToProcessQueue tricked me, what really goes to that queue are notifications.

  9. ryanofsky force-pushed on Apr 2, 2019
  10. in src/interfaces/chain.cpp:378 in 5f593ab2e1 outdated
     375 | +            return MakeUnique<NotificationsHandlerImpl>(*this, notifications);
     376 | +        });
     377 | +
     378 | +        // Snapshot mempool transactions, then schedule the task to execute.
     379 | +        // Hold locks while scheduling the task so notifications about added and
     380 | +        // removed transactions after the snapshot arrive after the snapshot.
    


    promag commented at 5:16 PM on April 2, 2019:

    also "arrive after task() is called" right?


    ryanofsky commented at 6:04 PM on April 2, 2019:

    re: #15719 (review)

    also "arrive after task() is called" right?

    Yes task() is what sends the snapshot, so that's what this is referring to. Replaced "transactions after the snapshot arrive after the snapshot" with "transactions after taking the snapshot arrive after the snapshot callback in task()" to reference it and be more specific.

  11. ryanofsky force-pushed on Apr 2, 2019
  12. promag commented at 8:07 PM on April 2, 2019: member

    Should we assert cs_main is held in the relevant validation interface notifications?

  13. ryanofsky commented at 8:24 PM on April 2, 2019: contributor

    re: #15719 (comment)

    Should we assert cs_main is held in the relevant validation interface notifications?

    cs_main can be acquired inside notification callbacks, if necessary, but it is not held when these notifications happen. This PR and #15632 disentangle notifications and locking, so locks are never held when notifications are received (right now they are inconsistently held in some cases but not others).

    Removing locking from notifications is a step toward eventually removing Chain::Lock completely, and having the wallet just asynchronously process notifications without being able to hold on to cs_main.

  14. promag commented at 1:35 PM on April 3, 2019: member

    Sorry for not being clear. I mean that cs_main is locked when GetMainSignals().TransactionAddedToMempool() is called, and it is important that doesn't change, by adding:

    @@ -152,6 +152,8 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd
     }
    
     void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) {
    +    AssertLockHeld(::cs_main);
    +    AssertLockHeld(::mempool.cs);
         m_internals->m_schedulerClient.AddToProcessQueue([ptx, this] {
             m_internals->TransactionAddedToMempool(ptx);
         });
    
  15. DrahtBot added the label Needs rebase on Apr 9, 2019
  16. ryanofsky force-pushed on Apr 10, 2019
  17. DrahtBot removed the label Needs rebase on Apr 10, 2019
  18. ryanofsky force-pushed on Apr 11, 2019
  19. jnewbery commented at 9:00 PM on April 11, 2019: contributor

    Concept ACK

    Mempool transactions are sent in a single IPC call instead of in a loop calling an IPC method repeatedly.

    Do we know what this does to memory usage if there's a very large mempool (either at startup when starting one or multiple wallets, or when loading a wallet dynamically during runtime)?

  20. ryanofsky commented at 9:49 PM on April 11, 2019: contributor

    Do we know what this does to memory usage if there's a very large mempool (either at startup when starting one or multiple wallets, or when loading a wallet dynamically during runtime)?

    With this PR by itself, a new temporary vector with one pointer per mempool transaction is created when a wallet is loaded. It's something to consider, but probably not likely to cause problems.

    With this PR combined with #10102 and running bitcoin-node with a wallet loading in a different process, this will use significantly more memory, because serializing the vector serializes all the transactions at once in memory before sending them. This might be about as expensive as a getrawmempool call. If this is a problem, the snapshot_fn callback could be tweaked to send the transactions in batches. I guess I wouldn't want to try to optimize this prematurely, though.

    Since I did write this PR "Improves performance over IPC" in the description, the increased memory usage might factor into that. But to be sure, the motivation for this PR is mainly to make notifications show up cleanly and in order on the client side.

  21. in src/wallet/wallet.cpp:4227 in 0ebc74e763 outdated
    4222 | +            for (const auto& mempool_tx : mempool_txs) {
    4223 | +                walletInstance->TransactionAddedToMempool(mempool_tx);
    4224 | +            }
    4225 | +        };
    4226 | +    }
    4227 | +    walletInstance->m_chain_notifications_handler = chain.handleNotifications(*walletInstance, snapshot_fn);
    


    ryanofsky commented at 2:33 PM on April 12, 2019:

    @jnewbery points out that it is too early to handle mempool transactions here at this point, because the rescan hasn't happened yet. Processing a mempool transaction before the rescan could result in the IsFromMe check in AddToWalletIfInvolvingMe failing to add the transaction to the walelt when it depends on a block that hasn't been scanned yet.

    So the mempool loop here needs to be moved below and will require some changes to locking, since handleNotifications in this PR can't be called with cs_main (it would deadlock).


    ryanofsky commented at 7:36 AM on April 30, 2020:

    re: #15719 (review)

    @jnewbery points out that it is too early to handle mempool transactions here at this point

    This is resolved in the current version of the PR which integrates rescans and finishes recsanning before creating the mempool snapshot

  22. in src/wallet/wallet.cpp:4218 in 0ebc74e763 outdated
    4214 | @@ -4213,6 +4215,17 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
    4215 |      // Try to top up keypool. No-op if the wallet is locked.
    4216 |      walletInstance->TopUpKeyPool();
    4217 |  
    4218 | +    // Register with the validation interface. Skip requesting mempool transactions if wallet is empty.
    


    jnewbery commented at 2:36 PM on April 12, 2019:

    I think moving the sync-from-mempool function to above the initial rescan breaks the (undocumented) expectation that the wallet is notified of transactions in dependency order. In fact, this was slightly broken by #15652, but this PR makes it worse.

    Prior to dynamically-loaded wallets, the wallet would always receive a transaction after it had received all of that transaction's ancestors. That wallet relies on that expectation:

    • CWallet::IsFromMe() will only match on a transaction if its parent is already in the wallet.
    • nTimeSmart should be monotonically increasing for transaction descendency. If transactions arrive in the wrong order then this is no longer true, and they'll be shown out-of-order in the GUI.

    Since #10740, a wallet could receive mempool transactions out-of-order (since a newly loaded wallet would not be notified of transactions in the mempool, but could be notified of their descendant transactions when they arrive). #15652 improved the situtation by notifying the wallet of all mempool transactions, but didn't sort them first, so a wallet could still be notified of the mempool txs in the wrong order. This PR potentially makes things worse by notifying the wallet of mempool txs before the blocks which could contain the mempool txs' ancestors.

    I think to fix this, we need to:

    • move the sync-from-mempool to after the rescan call, while cs_main is still held
    • sort the mempool txs by using mempool.infoAll() before notifying the wallet.

    We should also document that the wallet expects to see transactions in order!


    ryanofsky commented at 3:03 PM on April 12, 2019:

    I also wonder if there could brokenness in the case where a reorg happens during a rescanblockchain or importmulti RPC call (these rescans are different because cs_main isn't held the whole time).


    ryanofsky commented at 7:51 AM on April 30, 2020:

    re: #15719 (review)

    I think to fix this, we need to:

    • move the sync-from-mempool to after the rescan call, while cs_main is still held
    • sort the mempool txs by using mempool.infoAll() before notifying the wallet.

    We should also document that the wallet expects to see transactions in order!

    Moving sync is done. Rescanning happens before syncing from mempool in the current version of this PR. Sorting mempool isn't done yet. So this will still be as broken as it has been since #15652, but not more broken. There's already a lot going on here, so I think I would want to address this in a different PR. It seems pretty easy to fix independently of this PR, but writing a test case could be a challenge

  23. ryanofsky renamed this:
    Drop Chain::requestMempoolTransactions method
    WIP: Drop Chain::requestMempoolTransactions method
    on Apr 17, 2019
  24. DrahtBot added the label Needs rebase on Apr 17, 2019
  25. ryanofsky cross-referenced this on May 30, 2019 from issue gui: Enable console line edit on setClientModel by promag
  26. maflcko commented at 1:56 PM on August 16, 2019: member

    @ryanofsky Are you still working on this? If no, then please close it.

  27. ryanofsky commented at 3:35 PM on August 19, 2019: contributor

    @ryanofsky Are you still working on this? If no, then please close it.

    Thanks for the reminder. I looked into this again and I think the WIP tag still applies. I had been indecisive about involving rescan in this this change after John pointed out it was necessary, since I was unsure if it was better to incrementally change the rescan code (first synchronize rescan with notifications, then consolidate rescans across wallets https://gist.github.com/ariard/89f9bcc3a7ab9576fc6d15d251032cfa), or to just do both things in a single PR.

    But now I think it should be possible to synchronize rescan with notifications in a way that doesn't change existing code too much and should also make ariard's proposed change easier.

  28. ryanofsky cross-referenced this on Jan 22, 2020 from issue wallet: Remove calls to Chain::Lock methods by ryanofsky
  29. ryanofsky force-pushed on Jan 24, 2020
  30. ryanofsky force-pushed on Jan 27, 2020
  31. ryanofsky renamed this:
    WIP: Drop Chain::requestMempoolTransactions method
    Drop Chain::requestMempoolTransactions method
    on Jan 27, 2020
  32. ryanofsky force-pushed on Jan 28, 2020
  33. DrahtBot removed the label Needs rebase on Jan 29, 2020
  34. DrahtBot added the label Needs rebase on Jan 30, 2020
  35. promag cross-referenced this on Mar 10, 2020 from issue wallet: UnregisterValidationInterface before SyncWithValidationInterfaceQueue by promag
  36. ryanofsky force-pushed on Mar 31, 2020
  37. ryanofsky force-pushed on Mar 31, 2020
  38. ryanofsky force-pushed on Mar 31, 2020
  39. DrahtBot removed the label Needs rebase on Mar 31, 2020
  40. DrahtBot cross-referenced this on Apr 1, 2020 from issue gui: Bilingual GUI error messages by hebasto
  41. DrahtBot cross-referenced this on Apr 1, 2020 from issue Remove CWalletTx merging logic from AddToWallet by ryanofsky
  42. DrahtBot cross-referenced this on Apr 1, 2020 from issue Use shared pointers only in validation interface by bvbfan
  43. DrahtBot cross-referenced this on Apr 1, 2020 from issue Replace -upgradewallet startup option with upgradewallet RPC by achow101
  44. DrahtBot cross-referenced this on Apr 3, 2020 from issue Bugfix: Wallet: Safely deal with change in the address book by luke-jr
  45. DrahtBot cross-referenced this on Apr 5, 2020 from issue rpc: remove deprecated CRPCCommand constructor by maflcko
  46. DrahtBot added the label Needs rebase on Apr 6, 2020
  47. ryanofsky force-pushed on Apr 15, 2020
  48. ryanofsky commented at 4:30 PM on April 15, 2020: contributor

    Rebased 3eb122b4a10024d86561492db19a881d420e34c5 -> b7716d0272125f4b4d2bce179d86d896f67869bd (pr/pool.9 -> pr/pool.10, compare) after merge of #17954 and conflict with #18192

  49. DrahtBot removed the label Needs rebase on Apr 15, 2020
  50. DrahtBot cross-referenced this on Apr 16, 2020 from issue wallet: ensure wallet files are not reused across chains by rodentrabies
  51. DrahtBot cross-referenced this on Apr 16, 2020 from issue assumeutxo by jamesob
  52. DrahtBot cross-referenced this on Apr 17, 2020 from issue Reverse cs_main, cs_wallet lock order and reduce cs_main locking by ariard
  53. DrahtBot cross-referenced this on Apr 18, 2020 from issue wallet: Avoid translating RPC errors by maflcko
  54. DrahtBot added the label Needs rebase on Apr 19, 2020
  55. ryanofsky referenced this in commit f9cea7231d on Apr 21, 2020
  56. ryanofsky cross-referenced this on Apr 21, 2020 from issue test: Add CreateWalletFromFile test by ryanofsky
  57. ryanofsky force-pushed on Apr 21, 2020
  58. ryanofsky commented at 8:53 PM on April 21, 2020: contributor

    Rebased b7716d0272125f4b4d2bce179d86d896f67869bd -> bb7e272bd968adff75bed5038680f23b665880c5 (pr/pool.10 -> pr/pool.11, compare) due to minor conflicts with #15761 and #18601

  59. DrahtBot removed the label Needs rebase on Apr 21, 2020
  60. DrahtBot cross-referenced this on Apr 22, 2020 from issue [wallet] Track conflicted transactions removed from mempool and fix UI notifications by ariard
  61. DrahtBot cross-referenced this on Apr 25, 2020 from issue bench: Start nodes with -nodebuglogfile by maflcko
  62. ryanofsky referenced this in commit 2142bf14fc on Apr 27, 2020
  63. ryanofsky referenced this in commit 1dcbe64463 on Apr 27, 2020
  64. ryanofsky referenced this in commit 66a4d4b2ec on Apr 28, 2020
  65. DrahtBot added the label Needs rebase on Apr 29, 2020
  66. ryanofsky force-pushed on Apr 29, 2020
  67. ryanofsky commented at 5:45 PM on April 29, 2020: contributor

    Rebased bb7e272bd968adff75bed5038680f23b665880c5 -> 3c50af4c76889261d19262086542b44220c74457 (pr/pool.11 -> pr/pool.12, compare) due to conflict with #18759

  68. ryanofsky referenced this in commit 7918c1b019 on Apr 29, 2020
  69. DrahtBot removed the label Needs rebase on Apr 29, 2020
  70. maflcko referenced this in commit 0f204dd3f2 on Apr 29, 2020
  71. ryanofsky force-pushed on Apr 30, 2020
  72. ryanofsky commented at 7:22 AM on April 30, 2020: contributor

    Rebased 3c50af4c76889261d19262086542b44220c74457 -> 044418037a6ead2e9c38f404228ea8536369b683 (pr/pool.12 -> pr/pool.13, compare) after #18727 to deal with failures in new CreateWalletFromFile test. Rewrote test to handle new startup sequence.

  73. DrahtBot cross-referenced this on Apr 30, 2020 from issue Relog configuration args on debug.log rotation by LarryRuane
  74. ariard referenced this in commit 86ec6f7ce6 on Apr 30, 2020
  75. pierreN referenced this in commit 3eaf73ad8b on May 1, 2020
  76. DrahtBot added the label Needs rebase on May 1, 2020
  77. ryanofsky force-pushed on May 1, 2020
  78. ryanofsky commented at 3:20 PM on May 1, 2020: contributor

    Rebased 044418037a6ead2e9c38f404228ea8536369b683 -> 26e4ee32273ab232fa3898f86289131a6379b69b (pr/pool.13 -> pr/pool.14, compare) due to conflict with #16426

  79. DrahtBot removed the label Needs rebase on May 1, 2020
  80. DrahtBot cross-referenced this on May 2, 2020 from issue Multiprocess bitcoin by ryanofsky
  81. sidhujag referenced this in commit d7c1f85877 on May 2, 2020
  82. DrahtBot added the label Needs rebase on May 4, 2020
  83. ryanofsky force-pushed on May 4, 2020
  84. ryanofsky commented at 6:58 PM on May 4, 2020: contributor

    Rebased 26e4ee32273ab232fa3898f86289131a6379b69b -> 52d152f0bf2543fc6fbb80cdde7ae2249305662f (pr/pool.14 -> pr/pool.15, compare) due to conflict with #18699

  85. DrahtBot removed the label Needs rebase on May 4, 2020
  86. ryanofsky commented at 4:59 AM on May 5, 2020: contributor

    Travis error test_framework.authproxy.JSONRPCException: non-mandatory-script-verify-flag (Signature must be zero for failed CHECK(MULTI)SIG operation) (-26) in sendrawtransaction call in rpc_rawtransaction.py in bionic C++17 build https://travis-ci.org/github/bitcoin/bitcoin/jobs/683072285#L5042 seems unrelated. It's reported in #18803

  87. DrahtBot cross-referenced this on May 6, 2020 from issue More thread safety annotation coverage by ajtowns
  88. in src/wallet/wallet.cpp:2818 in 52d152f0bf outdated
    4027 | +            best_block_locator.emplace();
    4028 | +            WalletBatch(*wallet->database).ReadBestBlock(*best_block_locator);
    4029 | +        }
    4030 |  
    4031 | +        // No need to read and scan block if block was created before
    4032 | +        // our wallet birthday (as adjusted for block time variability)
    


    ariard commented at 8:32 AM on May 8, 2020:

    Block time variability is unclear, you mean the absence of order guarantee between nTime and nTimeFirstKey?


    ryanofsky commented at 3:57 PM on May 8, 2020:

    re: #15719 (review)

    Block time variability is unclear, you mean the absence of order guarantee between nTime and nTimeFirstKey?

    This is not a new comment (just moved) but it is just referring to TIMESTAMP_WINDOW

  89. in src/interfaces/chain.h:237 in 52d152f0bf outdated
     234 | +    using MempoolFn = std::function<void(std::vector<CTransactionRef>)>;
     235 | +
     236 | +    //! Register handler for notifications. Before returning and before sending
     237 | +    //! the first notification, call ScanFn with a scan range of blocks after a
     238 | +    //! specified location and time, and then call MempoolFn with a snapshot of
     239 | +    //! transactions in the mempool before the first transactionAddedToMempool /
    


    ariard commented at 8:37 AM on May 8, 2020:

    This part of comment may be made more intuitively instead of one chunk, like "Call MempoolFn with a snapshot of transaction before firing transactions back to caller".


    ryanofsky commented at 3:55 PM on May 8, 2020:

    re: #15719 (review)

    This part of comment may be made more intuitively instead of one chunk, like "Call MempoolFn with a snapshot of transaction before firing transactions back to caller".

    That seems good. I shortened existing description to match it

  90. in src/interfaces/chain.h:257 in 52d152f0bf outdated
     248 | +    //! @param[in] mempool_fn    callback invoked before notifications are sent
     249 | +    //!                          with snapshot of mempool transactions
     250 | +    //! @param[in] scan_locator  location of last block previously scanned.
     251 | +    //!                          scan_fn will be only be called for blocks after
     252 | +    //!                          this point. Can be null to scan from genesis.
     253 | +    //! @param[in] scan_time     minimum block timestamp for beginning the scan
    


    ariard commented at 8:40 AM on May 8, 2020:

    May we drop either or scan_locator or scan_time, like if wallet first key is older that locator use it as a starting point, if not use wallet last block nTime. Do we still really need wallet locator ?


    ryanofsky commented at 3:55 PM on May 8, 2020:

    re: #15719 (review)

    May we drop either or scan_locator or scan_time, like if wallet first key is older that locator use it as a starting point, if not use wallet last block nTime. Do we still really need wallet locator ?

    I think the idea is to allow scanning as little as necessary. Both arguments are optional so callers aren't obligated to provide them. This rescan behavior be revisited in a followup PR, but in this PR I think it makes sense for wallet to continue scanning same blocks it was scanning previously.

  91. in src/interfaces/chain.h:270 in 52d152f0bf outdated
     253 | +    //! @param[in] scan_time     minimum block timestamp for beginning the scan
     254 | +    //!                          scan_fn will only be called for blocks starting
     255 | +    //!                          from this timestamp
     256 | +    //! @param[out] tip          information about chain tip at the point where
     257 | +    //!                          notifications will begin
     258 | +    virtual std::unique_ptr<Handler> handleNotifications(std::shared_ptr<Notifications> notifications,
    


    ariard commented at 8:44 AM on May 8, 2020:

    I think it would be better to rename handleNotifications. IMO it does 3 different tasks:

    • trigger rescan
    • sync with mempool
    • register notification

    I'm fine with doing all of three in same functions rather than 3 chunks for each, but name could be something like syncClient ?

    Also you may be interested by rescan but not mempool-sync and therefore it may be made optional.


    ryanofsky commented at 3:55 PM on May 8, 2020:

    re: #15719 (review)

    I think it would be better to rename handleNotifications. IMO it does 3 different tasks:

    • trigger rescan
    • sync with mempool
    • register notification

    I'm fine with doing all of three in same functions rather than 3 chunks for each, but name could be something like syncClient ?

    Also you may be interested by rescan but not mempool-sync and therefore it may be made optional.

    I think it's important to stick with the convention used throughout src/interfaces/ where functions that register for notifications and return Handler begin with handle. There are 20 of these functions currently, though only 2 in chain.h.

    The main thing this function does, and only permanent thing it is intended to do is register a handler for notifications. scan_fn and mempool_fn are both optional arguments and are temporary. I want to rebase your https://github.com/bitcoin/bitcoin/compare/master...ariard:2019-08-rescan-index-refactor branch on top of this PR so the scan_fn callback will disappear and be replaced by normal blockConnected / blockDisconnected calls. And I want to update the transactionAddedToMempool function to take a list of transactions instead of a single transaction so the mempool_fn can go away.

  92. in src/interfaces/chain.h:260 in 52d152f0bf outdated
     251 | +    //!                          scan_fn will be only be called for blocks after
     252 | +    //!                          this point. Can be null to scan from genesis.
     253 | +    //! @param[in] scan_time     minimum block timestamp for beginning the scan
     254 | +    //!                          scan_fn will only be called for blocks starting
     255 | +    //!                          from this timestamp
     256 | +    //! @param[out] tip          information about chain tip at the point where
    


    ariard commented at 8:46 AM on May 8, 2020:

    You may precise tip included.


    ryanofsky commented at 3:55 PM on May 8, 2020:

    re: #15719 (review)

    You may precise tip included.

    This seems precise, but I may not be seeing what is is unclear. The important point is that there shouldn't be any gap between the tip and the blockConnected or blockDisconnected notification

  93. in src/interfaces/chain.cpp:367 in 52d152f0bf outdated
     348 | +            AssertLockNotHeld(::mempool.cs);
     349 | +            WAIT_LOCK(::cs_main, main_lock);
     350 | +
     351 | +            // Call scan_fn until it has scanned all blocks after specified
     352 | +            // location and time. Looping is necessary because new blocks may
     353 | +            // be connected during rescans.
    


    ariard commented at 8:47 AM on May 8, 2020:

    Isn't taking cs_main lock avoid block tip increase ?


    ryanofsky commented at 3:54 PM on May 8, 2020:

    re: #15719 (review)

    Isn't taking cs_main lock avoid block tip increase ?

    Nope, cs_main isn't held while rescanning so node will not lock up when loading a wallet

  94. ariard commented at 8:55 AM on May 8, 2020: member

    Approach ACK, thanks for moving forward with wallet initialization/rescan refactor.

    Is there a way to split commit ?

    • there is few-style changes around fRescan in src/wallet/rcpdump.cpp
    • maybe test modifications can be moved in their own commit ?
    • a wallet lock move from import* in src/rwallet/rpcdump.cpp to ReacceptWallet

    I'm personally fine with reviewing PR in a single chunk, but maybe other reviewers are less familiar with this part of the codebase.

  95. DrahtBot cross-referenced this on May 9, 2020 from issue test, build: Enable -Werror=sign-compare by Empact
  96. DrahtBot added the label Needs rebase on May 11, 2020
  97. ryanofsky force-pushed on May 12, 2020
  98. ryanofsky commented at 4:49 AM on May 12, 2020: contributor

    Updated 52d152f0bf2543fc6fbb80cdde7ae2249305662f -> 66588cfcfeb7d89f242439a97db85be44c3a2dae (pr/pool.15 -> pr/pool.16, compare) splitting commits and updating some names and comments Rebased 66588cfcfeb7d89f242439a97db85be44c3a2dae -> 78a42ce0251426051ff789e64a9bfae3200d1f83 (pr/pool.16 -> pr/pool.17, compare) due to conflict with #18216

  99. DrahtBot removed the label Needs rebase on May 12, 2020
  100. ryanofsky force-pushed on May 12, 2020
  101. ryanofsky commented at 12:37 PM on May 12, 2020: contributor

    Updated 78a42ce0251426051ff789e64a9bfae3200d1f83 -> de030e6258251fba2159788c2bdf135f6e2369f3 (pr/pool.17 -> pr/pool.18, compare) adding commit descriptions, locator test, and partially fixing travis macos error "comparison of integers of different signs" https://travis-ci.org/github/bitcoin/bitcoin/jobs/685957982#L3778 Updated de030e6258251fba2159788c2bdf135f6e2369f3 -> d1ae53126babf5dd7e714c3ea954913fe35b94bc (pr/pool.18 -> pr/pool.19, compare) with fix for travis errors "comparison of integers of different signs" https://travis-ci.org/github/bitcoin/bitcoin/jobs/686087628#L3688 Rebased d1ae53126babf5dd7e714c3ea954913fe35b94bc -> 9ffa3622484c55f15bd30daed61ef829421aa7de (pr/pool.19 -> pr/pool.20, compare) due to conflicts with #16127 and #18635, also rewriting PR description in light of #16426 and #18964 and #11756

  102. DrahtBot cross-referenced this on May 12, 2020 from issue Drop checkFinalTx and use Median Time Past to check finality of wallet transactions by ariard
  103. ryanofsky force-pushed on May 14, 2020
  104. DrahtBot cross-referenced this on May 15, 2020 from issue wallet: Minimal fix to restore conflicted transaction notifications by ryanofsky
  105. DrahtBot added the label Needs rebase on May 28, 2020
  106. ryanofsky renamed this:
    Drop Chain::requestMempoolTransactions method
    Wallet passive startup
    on Jun 1, 2020
  107. ryanofsky force-pushed on Jun 1, 2020
  108. DrahtBot removed the label Needs rebase on Jun 1, 2020
  109. DrahtBot cross-referenced this on Jun 2, 2020 from issue [WIP] wallet: use BlockFilterIndex in ScanForWalletTransactions by pstratem
  110. DrahtBot cross-referenced this on Jun 2, 2020 from issue refactor: remove ::vpwallets and related global variables by ryanofsky
  111. DrahtBot cross-referenced this on Jun 2, 2020 from issue refactor: Move wallet methods out of chain.h and node.h by ryanofsky
  112. ryanofsky cross-referenced this on Jun 2, 2020 from issue test: Potential deadlock in wallet_tests/CreateWalletFromFile by hebasto
  113. DrahtBot added the label Needs rebase on Jun 2, 2020
  114. ariard commented at 12:42 AM on June 4, 2020: member

    Actually rebasing #17484, I think to avoid breaking anti-fee snipping it would be better to setup IBD state at mempool/block notification sync. Ideally calling updateBlockTip at any ending of block rescan. But likely I need to wait for ScanForWalletTransactions being moved inside the node, if I want to drop isInitialBlockDownload

    Is this PR ready for review ?

  115. ariard cross-referenced this on Jun 4, 2020 from issue wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload by ariard
  116. vahidjalaliii commented at 1:54 AM on June 4, 2020: none

    .

    در تاریخ چهارشنبه ۳ ژوئن ۲۰۲۰،‏ ۳:۰۵ DrahtBot notifications@github.com نوشت:

    🐙 This pull request conflicts with the target branch and needs rebase https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes .

    — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/15719#issuecomment-637842411, or unsubscribe https://github.com/notifications/unsubscribe-auth/APRRJT7E4TN3UBHBPFS2U3LRUV5BNANCNFSM4HC2YJWQ .

  117. ryanofsky force-pushed on Jun 5, 2020
  118. DrahtBot removed the label Needs rebase on Jun 5, 2020
  119. ryanofsky force-pushed on Jun 5, 2020
  120. ryanofsky commented at 1:49 PM on June 5, 2020: contributor

    re: ariard #15719 (comment)

    Is this PR ready for review ?

    PR is complete and ready for review. Only caveat is that the unit test is triggering thread sanitizer errors. The errors look like they could be real, but I think they are errors in the test itself, not the code, and I'll look into them.


    Rebased 9ffa3622484c55f15bd30daed61ef829421aa7de -> 57108199fc6f81b4b6caef082390f6550b2e20b8 (pr/pool.20 -> pr/pool.21, compare) due to conflict with #18982 Rebased 57108199fc6f81b4b6caef082390f6550b2e20b8 -> 8db3520954e033f07986979fb77a46d8a08be351 (pr/pool.21 -> pr/pool.22, compare) after #19164 to try to avoid tsan errors in https://travis-ci.org/github/bitcoin/bitcoin/jobs/694884461 Updated 8db3520954e033f07986979fb77a46d8a08be351 -> 182df0f94a72668074b8b41105a2250a3900456f (pr/pool.22 -> pr/pool.23, compare) with TSAN suppression to fix spurious std::cout data race error https://travis-ci.org/github/bitcoin/bitcoin/jobs/695004018#L5755 Rebased 182df0f94a72668074b8b41105a2250a3900456f -> 3704d3b97e497ba69acc957760743f0ce81e20e3 (pr/pool.23 -> pr/pool.24, compare) due to silent conflict with #19249 Updated 3704d3b97e497ba69acc957760743f0ce81e20e3 -> 111a2ebec6c9ef3310a72db0d9456b839f8433ae (pr/pool.24 -> pr/pool.25, compare) to try to work around TSAN data race https://cirrus-ci.com/task/6561667200843776 Updated 111a2ebec6c9ef3310a72db0d9456b839f8433ae -> 0d41ed87fc0afbbe0916d363fd71b12c5fe981eb (pr/pool.25 -> pr/pool.26, compare) to work around TSAN data race https://cirrus-ci.com/task/6556801657208832 Rebased 0d41ed87fc0afbbe0916d363fd71b12c5fe981eb -> 68cd2f23e78843c903fb46b9dda1830639f7bf1e (pr/pool.26 -> pr/pool.27, compare) due to cherry-picked commit in #19538 Rebased 68cd2f23e78843c903fb46b9dda1830639f7bf1e -> 9eab04972bdeda48914ea32b9506667f34d718ba (pr/pool.27 -> pr/pool.28, compare) due to conflict with #15937 Rebased 9eab04972bdeda48914ea32b9506667f34d718ba -> 072e1d17569647105801b5bdc9f283e46d426f6c (pr/pool.28 -> pr/pool.29, compare) due to conflicts with #19099 and #19671 Rebased 072e1d17569647105801b5bdc9f283e46d426f6c -> 5328fe9623b47ac989c57df57bb34cf1038cbc13 (pr/pool.29 -> pr/pool.30, compare) due to conflict with #19572 Rebased 5328fe9623b47ac989c57df57bb34cf1038cbc13 -> 8aa64ef7f5d6e02407aa09904092e0213dcdd2c4 (pr/pool.30 -> pr/pool.31, compare) due to conflicts with #19425, #19980, #21404, #21415, #21525 and others

  121. DrahtBot cross-referenced this on Jun 5, 2020 from issue Add loadwallet and createwallet load_on_startup options by ryanofsky
  122. DrahtBot cross-referenced this on Jun 9, 2020 from issue wallet: Remove first parameter to ScanForWalletTransactions start_hash by pstratem
  123. DrahtBot cross-referenced this on Jun 10, 2020 from issue ci: tsan gui by maflcko
  124. DrahtBot cross-referenced this on Jun 17, 2020 from issue UI external signer support (e.g. hardware wallet) by Sjors
  125. DrahtBot cross-referenced this on Jun 17, 2020 from issue External signer support - Wallet Box edition by Sjors
  126. DrahtBot cross-referenced this on Jun 17, 2020 from issue util: add RunCommandParseJSON by Sjors
  127. DrahtBot cross-referenced this on Jun 20, 2020 from issue validation: re-delegate absurd fee checking from mempool to clients by glozow
  128. DrahtBot cross-referenced this on Jun 24, 2020 from issue ci: Increase test timeout for sanitizer configs by maflcko
  129. DrahtBot cross-referenced this on Jun 25, 2020 from issue Remove the automatic creation and loading of the default wallet by achow101
  130. ryanofsky cross-referenced this on Jun 30, 2020 from issue wallet: ScanForWalletTransactions cleanup by pstratem
  131. DrahtBot cross-referenced this on Jul 1, 2020 from issue refactor: Get rid of more redundant chain methods by ryanofsky
  132. DrahtBot cross-referenced this on Jul 7, 2020 from issue multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky
  133. DrahtBot cross-referenced this on Jul 7, 2020 from issue multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky
  134. DrahtBot cross-referenced this on Jul 8, 2020 from issue Prune locks by luke-jr
  135. ryanofsky force-pushed on Jul 10, 2020
  136. DrahtBot cross-referenced this on Jul 11, 2020 from issue multiprocess: Add basic spawn and IPC support by ryanofsky
  137. meshcollider added the label Wallet on Jul 11, 2020
  138. ryanofsky force-pushed on Jul 11, 2020
  139. ryanofsky force-pushed on Jul 13, 2020
  140. ryanofsky force-pushed on Jul 14, 2020
  141. DrahtBot cross-referenced this on Jul 20, 2020 from issue Remove mempool global by maflcko
  142. DrahtBot cross-referenced this on Jul 23, 2020 from issue ZMQ: Create "sequence" notifier, enabling client-side mempool tracking by instagibbs
  143. DrahtBot cross-referenced this on Jul 30, 2020 from issue Disable fee estimation in blocksonly mode (by removing the fee estimates global) by darosior
  144. DrahtBot cross-referenced this on Aug 4, 2020 from issue wallet: Replace -zapwallettxes with zapwallettxes RPC by achow101
  145. DrahtBot added the label Needs rebase on Aug 5, 2020
  146. ryanofsky force-pushed on Aug 7, 2020
  147. DrahtBot removed the label Needs rebase on Aug 7, 2020
  148. DrahtBot cross-referenced this on Aug 7, 2020 from issue wallet: Remove -zapwallettxes by achow101
  149. DrahtBot added the label Needs rebase on Aug 15, 2020
  150. luke-jr referenced this in commit 74de8d57b2 on Aug 15, 2020
  151. ryanofsky force-pushed on Aug 17, 2020
  152. DrahtBot removed the label Needs rebase on Aug 17, 2020
  153. DrahtBot cross-referenced this on Aug 20, 2020 from issue wallet: Replace -zapwallettxes with wallet tool command by achow101
  154. DrahtBot added the label Needs rebase on Aug 31, 2020
  155. ryanofsky force-pushed on Sep 2, 2020
  156. DrahtBot removed the label Needs rebase on Sep 2, 2020
  157. DrahtBot cross-referenced this on Sep 2, 2020 from issue Remove mempool global from interfaces by maflcko
  158. ariard cross-referenced this on Sep 3, 2020 from issue Clarify eviction protection scope of outbound block-relay-only peers by ariard
  159. DrahtBot added the label Needs rebase on Sep 5, 2020
  160. ryanofsky force-pushed on Sep 25, 2020
  161. DrahtBot removed the label Needs rebase on Sep 25, 2020
  162. DrahtBot cross-referenced this on Sep 26, 2020 from issue Drop some TSan suppressions by hebasto
  163. DrahtBot cross-referenced this on Sep 26, 2020 from issue test: Fix inconsistent lock order in wallet_tests/CreateWallet by hebasto
  164. DrahtBot cross-referenced this on Sep 26, 2020 from issue refactor: Some wallet cleanups by promag
  165. DrahtBot cross-referenced this on Sep 26, 2020 from issue wallet: Fix wallet loading race during node start by fjahr
  166. DrahtBot cross-referenced this on Oct 15, 2020 from issue tree-wide: De-globalize ChainstateManager by dongcarl
  167. DrahtBot cross-referenced this on Oct 31, 2020 from issue Add missing thread safety annotations by vasild
  168. DrahtBot cross-referenced this on Nov 2, 2020 from issue Remove references to CreateWalletFromFile by fanquake
  169. DrahtBot cross-referenced this on Nov 11, 2020 from issue test: Suppress epoll_ctl data race by maflcko
  170. DrahtBot added the label Needs rebase on Nov 11, 2020
  171. janus referenced this in commit c0add1ba79 on Nov 15, 2020
  172. ryanofsky cross-referenced this on Nov 16, 2020 from issue wallettool: add parameter to create descriptors wallet by S3RK
  173. S3RK cross-referenced this on Dec 26, 2020 from issue refactor: split CWallet::Create by S3RK
  174. Fabcien referenced this in commit a8ce4f8ed8 on Feb 9, 2021
  175. ryanofsky force-pushed on Apr 11, 2021
  176. DrahtBot removed the label Needs rebase on Apr 12, 2021
  177. DrahtBot cross-referenced this on Apr 14, 2021 from issue test: Use called_from_lib to point uninstrumented libs to TSan by hebasto
  178. rebroad commented at 8:17 PM on April 18, 2021: contributor

    @ryanofsky I've not looked at this yet, but does this cause the GUI to become available sooner (e.g. the rescan can run after the splash screen rather than during)? I'd test this if so, or if I understand what benefits it provides - e.g. I'm not sure why we'd want parallel multiwallet scans - unless it means it would scan them all faster in total, for example.

  179. ryanofsky commented at 12:49 PM on April 21, 2021: contributor

    Thanks for looking at this.

    @ryanofsky I've not looked at this yet, but does this cause the GUI to become available sooner (e.g. the rescan can run after the splash screen rather than during)?

    Yes, this separates opening a wallet from attaching to the node, so should it be possible to interact with the wallet before the rescan finishes if GUI is changed to remove the rescan dialog or make it nonmodal.

    Also if there are multiple wallets, rescanning once instead of multiple times should make loading faster in general.

    I'd test this if so, or if I understand what benefits it provides - e.g. I'm not sure why we'd want parallel multiwallet scans - unless it means it would scan them all faster in total, for example.

    Updated the description to drop the word parallel. It's not really a question of parallel or serial, it's a question of scanning blocks one time for all wallets, or scanning blocks multiple times, once for each wallet.

  180. ryanofsky cross-referenced this on Apr 21, 2021 from issue MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky
  181. DrahtBot cross-referenced this on May 2, 2021 from issue scripted-diff: Replace three dots with ellipsis in the UI strings by hebasto
  182. DrahtBot cross-referenced this on May 6, 2021 from issue [Bundle 7/7] validation: Farewell, global Chainstate! by dongcarl
  183. DrahtBot added the label Needs rebase on May 10, 2021
  184. laanwj referenced this in commit d4c409cf09 on May 19, 2021
  185. ryanofsky force-pushed on May 19, 2021
  186. ryanofsky commented at 6:16 PM on May 19, 2021: contributor

    Rebased 8aa64ef7f5d6e02407aa09904092e0213dcdd2c4 -> d36335f643f309790c4b26307eca622a3a9ed688 (pr/pool.31 -> pr/pool.32, compare) due to conflict with #20773 Rebased d36335f643f309790c4b26307eca622a3a9ed688 -> 6a3705214086420f8548b9ab9456559358911f8c (pr/pool.32 -> pr/pool.33, compare) due to conflict with #22156 and #21866 Updated 6a3705214086420f8548b9ab9456559358911f8c -> 4c2532cf31033120d9d47575c926f618a78dae51 (pr/pool.33 -> pr/pool.34, compare) to fix thread sanitizer failure https://cirrus-ci.com/task/4605787663237120 Updated 4c2532cf31033120d9d47575c926f618a78dae51 -> cfe2009ff8671724908c69ae48c84c77e3793c25 (pr/pool.34 -> pr/pool.35, compare) to fix thread sanitizer failure https://cirrus-ci.com/task/6525364810809344?logs=ci#L3365 Rebased cfe2009ff8671724908c69ae48c84c77e3793c25 -> 591dd974dceec8bf0154c7ac3b305d8811ed51c4 (pr/pool.35 -> pr/pool.36, compare) due to conflict with #19101 Rebased 591dd974dceec8bf0154c7ac3b305d8811ed51c4 -> 793f40361133f9abcf79cd87d1d9f24308094b09 (pr/pool.36 -> pr/pool.37, compare) due to conflict with #22100 Rebased 793f40361133f9abcf79cd87d1d9f24308094b09 -> 74821eda663561548f575bff6cc1c57286c8952b (pr/pool.37 -> pr/pool.38, compare) due to conflict with #23123 Rebased 74821eda663561548f575bff6cc1c57286c8952b -> 24dfff610ecf01338c9a9b7c1a28307c3fcfbc78 (pr/pool.38 -> pr/pool.39, compare) due to conflict with #23003 Rebased 24dfff610ecf01338c9a9b7c1a28307c3fcfbc78 -> b5635c5cfd537dd9db18d2e29f9fbfeb01767b00 (pr/pool.39 -> pr/pool.40, compare) due to conflict with #23512 Rebased b5635c5cfd537dd9db18d2e29f9fbfeb01767b00 -> 2b898a0456d742911c1f74229c9fc64a0012ca42 (pr/pool.40 -> pr/pool.41, compare) due to conflict with #22868

  187. DrahtBot removed the label Needs rebase on May 19, 2021
  188. DrahtBot cross-referenced this on May 21, 2021 from issue refactor: Make CWalletTx sync state type-safe by ryanofsky
  189. DrahtBot cross-referenced this on May 30, 2021 from issue refactor: Clean up new wallet spend, receive files added #21207 by ryanofsky
  190. DrahtBot cross-referenced this on Jun 5, 2021 from issue Allow tr() import only when Taproot is active by achow101
  191. DrahtBot cross-referenced this on Jun 7, 2021 from issue Remove `gArgs` from `wallet.h` and `wallet.cpp` by kiminuo
  192. DrahtBot cross-referenced this on Jun 11, 2021 from issue refactor: Avoid wallet code writing node settings file by ryanofsky
  193. DrahtBot added the label Needs rebase on Jun 12, 2021
  194. ryanofsky force-pushed on Jun 14, 2021
  195. DrahtBot removed the label Needs rebase on Jun 14, 2021
  196. ryanofsky force-pushed on Jun 14, 2021
  197. ryanofsky force-pushed on Jun 15, 2021
  198. DrahtBot cross-referenced this on Jun 15, 2021 from issue log: Mitigate disk filling attacks by globally rate limiting LogPrintf(…) by dergoegge
  199. DrahtBot cross-referenced this on Jun 15, 2021 from issue log: Mitigate disk filling attacks by rate limiting LogPrintf by dergoegge
  200. ryanofsky cross-referenced this on Jul 19, 2021 from issue wallet: Reorder locks in dumpwallet to avoid lock order assertion by achow101
  201. ryanofsky cross-referenced this on Jul 21, 2021 from issue assumeutxo by jamesob
  202. DrahtBot cross-referenced this on Aug 14, 2021 from issue Rotate debug.log file by LarryRuane
  203. DrahtBot added the label Needs rebase on Aug 19, 2021
  204. ryanofsky force-pushed on Aug 19, 2021
  205. DrahtBot removed the label Needs rebase on Aug 19, 2021
  206. DrahtBot added the label Needs rebase on Sep 3, 2021
  207. ryanofsky force-pushed on Sep 3, 2021
  208. DrahtBot removed the label Needs rebase on Sep 3, 2021
  209. DrahtBot cross-referenced this on Sep 4, 2021 from issue wallet: Call load handlers without cs_wallet locked by promag
  210. DrahtBot cross-referenced this on Sep 4, 2021 from issue wallet: Properly support a wallet id by achow101
  211. DrahtBot cross-referenced this on Sep 9, 2021 from issue refactor: Remove `gArgs` from `wallet.h` and `wallet.cpp` (2) by kiminuo
  212. DrahtBot cross-referenced this on Sep 18, 2021 from issue multiprocess: Make interfaces::Chain::isTaprootActive non-const by ryanofsky
  213. DrahtBot cross-referenced this on Sep 29, 2021 from issue Remove `-rescan` startup parameter by meshcollider
  214. DrahtBot cross-referenced this on Sep 30, 2021 from issue rpc, wallet: Add listaddresses RPC by lsilva01
  215. DrahtBot added the label Needs rebase on Sep 30, 2021
  216. ryanofsky force-pushed on Oct 5, 2021
  217. DrahtBot removed the label Needs rebase on Oct 5, 2021
  218. DrahtBot added the label Needs rebase on Oct 13, 2021
  219. ryanofsky force-pushed on Oct 13, 2021
  220. DrahtBot removed the label Needs rebase on Oct 13, 2021
  221. in test/sanitizer_suppressions/tsan:40 in 24dfff610e outdated
      35 | +# Location is global 'std::__1::cout' of size 160 at 0x7f492785e270 (libc++.so.1+0x0000000c0290)
      36 | +# https://travis-ci.org/github/bitcoin/bitcoin/jobs/695004018
      37 | +#
      38 | +# Uses of std::cout are guaranteed thread safe by the c++ standard
      39 | +# https://stackoverflow.com/questions/50322790/thread-safety-and-piping-to-streams
      40 | +race:std::__1::ios_base::width
    


    maflcko commented at 10:09 AM on October 27, 2021:

    Can remove this after #23370

  222. DrahtBot cross-referenced this on Nov 3, 2021 from issue wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101
  223. DrahtBot cross-referenced this on Nov 6, 2021 from issue rpc: add getxpub by Sjors
  224. DrahtBot cross-referenced this on Nov 16, 2021 from issue policy: Treat taproot as always active by maflcko
  225. DrahtBot cross-referenced this on Nov 18, 2021 from issue rpc, wallet: addhdseed, infer seed when importing descriptor with xpub by Sjors
  226. DrahtBot cross-referenced this on Nov 24, 2021 from issue Move BlockManager to node/blockstorage by maflcko
  227. DrahtBot added the label Needs rebase on Nov 25, 2021
  228. lib: Add FoundBlock locator support
    Allow retrieving locator from Chain findBlock methods, used in upcoming commit
    by the wallet to get the locator for the chain tip on startup.
    a1c9b87413
  229. util: Release logger lock while calling log callbacks
    Prevents log callbacks from deadlocking if they call code containing log
    statements. In most cases it will not make sense to call logging code from a
    log hook, because it could trigger and endless chain of log messages. But if a
    hook is conditioned on the content of messages, like the DebugLogHelper hook,
    it can make sense and be useful.
    
    The new functionality is used in the CreateWalletFromFile test in an upcoming
    commit to be able to create transactions and blocks at specific points during
    test execution and check for race conditions.
    f6bd2b1e2b
  230. refactor: Make CWallet:::AttachChain return scan status
    Make AttachChain just responsible for syncing to the chain, moving error
    handling and chain assignment to the caller before more syncing
    functionality is moved in the next commit.
    
    Also, start using the AttachChain method in tests instead of trying
    attach in a more partial way using Chain::handleNotifications.
    37977ba4ec
  231. Wallet passive startup
    Move wallet startup code closer to a simple model where the wallet attaches to
    the chain with a single chain.handleNotifications() call, and just starts
    passively receiving blocks and mempool notifications from the last update point,
    instead having to actively rescan blocks and request a mempool snapshot, and
    deal with the tip changing, and deal with early or stale notifications.
    
    Also, stop locking the cs_wallet mutex and registering for validationinterface
    notifications before the rescan. This was new behavior since
    6a72f26968cf931c985d8d4797b6264274cabd06
    https://github.com/bitcoin/bitcoin/pull/16426 and is not ideal because it stops
    other wallets and rpcs and the gui from receiving new notifications until after the
    scan completes.
    
    This change is a half-step towards implementing multiwallet parallel scans
    (https://github.com/bitcoin/bitcoin/issues/11756), since it provides needed
    locator and birthday timestamp information to the Chain interface, and it
    rationalizes locking and event ordering in the startup code. The second half of
    implementing parallel rescans requires moving the ScanForWalletTransactions
    implementation (which this PR does not touch) from the wallet to the node.
    d6b8f93fb7
  232. refactor: Move cs_wallet to ReacceptWalletTransactions
    This is just a code simplification and revert of
    0440481c6bf5683eff669c789bdf6a306d99adc5 from
    https://github.com/bitcoin/bitcoin/pull/15652. No behavior is changing.
    2b898a0456
  233. ryanofsky force-pushed on Nov 29, 2021
  234. DrahtBot removed the label Needs rebase on Nov 29, 2021
  235. DrahtBot commented at 8:40 AM on December 3, 2021: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

    🐙 This pull request conflicts with the target branch and needs rebase.

    <sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

  236. DrahtBot added the label Needs rebase on Dec 3, 2021
  237. ryanofsky cross-referenced this on Dec 9, 2021 from issue Make rescans faster by jamesob
  238. ryanofsky cross-referenced this on Feb 1, 2022 from issue indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky
  239. uvhw referenced this in commit 47d44ccc3e on Feb 14, 2022
  240. ryanofsky cross-referenced this on Mar 7, 2022 from issue Automatic wallet rescan skipped after abort by laanwj
  241. maxim85200 approved
  242. achow101 closed this on Oct 12, 2022

  243. bitcoin locked this on Oct 12, 2023
Linked (view graph)
#10740 [wallet] `loadwallet` RPC - load wallet at runtime#11756 Multiwallet rescan sequentially scans multiple wallets instead of in parallel#15605 assumeutxo#15632 Remove ResendWalletTransactions from the Validation Interface#15652 wallet: Update transactions with current mempool after load#16122 gui: Enable console line edit on setClientModel#16426 Reverse cs_main, cs_wallet lock order and reduce cs_main locking#17484 wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload#17954 wallet: Remove calls to Chain::Lock methods#18280 wallet: UnregisterValidationInterface before SyncWithValidationInterfaceQueue#18601 wallet: Refactor WalletRescanReserver to use wallet reference#18635 Replace -Wthread-safety-analysis with broader -Wthread-safety#18727 test: Add CreateWalletFromFile test#18803 intermittent test failure "non-mandatory-script-verify-flag (Signature must be zero for failed CHECK(MULTI)SIG operation) (-26)"#18964 rpc, wallet: Scan mempool after import*#19049 test: Potential deadlock in wallet_tests/CreateWalletFromFile#19164 ci: tsan with wallet#19195 wallet: ScanForWalletTransactions cleanup#19249 Add means to handle negative capabilities in the Clang Thread Safety annotations#19538 ci: Add tsan suppression for race in DatabaseBatch#19863 Clarify eviction protection scope of outbound block-relay-only peers#20365 wallettool: add parameter to create descriptors wallet#20773 refactor: split CWallet::Create#21207 MOVEONLY: CWallet transaction code out of wallet.cpp/.h#21404 refactor: Remove MakeUnique<T>()#21415 refactor: remove Optional & nullopt#21525 [Bundle 4.5/n] Followup fixups to bundle 4#22492 wallet: Reorder locks in dumpwallet to avoid lock order assertion#23366 CI failure: `ThreadSanitizer: data race /usr/lib/llvm-12/bin/../include/c++/v1/ios:523:12 in std::__1::ios_base::width() const`#23370 test: Add ios_base::width tsan suppression#23727 Make rescans faster#24230 indexes: Stop using node internal types and locking cs_main, improve sync logic#24487 Automatic wallet rescan skipped after abort

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: 2026-05-20 06:54 UTC