Remove uses of chainActive and mapBlockIndex in wallet code #14711

ryanofsky · 2018-11-12T19:35:40Z

This change removes uses of chainActive and mapBlockIndex globals in wallet code. It is a refactoring change which does not affect external behavior.

This is the next step in the larger #10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in #10102.

maflcko · 2018-11-12T22:09:34Z

Concept ACK. Though, I'd prefer if we first cleaned up the interface for ScanForWalletTransactions. I believe that right now it is relying on too much undefined behaviour that this refactoring could be meaningfully reviewed.

ryanofsky · 2018-11-12T22:34:40Z

too much undefined behaviour that this refactoring could be meaningfully reviewed.

Are you talking about a small part of this PR or the whole thing? Most of the changes here don't have anything to do with ScanForWalletTransactions. I could even drop the rescan changes and save them for a different PR if you are only worried about them.

practicalswift · 2018-11-13T10:16:18Z

Concept ACK

Nice readability and robustness improvement

Empact · 2018-11-13T10:21:24Z

Concept ACK

Would be easier/safer to review if split up. Like Marco, I’d like to see ScanFor improved first.

DrahtBot · 2018-11-13T15:20:05Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15129 (rpc: Added ability to remove watch only addresses by benthecarman)
#14942 (wallet: Return a ScanResult from CWallet::RescanFromTime by Empact)
#14533 (wallet: ensure wallet files are not reused across chains by mrwhythat)
#10973 (Refactor: separate wallet from node by ryanofsky)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)
#9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

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.

meshcollider

utACK d568bdb

meshcollider · 2018-11-14T10:37:11Z

src/wallet/wallet.cpp

