This PR doesn't change behaviour, it's purely a refactor.
All CWalletTx methods that use the wallet pointer are moved to CWallet and then the wallet pointer is removed from CWalletTx.
This should also ease adding thread safety analysis annotations like #11634.
@practicalswift you may find this useful for #11634.
concept ACK!
496 | @@ -497,6 +497,15 @@ std::set<uint256> CWallet::GetConflicts(const uint256& txid) const 497 | return result; 498 | } 499 | 500 | +std::set<uint256> CWallet::GetConflicts(const CWalletTx& wtx) const 501 | +{ 502 | + std::set<uint256> result; 503 | + uint256 hash = wtx.GetHash(); 504 | + result = GetConflicts(hash); 505 | + result.erase(hash); // why?
Why would GetConflicts(hash) include the hash?
Concept ACK
Thanks!
4313 | @@ -4314,3 +4314,11 @@ CTxDestination CWallet::AddAndGetDestinationForScript(const CScript& script, Out 4314 | default: assert(false); 4315 | } 4316 | } 4317 | + 4318 | +COutput CWallet::MakeOutput(const CWalletTx& wtx, int index, int depth, bool is_spendable, bool is_solvable, bool is_safe) const
In commit "refactor: Add COutput factory CWallet::MakeOutput"
This should just be a constructor. Make functions like this only make sense when they are needed for template deduction.
Made it like this because of GetSpendSize(), otherwise I have to pass the wallet instance to the constructor.
496 | @@ -497,6 +497,15 @@ std::set<uint256> CWallet::GetConflicts(const uint256& txid) const 497 | return result; 498 | } 499 | 500 | +std::set<uint256> CWallet::GetConflicts(const CWalletTx& wtx) const
In commit "Move CWalletTx::GetConflicts to CWallet"
Would be better to inline this the one place where it is called, because having two GetConflicts overloads where one overload includes the original transaction and one overloads excludes it is pretty confusing.
Agree, and I thought doing that, but decided to just move the code until someone also suggested it.
343 | @@ -347,14 +344,13 @@ class CWalletTx : public CMerkleTx 344 | mutable CAmount nAvailableWatchCreditCached; 345 | mutable CAmount nChangeCached; 346 | 347 | - CWalletTx(const CWallet* pwalletIn, CTransactionRef arg) : CMerkleTx(std::move(arg)) 348 | + CWalletTx(CTransactionRef arg) : CMerkleTx(std::move(arg))
In commit "refactor: Remove CWallet pointer from CWalletTx
Think this constructor should be marked explicit now to prevent implicit conversion.
Sure.
343 | @@ -347,14 +344,13 @@ class CWalletTx : public CMerkleTx 344 | mutable CAmount nAvailableWatchCreditCached; 345 | mutable CAmount nChangeCached; 346 | 347 | - CWalletTx(const CWallet* pwalletIn, CTransactionRef arg) : CMerkleTx(std::move(arg)) 348 | + CWalletTx(CTransactionRef arg) : CMerkleTx(std::move(arg)) 349 | { 350 | - Init(pwalletIn); 351 | + Init();
In commit "refactor: Remove CWallet pointer from CWalletTx
It would be really nice to drop these init methods (which are only called from constructors). Also would be nice to initialize members where they are defined https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures and maybe drop the code inside the init methods too.
which are only called from constructors
It's also called from Unserialize(), although I can check if it's necessary.
Edit: IMO should be done in a follow up PR?
4306 | @@ -4307,6 +4307,6 @@ COutput CWallet::MakeOutput(const CWalletTx& wtx, int index, int depth, bool is_ 4307 | { 4308 | // If known and signable by the given wallet, compute input bytes 4309 | // Failure will keep this value -1 4310 | - int input_bytes = is_spendable ? wtx.GetSpendSize(index) : -1; 4311 | + int input_bytes = is_spendable ? GetSpendSize(wtx, index) : -1;
In commit "refactor: Move CWalletTx::GetSpendSize to CWallet"
Maybe inline GetSpendSize since it is only called this one place and is one line long. (Would be good to keep the "Get the marginal bytes..." comment though.)
utACK b2f5def07f910d8ee80a0a1afcfce92989c32727
Code seems correct, and I think it's good to get rid of the CWallet* member inside CWalletTx but I think it is bad to blindly move all the CWalletTx member functions to CWallet. It seems especially awkward to now have CWallet methods that take const CWalletTx& references and then change their mutable members.
Moving all these methods also makes this PR much bigger than it needs to be. It seems like it would be better to just leave most of these where they are and just pass in new pwallet arguments where needed.
Thanks @ryanofsky for the review!
It seems especially awkward to now have CWallet methods that take const CWalletTx& references and then change their mutable members.
For instance, should this
CAmount CWallet::GetDebit(const CTxIn &txin, const isminefilter& filter) const;
CAmount CWallet::GetDebit(const CWalletTx& wtx, const isminefilter& filter) const;
be this (without const CWalletTx)?
CAmount CWallet::GetDebit(const CTxIn &txin, const isminefilter& filter) const;
CAmount CWallet::GetDebit(CWalletTx& wtx, const isminefilter& filter) const;
IMO it's no worse than the original code (const method):
CAmount CWalletTx::GetDebit(const isminefilter& filter) const;
Hence the mutable members.
and just pass in new pwallet arguments where needed.
Started doing that but sounded very lame, for instance (feels wrong doesn't it):
wtx.GetDebit(pwallet, filter);
Also, I'd have to check if that approach helps in #11634.
Moving all these methods also makes this PR much bigger than it needs to be
I've moved the code in wallet.cpp up and down and made some fixes to the code convention. I can revert those and keep the code in the same place, without reorganising it, the diff will be pretty smaller. WDYT?
@ryanofsky updated b2f5def...721f499.
This needs a rebase.
4294 | @@ -4314,3 +4295,11 @@ CTxDestination CWallet::AddAndGetDestinationForScript(const CScript& script, Out 4295 | default: assert(false); 4296 | } 4297 | } 4298 | + 4299 | +COutput CWallet::MakeOutput(const CWalletTx& wtx, int index, int depth, bool is_spendable, bool is_solvable, bool is_safe) const 4300 | +{ 4301 | + // If known and signable by the given wallet, compute input bytes 4302 | + // Failure will keep this value -1 4303 | + int input_bytes = is_spendable ? GetSpendSize(wtx, index) : -1;
How about dropping GetSpendSize in favor of CalculateMaximumSignedInputSize? This is the only call. Previously GetSpendSize had fewer args but now they're basically equivalent. Or dropping CalculateMaximumSignedInputSize for some version of GetSpendSize, given the former is only called in CWallet contexts after this change. Could also wait for later. :P
utACK 721f499
@Empact @ryanofsky if you don't mind I'd prefer to keep this as "move only" as possible. The idea here is to remove wallet pointer from CWalletTx. I can follow up those suggestions though.
@ryanofsky if you don't mind I'd prefer to keep this as "move only" as possible
I'm actually still not sure what was wrong with my suggestion to pass wallet references where needed and not move code around out all all: #12929#pullrequestreview-111389750. In your last comment you only wrote that this "sounded very lame" and "feels wrong," which is fine, though I didn't understand the objection.
CWallet is a already huge, monolithic class so your current change which removes all CWalletTx methods and moves them to CWallet doesn't really make it much worse. It just also seems perfectly logical to me that methods accessing wallet transactions would be part of the CWalletTx instead of CWallet.
Anyway, I did ACK previously, and can rereview if needed for merge.
Between wtx.GetDebit(pwallet, filter) or pwallet->GetDebit(wtx, filter) I prefer the second because the call is made to the "parent" asking for "child" details. But that is my preference and your suggestion patch is for sure smaller.
I was thinking removing all cache members from CWalletTx and have the wallet manage that cache as a whole and I think this change would simplify that.
Anyway, this started because of #11634, and I'm going to rebase that to see if #11634 (review) is resolved.
Concept ACK, though I prefer @ryanofsky's suggestion to just pass wallet references where needed.
Closing due to refactoring guidelines.