This is a followup for #20365 (review)
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 be164f9cf89b123f03b926aa980996919924ee64 from #15719 (thanks ryanofsky)
cc ryanofsky achow101
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
719@@ -720,6 +720,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
720 // ScriptPubKeyMan::GetID. In many cases it will be the hash of an internal structure
721 std::map<uint256, std::unique_ptr<ScriptPubKeyMan>> m_spk_managers;
722
723+ /** True if this wallet is loaded for the first time. */
724+ bool m_first_run;
In commit “refactor: Track first run state in CWallet” (b528485dc7701dcd8fafcf2da996160c52546f07)
Could this be initialized with bool m_first_run = true or bool m_first_run = false here? It would be nice to avoid possibility of non-determinism since it is technically possible to create a CWallet object without loading it
false so it wouldn’t call chainStateFlushed by default. I think this is a safer option.
3954@@ -3955,11 +3955,6 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, const std::st
3955 _("This is the transaction fee you will pay if you send a transaction."));
3956 }
3957 walletInstance->m_pay_tx_fee = CFeeRate(nFeePerK, 1000);
3958- if (walletInstance->m_pay_tx_fee < chain.relayMinFee()) {
3959- error = strprintf(_("Invalid amount for -paytxfee=<amount>: '%s' (must be at least %s)"),
In commit “refactor: Move chain checks into CWallet::AttachChain” (eabcd952d1c664cd964eb3376bc2865a49b33f16)
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.
In commit “refactor: Move chain checks into CWallet::AttachChain” (88f19822bd14d23e2e0f4ebc7e7221f029d0abad)
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” (37b55f38d31c05f1ae6e22537eea8707463168a6).
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.
11 {
12 m_wallet.LoadWallet();
13- CWallet::AttachChain({ &m_wallet, [](CWallet*) {} });
14+ bilingual_str error;
15+ std::vector<bilingual_str> warnings;
16+ CWalle::AttachChain({&m_wallet, [](CWallet*) {}}, *m_node.chain.get(), error, warnings);
In commit “refactor: Update CWallet::AttachChain signature” (847588a4b8e0dcc9975a675c46eca095c7cce4eb)
Compile error this line (should say CWallet)
790@@ -791,15 +791,12 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
791 /** Registered interfaces::Chain::Notifications handler. */
792 std::unique_ptr<interfaces::Handler> m_chain_notifications_handler;
793
794- /** Result of scanning a chain for new transactions */
795- enum class ScanStatus { SUCCESS, FAILED, MISSING_BLOCKS, SKIPPED };
In commit “refactor: Update CWallet::AttachChain signature” (847588a4b8e0dcc9975a675c46eca095c7cce4eb)
This change seems good but it would be nice to squash this commit into the first commit 4398236619aa1f70886a0e9ae7093b413ec172c6 to make the PR simpler and avoid defining an enum in an an earlier change that gets deleted later
3989@@ -3980,10 +3990,6 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, const std::st
3990
3991 LOCK(walletInstance->cs_wallet);
3992
3993- if (!walletInstance->AttachChain(chain, error, warnings)) {
In commit “CWallet::Create move CWallet::AttachChain up into calling code” (9b575783f0f075d66350def60a842aee9ee05267)
I don’t think it’s safe to reorder loading sequence and call AttachChain after g_load_wallet_fns callbacks. It also adds extra code to give callers responsibility to call AttachChain. If the point is to allow wallets to be created without syncing, would suggest just changing Create methods’s Chain& argument to Chain* and if (!walletInstance->AttachChain(chain)) to if (chain && !walletInstance->AttachChain(*chain))
Thanks for bringing it up. I looked at g_load_wallet_fns and there are two use-cases for it both in UI:
I have almost zero understanding of GUI code and QT, but it looks like a safe change to me. Can we ask someone with better understanding of GUI to comment on that?
Also see my general comment below regarding the responsibility to call AttachChain.
4033@@ -4036,6 +4034,10 @@ CWallet::ScanStatus CWallet::AttachChain(std::shared_ptr<CWallet> wallet, bool s
4034 auto& walletInstance = wallet;
4035 LOCK(walletInstance->cs_wallet);
4036
4037+ if (walletInstance->m_first_run) {
In commit “refactor: Handle first run in CWallet::AttachChain” (b20c1e516d5bfc87e8d75a34799a91d71475ee8b)
I think it’s good that this PR adds m_first_run, to be able to freely move code around without changing behavior. In the future, though I think it would be good to eliminate m_first_run. This chainStateFlushed call seems to be the main place it is used, and it would seem better if this code updated the best block in a smarter way less awkwardly tied to the presence of keys and descriptors.
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:
@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.
Code review ACK a481665d5d90aa43f34434396eea174de5277df3. 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.
I believe it’s good to discuss a bit the design and future of
CWalletbefore 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 thewallet toolwhich operates completely “offline”. In my opinion this provides nice and clear wallet lifecycle and delineation of concerns. Thus we would removechainargument fromCWalletctr andCreatemethods 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
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.
Thanks ryanofsky and achow101, I have a better understanding now and adjusted my PR accordingly:
AttachChain call within CWallet::Createchain arg of CWallet::Create optional to accommodate wallet tool use-caseI 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.
Code review ACK f71618c2a18afea6dfe52dfd5282f82f05b93686. Looks very good and thanks again! I still think commit 3 “refactor: Move chain checks into CWallet::AttachChain” (fcf56ee612ae2ff5422d9a2bd801e6af22f4ab2c) 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.
Yep I will review this sometime in the next couple of days 👍
Concept ACK
src/wallet/test/wallet_tests.cpp
Code review ACK 84980ae4deb828f0675478fad2f7e0b6dec69520
Overall very nice change set, thanks S3RK
3876@@ -3879,7 +3877,7 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, const std::st
3877 }
3878 }
3879
3880- if (fFirstRun)
3881+ if (walletInstance->m_first_run)
Question: How does one distinguish whether accessing a wallet field (in this case walletInstance->m_first_run) requires cs_wallet lock or not? I’m asking because cs_wallet is described like this:
0/*
1 * Main wallet lock.
2 * This lock protects all the fields added by CWallet. */
3mutable RecursiveMutex cs_wallet;
Can anybody shed light on this for my information?
Question: How does one distinguish whether accessing a wallet field (in this case
walletInstance->m_first_run) requirescs_walletlock or not?
People may have different ideas about what “requires” means, but I would say a variable is required to be locked if not locking it would cause bugs. I.e. if it can be read and written at the same time from multiple threads. So a lock is not required for this because parts of wallet loading code which use this are single threaded.
But I do think it would be a small improvement to write bool m_first_run GUARDED_BY(cs_wallet) = false; and lock during init to avoid questions and doubts about races during loading.
People may have different ideas about what “requires” means, but I would say a variable is required to be locked if not locking it would cause bugs. I.e. if it can be read and written at the same time from multiple threads. So a lock is not required for this because parts of wallet loading code which use this are single threaded.
Thank you for the explanation.
But I do think it would be a small improvement to write
bool m_first_run GUARDED_BY(cs_wallet) = false;and lock during init to avoid questions and doubts about races during loading.
This makes a lot of sense to me because it’s much easier to follow comments specifying a class design rather than read all code that uses given class (possibly many many places).
m_first_run altogether
While I think this is a good improvement, there are a few changes I would like to see before ACK’ing this.
We don’t need m_first_run. The only reason it needs to be in AttachChain is to allow for new wallets to be initialized with a best block that is the current tip to avoid unnecessary rescanning. But because we shouldn’t be using AttachChain outside of Create, I think it would be better to have first_run parameter on AttachChain rather than CWallet::m_first_run.
Even so, I’m not sure if it really makes sense to have AttachChain be setting that for newly created wallets. Instead, because Create has a chain optionally, we can just check if the chain exists and do the walletInstance->chainStateFlushed if it does.
AttachChain should be a private function since I don’t think we want to have callers to be able to change the chain after the wallet has been created.
Also, needs rebase.
Agree with Achow’s suggestions. But if the suggestions turn out to be hard to implement, I hope we consider them optional. The big improvement I see in this PR is that that it unscrambles wallet loading code from wallet syncing code. This is important so syncing code can be moved to a higher-level, so wallet code can be less monolithic and so multiple wallets can be synced in a single scan during loading.
If having a first run member or a having public attach method helps keep this PR a plain refactor instead of introducing new logic, or helps keep this PR small, while still separating loading from syncing, this doesn’t seem like a bad tradeoff. But it’s pretty likely no tradeoff is necessary, so this does seem worth looking into.
m_frist_run, but rather just moves the first run detection closer to the point of useAttachChain signature. Made it private and changed the first param to const reference3919@@ -3924,6 +3920,10 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, const std::st
3920 }
3921 }
3922
3923+ // This wallet is in its first run if there are no ScriptPubKeyMans and it isn't blank or no privkeys
3924+ bool fFirstRun = walletInstance->m_spk_managers.empty() &&
const
Code review ACK 37b55f38d31c05f1ae6e22537eea8707463168a6
I agree with @ryanofsky that the 3rd commit should be dropped but it’s not a blocker.
4113+
4114+bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interfaces::Chain& chain, bilingual_str& error, std::vector<bilingual_str>& warnings)
4115+{
4116+ LOCK(walletInstance->cs_wallet);
4117+ // allow setting the chain if it hasn't been set already but prevent changing it
4118+ assert(!walletInstance->m_chain || walletInstance->m_chain == &chain);
In commit “refactor: Add CWallet:::AttachChain method” (491feef0e97adf4ec5ca1b17a0c097a4f7324a9e)
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.
Code review ACK 37b55f38d31c05f1ae6e22537eea8707463168a6. I still would really like to drop the third commit “refactor: Move chain checks into CWallet::AttachChain” 88f19822bd14d23e2e0f4ebc7e7221f029d0abad (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
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.