-            while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA) && block->pprev->nTx > 0 && pindexRescan != block)
-                block = block->pprev;
+            int block_height = *tip_height;
+            while (block_height > 0 && locked_chain->haveBlockOnDisk(block_height - 1) && rescan_height != block_height) {


Why not use findPruned(rescan_height, *tip_height)?

re: #14711 (comment)

Why not use findPruned

I used haveBlockOnDisk here because I was doing a very literal translation and findPruned doesn't have the block->pprev->nTx > 0 condition. If can drop that condition or add it to findPruned, though, if it would be an improvement.

src/wallet/wallet.cpp

promag

Concept ACK.

promag · 2018-11-14T23:34:20Z

src/wallet/rpcdump.cpp

@@ -366,8 +366,7 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
    if (merkleBlock.txn.ExtractMatches(vMatch, vIndex) == merkleBlock.header.hashMerkleRoot) {

        auto locked_chain = pwallet->chain().lock();
-        const CBlockIndex* pindex = LookupBlockIndex(merkleBlock.header.GetHash());
-        if (!pindex || !chainActive.Contains(pindex)) {
+        if (!locked_chain->getBlockHeight(merkleBlock.header.GetHash())) {


This sounds wrong, as if it wants to discard genesis block (height == 0). Only got it after seeing the return type Optional<int>. IMO .contains(hash) would be preferable.

Could be more clear with a comparison with nullopt

re: #14711 (comment)

Could be more clear with a comparison with nullopt

Took suggestion

promag · 2018-11-15T00:16:53Z

src/wallet/rpcwallet.cpp

@@ -1526,24 +1529,18 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
    auto locked_chain = pwallet->chain().lock();
    LOCK(pwallet->cs_wallet);

-    const CBlockIndex* pindex = nullptr;    // Block index of the specified block or the common ancestor, if the block provided was in a deactivated chain.
-    const CBlockIndex* paltindex = nullptr; // Block index of the specified block, even if it's in a deactivated chain.
+    Optional<int> height;    // Height of the specified block or the common ancestor, if the block provided was in a deactivated chain.


Isn't this change conceptually different? Even though the chain is locked, this can be copied and used without the lock and therefore can "point" to other block if a reorg occurs. This wouldn't occur with CBlockIndex.

Looks like you could either:

create interfaces::BlockIndex

replace CBlockIndex* with uint256.

re: #14711 (comment)

Isn't this change conceptually different?

I think so, a height is conceptually different from a block pointer.

Looks like you could either:

When the chain is locked, a height uniquely identifies a block, so many locked_chain methods take height arguments. I think it would good to eliminate these calls entirely (#10973 (comment)) and not very useful to tweak data types and just hide the fact that the chain is locked.

But either way, I want the API to change in the future and become nicer, more minimal, and more useful over time. Right now the API just reflects how the wallet currently works, to keep changes to wallet code in this PR as minimal as possible.

src/wallet/wallet.h

maflcko · 2018-12-12T03:54:07Z

src/interfaces/chain.h

+        int64_t* max_time = nullptr) = 0;
+
+    //! Estimate fraction of total transactions verified if blocks up to
+    //! given height are verified.


doc-nit: height is not given (at least not directly)

re: #14711 (comment)

doc-nit: height is not given (at least not directly)

Thanks, fixed comment

jonasschnelli · 2018-12-12T06:02:03Z

Strong Concept ACK
Plans to review...

Empact · 2019-01-08T08:34:58Z

@ryanofsky I went ahead and split this up into a few commits, found it easier to review. Built and ran tests on each commit. https://github.com/Empact/bitcoin/tree/pr/wchain2

Empact · 2019-01-08T08:36:06Z

src/interfaces/chain.cpp

 #include <util/system.h>
 #include <validation.h>

 #include <memory>
+#include <unordered_map>


nit: I believe this belongs in validation.h, as unordered_map isn't used directly here.

re: #14711 (comment)

nit: I believe this belongs in validation.h, as unordered_map isn't used directly here.

This was added by IWYU because unordered map methods are called here. In theory this allows validation.h to switch to forward declarations and drop its unordered_map include without this file having to change. I don't think IWYU rationale is completely airtight (particularly in this case where validation.h might change mapBlockIndex into a different type of map with the same methods), but I like the IWYU tool, and don't think its worth spending a lot of time to analyze its decisions absent a compelling reason.

Empact · 2019-01-08T08:43:36Z

src/interfaces/chain.cpp

@@ -35,6 +150,34 @@ class ChainImpl : public Chain
        return std::move(result);
    }
    std::unique_ptr<Chain::Lock> assumeLocked() override { return MakeUnique<LockImpl>(); }
+    bool findBlock(const uint256& hash, CBlock* block, int64_t* time, int64_t* time_max) override


nit: none of the callers use more than one of the out args, so splitting this up would make the call sites simpler/clearer.

re: #14711 (comment)

nit: none of the callers use more than one of the out args, so splitting this up would make the call sites simpler/clearer.

There are many ways to design this API, and I just opted for one here that I thought would keep chain.h a little shorter and more organized, at the expense of being more verbose at call sites. If you have a specific alternative in mind and want to convince me or another reviewer that it's better, I'd be happy to adopt it here, or review a followup PR.

Empact · 2019-01-08T08:45:46Z

src/wallet/rpcwallet.cpp

    CWallet::ScanResult result =
-        pwallet->ScanForWalletTransactions(pindexStart, pindexStop, reserver, failed_block, stopBlock, true);
+        pwallet->ScanForWalletTransactions(start_block, stop_block, reserver, failed_block, stopBlock, true /* prune */);


stop_block and stopBlock are confusingly similar.

re: #14711 (comment)

stop_block and stopBlock are confusingly similar.

Added more renames

ryanofsky · 2019-01-08T15:48:54Z

Thanks @Empact! I reset the PR to your branch.

Rebased d568bdb -> 5cad675 (pr/wchain2.1 -> pr/wchain2.2) due to conflict with #14380
Rebased 5cad675 -> 591c2c8 (pr/wchain2.2 -> pr/wchain2.3) due to conflict with #13076
Rebased 591c2c8 -> 939a400 (pr/wchain2.3 -> pr/wchain2.4) due to conflict with #14957
Split 939a400 -> 7a87272 (pr/wchain2.4 -> pr/wchain2.5) resetting to Empact:pr/wchain2 (no diffs)
Rebased 7a87272 -> ca76bcc (pr/wchain2.5 -> pr/wchain2.6) due to conflict with #15039

…eAndHeight As suggested in bitcoin#14711, pass height to CChain::FindEarliestAtLeast to simplify Chain interface by combining findFirstBlockWithTime and findFirstBlockWithTimeAndHeight into one Extend findearliestatleast_edge_test in consequence

@ariard

…stBlockWithTimeAndHeight 765c0b3 refactor: combine Chain::findFirstBlockWithTime/findFirstBlockWithTimeAndHeight (Antoine Riard) Pull request description: As suggested in #14711, pass height to CChain::FindEarliestAtLeast to simplify Chain interface by combining findFirstBlockWithTime and findFirstBlockWithTimeAndHeight into one ACKs for commit 765c0b: jnewbery: utACK 765c0b3. Nice work @ariard! ryanofsky: utACK 765c0b3. Looks good, thanks for implementing the suggestion! Tree-SHA512: 63f98252a93da95f08c0b6325ea98f717aa9ae4036d17eaa6edbec68e5ddd65672d66a6af267b80c36311fffa9b415a47308e95ea7718b300b685e23d4e9e6ec

…eAndHeight As suggested in bitcoin#14711, pass height to CChain::FindEarliestAtLeast to simplify Chain interface by combining findFirstBlockWithTime and findFirstBlockWithTimeAndHeight into one Extend findearliestatleast_edge_test in consequence

Summary: ``` This change removes uses of chainActive and mapBlockIndex globals in wallet code. It is a refactoring change which does not affect external behavior. ``` Backport of core [[bitcoin/bitcoin#14711 | PR14711]]. Test Plan: ninja all check-extended Ran the tests with UBSAN and ASAN as well. Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5683

…wallet code

…wallet code 44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky) db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky) 2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky) d93c4c1 Add time methods to the Chain interface (Russell Yanofsky) 700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky) Pull request description: This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior. This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102. Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8 # Conflicts: # src/Makefile.am # src/interfaces/wallet.cpp # src/qt/test/wallettests.cpp # src/wallet/rpcdump.cpp # src/wallet/rpcwallet.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp # src/wallet/wallet.h

aebafd0 Rename Chain getLocator -> getTipLocator (Russell Yanofsky) 2c1fbaa Drop redundant get_value_or (Russell Yanofsky) 84adb20 Fix ScanForWalletTransactions start_block comment (Russell Yanofsky) 2efa66b Document rescanblockchain returned stop_height being null (Russell Yanofsky) db2d093 Add suggested rescanblockchain comments (Russell Yanofsky) a8d645c Update ScanForWalletTransactions result comment (Russell Yanofsky) 95a812b Rename ScanResult stop_block field (Russell Yanofsky) Pull request description: This implements suggested changes from bitcoin#14711 review comments that didn't make make it in before merging. There are no changes in behavior in this PR, just documentation updates, simplifications, and variable renames. Tree-SHA512: 39f1a5718195732b70b5e427c3b3e4295ea5af6328a5991763a422051212dfb95383186db0c0504ce2c2782fb61998dfd2fe9851645b7cb4e75d849049483cc8 # Conflicts: # src/interfaces/chain.cpp # src/qt/test/wallettests.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp

…wallet code 44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky) db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky) 2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky) d93c4c1 Add time methods to the Chain interface (Russell Yanofsky) 700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky) Pull request description: This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior. This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102. Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8 # Conflicts: # src/Makefile.am # src/interfaces/wallet.cpp # src/qt/test/wallettests.cpp # src/wallet/rpcdump.cpp # src/wallet/rpcwallet.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp # src/wallet/wallet.h

