refactor: split CWallet::Create #20773

S3RK · 2020-12-26T16:05:04Z

This is a followup for #20365 (comment)
First part of a refactoring with overall goal to simplify CWallet and de-duplicate code with wallettool

Rationale: split CWallet::Create and create CWallet::AttachChain.

CWallet::AttachChain takes chain as first parameter on purpose. In future I suggest we can remove chain from CWallet constructor.

The second commit is based on be164f9 from #15719 (thanks ryanofsky)

cc ryanofsky achow101

DrahtBot · 2020-12-26T18:04:44Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc, wallet: Expose wallet id in getwalletinfo RPC output #20243 (rpc, wallet: Expose wallet id in getwalletinfo RPC output by hebasto)
wallet: Properly support a wallet id #20205 (wallet: Properly support a wallet id by achow101)
refactor: remove ::vpwallets and related global variables #19101 (refactor: remove ::vpwallets and related global variables 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

Nice PR! This helps more clearly separate CWallet creation from syncing so duplicate wallet tool code can be removed without changing behavior. Would suggest a few changes, though:

It would be good to squash 5th commit into 1st commit. No need to add a method and enum in one commit and then change the signature and drop the enum right after.
I think 4th and 7th commits should probably be dropped, but see specific comments below.

Reviewed:

4398236 refactor: Add CWallet:::AttachChain method (1/7)
b528485 refactor: Track first run state in CWallet (2/7)
b20c1e5 refactor: Handle first run in CWallet::AttachChain (3/7)
eabcd95 refactor: Move chain checks into CWallet::AttachChain (4/7)
847588a refactor: Update CWallet::AttachChain signature (5/7)
190317a CWallet::Create move chain init message up into calling code (6/7)
9b57578 CWallet::Create move CWallet::AttachChain up into calling code (7/7)

src/wallet/wallet.h

ryanofsky · 2021-01-12T00:52:12Z

src/wallet/wallet.cpp

@@ -3955,11 +3955,6 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, const std::st
                               _("This is the transaction fee you will pay if you send a transaction."));
        }
        walletInstance->m_pay_tx_fee = CFeeRate(nFeePerK, 1000);
