Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove uses of chainActive and mapBlockIndex in wallet code #14711

Merged
merged 5 commits into from Jan 30, 2019

Conversation

@ryanofsky
Copy link
Contributor

commented Nov 12, 2018

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

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.

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Nov 12, 2018

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

Concept ACK

Nice readability and robustness improvement

@Empact

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

Concept ACK

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

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2018

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
Copy link
Member

left a comment

utACK d568bdb

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) {

This comment has been minimized.

Copy link
@meshcollider

meshcollider Nov 14, 2018

Member

Why not use findPruned(rescan_height, *tip_height)?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Nov 14, 2018

Author Contributor

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.

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

@meshcollider meshcollider added the Wallet label Nov 14, 2018

@promag
Copy link
Member

left a comment

Concept ACK.

@@ -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 comment has been minimized.

Copy link
@promag

promag Nov 15, 2018

Member

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.

This comment has been minimized.

Copy link
@Empact

Empact Jan 9, 2019

Member

Could be more clear with a comparison with nullopt

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 10, 2019

Author Contributor

re: #14711 (comment)

Could be more clear with a comparison with nullopt

Took suggestion

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

This comment has been minimized.

Copy link
@promag

promag Nov 15, 2018

Member

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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 10, 2019

Author Contributor

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.

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

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wchain2 branch from d568bdb to 5cad675 Dec 6, 2018

@DrahtBot DrahtBot removed the Needs rebase label Dec 6, 2018

int64_t* max_time = nullptr) = 0;

//! Estimate fraction of total transactions verified if blocks up to
//! given height are verified.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 12, 2018

Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 10, 2019

Author Contributor

re: #14711 (comment)

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

Thanks, fixed comment

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Strong Concept ACK
Plans to review...

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wchain2 branch from 5cad675 to 591c2c8 Dec 17, 2018

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

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wchain2 branch from 591c2c8 to 939a400 Dec 18, 2018

@DrahtBot DrahtBot removed the Needs rebase label Dec 18, 2018

@laanwj laanwj added this to Blockers in High-priority for review Jan 3, 2019

@Empact

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

@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

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

#include <memory>
#include <unordered_map>

This comment has been minimized.

Copy link
@Empact

Empact Jan 8, 2019

Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 10, 2019

Author Contributor

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.

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

This comment has been minimized.

Copy link
@Empact

Empact Jan 8, 2019

Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 10, 2019

Author Contributor

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.

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

This comment has been minimized.

Copy link
@Empact

Empact Jan 8, 2019

Member

stop_block and stopBlock are confusingly similar.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 10, 2019

Author Contributor

re: #14711 (comment)

stop_block and stopBlock are confusingly similar.

Added more renames

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/wchain2 branch from 939a400 to 7a87272 Jan 8, 2019

@meshcollider

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

Agreed, any comments/nits can be addressed in followup

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

Thanks for reviews, all! I will follow up with the remaining unaddressed comments.

The next PR in the series is #15288 (which is much simpler than this PR)

@jnewbery jnewbery removed this from Blockers in High-priority for review Jan 30, 2019

laanwj added a commit that referenced this pull request Jan 30, 2019

Merge #15292: Remove 'boost::optional'-related false positive -Wmaybe…
…-uninitialized warnings on GCC compiler

2d48314 Remove 'boost::optional'-related gcc warnings (Hennadii Stepanov)