aebafd0 Rename Chain getLocator -> getTipLocator (Russell Yanofsky) 2c1fbaa Drop redundant get_value_or (Russell Yanofsky) 84adb20 Fix ScanForWalletTransactions start_block comment (Russell Yanofsky) 2efa66b Document rescanblockchain returned stop_height being null (Russell Yanofsky) db2d093 Add suggested rescanblockchain comments (Russell Yanofsky) a8d645c Update ScanForWalletTransactions result comment (Russell Yanofsky) 95a812b Rename ScanResult stop_block field (Russell Yanofsky) Pull request description: This implements suggested changes from bitcoin#14711 review comments that didn't make make it in before merging. There are no changes in behavior in this PR, just documentation updates, simplifications, and variable renames. Tree-SHA512: 39f1a5718195732b70b5e427c3b3e4295ea5af6328a5991763a422051212dfb95383186db0c0504ce2c2782fb61998dfd2fe9851645b7cb4e75d849049483cc8 # Conflicts: # src/interfaces/chain.cpp # src/qt/test/wallettests.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp

…wallet code 44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky) db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky) 2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky) d93c4c1 Add time methods to the Chain interface (Russell Yanofsky) 700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky) Pull request description: This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior. This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102. Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8 # Conflicts: # src/Makefile.am # src/interfaces/wallet.cpp # src/qt/test/wallettests.cpp # src/wallet/rpcdump.cpp # src/wallet/rpcwallet.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp # src/wallet/wallet.h

aebafd0 Rename Chain getLocator -> getTipLocator (Russell Yanofsky) 2c1fbaa Drop redundant get_value_or (Russell Yanofsky) 84adb20 Fix ScanForWalletTransactions start_block comment (Russell Yanofsky) 2efa66b Document rescanblockchain returned stop_height being null (Russell Yanofsky) db2d093 Add suggested rescanblockchain comments (Russell Yanofsky) a8d645c Update ScanForWalletTransactions result comment (Russell Yanofsky) 95a812b Rename ScanResult stop_block field (Russell Yanofsky) Pull request description: This implements suggested changes from bitcoin#14711 review comments that didn't make make it in before merging. There are no changes in behavior in this PR, just documentation updates, simplifications, and variable renames. Tree-SHA512: 39f1a5718195732b70b5e427c3b3e4295ea5af6328a5991763a422051212dfb95383186db0c0504ce2c2782fb61998dfd2fe9851645b7cb4e75d849049483cc8 # Conflicts: # src/interfaces/chain.cpp # src/qt/test/wallettests.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp

…wallet code 44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky) db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky) 2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky) d93c4c1 Add time methods to the Chain interface (Russell Yanofsky) 700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky) Pull request description: This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior. This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102. Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8 # Conflicts: # src/Makefile.am # src/interfaces/wallet.cpp # src/qt/test/wallettests.cpp # src/wallet/rpcdump.cpp # src/wallet/rpcwallet.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp # src/wallet/wallet.h

aebafd0 Rename Chain getLocator -> getTipLocator (Russell Yanofsky) 2c1fbaa Drop redundant get_value_or (Russell Yanofsky) 84adb20 Fix ScanForWalletTransactions start_block comment (Russell Yanofsky) 2efa66b Document rescanblockchain returned stop_height being null (Russell Yanofsky) db2d093 Add suggested rescanblockchain comments (Russell Yanofsky) a8d645c Update ScanForWalletTransactions result comment (Russell Yanofsky) 95a812b Rename ScanResult stop_block field (Russell Yanofsky) Pull request description: This implements suggested changes from bitcoin#14711 review comments that didn't make make it in before merging. There are no changes in behavior in this PR, just documentation updates, simplifications, and variable renames. Tree-SHA512: 39f1a5718195732b70b5e427c3b3e4295ea5af6328a5991763a422051212dfb95383186db0c0504ce2c2782fb61998dfd2fe9851645b7cb4e75d849049483cc8 # Conflicts: # src/interfaces/chain.cpp # src/qt/test/wallettests.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp

…wallet code

…wallet code 44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky) db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky) 2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky) d93c4c1 Add time methods to the Chain interface (Russell Yanofsky) 700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky) Pull request description: This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior. This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102. Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8

aebafd0 Rename Chain getLocator -> getTipLocator (Russell Yanofsky) 2c1fbaa Drop redundant get_value_or (Russell Yanofsky) 84adb20 Fix ScanForWalletTransactions start_block comment (Russell Yanofsky) 2efa66b Document rescanblockchain returned stop_height being null (Russell Yanofsky) db2d093 Add suggested rescanblockchain comments (Russell Yanofsky) a8d645c Update ScanForWalletTransactions result comment (Russell Yanofsky) 95a812b Rename ScanResult stop_block field (Russell Yanofsky) Pull request description: This implements suggested changes from bitcoin#14711 review comments that didn't make make it in before merging. There are no changes in behavior in this PR, just documentation updates, simplifications, and variable renames. Tree-SHA512: 39f1a5718195732b70b5e427c3b3e4295ea5af6328a5991763a422051212dfb95383186db0c0504ce2c2782fb61998dfd2fe9851645b7cb4e75d849049483cc8 # Conflicts: # src/interfaces/chain.cpp # src/qt/test/wallettests.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp

…wallet code 44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky) db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky) 2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky) d93c4c1 Add time methods to the Chain interface (Russell Yanofsky) 700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky) Pull request description: This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior. This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102. Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8

aebafd0 Rename Chain getLocator -> getTipLocator (Russell Yanofsky) 2c1fbaa Drop redundant get_value_or (Russell Yanofsky) 84adb20 Fix ScanForWalletTransactions start_block comment (Russell Yanofsky) 2efa66b Document rescanblockchain returned stop_height being null (Russell Yanofsky) db2d093 Add suggested rescanblockchain comments (Russell Yanofsky) a8d645c Update ScanForWalletTransactions result comment (Russell Yanofsky) 95a812b Rename ScanResult stop_block field (Russell Yanofsky) Pull request description: This implements suggested changes from bitcoin#14711 review comments that didn't make make it in before merging. There are no changes in behavior in this PR, just documentation updates, simplifications, and variable renames. Tree-SHA512: 39f1a5718195732b70b5e427c3b3e4295ea5af6328a5991763a422051212dfb95383186db0c0504ce2c2782fb61998dfd2fe9851645b7cb4e75d849049483cc8 # Conflicts: # src/interfaces/chain.cpp # src/qt/test/wallettests.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp

…wallet code 44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky) db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky) 2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky) d93c4c1 Add time methods to the Chain interface (Russell Yanofsky) 700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky) Pull request description: This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior. This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102. Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8

aebafd0 Rename Chain getLocator -> getTipLocator (Russell Yanofsky) 2c1fbaa Drop redundant get_value_or (Russell Yanofsky) 84adb20 Fix ScanForWalletTransactions start_block comment (Russell Yanofsky) 2efa66b Document rescanblockchain returned stop_height being null (Russell Yanofsky) db2d093 Add suggested rescanblockchain comments (Russell Yanofsky) a8d645c Update ScanForWalletTransactions result comment (Russell Yanofsky) 95a812b Rename ScanResult stop_block field (Russell Yanofsky) Pull request description: This implements suggested changes from bitcoin#14711 review comments that didn't make make it in before merging. There are no changes in behavior in this PR, just documentation updates, simplifications, and variable renames. Tree-SHA512: 39f1a5718195732b70b5e427c3b3e4295ea5af6328a5991763a422051212dfb95383186db0c0504ce2c2782fb61998dfd2fe9851645b7cb4e75d849049483cc8 # Conflicts: # src/interfaces/chain.cpp # src/qt/test/wallettests.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp

…wallet code 44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky) db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky) 2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky) d93c4c1 Add time methods to the Chain interface (Russell Yanofsky) 700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky) Pull request description: This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior. This is the next step in the larger bitcoin#10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in bitcoin#10102. Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8

aebafd0 Rename Chain getLocator -> getTipLocator (Russell Yanofsky) 2c1fbaa Drop redundant get_value_or (Russell Yanofsky) 84adb20 Fix ScanForWalletTransactions start_block comment (Russell Yanofsky) 2efa66b Document rescanblockchain returned stop_height being null (Russell Yanofsky) db2d093 Add suggested rescanblockchain comments (Russell Yanofsky) a8d645c Update ScanForWalletTransactions result comment (Russell Yanofsky) 95a812b Rename ScanResult stop_block field (Russell Yanofsky) Pull request description: This implements suggested changes from bitcoin#14711 review comments that didn't make make it in before merging. There are no changes in behavior in this PR, just documentation updates, simplifications, and variable renames. Tree-SHA512: 39f1a5718195732b70b5e427c3b3e4295ea5af6328a5991763a422051212dfb95383186db0c0504ce2c2782fb61998dfd2fe9851645b7cb4e75d849049483cc8 # Conflicts: # src/interfaces/chain.cpp # src/qt/test/wallettests.cpp # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp

ryanofsky mentioned this pull request Nov 12, 2018

Refactor: separate wallet from node #10973

Merged

maflcko added the Refactoring label Nov 12, 2018

maflcko added this to the 0.18.0 milestone Nov 12, 2018

meshcollider mentioned this pull request Nov 13, 2018

wallet: Require a start block in ScanForWalletTransactions #14712

Closed

meshcollider reviewed Nov 14, 2018

View reviewed changes

meshcollider added the Wallet label Nov 14, 2018

promag reviewed Nov 15, 2018

View reviewed changes

maflcko reviewed Nov 15, 2018

View reviewed changes

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

DrahtBot added the Needs rebase label Nov 30, 2018

ryanofsky force-pushed the pr/wchain2 branch from d568bdb to 5cad675 Compare December 6, 2018 19:17

DrahtBot removed the Needs rebase label Dec 6, 2018

maflcko reviewed Dec 12, 2018

View reviewed changes

DrahtBot added the Needs rebase label Dec 12, 2018

ryanofsky force-pushed the pr/wchain2 branch from 5cad675 to 591c2c8 Compare December 17, 2018 21:35

DrahtBot added Needs rebase and removed Needs rebase labels Dec 17, 2018

ryanofsky force-pushed the pr/wchain2 branch from 591c2c8 to 939a400 Compare December 18, 2018 18:01

DrahtBot removed the Needs rebase label Dec 18, 2018

Empact reviewed Jan 8, 2019

View reviewed changes

ryanofsky force-pushed the pr/wchain2 branch from 939a400 to 7a87272 Compare January 8, 2019 15:39

hebasto mentioned this pull request Mar 20, 2020

wallet: Remove calls to Chain::Lock methods #17954

Merged

random-zebra mentioned this pull request Mar 3, 2021

Killing wallet and GUI cs_main locks PIVX-Project/PIVX#2209

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Oct 16, 2021

merge bitcoin#14711: Remove uses of chainActive and mapBlockIndex in …

6486787

…wallet code

kwvg added a commit to kwvg/dash that referenced this pull request Oct 16, 2021

merge bitcoin#14711: Remove uses of chainActive and mapBlockIndex in …

5d1e8a1

…wallet code

kwvg pushed a commit to kwvg/dash that referenced this pull request Oct 31, 2021

merge bitcoin#14711: Remove uses of chainActive and mapBlockIndex in …

ce06a98

…wallet code

kwvg added a commit to kwvg/dash that referenced this pull request Oct 31, 2021

merge bitcoin#14711: Remove uses of chainActive and mapBlockIndex in …

348fe23

…wallet code

This was referenced Oct 31, 2021

merge bitcoin#10973: separate wallet from node dashpay/dash#4513

Closed

merge bitcoin#10973, #15039, #15288: separate wallet from node dashpay/dash#4558

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Remove uses of chainActive and mapBlockIndex in wallet code #14711

Remove uses of chainActive and mapBlockIndex in wallet code #14711

Uh oh!

Conversation

ryanofsky commented Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 12, 2018

Uh oh!

ryanofsky commented Nov 12, 2018

Uh oh!

practicalswift commented Nov 13, 2018

Uh oh!

Empact commented Nov 13, 2018

Uh oh!

DrahtBot commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Nov 15, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Dec 12, 2018

Uh oh!

Empact commented Jan 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact Jan 8, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ryanofsky commented Nov 12, 2018 •

edited

Loading

DrahtBot commented Nov 13, 2018 •

edited

Loading

promag Nov 15, 2018 •

edited

Loading

Empact Jan 8, 2019 •

edited

Loading

ryanofsky commented Jan 8, 2019 •

edited

Loading