-        if (walletInstance->m_pay_tx_fee < chain.relayMinFee()) {
-            error = strprintf(_("Invalid amount for -paytxfee=<amount>: '%s' (must be at least %s)"),


In commit "refactor: Move chain checks into CWallet::AttachChain" (eabcd95)

I think this commit is not an improvement and that it would be better to drop. Safer and clearer to check these values for errors early as they are initialized, than to initialize one place and do checking later.

Addressed in a general comment below

In commit "refactor: Move chain checks into CWallet::AttachChain" (88f1982)

I still think this commit is not an improvement and should be dropped because it makes the already complicated handling of these fee options and errors more complicated and less transparent. Instead of handling the settings in one place in Create, it splits up handling between two functions Create and AttachChain, when the code doesn't actually have anything to do with attaching to the chain. It is true that this code currently depends on some chain methods. But this should be fixed in the future by making the chain object less monolithic and by improving options handling in general.

I understand the motivation for this commit which is stated at #20773 (comment) and is basically to help implement the subsequent commit "wallet: make chain optional for CWallet::Create" (37b55f3).

But I don't think a good way to make the chain optional is to move these checks to an unrelated place where they don't belong. The simplest and clearest way to make the chain optional for now is simply to skip these checks if the chain pointer is null. And in the future, these checks can be updated to not use the chain interface at all.

Hm... thanks for mentioning that the dependency on the chain is accidental. I didn't look deep into it before.

I'm much more happy to drop that commit now, but should we explore other options as well?

It looks like we're just comparing one arg against other args. paytxfee and maxtxfee against minRelayFee, plus minRelayFee against some constant. There should be a better place for those checks rather than wallet creation code. I'm not familiar with the codebase enough to say where it belongs.

@ryanofsky and @fjahr could you help me locate a better place for those checks?

@ryanofsky and @fjahr could you help me locate a better place for those checks?

I think the checks are in a good place. Good to validate settings in the same place where they are parsed. There are just other ways this code could access ::minRelayTxFee value other than by calling a chain method. It could passed as a create option or be part of a different struct or interface. Simply changing the check to if (chain && walletInstance->m_pay_tx_fee < chain->relayMinFee()) seems good for now though.

And in the future, these checks can be updated to not use the chain interface at all.

For the chain topic this seems a good solution going forward.

Otherwise, I am not sure why you are interested in moving this code. These are checks that can make the wallet creation process fail or at least print a warning. So this seems to be a good place for it. Fee options are part of the wallet "world". Maybe the confusion is because these are config options and not args of createwallet? Or you don't see the direct connection between fee options and the wallet? These options are only relevant to the wallet and thus are only checked once the user wants to create a wallet because only then weird numbers there can be dangerous for the user.

On a smaller, organizational scale some of these checks could probably be helper functions and overall the fee part of wallet create can probably be encapsulated similar to AttachChain. If, however, you want to challenge that fee checks are done at all during wallet creation, that would require a lot more conceptual discussion. After that conceptual discussion, there could be a decision on where to put it. But the chances of a significant change in this regard are slim because this would be a huge behavior change I think, potentially breaking compatibility with any program that uses the wallet because these options will need to be set and checked in some kind of way.

Just some first thoughts by someone also not very familiar with the codebase :) If you have ideas or want to challenge this in general as I teased above I would recommend you to bring that as a topic to one of the next wallet IRC meetings, this Friday for example.

Maybe the confusion is because these are config options and not args of createwallet?

yes, and

and thus are only checked once the user wants to create a wallet.

Shouldn't we check them earlier? They are from the wallet "world" but have nothing to do with the wallet creation. Especially chain.relayMinFee().GetFeePerK() > HIGH_TX_FEE_PER_KB. It seems just a coincidence that the option parsing is done during wallet creation process.

Nevermind, I've dropped the commit. Thanks for your time and patience.

Shouldn't we check them earlier? They are from the wallet "world" but have nothing to do with the wallet creation.

Yes, these settings could be WalletContext members and shared across multiple CWallet instances instead of being CWallet members. (Or maybe if we want keep these as global settings but allow individual wallet overrides they could be members of both CWallet and WalletContext, with CWallet copying the WalletContext values on construction.) There's a general issue about wallet setting in #13044 for background & discussion about these things. And #19101 is another PR moving more variables to WalletContext.

My only reason for wanting to drop the commit is wanting to keep options parsing and option validation code together. I wouldn't want to move the checks without also moving the IsArgSet/GetArg code they are a part of. But all of this code could run before wallet creation if settings were stored outside CWallet in a place like WalletContext.

Especially chain.relayMinFee().GetFeePerK() > HIGH_TX_FEE_PER_KB. It seems just a coincidence that the option parsing is done during wallet creation process.

I'm not sure. This is also a warning passed back to the loadwallet and createwallet RPCs. It seems like a reasonable thing to warn user about when they are loading a wallet, but maybe it could happen at another time. It's more of a UX question than a code organization question, I think.

src/wallet/test/wallet_test_fixture.cpp

src/wallet/wallet.h

src/wallet/wallet.cpp

S3RK · 2021-01-14T09:52:36Z

@ryanofsky greatly appreciate your review.

It would be good to squash 5th commit into 1st commit. No need to add a method and enum in one commit and then change the signature and drop the enum right after.

I squashed handling first run and changing signature commits into adding AttachChain. There are less changing back and forth and overall it looks a bit simpler now.

I think 4th and 7th commits should probably be dropped, but see specific comments below.

I believe it's good to discuss a bit the design and future of CWallet before moving forward. I made those changes based on the following assumptions:
A) a wallet instance is not dependent on a chain object and could exist without a chain
B) a wallet instance could be later attached to a chain if required
Those assumption are based on my observation of the wallet tool which operates completely "offline". In my opinion this provides nice and clear wallet lifecycle and delineation of concerns. Thus we would remove chain argument from CWallet ctr and Create methods in the future. It would require changing how tx are loaded, but I think it's doable and would simplify the code.

I'd like to know what do you think.

ryanofsky

Code review ACK a481665. Overall is this is a very good change, but I'd still encourage dropping the 3th and 5th commits (or moving them to another PR where they can be better motivated). I think they complicate things unnecessarily and are based on an incorrect assumption.

6f8b907 refactor: Track first run state in CWallet (1/5)
2a9fbc4 refactor: Add CWallet:::AttachChain method (2/5)
a888db2 refactor: Move chain checks into CWallet::AttachChain (3/5)
2b77279 CWallet::Create move chain init message up into calling code (4/5)
a481665 CWallet::Create move CWallet::AttachChain up into calling code (5/5)