Pull request description:

  #14711 introduced some warnings when building with gcc compiler.

  See:
  - #14711 (comment) by @laanwj
  - #14711 (review) by @ryanofsky

  This gcc [issue](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=47679) has been known since version 4.6.0 and last updated in 2017.
  From the boost [docs](https://www.boost.org/doc/libs/1_69_0/libs/optional/doc/html/boost_optional/quick_start/optional_automatic_variables.html):
  > The default constructor of `optional` creates an _uninitialized_ `optional` object.

  Also: [False positive with -Wmaybe-uninitialized](https://www.boost.org/doc/libs/1_69_0/libs/optional/doc/html/boost_optional/tutorial/gotchas/false_positive_with__wmaybe_uninitialized.html) ([pointed out](#15292 (comment)) by @Empact)

  This PR removes these warnings.

  cc: @Empact @practicalswift

Tree-SHA512: 752ae3c3ca6282bbf98726236fbc3069ab9d1aee57ae2ec2668b32e4541e7bc1acb15b7d6fa9e2b6daf1ec29c0987a1053ee1ca0f523b71367ff911221c58c94

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 4, 2019

Rename ScanResult stop_block field
Avoid confusion with stop_block argument as suggested
bitcoin#14711 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 4, 2019

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 4, 2019

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 4, 2019

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 4, 2019

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 4, 2019

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 4, 2019

@ryanofsky
Copy link
Contributor Author

left a comment

Suggested changes are in #15342

Show resolved Hide resolved src/interfaces/chain.cpp

if (tip_height) {
start_block = locked_chain->getBlockHash(start_height);
if (stop_height) {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 4, 2019

Author Contributor

re: #14711 (comment)

A comment here could prevent other people from falling into the same trap. Something like:

added

Show resolved Hide resolved src/wallet/rpcwallet.cpp
if (result.status == ScanResult::FAILURE) {
int64_t time_max;
if (!chain().findBlock(result.failed_block, nullptr /* block */, nullptr /* time */, &time_max)) {
throw std::logic_error("ScanForWalletTransactions returned invalid block hash");

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 4, 2019

Author Contributor

re: #14711 (comment)

This seems like a condition that can never be hit unless there is a coding error, in which case an assert appears more appropriate.

I don't think logic_error means anything different from assert. https://en.cppreference.com/w/cpp/error/logic_error at least says "It reports errors that are a consequence of faulty logic within the program." I would use an assert over a logic error only in cases where it seemed safer to abort the entire process than to just abort the current request. Or maybe to match preexisting code.

Show resolved Hide resolved src/wallet/wallet.cpp
* @param[in] start_block if not null, the scan will start at this block instead
* of the genesis block
* @param[in] stop_block if not null, the scan will stop at this block instead
* of the chain tip
*
* @return ScanResult indicating success or failure of the scan. SUCCESS if

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 4, 2019

Author Contributor

re: #14711 (comment)

Needs updating now that ScanResult is an object.

done

* the main chain after to the addition of any new keys you want to detect
* transactions for.
*/
CWallet::ScanResult CWallet::ScanForWalletTransactions(const CBlockIndex* const pindexStart, const CBlockIndex* const pindexStop, const WalletRescanReserver& reserver, const CBlockIndex*& failed_block, const CBlockIndex*& stop_block, bool fUpdate)
CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_block, const uint256& stop_block, const WalletRescanReserver& reserver, bool fUpdate)

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 4, 2019

Author Contributor

re: #14711 (comment)

Could you rename the members of the return object and update the parameter names in the header file?

done

Show resolved Hide resolved src/wallet/wallet.cpp
Show resolved Hide resolved src/wallet/wallet.cpp
throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height.");
}

if (tip_height) {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 4, 2019

Author Contributor

re: #14711 (comment)

nit: Not sure why you check for this

In this case even though "documentation states" and "In current operation" the chain will never be empty, I think handling a rescan of an empty chain by just returning success is more natural than making assumptions about when the chain is initialized or what other code expects it to be initialized. If you think adding the assumption will simplify things, though, I'd be happy to take a specific suggestion or review a change.

MarcoFalke added a commit that referenced this pull request Feb 5, 2019

Merge #15342: Suggested wallet code cleanups from #14711
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 #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

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Feb 5, 2019

Rename ScanResult stop_block field
Avoid confusion with stop_block argument as suggested
bitcoin#14711 (comment)

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Feb 5, 2019

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Feb 5, 2019

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Feb 5, 2019

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Feb 5, 2019

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Feb 5, 2019

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Feb 5, 2019

meshcollider added a commit that referenced this pull request Mar 21, 2019

Merge #10973: Refactor: separate wallet from node
d358466 Remove remaining wallet accesses to node globals (Russell Yanofsky)
b1b2b23 Remove use of CCoinsViewMemPool::GetCoin in wallet code (Russell Yanofsky)
4e4d9e9 Remove use of CRPCTable::appendCommand in wallet code (Russell Yanofsky)
91868e6 Remove use CValidationInterface in wallet code (Russell Yanofsky)

Pull request description:

  This PR is the last in a chain of PRs (#14437, #14711, and #15288) that make the wallet code access node state through an abstract [`Chain`](https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep/src/interfaces/chain.h) class in [`src/interfaces/`](https://github.com/ryanofsky/bitcoin/tree/pr/wipc-sep/src/interfaces) instead of using global variables like `cs_main`, `chainActive`, and `g_connman`. After this PR, wallet code no longer accesses global variables declared outside the wallet directory, and no longer calls functions accessing those globals (as verified by the `hide-globals` script in #10244).

  This PR and the previous PRs have been refactoring changes that do not affect behavior. Previous PRs have consisted of lots of mechanical changes like:

  ```diff
  -    wtx.nTimeReceived = GetAdjustedTime();
  +    wtx.nTimeReceived = m_chain->getAdjustedTime();
  ```

  This PR is smaller, but less mechanical. It replaces last few bits of wallet code that access node state directly (through `CValidationInterface`, `CRPCTable`, and `CCoinsViewMemPool` interfaces) with code that uses the `Chain` interface.

  These changes allow followup PR #10102 (multiprocess gui & wallet PR) to work without any significant updates to wallet code. Additionally they:

  * Provide a single place to describe the interface between wallet and node code.
  * Can make better wallet testing possible, because the `Chain` object consists of virtual methods that can be overloaded for mocking. (This could be used to test edge cases in the rescan code, for example).

Tree-SHA512: e6291d8a3c50bdff18a9c8ad11e729beb30b5b7040d7aaf31ba678800b4a97b2dd2be76340b1e5c01fe2827d67d37ed1bb4c8380cf8ed653aadfea003e9b22e7

ariard added a commit to ariard/bitcoin that referenced this pull request Mar 26, 2019

refactor: combine Chain::findFirstBlockWithTime/findFirstBlockWithTim…
…eAndHeight

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

ariard added a commit to ariard/bitcoin that referenced this pull request Mar 27, 2019

refactor: combine Chain::findFirstBlockWithTime/findFirstBlockWithTim…
…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

MarcoFalke added a commit that referenced this pull request Apr 19, 2019

Merge #15670: refactor: combine Chain::findFirstBlockWithTime/findFir…
…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

Kiku-git added a commit to Kiku-git/bitcoin that referenced this pull request May 16, 2019

refactor: combine Chain::findFirstBlockWithTime/findFirstBlockWithTim…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.