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.

Sign up for GitHub

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

Jump to bottom

Refactor: Start to separate wallet from node #14437

Merged
merged 5 commits into from
Nov 9, 2018
Merged

Refactor: Start to separate wallet from node #14437

merged 5 commits into from
Nov 9, 2018

Conversation

ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Oct 9, 2018 •
edited
Loading

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 commented Oct 9, 2018 •
edited
Loading

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.

This was referenced Oct 9, 2018
Merged
Merged
Closed
Merged
Closed
Merged
Closed
Closed
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Merged
Merged
@scravy scravy mentioned this pull request Oct 9, 2018
Merged
@DrahtBot DrahtBot mentioned this pull request Oct 9, 2018
Closed
@ryanofsky
Copy link
Contributor Author

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

@jamesob jamesob mentioned this pull request Oct 10, 2018
Merged
scravy added a commit to dtr-org/unit-e that referenced this pull request Oct 10, 2018
@scravy
add proposer::TransactionPicker (#153)
2f4a236 
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
Contributor

Concept ACK 👍

@sipa
Copy link
Member

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

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

random-zebra added a commit to random-zebra/PIVX that referenced this pull request Mar 22, 2021
@random-zebra
[Refactoring] Make CWallet::FundTransaction atomic
7aa3a21 
>>> 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
@random-zebra
[Refactoring] Make CWallet::FundTransaction atomic
284c5a7 
>>> 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
@random-zebra
[Refactoring] Make CWallet::FundTransaction atomic
ca3cd7d 
>>> 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
@random-zebra
[Refactoring] Make CWallet::FundTransaction atomic
4337b0e 
>>> 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
@random-zebra
[Refactoring] Make CWallet::FundTransaction atomic
dd660df 
>>> 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
@random-zebra
[Refactoring] Make CWallet::FundTransaction atomic
c0f64c9 
>>> 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
@random-zebra
[Refactoring] Make CWallet::FundTransaction atomic
fcf958f 
>>> 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
@random-zebra
[Refactoring] Make CWallet::FundTransaction atomic
f92b17d 
>>> 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
@random-zebra
[Refactoring] Make CWallet::FundTransaction atomic
6479d21 
>>> 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
@random-zebra
[Refactoring] Make CWallet::FundTransaction atomic
3c7b524 
>>> 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
@random-zebra
[Refactoring] Make CWallet::FundTransaction atomic
fad741a 
>>> 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
@random-zebra
[Refactoring] Make CWallet::FundTransaction atomic
9f31559 
>>> 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
@random-zebra
[Refactoring] Make CWallet::FundTransaction atomic
286a35e 
>>> 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)
kwvg added a commit to kwvg/dash that referenced this pull request Aug 22, 2021
@kwvg
bitcoin#14437: Start to separate wallet from node
84efaab
kwvg added a commit to kwvg/dash that referenced this pull request Aug 30, 2021
@kwvg
bitcoin#14437: Start to separate wallet from node
4d58e89
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 18, 2021
@PastaPastaPasta
Merge bitcoin#14437: Refactor: Start to separate wallet from node
ba78c27
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 24, 2021
@PastaPastaPasta
Merge bitcoin#14437: Refactor: Start to separate wallet from node
abb144c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 28, 2021
@PastaPastaPasta
Merge bitcoin#14437: Refactor: Start to separate wallet from node
6deaf36
kwvg pushed a commit to kwvg/dash that referenced this pull request Oct 8, 2021
@kwvg
Merge bitcoin#14437: Refactor: Start to separate wallet from node
ba2179f
@kwvg kwvg mentioned this pull request Oct 12, 2021
Closed
kwvg pushed a commit to kwvg/dash that referenced this pull request Oct 12, 2021
@kwvg
Merge bitcoin#14437: Refactor: Start to separate wallet from node
9a8a8f6
kwvg pushed a commit to kwvg/dash that referenced this pull request Oct 16, 2021
@kwvg
Merge bitcoin#14437: Refactor: Start to separate wallet from node
3ebf514
kwvg pushed a commit to kwvg/dash that referenced this pull request Oct 16, 2021
@PastaPastaPasta @kwvg
Merge bitcoin#14437: Refactor: Start to separate wallet from node
d733c18
kwvg pushed a commit to kwvg/dash that referenced this pull request Oct 19, 2021
@PastaPastaPasta @kwvg
Merge bitcoin#14437: Refactor: Start to separate wallet from node
6fb12f8
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Oct 20, 2021
@PastaPastaPasta
Merge bitcoin#14437: Refactor: Start to separate wallet from node
ff22aa2
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Nov 1, 2021
@meshcollider @PastaPastaPasta
Merge bitcoin#16830: refactor: wallet: Cleanup walletinitinterface.h
8677f45 
4be3b76 refactor: Cleanup walletinitinterface.h (Hennadii Stepanov)

Pull request description:

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

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

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

Tree-SHA512: 5ed72e3deda3d7c7fb698a1a11db76199727e6c570dfc78422690dbda9a92af32e1913920062dd3c9f618095e7498c219ff9c145a4c151486865ebeaa20a1d3c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Nov 3, 2021
@meshcollider @PastaPastaPasta
Merge bitcoin#16830: refactor: wallet: Cleanup walletinitinterface.h
d33c479 
4be3b76 refactor: Cleanup walletinitinterface.h (Hennadii Stepanov)

Pull request description:

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

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

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

Tree-SHA512: 5ed72e3deda3d7c7fb698a1a11db76199727e6c570dfc78422690dbda9a92af32e1913920062dd3c9f618095e7498c219ff9c145a4c151486865ebeaa20a1d3c
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@PastaPastaPasta @pravblockc
Merge bitcoin#14437: Refactor: Start to separate wallet from node
7d25c86
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@meshcollider @pravblockc
Merge bitcoin#16830: refactor: wallet: Cleanup walletinitinterface.h
d8a0855 
4be3b76 refactor: Cleanup walletinitinterface.h (Hennadii Stepanov)

Pull request description:

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

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

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

Tree-SHA512: 5ed72e3deda3d7c7fb698a1a11db76199727e6c570dfc78422690dbda9a92af32e1913920062dd3c9f618095e7498c219ff9c145a4c151486865ebeaa20a1d3c
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@practicalswift practicalswift practicalswift left review comments

@maflcko maflcko maflcko left review comments

@promag promag promag left review comments

@meshcollider meshcollider meshcollider left review comments

@ken2812221 ken2812221 ken2812221 approved these changes

@jamesob jamesob jamesob approved these changes

Assignees
No one assigned
Labels
Refactoring Wallet
Projects
Process Separation
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.