I believe it's good to discuss a bit the design and future of CWallet before moving forward. I made those changes based on the following assumptions:
A) a wallet instance is not dependent on a chain object and could exist without a chain
B) a wallet instance could be later attached to a chain if required
Those assumption are based on my observation of the wallet tool which operates completely "offline". In my opinion this provides nice and clear wallet lifecycle and delineation of concerns. Thus we would remove chain argument from CWallet ctr and Create methods in the future. It would require changing how tx are loaded, but I think it's doable and would simplify the code.

I'd like to know what do you think.

This is well put. Basically I just think the second assumption is wrong and complicates things unnecessarily. The only change we need to make to remove duplicate code in wallet tool is to tweak the wallet create function to take a null chain and stop requiring a non-null chain. There's no need to allow a different chain to be swapped in after the wallet is created.

Trying to separate creation from loading seems like all cost and no benefit. Callers have to do more work, settings parsing gets separated from settings validation, and the benefit is __? Maybe there could be a benefit in a future PR, but the 3rd and 5th commits seem a bit messy and out of place here to me. I don't think they cause much harm though, so if you really think they improve things then please do keep them! Thanks for working on this!

EDIT: Fixed references to 3rd and 5th commits instead of 4th and 5th commits

achow101 · 2021-01-15T20:31:05Z

ISTM there are 5 main components of CWallet::Create: Loading/creating the wallet file, initializing new wallets, processing all of the wallet CLI args, setting up the chain, and setting up wallet client interfaces.

What I had envisioned for splitting up CWallet::Create was to have separate functions for each of those components but still having CWallet::Create call all of them. This would allow for the typical use case to have a convenience function that does everything. But for the wallet tool where we don't need to do some of those parts, we could just call the underlying functions directly. For example, for bitcoin-wallet create, we would just call the load/create wallet file function, and then the setup new wallet function.

I agree with @ryanofsky that the assumption that changing the attached chain later is incorrect. I don't think it makes sense that the wallet would want to be able to change to a new chain after being initialized. However it does make sense that we would want to create a wallet with no chain for the offline mode stuff.

S3RK · 2021-01-18T08:51:24Z

Thanks ryanofsky and achow101, I have a better understanding now and adjusted my PR accordingly:

I removed the last commit and restored AttachChain call within CWallet::Create
I made chain arg of CWallet::Create optional to accommodate wallet tool use-case

I kept the 3rd commit which moves chain related checks to AttachChain. This way we can have all chain related code in AttachChain method in alignment with what achow101 said. As a benefit we need to check if chain is passed into CWallet::Create only once.

Thanks for your reviews, I'm more satisfied with how this PR looks right now.

ryanofsky

Code review ACK f71618c. Looks very good and thanks again! I still think commit 3 "refactor: Move chain checks into CWallet::AttachChain" (fcf56ee) is a little worse for code organization and error handling and would be better to drop, but it seems fine to keep if you just prefer it or think having fewer null chain checks justifies it.

Changes since last review: tweaking AttachChain call in second commit to fix interim compile error, replacing last commit with new commit making CWallet::Create chain argument optional & adding test case for this invocation.

Would encourage other reviewers to review this. No behavior is changing here, just a big clumsy Create function being broken up and made more sane.

This commit just moves code without making any changes. It can be reviewed with `git log -p -n1 --color-moved=dimmed_zebra` Motivation for this change is to make wallet.cpp/h less monolithic and start to make wallet transaction state tracking comprehensible so bugs in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking can be fixed safely without introducing new problems. This commit moves wallet classes and methods that deal with transactions out of wallet.cpp/.h into better organized files: - transaction.cpp/.h - CWalletTx and CMerkleTX class definitions - receive.cpp/.h - functions checking received transactions and computing balances - spend.cpp/.h - functions creating transactions and finding spendable coins After bitcoin#20773, when loading is separated from syncing it will also be possible to move more wallet.cpp/.h functions to: - sync.cpp/.h - functions handling chain notifications and rescanning This commit arranges receive.cpp and spend.cpp functions in dependency order so it's possible to skim receive.cpp and get an idea of how computing balances works, and skim spend.cpp and get an idea of how transactions are created, without having to jump all over wallet.cpp where functions are not in order and there is a lot of unrelated uncode. Followup commit "refactor: Detach wallet transaction methods" in pr/txstate follows up this PR and tweaks function names and arguments to reflect new locations. The two commits are split into separate PRs because this commit is more work to maintain and less work to review, while the other commit is less work to maintain and more work to review, so hopefully this commit can be merged earlier.

This commit just moves code without making any changes. It can be reviewed with `git log -p -n1 --color-moved=dimmed_zebra` Motivation for this change is to make wallet.cpp/h less monolithic and start to make wallet transaction state tracking comprehensible so bugs in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking can be fixed safely without introducing new problems. This commit moves wallet classes and methods that deal with transactions out of wallet.cpp/.h into better organized files: - transaction.cpp/.h - CWalletTx and CMerkleTX class definitions - receive.cpp/.h - functions checking received transactions and computing balances - spend.cpp/.h - functions creating transactions and finding spendable coins After bitcoin#20773, when loading is separated from syncing it will also be possible to move more wallet.cpp/.h functions to: - sync.cpp/.h - functions handling chain notifications and rescanning This commit arranges receive.cpp and spend.cpp functions in dependency order so it's possible to skim receive.cpp and get an idea of how computing balances works, and skim spend.cpp and get an idea of how transactions are created, without having to jump all over wallet.cpp where functions are not in order and there is a lot of unrelated uncode. Followup commit "refactor: Detach wallet transaction methods" in bitcoin#21206 follows up this PR and tweaks function names and arguments to reflect new locations. The two commits are split into separate PRs because this commit is more work to maintain and less work to review, while the other commit is less work to maintain and more work to review, so hopefully this commit can be merged earlier.

This commit just moves functions without making any changes. It can be reviewed with `git log -p -n1 --color-moved=dimmed_zebra` Motivation for this change is to make wallet.cpp/h less monolithic and start to make wallet transaction state tracking comprehensible so bugs in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking can be fixed safely without introducing new problems. This commit moves wallet classes and methods that deal with transactions out of wallet.cpp/.h into better organized files: - transaction.cpp/.h - CWalletTx and CMerkleTx class definitions - receive.cpp/.h - functions checking received transactions and computing balances - spend.cpp/.h - functions creating transactions and finding spendable coins After bitcoin#20773, when loading is separated from syncing it will also be possible to move more wallet.cpp/.h functions to: - sync.cpp/.h - functions handling chain notifications and rescanning This commit arranges receive.cpp and spend.cpp functions in dependency order so it's possible to skim receive.cpp and get an idea of how computing balances works, and skim spend.cpp and get an idea of how transactions are created, without having to jump all over wallet.cpp where functions are not in order and there is a lot of unrelated code. Followup commit "refactor: Detach wallet transaction methods" in bitcoin#21206 follows up this PR and tweaks function names and arguments to reflect new locations. The two commits are split into separate PRs because this commit is more work to maintain and less work to review, while the other commit is less work to maintain and more work to review, so hopefully this commit can be merged earlier.

S3RK · 2021-05-15T13:00:58Z

Reworked the first commit. It doesn't introduce m_frist_run, but rather just moves the first run detection closer to the point of use
Updated AttachChain signature. Made it private and changed the first param to const reference
Added a check that chain can't be changed.

fjahr

Code review ACK 37b55f3

I agree with @ryanofsky that the 3rd commit should be dropped but it's not a blocker.

src/wallet/wallet.cpp

achow101 · 2021-05-17T19:10:22Z

Code Review ACK 37b55f3

ryanofsky

Code review ACK 37b55f3. I still would really like to drop the third commit "refactor: Move chain checks into CWallet::AttachChain" 88f1982 (see reasoning and suggested alternative below), but I think this PR is great overall and all the other parts are a lot more significant than the one questionable commit.

Changes since last review: rebase due to conflicts, cleverly dropping m_first_run while still dropping first_run output arguments, adding assert to make sure AttachChain is called correctly, tweaking AttachChain signature

ryanofsky · 2021-05-18T13:55:32Z

src/wallet/wallet.cpp

