From 172213be5b174243dc501c1103ad5fe2fee67a16 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 18 Jun 2019 15:19:13 -0400 Subject: [PATCH 1/3] Add GetNewDestination to CWallet to fetch new destinations Instead of having the same multiple lines of code everywhere that new destinations are fetched, introduce GetNewDestination as a member function of CWallet which does the key fetching, label setting, script generation, and destination generation. --- src/interfaces/wallet.cpp | 6 ++++-- src/interfaces/wallet.h | 4 ++-- src/qt/addresstablemodel.cpp | 16 +++++++--------- src/qt/paymentserver.cpp | 8 +++----- src/test/util.cpp | 11 +++-------- src/wallet/rpcwallet.cpp | 16 ++++------------ src/wallet/test/wallet_tests.cpp | 5 +++-- src/wallet/wallet.cpp | 21 +++++++++++++++++++++ src/wallet/wallet.h | 6 +++++- 9 files changed, 52 insertions(+), 41 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 240670cbe..93374ea82 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -140,9 +140,11 @@ public: void abortRescan() override { m_wallet->AbortRescan(); } bool backupWallet(const std::string& filename) override { return m_wallet->BackupWallet(filename); } std::string getWalletName() override { return m_wallet->GetName(); } - bool getKeyFromPool(bool internal, CPubKey& pub_key) override + bool getNewDestination(const OutputType type, const std::string label, CTxDestination& dest) override { - return m_wallet->GetKeyFromPool(pub_key, internal); + LOCK(m_wallet->cs_wallet); + std::string error; + return m_wallet->GetNewDestination(type, label, dest, error); } bool getPubKey(const CKeyID& address, CPubKey& pub_key) override { return m_wallet->GetPubKey(address, pub_key); } bool getPrivKey(const CKeyID& address, CKey& key) override { return m_wallet->GetKey(address, key); } diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 7096f5404..25815d853 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -76,8 +76,8 @@ public: //! Get wallet name. virtual std::string getWalletName() = 0; - // Get key from pool. - virtual bool getKeyFromPool(bool internal, CPubKey& pub_key) = 0; + // Get a new address. + virtual bool getNewDestination(const OutputType type, const std::string label, CTxDestination& dest) = 0; //! Get public key. virtual bool getPubKey(const CKeyID& address, CPubKey& pub_key) = 0; diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index fa6c9c9f7..29423db3d 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -358,12 +358,15 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con return QString(); } } + + // Add entry + walletModel->wallet().setAddressBook(DecodeDestination(strAddress), strLabel, "send"); } else if(type == Receive) { // Generate a new address to associate with given label - CPubKey newKey; - if(!walletModel->wallet().getKeyFromPool(false /* internal */, newKey)) + CTxDestination dest; + if(!walletModel->wallet().getNewDestination(address_type, strLabel, dest)) { WalletModel::UnlockContext ctx(walletModel->requestUnlock()); if(!ctx.isValid()) @@ -372,23 +375,18 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con editStatus = WALLET_UNLOCK_FAILURE; return QString(); } - if(!walletModel->wallet().getKeyFromPool(false /* internal */, newKey)) + if(!walletModel->wallet().getNewDestination(address_type, strLabel, dest)) { editStatus = KEY_GENERATION_FAILURE; return QString(); } } - walletModel->wallet().learnRelatedScripts(newKey, address_type); - strAddress = EncodeDestination(GetDestinationForKey(newKey, address_type)); + strAddress = EncodeDestination(dest); } else { return QString(); } - - // Add entry - walletModel->wallet().setAddressBook(DecodeDestination(strAddress), strLabel, - (type == Send ? "send" : "receive")); return QString::fromStdString(strAddress); } diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp index 43dccec4e..345db7acd 100644 --- a/src/qt/paymentserver.cpp +++ b/src/qt/paymentserver.cpp @@ -666,16 +666,14 @@ void PaymentServer::fetchPaymentACK(WalletModel* walletModel, const SendCoinsRec payment.add_transactions(transaction.data(), transaction.size()); // Create a new refund address, or re-use: - CPubKey newKey; - if (walletModel->wallet().getKeyFromPool(false /* internal */, newKey)) { + CTxDestination dest; + const OutputType change_type = walletModel->wallet().getDefaultChangeType() != OutputType::CHANGE_AUTO ? walletModel->wallet().getDefaultChangeType() : walletModel->wallet().getDefaultAddressType(); + if (walletModel->wallet().getNewDestination(change_type, "", dest)) { // BIP70 requests encode the scriptPubKey directly, so we are not restricted to address // types supported by the receiver. As a result, we choose the address format we also // use for change. Despite an actual payment and not change, this is a close match: // it's the output type we use subject to privacy issues, but not restricted by what // other software supports. - const OutputType change_type = walletModel->wallet().getDefaultChangeType() != OutputType::CHANGE_AUTO ? walletModel->wallet().getDefaultChangeType() : walletModel->wallet().getDefaultAddressType(); - walletModel->wallet().learnRelatedScripts(newKey, change_type); - CTxDestination dest = GetDestinationForKey(newKey, change_type); std::string label = tr("Refund from %1").arg(recipient.authenticatedMerchant).toStdString(); walletModel->wallet().setAddressBook(dest, label, "refund"); diff --git a/src/test/util.cpp b/src/test/util.cpp index bc09d00b7..44afba1a0 100644 --- a/src/test/util.cpp +++ b/src/test/util.cpp @@ -25,14 +25,9 @@ const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqq std::string getnewaddress(CWallet& w) { constexpr auto output_type = OutputType::BECH32; - - CPubKey new_key; - if (!w.GetKeyFromPool(new_key)) assert(false); - - w.LearnRelatedScripts(new_key, output_type); - const auto dest = GetDestinationForKey(new_key, output_type); - - w.SetAddressBook(dest, /* label */ "", "receive"); + CTxDestination dest; + std::string error; + if (!w.GetNewDestination(output_type, "", dest, error)) assert(false); return EncodeDestination(dest); } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index f9baabeda..1801493f5 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -185,20 +185,12 @@ static UniValue getnewaddress(const JSONRPCRequest& request) } } - if (!pwallet->IsLocked()) { - pwallet->TopUpKeyPool(); + CTxDestination dest; + std::string error; + if (!pwallet->GetNewDestination(output_type, label, dest, error)) { + throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, error); } - // Generate a new key that is added to wallet - CPubKey newKey; - if (!pwallet->GetKeyFromPool(newKey)) { - throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first"); - } - pwallet->LearnRelatedScripts(newKey, output_type); - CTxDestination dest = GetDestinationForKey(newKey, output_type); - - pwallet->SetAddressBook(dest, label, "receive"); - return EncodeDestination(dest); } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 922bb0fe6..afbd8a5fb 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -466,8 +466,9 @@ BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) wallet->SetMinVersion(FEATURE_LATEST); wallet->SetWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS); BOOST_CHECK(!wallet->TopUpKeyPool(1000)); - CPubKey pubkey; - BOOST_CHECK(!wallet->GetKeyFromPool(pubkey, false)); + CTxDestination dest; + std::string error; + BOOST_CHECK(!wallet->GetNewDestination(OutputType::BECH32, "", dest, error)); } // Explicit calculation which is used to test the wallet constant diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index bde52d779..7a7ff2b6c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3513,6 +3513,27 @@ bool CWallet::GetKeyFromPool(CPubKey& result, bool internal) return true; } +bool CWallet::GetNewDestination(const OutputType type, const std::string label, CTxDestination& dest, std::string& error) +{ + LOCK(cs_wallet); + error.clear(); + if (!IsLocked()) { + TopUpKeyPool(); + } + + // Generate a new key that is added to wallet + CPubKey new_key; + if (!GetKeyFromPool(new_key)) { + error = "Error: Keypool ran out, please call keypoolrefill first"; + return false; + } + LearnRelatedScripts(new_key, type); + dest = GetDestinationForKey(new_key, type); + + SetAddressBook(dest, label, "receive"); + return true; +} + static int64_t GetOldestKeyTimeInPool(const std::set& setKeyPool, WalletBatch& batch) { if (setKeyPool.empty()) { return GetTime(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 87aff0903..09c2ce379 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -815,6 +815,9 @@ private: */ uint256 m_last_block_processed GUARDED_BY(cs_wallet); + //! Fetches a key from the keypool + bool GetKeyFromPool(CPubKey &key, bool internal = false); + public: /* * Main wallet lock. @@ -1110,7 +1113,6 @@ public: bool ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal); void KeepKey(int64_t nIndex); void ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey); - bool GetKeyFromPool(CPubKey &key, bool internal = false); int64_t GetOldestKeyPoolTime(); /** * Marks all keys in the keypool up to and including reserve_key as used. @@ -1123,6 +1125,8 @@ public: std::set GetLabelAddresses(const std::string& label) const; + bool GetNewDestination(const OutputType type, const std::string label, CTxDestination& dest, std::string& error); + isminetype IsMine(const CTxIn& txin) const; /** * Returns amount of debit if the input matches the From 33d13edd2bda0af90660e275ea4fa96ca9896f2a Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 18 Jun 2019 15:48:20 -0400 Subject: [PATCH 2/3] Replace CReserveKey with ReserveDestinatoin Instead of reserving keys, reserve destinations which are backed by keys --- src/interfaces/wallet.cpp | 8 ++--- src/wallet/feebumper.cpp | 10 +++--- src/wallet/rpcwallet.cpp | 24 ++++++------- src/wallet/test/wallet_tests.cpp | 6 ++-- src/wallet/wallet.cpp | 41 +++++++++++----------- src/wallet/wallet.h | 58 +++++++++++++++++--------------- 6 files changed, 73 insertions(+), 74 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 93374ea82..4a97ee0c2 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -36,7 +36,7 @@ namespace { class PendingWalletTxImpl : public PendingWalletTx { public: - explicit PendingWalletTxImpl(CWallet& wallet) : m_wallet(wallet), m_key(&wallet) {} + explicit PendingWalletTxImpl(CWallet& wallet) : m_wallet(wallet), m_dest(&wallet) {} const CTransaction& get() override { return *m_tx; } @@ -47,7 +47,7 @@ public: auto locked_chain = m_wallet.chain().lock(); LOCK(m_wallet.cs_wallet); CValidationState state; - if (!m_wallet.CommitTransaction(m_tx, std::move(value_map), std::move(order_form), m_key, state)) { + if (!m_wallet.CommitTransaction(m_tx, std::move(value_map), std::move(order_form), m_dest, state)) { reject_reason = state.GetRejectReason(); return false; } @@ -56,7 +56,7 @@ public: CTransactionRef m_tx; CWallet& m_wallet; - CReserveKey m_key; + ReserveDestination m_dest; }; //! Construct wallet tx struct. @@ -238,7 +238,7 @@ public: auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); auto pending = MakeUnique(*m_wallet); - if (!m_wallet->CreateTransaction(*locked_chain, recipients, pending->m_tx, pending->m_key, fee, change_pos, + if (!m_wallet->CreateTransaction(*locked_chain, recipients, pending->m_tx, pending->m_dest, fee, change_pos, fail_reason, coin_control, sign)) { return {}; } diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 46cf6b761..0d07ae860 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -272,17 +272,17 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo new_coin_control.m_min_depth = 1; CTransactionRef tx_new = MakeTransactionRef(); - CReserveKey reservekey(wallet); + ReserveDestination reservedest(wallet); CAmount fee_ret; int change_pos_in_out = -1; // No requested location for change std::string fail_reason; - if (!wallet->CreateTransaction(*locked_chain, recipients, tx_new, reservekey, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) { + if (!wallet->CreateTransaction(*locked_chain, recipients, tx_new, reservedest, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) { errors.push_back("Unable to create transaction: " + fail_reason); return Result::WALLET_ERROR; } // If change key hasn't been ReturnKey'ed by this point, we take it out of keypool - reservekey.KeepKey(); + reservedest.KeepDestination(); // Write back new fee if successful new_fee = fee_ret; @@ -330,9 +330,9 @@ Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransacti mapValue_t mapValue = oldWtx.mapValue; mapValue["replaces_txid"] = oldWtx.GetHash().ToString(); - CReserveKey reservekey(wallet); + ReserveDestination reservedest(wallet); CValidationState state; - if (!wallet->CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, reservekey, state)) { + if (!wallet->CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, reservedest, state)) { // NOTE: CommitTransaction never returns false, so this should never happen. errors.push_back(strprintf("The transaction was rejected: %s", FormatStateMessage(state))); return Result::WALLET_ERROR; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 1801493f5..40ad69f61 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -237,16 +237,12 @@ static UniValue getrawchangeaddress(const JSONRPCRequest& request) } } - CReserveKey reservekey(pwallet); - CPubKey vchPubKey; - if (!reservekey.GetReservedKey(vchPubKey, true)) + ReserveDestination reservedest(pwallet); + CTxDestination dest; + if (!reservedest.GetReservedDestination(output_type, dest, true)) throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first"); - reservekey.KeepKey(); - - pwallet->LearnRelatedScripts(vchPubKey, output_type); - CTxDestination dest = GetDestinationForKey(vchPubKey, output_type); - + reservedest.KeepDestination(); return EncodeDestination(dest); } @@ -313,7 +309,7 @@ static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet CScript scriptPubKey = GetScriptForDestination(address); // Create and send the transaction - CReserveKey reservekey(pwallet); + ReserveDestination reservedest(pwallet); CAmount nFeeRequired; std::string strError; std::vector vecSend; @@ -321,13 +317,13 @@ static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet CRecipient recipient = {scriptPubKey, nValue, fSubtractFeeFromAmount}; vecSend.push_back(recipient); CTransactionRef tx; - if (!pwallet->CreateTransaction(locked_chain, vecSend, tx, reservekey, nFeeRequired, nChangePosRet, strError, coin_control)) { + if (!pwallet->CreateTransaction(locked_chain, vecSend, tx, reservedest, nFeeRequired, nChangePosRet, strError, coin_control)) { if (!fSubtractFeeFromAmount && nValue + nFeeRequired > curBalance) strError = strprintf("Error: This transaction requires a transaction fee of at least %s", FormatMoney(nFeeRequired)); throw JSONRPCError(RPC_WALLET_ERROR, strError); } CValidationState state; - if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, reservekey, state)) { + if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, reservedest, state)) { strError = strprintf("Error: The transaction was rejected! Reason given: %s", FormatStateMessage(state)); throw JSONRPCError(RPC_WALLET_ERROR, strError); } @@ -921,16 +917,16 @@ static UniValue sendmany(const JSONRPCRequest& request) std::shuffle(vecSend.begin(), vecSend.end(), FastRandomContext()); // Send - CReserveKey keyChange(pwallet); + ReserveDestination changedest(pwallet); CAmount nFeeRequired = 0; int nChangePosRet = -1; std::string strFailReason; CTransactionRef tx; - bool fCreated = pwallet->CreateTransaction(*locked_chain, vecSend, tx, keyChange, nFeeRequired, nChangePosRet, strFailReason, coin_control); + bool fCreated = pwallet->CreateTransaction(*locked_chain, vecSend, tx, changedest, nFeeRequired, nChangePosRet, strFailReason, coin_control); if (!fCreated) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason); CValidationState state; - if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, keyChange, state)) { + if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, changedest, state)) { strFailReason = strprintf("Transaction commit failed:: %s", FormatStateMessage(state)); throw JSONRPCError(RPC_WALLET_ERROR, strFailReason); } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index afbd8a5fb..a72212679 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -363,17 +363,17 @@ public: CWalletTx& AddTx(CRecipient recipient) { CTransactionRef tx; - CReserveKey reservekey(wallet.get()); + ReserveDestination reservedest(wallet.get()); CAmount fee; int changePos = -1; std::string error; CCoinControl dummy; { auto locked_chain = m_chain->lock(); - BOOST_CHECK(wallet->CreateTransaction(*locked_chain, {recipient}, tx, reservekey, fee, changePos, error, dummy)); + BOOST_CHECK(wallet->CreateTransaction(*locked_chain, {recipient}, tx, reservedest, fee, changePos, error, dummy)); } CValidationState state; - BOOST_CHECK(wallet->CommitTransaction(tx, {}, {}, reservekey, state)); + BOOST_CHECK(wallet->CommitTransaction(tx, {}, {}, reservedest, state)); CMutableTransaction blocktx; { LOCK(wallet->cs_wallet); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7a7ff2b6c..4f632266b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2609,9 +2609,9 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC auto locked_chain = chain().lock(); LOCK(cs_wallet); - CReserveKey reservekey(this); + ReserveDestination reservedest(this); CTransactionRef tx_new; - if (!CreateTransaction(*locked_chain, vecSend, tx_new, reservekey, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) { + if (!CreateTransaction(*locked_chain, vecSend, tx_new, reservedest, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) { return false; } @@ -2619,7 +2619,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC tx.vout.insert(tx.vout.begin() + nChangePosInOut, tx_new->vout[nChangePosInOut]); // We don't have the normal Create/Commit cycle, and don't want to risk // reusing change, so just remove the key from the keypool here. - reservekey.KeepKey(); + reservedest.KeepDestination(); } // Copy output sizes from new transaction; they may have had the fee @@ -2730,7 +2730,7 @@ OutputType CWallet::TransactionChangeType(OutputType change_type, const std::vec return m_default_address_type; } -bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector& vecSend, CTransactionRef& tx, CReserveKey& reservekey, CAmount& nFeeRet, +bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector& vecSend, CTransactionRef& tx, ReserveDestination& reservedest, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign) { CAmount nValue = 0; @@ -2771,7 +2771,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy // Create change script that will be used if we need change - // TODO: pass in scriptChange instead of reservekey so + // TODO: pass in scriptChange instead of reservedest so // change transaction isn't always pay-to-bitcoin-address CScript scriptChange; @@ -2791,19 +2791,16 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std strFailReason = _("Can't generate a change-address key. No keys in the internal keypool and can't generate any keys."); return false; } - CPubKey vchPubKey; - bool ret; - ret = reservekey.GetReservedKey(vchPubKey, true); + 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); if (!ret) { - strFailReason = _("Keypool ran out, please call keypoolrefill first"); + strFailReason = "Keypool ran out, please call keypoolrefill first"; return false; } - const OutputType change_type = TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : m_default_change_type, vecSend); - - LearnRelatedScripts(vchPubKey, change_type); - scriptChange = GetScriptForDestination(GetDestinationForKey(vchPubKey, change_type)); + scriptChange = GetScriptForDestination(dest); } CTxOut change_prototype_txout(0, scriptChange); coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout); @@ -3024,7 +3021,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std } } - if (nChangePosInOut == -1) reservekey.ReturnKey(); // Return any reserved key if we don't have change + if (nChangePosInOut == -1) reservedest.ReturnDestination(); // Return any reserved address if we don't have change // Shuffle selected coins and fill in final vin txNew.vin.clear(); @@ -3097,7 +3094,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std /** * Call after CreateTransaction unless you want to abort */ -bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, CReserveKey& reservekey, CValidationState& state) +bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, ReserveDestination& reservedest, CValidationState& state) { { auto locked_chain = chain().lock(); @@ -3112,7 +3109,7 @@ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve WalletLogPrintf("CommitTransaction:\n%s", wtxNew.tx->ToString()); /* Continued */ { // Take key pair from key pool so it won't be used again - reservekey.KeepKey(); + reservedest.KeepDestination(); // Add tx to wallet, because if it has change it's also ours, // otherwise just for transaction history. @@ -3713,7 +3710,7 @@ std::set CWallet::GetLabelAddresses(const std::string& label) co return result; } -bool CReserveKey::GetReservedKey(CPubKey& pubkey, bool internal) +bool ReserveDestination::GetReservedDestination(const OutputType type, CTxDestination& dest, bool internal) { if (!pwallet->CanGetAddresses(internal)) { return false; @@ -3729,25 +3726,29 @@ bool CReserveKey::GetReservedKey(CPubKey& pubkey, bool internal) fInternal = keypool.fInternal; } assert(vchPubKey.IsValid()); - pubkey = vchPubKey; + pwallet->LearnRelatedScripts(vchPubKey, type); + address = GetDestinationForKey(vchPubKey, type); + dest = address; return true; } -void CReserveKey::KeepKey() +void ReserveDestination::KeepDestination() { if (nIndex != -1) pwallet->KeepKey(nIndex); nIndex = -1; vchPubKey = CPubKey(); + address = CNoDestination(); } -void CReserveKey::ReturnKey() +void ReserveDestination::ReturnDestination() { if (nIndex != -1) { pwallet->ReturnKey(nIndex, fInternal, vchPubKey); } nIndex = -1; vchPubKey = CPubKey(); + address = CNoDestination(); } void CWallet::MarkReserveKeysAsUsed(int64_t keypool_id) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 09c2ce379..f1a3c760a 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -85,11 +85,11 @@ static constexpr size_t DUMMY_NESTED_P2WPKH_INPUT_SIZE = 91; class CCoinControl; class COutput; -class CReserveKey; class CScript; class CWalletTx; struct FeeCalculation; enum class FeeEstimateMode; +class ReserveDestination; /** (client) version numbers for particular wallet features */ enum WalletFeature @@ -234,55 +234,57 @@ public: } }; -/** A wrapper to reserve a key from a wallet keypool +/** A wrapper to reserve an address from a wallet * - * CReserveKey is used to reserve a key from the keypool. It is passed around + * ReserveDestination is used to reserve an address. It is passed around * during the CreateTransaction/CommitTransaction procedure. * - * Instantiating a CReserveKey does not reserve a keypool key. To do so, - * GetReservedKey() needs to be called on the object. Once a key has been - * reserved, call KeepKey() on the CReserveKey object to make sure it is not - * returned to the keypool. Call ReturnKey() to return the key to the keypool - * so it can be re-used (for example, if the key was used in a new transaction + * Instantiating a ReserveDestination does not reserve an address. To do so, + * GetReservedDestination() needs to be called on the object. Once an address has been + * reserved, call KeepDestination() on the ReserveDestination object to make sure it is not + * returned. Call ReturnDestination() to return the address so it can be re-used (for + * example, if the address was used in a new transaction * and that transaction was not completed and needed to be aborted). * - * If a key is reserved and KeepKey() is not called, then the key will be - * returned to the keypool when the CReserveObject goes out of scope. + * If an address is reserved and KeepDestination() is not called, then the address will be + * returned when the ReserveDestination goes out of scope. */ -class CReserveKey +class ReserveDestination { protected: - //! The wallet to reserve the keypool key from + //! The wallet to reserve from CWallet* pwallet; - //! The index of the key in the keypool + //! The index of the address's key in the keypool int64_t nIndex{-1}; - //! The public key + //! The public key for the address CPubKey vchPubKey; + //! The destination + CTxDestination address; //! Whether this is from the internal (change output) keypool bool fInternal{false}; public: - //! Construct a CReserveKey object. This does NOT reserve a key from the keypool yet - explicit CReserveKey(CWallet* pwalletIn) + //! Construct a ReserveDestination object. This does NOT reserve an address yet + explicit ReserveDestination(CWallet* pwalletIn) { pwallet = pwalletIn; } - CReserveKey(const CReserveKey&) = delete; - CReserveKey& operator=(const CReserveKey&) = delete; + ReserveDestination(const ReserveDestination&) = delete; + ReserveDestination& operator=(const ReserveDestination&) = delete; //! Destructor. If a key has been reserved and not KeepKey'ed, it will be returned to the keypool - ~CReserveKey() + ~ReserveDestination() { - ReturnKey(); + ReturnDestination(); } - //! Reserve a key from the keypool - bool GetReservedKey(CPubKey &pubkey, bool internal = false); - //! Return a key to the keypool - void ReturnKey(); - //! Keep the key. Do not return it to the keypool when this object goes out of scope - void KeepKey(); + //! Reserve an address + bool GetReservedDestination(const OutputType type, 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 + void KeepDestination(); }; /** Address book data */ @@ -1056,9 +1058,9 @@ public: * selected by SelectCoins(); Also create the change output, when needed * @note passing nChangePosInOut as -1 will result in setting a random position */ - bool CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector& vecSend, CTransactionRef& tx, CReserveKey& reservekey, CAmount& nFeeRet, int& nChangePosInOut, + bool CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector& vecSend, CTransactionRef& tx, ReserveDestination& reservedest, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign = true); - bool CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, CReserveKey& reservekey, CValidationState& state); + bool CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, ReserveDestination& reservedest, CValidationState& state); bool DummySignTx(CMutableTransaction &txNew, const std::set &txouts, bool use_max_sig = false) const { From 8e7f930828a9f8f9be1c90ff45e3fdfef1980eaf Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 18 Jun 2019 15:49:02 -0400 Subject: [PATCH 3/3] Add GetNewChangeDestination for getting new change Destinations Adds a GetNewChangeDestination that has the same objective as GetNewDestination --- src/wallet/rpcwallet.cpp | 13 ++++--------- src/wallet/wallet.cpp | 17 +++++++++++++++++ src/wallet/wallet.h | 1 + 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 40ad69f61..23900cbd6 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -226,10 +226,6 @@ static UniValue getrawchangeaddress(const JSONRPCRequest& request) throw JSONRPCError(RPC_WALLET_ERROR, "Error: This wallet has no available keys"); } - if (!pwallet->IsLocked()) { - pwallet->TopUpKeyPool(); - } - OutputType output_type = pwallet->m_default_change_type != OutputType::CHANGE_AUTO ? pwallet->m_default_change_type : pwallet->m_default_address_type; if (!request.params[0].isNull()) { if (!ParseOutputType(request.params[0].get_str(), output_type)) { @@ -237,12 +233,11 @@ static UniValue getrawchangeaddress(const JSONRPCRequest& request) } } - ReserveDestination reservedest(pwallet); CTxDestination dest; - if (!reservedest.GetReservedDestination(output_type, dest, true)) - throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first"); - - reservedest.KeepDestination(); + std::string error; + if (!pwallet->GetNewChangeDestination(output_type, dest, error)) { + throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, error); + } return EncodeDestination(dest); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4f632266b..108f4d679 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3531,6 +3531,23 @@ bool CWallet::GetNewDestination(const OutputType type, const std::string label, return true; } +bool CWallet::GetNewChangeDestination(const OutputType type, CTxDestination& dest, std::string& error) +{ + error.clear(); + if (!IsLocked()) { + TopUpKeyPool(); + } + + ReserveDestination reservedest(this); + if (!reservedest.GetReservedDestination(type, dest, true)) { + error = "Error: Keypool ran out, please call keypoolrefill first"; + return false; + } + + reservedest.KeepDestination(); + return true; +} + static int64_t GetOldestKeyTimeInPool(const std::set& setKeyPool, WalletBatch& batch) { if (setKeyPool.empty()) { return GetTime(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index f1a3c760a..8a0665b19 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1128,6 +1128,7 @@ public: std::set GetLabelAddresses(const std::string& label) const; bool GetNewDestination(const OutputType type, const std::string label, CTxDestination& dest, std::string& error); + bool GetNewChangeDestination(const OutputType type, CTxDestination& dest, std::string& error); isminetype IsMine(const CTxIn& txin) const; /**