Merge #17237: wallet: LearnRelatedScripts only if KeepDestination

3958295bc8 wallet: LearnRelatedScripts only if KeepDestination (João Barbosa)
55295fba4c wallet: Lock address type in ReserveDestination (João Barbosa)

Pull request description:

  Only mutates the wallet if the reserved key is kept.

  First commit is a refactor that makes the address type a class member.

  The second commit moves `LearnRelatedScripts` from `GetReservedDestination` to `KeepDestination` to avoid an unnecessary call to `AddCScript` - which in turn prevents multiple entries of the same script in the wallet DB.

ACKs for top commit:
  achow101:
    Re-ACK 3958295bc8
  Sjors:
    ACK 3958295bc8
  ryanofsky:
    Code review ACK 3958295bc8. I like this change. The new behavior makes more sense, and the change makes the code clearer, since the current LearnRelatedScripts call is hard to understand and explain. (Personally, I'd like it if this PR were merged before #17373 or that PR was rebased on top of this one so it would be less confusing.)
  meshcollider:
    utACK 3958295bc8

Tree-SHA512: 49a5f4b022b28042ad37ea309b28378a3983cb904e234a25795b5a360356652e0f8e60f15e3e64d85094ea63af9be01812d90ccfc08ca4f1dd927fdd8566e33f
This commit is contained in:
Samuel Dobson 2019-11-23 09:26:13 +13:00
commit 7127c31020
No known key found for this signature in database
GPG key ID: D300116E1C875A3D
2 changed files with 15 additions and 14 deletions

View file

@ -2544,7 +2544,8 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign)
{
CAmount nValue = 0;
ReserveDestination reservedest(this);
const OutputType change_type = TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : m_default_change_type, vecSend);
ReserveDestination reservedest(this, change_type);
int nChangePosRequest = nChangePosInOut;
unsigned int nSubtractFeeFromAmount = 0;
for (const auto& recipient : vecSend)
@ -2603,8 +2604,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
return false;
}
CTxDestination dest;
const OutputType change_type = TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : m_default_change_type, vecSend);
bool ret = reservedest.GetReservedDestination(change_type, dest, true);
bool ret = reservedest.GetReservedDestination(dest, true);
if (!ret)
{
strFailReason = "Keypool ran out, please call keypoolrefill first";
@ -3128,8 +3128,8 @@ bool CWallet::GetNewChangeDestination(const OutputType type, CTxDestination& des
m_spk_man->TopUp();
ReserveDestination reservedest(this);
if (!reservedest.GetReservedDestination(type, dest, true)) {
ReserveDestination reservedest(this, type);
if (!reservedest.GetReservedDestination(dest, true)) {
error = "Error: Keypool ran out, please call keypoolrefill first";
return false;
}
@ -3295,7 +3295,7 @@ std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) co
return result;
}
bool ReserveDestination::GetReservedDestination(const OutputType type, CTxDestination& dest, bool internal)
bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool internal)
{
m_spk_man = pwallet->GetLegacyScriptPubKeyMan();
if (!m_spk_man) {
@ -3316,7 +3316,6 @@ bool ReserveDestination::GetReservedDestination(const OutputType type, CTxDestin
fInternal = keypool.fInternal;
}
assert(vchPubKey.IsValid());
m_spk_man->LearnRelatedScripts(vchPubKey, type);
address = GetDestinationForKey(vchPubKey, type);
dest = address;
return true;
@ -3324,8 +3323,10 @@ bool ReserveDestination::GetReservedDestination(const OutputType type, CTxDestin
void ReserveDestination::KeepDestination()
{
if (nIndex != -1)
if (nIndex != -1) {
m_spk_man->KeepDestination(nIndex);
m_spk_man->LearnRelatedScripts(vchPubKey, type);
}
nIndex = -1;
vchPubKey = CPubKey();
address = CNoDestination();

View file

@ -140,8 +140,9 @@ class ReserveDestination
{
protected:
//! The wallet to reserve from
CWallet* pwallet;
CWallet* const pwallet;
LegacyScriptPubKeyMan* m_spk_man{nullptr};
OutputType const type;
//! The index of the address's key in the keypool
int64_t nIndex{-1};
//! The public key for the address
@ -153,10 +154,9 @@ protected:
public:
//! Construct a ReserveDestination object. This does NOT reserve an address yet
explicit ReserveDestination(CWallet* pwalletIn)
{
pwallet = pwalletIn;
}
explicit ReserveDestination(CWallet* pwallet, OutputType type)
: pwallet(pwallet)
, type(type) { }
ReserveDestination(const ReserveDestination&) = delete;
ReserveDestination& operator=(const ReserveDestination&) = delete;
@ -168,7 +168,7 @@ public:
}
//! Reserve an address
bool GetReservedDestination(const OutputType type, CTxDestination& pubkey, bool internal);
bool GetReservedDestination(CTxDestination& pubkey, bool internal);
//! Return reserved address
void ReturnDestination();
//! Keep the address. Do not return it's key to the keypool when this object goes out of scope