+{
+    LOCK(walletInstance->cs_wallet);
+    // allow setting the chain if it hasn't been set already but prevent changing it
+    assert(!walletInstance->m_chain || walletInstance->m_chain == &chain);


In commit "refactor: Add CWallet:::AttachChain method" (491feef)

Not very important but I wonder if you could drop the || walletInstance->m_chain == &chain condition. It seems like it would be a bug to call attachchain more than once, even with the same chain pointer.

m_chain is initialized in the CWallet ctr. I've tried to drop it originally, but there were some issues, so I decided to leave it for the follow up.

ryanofsky · 2021-05-18T14:19:12Z

src/wallet/wallet.cpp

@@ -3955,11 +3955,6 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, const std::st
                               _("This is the transaction fee you will pay if you send a transaction."));
        }
        walletInstance->m_pay_tx_fee = CFeeRate(nFeePerK, 1000);
-        if (walletInstance->m_pay_tx_fee < chain.relayMinFee()) {
-            error = strprintf(_("Invalid amount for -paytxfee=<amount>: '%s' (must be at least %s)"),


In commit "refactor: Move chain checks into CWallet::AttachChain" (88f1982)

I still think this commit is not an improvement and should be dropped because it makes the already complicated handling of these fee options and errors more complicated and less transparent. Instead of handling the settings in one place in Create, it splits up handling between two functions Create and AttachChain, when the code doesn't actually have anything to do with attaching to the chain. It is true that this code currently depends on some chain methods. But this should be fixed in the future by making the chain object less monolithic and by improving options handling in general.

I understand the motivation for this commit which is stated at #20773 (comment) and is basically to help implement the subsequent commit "wallet: make chain optional for CWallet::Create" (37b55f3).

But I don't think a good way to make the chain optional is to move these checks to an unrelated place where they don't belong. The simplest and clearest way to make the chain optional for now is simply to skip these checks if the chain pointer is null. And in the future, these checks can be updated to not use the chain interface at all.

This commit does not change behavior, it just moves code from CWallet::CreateWalletFromFile to CWallet:::AttachChain so it can be updated in the next commit. This commit is most easily reviewed with "git diff -w --color-moved=dimmed_zebra" or by diffing CWallet:::AttachChain against the previous code with an external diff tool.

S3RK · 2021-05-19T06:54:49Z

I've dropped "refactor: Move chain checks into CWallet::AttachChain" (88f1982)

ryanofsky

Code review ACK 489ebb7. Only changes since last review were adding a const variable declaration, and implementing suggestion not to move feerate option checks to AttachChain. Thanks for updates and fast responses!

This commit just moves functions without making any changes. It can be reviewed with `git log -p -n1 --color-moved=dimmed_zebra` Motivation for this change is to make wallet.cpp/h less monolithic and start to make wallet transaction state tracking comprehensible so bugs in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking can be fixed safely without introducing new problems. This commit moves wallet classes and methods that deal with transactions out of wallet.cpp/.h into better organized files: - transaction.cpp/.h - CWalletTx and CMerkleTx class definitions - receive.cpp/.h - functions checking received transactions and computing balances - spend.cpp/.h - functions creating transactions and finding spendable coins After bitcoin#20773, when loading is separated from syncing it will also be possible to move more wallet.cpp/.h functions to: - sync.cpp/.h - functions handling chain notifications and rescanning This commit arranges receive.cpp and spend.cpp functions in dependency order so it's possible to skim receive.cpp and get an idea of how computing balances works, and skim spend.cpp and get an idea of how transactions are created, without having to jump all over wallet.cpp where functions are not in order and there is a lot of unrelated code. Followup commit "refactor: Detach wallet transaction methods" in bitcoin#21206 follows up this PR and tweaks function names and arguments to reflect new locations. The two commits are split into separate PRs because this commit is more work to maintain and less work to review, while the other commit is less work to maintain and more work to review, so hopefully this commit can be merged earlier.

c7bd584 MOVEONLY: CWallet transaction code out of wallet.cpp/.h (Russell Yanofsky) Pull request description: This commit just moves function without making any changes. It can be reviewed with `git log -p -n1 --color-moved=dimmed_zebra` Motivation for this change is to make `wallet.cpp/h` less monolithic and start to make wallet transaction state tracking comprehensible so bugs in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking can be fixed safely without introducing new problems. This moves wallet classes and methods that deal with transactions out of `wallet.cpp/.h` into better organized files: - `transaction.cpp/.h` - CWalletTx and CMerkleTx class definitions - `receive.cpp/.h` - functions checking received transactions and computing balances - `spend.cpp/.h` - functions creating transactions and finding spendable coins After #20773, when loading is separated from syncing it will also be possible to move more `wallet.cpp/.h` functions to: - `sync.cpp/.h` - functions handling chain notifications and rescanning This commit arranges `receive.cpp` and `spend.cpp` functions in dependency order so it's possible to skim `receive.cpp` and get an idea of how computing balances works, and skim `spend.cpp` and get an idea of how transactions are created, without having to jump all over `wallet.cpp` where functions are not in order and there is a lot of unrelated code. Followup commit "refactor: Detach wallet transaction methods" in #21206 follows up this PR and tweaks function names and arguments to reflect new locations. The two commits are split into separate PRs because this commit is more work to maintain and less work to review, while the other commit is less work to maintain and more work to review, so hopefully this commit can be merged earlier. ACKs for top commit: Sjors: re-utACK c7bd584 fjahr: utACK c7bd584 promag: Code review ACK c7bd584, verified move only claim. meshcollider: Dimmed-zebra-check and functional test run ACK c7bd584 Tree-SHA512: 4981de6911cb1196774db375494355cc9af59b52456129c002d264a77cd9ed6175f8ecbb6b2f492a59a4d5a0def21a39d96fa79c9f4d99be0992985f553be32f

This commit just moves functions without making any changes. It can be reviewed with `git log -p -n1 --color-moved=dimmed_zebra` Motivation for this change is to make wallet.cpp/h less monolithic and start to make wallet transaction state tracking comprehensible so bugs in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking can be fixed safely without introducing new problems. This commit moves wallet classes and methods that deal with transactions out of wallet.cpp/.h into better organized files: - transaction.cpp/.h - CWalletTx and CMerkleTx class definitions - receive.cpp/.h - functions checking received transactions and computing balances - spend.cpp/.h - functions creating transactions and finding spendable coins After bitcoin#20773, when loading is separated from syncing it will also be possible to move more wallet.cpp/.h functions to: - sync.cpp/.h - functions handling chain notifications and rescanning This commit arranges receive.cpp and spend.cpp functions in dependency order so it's possible to skim receive.cpp and get an idea of how computing balances works, and skim spend.cpp and get an idea of how transactions are created, without having to jump all over wallet.cpp where functions are not in order and there is a lot of unrelated code. Followup commit "refactor: Detach wallet transaction methods" in bitcoin#21206 follows up this PR and tweaks function names and arguments to reflect new locations. The two commits are split into separate PRs because this commit is more work to maintain and less work to review, while the other commit is less work to maintain and more work to review, so hopefully this commit can be merged earlier.

DrahtBot added GUI Refactoring Wallet labels Dec 26, 2020

This was referenced Dec 26, 2020

wallet, refactor: return out-params of CreateTransaction() as optional struct #20640

Merged

Remove names from translatable strings #20404

Closed

S3RK force-pushed the refactor_wallet_create branch 2 times, most recently from 78ad825 to 0f4d56d Compare January 1, 2021 12:04

S3RK force-pushed the refactor_wallet_create branch from 0f4d56d to 9b57578 Compare January 8, 2021 08:58

ryanofsky approved these changes Jan 12, 2021

View reviewed changes

S3RK force-pushed the refactor_wallet_create branch from 9b57578 to a481665 Compare January 14, 2021 09:37

ryanofsky approved these changes Jan 15, 2021

View reviewed changes

S3RK force-pushed the refactor_wallet_create branch from a481665 to f71618c Compare January 18, 2021 08:49

ryanofsky approved these changes Jan 20, 2021

View reviewed changes

This was referenced Jan 23, 2021

wallet: check for non-representable CFeeRates #20546

Closed

test: Move MakeNoLogFileContext to libtest_util, and use it in bench #21003

Merged

ryanofsky mentioned this pull request Feb 17, 2021

MOVEONLY: CWallet transaction code out of wallet.cpp/.h #21207

Merged

DrahtBot mentioned this pull request Feb 17, 2021

refactor: Make CWalletTx sync state type-safe #21206

Merged

DrahtBot added the Needs rebase label Mar 4, 2021

S3RK force-pushed the refactor_wallet_create branch from f71618c to 0d6f367 Compare March 8, 2021 08:10

DrahtBot removed the Needs rebase label Mar 8, 2021

DrahtBot mentioned this pull request Mar 8, 2021

rpc: include_unsafe option for fundrawtransaction #21359

Merged

S3RK force-pushed the refactor_wallet_create branch from 84980ae to 37b55f3 Compare May 15, 2021 12:56

DrahtBot removed the Needs rebase label May 15, 2021

This was referenced May 15, 2021

rpc, wallet: Expose wallet id in getwalletinfo RPC output #20243

Closed

wallet: Properly support a wallet id #20205

Closed

fjahr reviewed May 16, 2021

View reviewed changes

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

ryanofsky approved these changes May 18, 2021

View reviewed changes

S3RK and others added 4 commits May 19, 2021 08:50

refactor: move first run detection to client code

e2a47ce

CWallet::Create move chain init message up into calling code

d73ae93

wallet: make chain optional for CWallet::Create

489ebb7

S3RK force-pushed the refactor_wallet_create branch from 37b55f3 to 489ebb7 Compare May 19, 2021 06:50

ryanofsky approved these changes May 19, 2021

View reviewed changes

laanwj merged commit d4c409c into bitcoin:master May 19, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 19, 2021

Merge bitcoin#20773: refactor: split CWallet::Create

e3bbf04

laanwj mentioned this pull request May 27, 2021

English translations update bitcoin-core/gui#346

Merged

ryanofsky mentioned this pull request May 31, 2021

refactor: Clean up new wallet spend, receive files added #21207 #22100

Merged

ryanofsky mentioned this pull request Nov 24, 2021

wallet: Call load handlers without cs_wallet locked #22868

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge d4c409c into merged_master (Bitcoin PR bitcoin/bitcoin#20773)

e552043

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: split CWallet::Create #20773

refactor: split CWallet::Create #20773

S3RK commented Dec 26, 2020 •

edited

Loading

DrahtBot commented Dec 26, 2020 •

edited

Loading

ryanofsky left a comment

ryanofsky Jan 12, 2021

S3RK Jan 14, 2021

ryanofsky May 18, 2021 •

edited

Loading

S3RK May 18, 2021 •

edited

Loading

ryanofsky May 18, 2021

fjahr May 18, 2021 •

edited

Loading

S3RK May 19, 2021

ryanofsky May 19, 2021

S3RK commented Jan 14, 2021

ryanofsky left a comment •

edited

Loading

achow101 commented Jan 15, 2021

S3RK commented Jan 18, 2021

ryanofsky left a comment

S3RK commented May 15, 2021

fjahr left a comment

achow101 commented May 17, 2021

ryanofsky left a comment

ryanofsky May 18, 2021

S3RK May 18, 2021

ryanofsky May 18, 2021 •

edited

Loading

S3RK commented May 19, 2021

ryanofsky left a comment

refactor: split CWallet::Create #20773

refactor: split CWallet::Create #20773

Conversation

S3RK commented Dec 26, 2020 • edited Loading

DrahtBot commented Dec 26, 2020 • edited Loading

Conflicts

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Jan 12, 2021

Choose a reason for hiding this comment

S3RK Jan 14, 2021

Choose a reason for hiding this comment

ryanofsky May 18, 2021 • edited Loading

Choose a reason for hiding this comment

S3RK May 18, 2021 • edited Loading

Choose a reason for hiding this comment

ryanofsky May 18, 2021

Choose a reason for hiding this comment

fjahr May 18, 2021 • edited Loading

Choose a reason for hiding this comment

S3RK May 19, 2021

Choose a reason for hiding this comment

ryanofsky May 19, 2021

Choose a reason for hiding this comment

S3RK commented Jan 14, 2021

ryanofsky left a comment • edited Loading

Choose a reason for hiding this comment

achow101 commented Jan 15, 2021

S3RK commented Jan 18, 2021

ryanofsky left a comment

Choose a reason for hiding this comment

S3RK commented May 15, 2021

fjahr left a comment

Choose a reason for hiding this comment

achow101 commented May 17, 2021

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky May 18, 2021

Choose a reason for hiding this comment

S3RK May 18, 2021

Choose a reason for hiding this comment

ryanofsky May 18, 2021 • edited Loading

Choose a reason for hiding this comment

S3RK commented May 19, 2021

ryanofsky left a comment

Choose a reason for hiding this comment

S3RK commented Dec 26, 2020 •

edited

Loading

DrahtBot commented Dec 26, 2020 •

edited

Loading

ryanofsky May 18, 2021 •

edited

Loading

S3RK May 18, 2021 •

edited

Loading

fjahr May 18, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading

ryanofsky May 18, 2021 •

edited

Loading