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

Refactor: Start to separate wallet from node #14437

Merged
merged 5 commits into from Nov 9, 2018
Merged

Conversation

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Oct 9, 2018

This creates an incomplete Chain interface in src/interfaces/ and begins to update wallet code to use it.

#10973 builds on this, changing the wallet to use the new interface to access chain state, instead of using CBlockIndex pointers and global variables like chainActive.

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Oct 9, 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:

  • #14641 (RPC: Add min/max confirmation options to fund transaction calls by promag)
  • #14602 (Bugfix: Correctly calculate balances when min_conf is used, and for getbalance("*") by luke-jr)
  • #14582 (wallet: try -avoidpartialspends mode and use its result if fees do not change by kallewoof)
  • #14530 (Use RPCHelpMan to generate RPC doc strings by MarcoFalke)
  • #14411 ([wallet] Restore ability to list incoming transactions by label by ryanofsky)
  • #14384 (Resolve validationinterface circular dependencies by 251Labs)
  • #14121 (Index for BIP 157 block filters by jimpo)
  • #13756 (wallet: "avoid_reuse" wallet flag for improved privacy by kallewoof)
  • #13612 (Qt: Only call tryGetBalances in pollBalanceChanged if the result will be used. by tecnovert)
  • #13582 (Extract AppInitLoadBlockIndex from AppInitMain by Empact)
  • #12508 (IsAllFromMe by kallewoof)
  • #11652 (Add missing locks: validation.cpp + related by practicalswift)
  • #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.

@ryanofsky
Copy link
Contributor Author

@ryanofsky ryanofsky commented Oct 10, 2018

Added 2 more small commits d0af21f -> a10e919 (pr/wchain.1 -> pr/wchain.2, compare) from #10973 to fix the thread safety warnings (which are harmless but treated like errors).

scravy added a commit to dtr-org/unit-e that referenced this pull request Oct 10, 2018
This adds a class named TransactionPicker and introduces a new namespace proposer in its own directory. I sort of got fed up of the untestability of bitcoin and that everything is intertwined with each other directly.

In order to propose a block I need to build one which is I have to (1) pick transactions (2) put a coinstake transaction (includes block height, signature, reward output) (3) sign the thing. Picking transactions is already implemented in bitcoin but it's well buried inside CBlockAssembler which does too many things at once for my taste + deals with that horrendous CBlockTemplate (see comment in BlockAssemblerAdapter in the source).

Since I want my Proposer to be testable and (1+2+3) are essentially simple operations I want the code to be simple. So here is the part which performs 1, by delegating to bitcoin's block assembler (for now). This way I do not have to change the block assembler or overwrite the coinbase transaction in the block template.

I would be interested in hearing your thoughts about my approach, as I intend to follow it with everything I do from now on. Maybe you are fundamentally opposed to virtual methods or whatever.

In bitcoin there have been small efforts to extract things into Interfaces (CValidationInterface for instance) also. More recent work is on separating wallet and node and also here they start to create interfaces (bitcoin/bitcoin#14437, src/interfaces).
@meshcollider
Copy link
Member

@meshcollider meshcollider commented Oct 12, 2018

Concept ACK 👍

@sipa
Copy link
Member

@sipa sipa commented Oct 12, 2018

Is it intentional that the interfaces code uses a different coding style than the rest of the code?

If so, does this need documenting somewhere, and if not, can it be converted?

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Oct 14, 2018

This PR does not seem to compile when rebased on master :-)

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@MarcoFalke MarcoFalke removed this from Blockers in High-priority for review Nov 9, 2018
@meshcollider
Copy link
Member

@meshcollider meshcollider commented Nov 11, 2018

post-merge-utACK ea961c3

This is looking good, thanks Russ! 🎉

meshcollider added a commit that referenced this pull request Mar 21, 2019
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
meshcollider added a commit that referenced this pull request Sep 8, 2019
4be3b76 refactor: Cleanup walletinitinterface.h (Hennadii Stepanov)

Pull request description:

  Forward declarations of `CScheduler` and `CRPCTable` classes are no longer needed after ea961c3 (#14437) commit.

  Including `<string>` is no longer needed after 4d4185a (#13190) commit.

ACKs for top commit:
  theStack:
    ACK 4be3b76
  promag:
    ACK 4be3b76.
  kristapsk:
    ACK 4be3b76 (tested that it builds)

Tree-SHA512: 5ed72e3deda3d7c7fb698a1a11db76199727e6c570dfc78422690dbda9a92af32e1913920062dd3c9f618095e7498c219ff9c145a4c151486865ebeaa20a1d3c
@laanwj laanwj mentioned this pull request Sep 30, 2019
@ryanofsky ryanofsky added this to Done in Process Separation Mar 6, 2020
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Mar 11, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Mar 12, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Mar 18, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Mar 22, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Mar 25, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Mar 25, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Mar 25, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Mar 25, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Mar 25, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Mar 26, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Mar 26, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Mar 31, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Mar 31, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Apr 5, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Apr 6, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Apr 7, 2021
>>> Inspired by bitcoin#11864 (+bitcoin#10784)

Since CreateTransaction still needs cs_main we need to obtain both locks
in FundTransaction, to preserve locking order.

After introducing the chain interface (bitcoin#14437) we can change the
LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet