Skip to content

Commit 33d13ed

Browse files
achow101achow101
committed
Replace CReserveKey with ReserveDestinatoin
Instead of reserving keys, reserve destinations which are backed by keys
1 parent 172213b commit 33d13ed

File tree

6 files changed

+73
-74
lines changed

6 files changed

+73
-74
lines changed

‎src/interfaces/wallet.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ namespace {
3636
class PendingWalletTxImpl : public PendingWalletTx
3737
{
3838
public:
39-
explicit PendingWalletTxImpl(CWallet& wallet) : m_wallet(wallet), m_key(&wallet) {}
39+
explicit PendingWalletTxImpl(CWallet& wallet) : m_wallet(wallet), m_dest(&wallet) {}
4040

4141
const CTransaction& get() override { return *m_tx; }
4242

@@ -47,7 +47,7 @@ class PendingWalletTxImpl : public PendingWalletTx
4747
auto locked_chain = m_wallet.chain().lock();
4848
LOCK(m_wallet.cs_wallet);
4949
CValidationState state;
50-
if (!m_wallet.CommitTransaction(m_tx, std::move(value_map), std::move(order_form), m_key, state)) {
50+
if (!m_wallet.CommitTransaction(m_tx, std::move(value_map), std::move(order_form), m_dest, state)) {
5151
reject_reason = state.GetRejectReason();
5252
return false;
5353
}
@@ -56,7 +56,7 @@ class PendingWalletTxImpl : public PendingWalletTx
5656

5757
CTransactionRef m_tx;
5858
CWallet& m_wallet;
59-
CReserveKey m_key;
59+
ReserveDestination m_dest;
6060
};
6161

6262
//! Construct wallet tx struct.
@@ -238,7 +238,7 @@ class WalletImpl : public Wallet
238238
auto locked_chain = m_wallet->chain().lock();
239239
LOCK(m_wallet->cs_wallet);
240240
auto pending = MakeUnique<PendingWalletTxImpl>(*m_wallet);
241-
if (!m_wallet->CreateTransaction(*locked_chain, recipients, pending->m_tx, pending->m_key, fee, change_pos,
241+
if (!m_wallet->CreateTransaction(*locked_chain, recipients, pending->m_tx, pending->m_dest, fee, change_pos,
242242
fail_reason, coin_control, sign)) {
243243
return {};
244244
}

‎src/wallet/feebumper.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -272,17 +272,17 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo
272272
new_coin_control.m_min_depth = 1;
273273

274274
CTransactionRef tx_new = MakeTransactionRef();
275-
CReserveKey reservekey(wallet);
275+
ReserveDestination reservedest(wallet);
276276
CAmount fee_ret;
277277
int change_pos_in_out = -1; // No requested location for change
278278
std::string fail_reason;
279-
if (!wallet->CreateTransaction(*locked_chain, recipients, tx_new, reservekey, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) {
279+
if (!wallet->CreateTransaction(*locked_chain, recipients, tx_new, reservedest, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) {
280280
errors.push_back("Unable to create transaction: " + fail_reason);
281281
return Result::WALLET_ERROR;
282282
}
283283

284284
// If change key hasn't been ReturnKey'ed by this point, we take it out of keypool
285-
reservekey.KeepKey();
285+
reservedest.KeepDestination();
286286

287287
// Write back new fee if successful
288288
new_fee = fee_ret;
@@ -330,9 +330,9 @@ Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransacti
330330
mapValue_t mapValue = oldWtx.mapValue;
331331
mapValue["replaces_txid"] = oldWtx.GetHash().ToString();
332332

333-
CReserveKey reservekey(wallet);
333+
ReserveDestination reservedest(wallet);
334334
CValidationState state;
335-
if (!wallet->CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, reservekey, state)) {
335+
if (!wallet->CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, reservedest, state)) {
336336
// NOTE: CommitTransaction never returns false, so this should never happen.
337337
errors.push_back(strprintf("The transaction was rejected: %s", FormatStateMessage(state)));
338338
return Result::WALLET_ERROR;

‎src/wallet/rpcwallet.cpp

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -237,16 +237,12 @@ static UniValue getrawchangeaddress(const JSONRPCRequest& request)
237237
}
238238
}
239239

240-
CReserveKey reservekey(pwallet);
241-
CPubKey vchPubKey;
242-
if (!reservekey.GetReservedKey(vchPubKey, true))
240+
ReserveDestination reservedest(pwallet);
241+
CTxDestination dest;
242+
if (!reservedest.GetReservedDestination(output_type, dest, true))
243243
throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first");
244244

245-
reservekey.KeepKey();
246-
247-
pwallet->LearnRelatedScripts(vchPubKey, output_type);
248-
CTxDestination dest = GetDestinationForKey(vchPubKey, output_type);
249-
245+
reservedest.KeepDestination();
250246
return EncodeDestination(dest);
251247
}
252248

@@ -313,21 +309,21 @@ static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet
313309
CScript scriptPubKey = GetScriptForDestination(address);
314310

315311
// Create and send the transaction
316-
CReserveKey reservekey(pwallet);
312+
ReserveDestination reservedest(pwallet);
317313
CAmount nFeeRequired;
318314
std::string strError;
319315
std::vector<CRecipient> vecSend;
320316
int nChangePosRet = -1;
321317
CRecipient recipient = {scriptPubKey, nValue, fSubtractFeeFromAmount};
322318
vecSend.push_back(recipient);
323319
CTransactionRef tx;
324-
if (!pwallet->CreateTransaction(locked_chain, vecSend, tx, reservekey, nFeeRequired, nChangePosRet, strError, coin_control)) {
320+
if (!pwallet->CreateTransaction(locked_chain, vecSend, tx, reservedest, nFeeRequired, nChangePosRet, strError, coin_control)) {
325321
if (!fSubtractFeeFromAmount && nValue + nFeeRequired > curBalance)
326322
strError = strprintf("Error: This transaction requires a transaction fee of at least %s", FormatMoney(nFeeRequired));
327323
throw JSONRPCError(RPC_WALLET_ERROR, strError);
328324
}
329325
CValidationState state;
330-
if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, reservekey, state)) {
326+
if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, reservedest, state)) {
331327
strError = strprintf("Error: The transaction was rejected! Reason given: %s", FormatStateMessage(state));
332328
throw JSONRPCError(RPC_WALLET_ERROR, strError);
333329
}
@@ -921,16 +917,16 @@ static UniValue sendmany(const JSONRPCRequest& request)
921917
std::shuffle(vecSend.begin(), vecSend.end(), FastRandomContext());
922918

923919
// Send
924-
CReserveKey keyChange(pwallet);
920+
ReserveDestination changedest(pwallet);
925921
CAmount nFeeRequired = 0;
926922
int nChangePosRet = -1;
927923
std::string strFailReason;
928924
CTransactionRef tx;
929-
bool fCreated = pwallet->CreateTransaction(*locked_chain, vecSend, tx, keyChange, nFeeRequired, nChangePosRet, strFailReason, coin_control);
925+
bool fCreated = pwallet->CreateTransaction(*locked_chain, vecSend, tx, changedest, nFeeRequired, nChangePosRet, strFailReason, coin_control);
930926
if (!fCreated)
931927
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason);
932928
CValidationState state;
933-
if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, keyChange, state)) {
929+
if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, changedest, state)) {
934930
strFailReason = strprintf("Transaction commit failed:: %s", FormatStateMessage(state));
935931
throw JSONRPCError(RPC_WALLET_ERROR, strFailReason);
936932
}

‎src/wallet/test/wallet_tests.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -363,17 +363,17 @@ class ListCoinsTestingSetup : public TestChain100Setup
363363
CWalletTx& AddTx(CRecipient recipient)
364364
{
365365
CTransactionRef tx;
366-
CReserveKey reservekey(wallet.get());
366+
ReserveDestination reservedest(wallet.get());
367367
CAmount fee;
368368
int changePos = -1;
369369
std::string error;
370370
CCoinControl dummy;
371371
{
372372
auto locked_chain = m_chain->lock();
373-
BOOST_CHECK(wallet->CreateTransaction(*locked_chain, {recipient}, tx, reservekey, fee, changePos, error, dummy));
373+
BOOST_CHECK(wallet->CreateTransaction(*locked_chain, {recipient}, tx, reservedest, fee, changePos, error, dummy));
374374
}
375375
CValidationState state;
376-
BOOST_CHECK(wallet->CommitTransaction(tx, {}, {}, reservekey, state));
376+
BOOST_CHECK(wallet->CommitTransaction(tx, {}, {}, reservedest, state));
377377
CMutableTransaction blocktx;
378378
{
379379
LOCK(wallet->cs_wallet);

‎src/wallet/wallet.cpp

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2609,17 +2609,17 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
26092609
auto locked_chain = chain().lock();
26102610
LOCK(cs_wallet);
26112611

2612-
CReserveKey reservekey(this);
2612+
ReserveDestination reservedest(this);
26132613
CTransactionRef tx_new;
2614-
if (!CreateTransaction(*locked_chain, vecSend, tx_new, reservekey, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) {
2614+
if (!CreateTransaction(*locked_chain, vecSend, tx_new, reservedest, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) {
26152615
return false;
26162616
}
26172617

26182618
if (nChangePosInOut != -1) {
26192619
tx.vout.insert(tx.vout.begin() + nChangePosInOut, tx_new->vout[nChangePosInOut]);
26202620
// We don't have the normal Create/Commit cycle, and don't want to risk
26212621
// reusing change, so just remove the key from the keypool here.
2622-
reservekey.KeepKey();
2622+
reservedest.KeepDestination();
26232623
}
26242624

26252625
// 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
27302730
return m_default_address_type;
27312731
}
27322732

2733-
bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CReserveKey& reservekey, CAmount& nFeeRet,
2733+
bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, ReserveDestination& reservedest, CAmount& nFeeRet,
27342734
int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign)
27352735
{
27362736
CAmount nValue = 0;
@@ -2771,7 +2771,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
27712771
CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy
27722772

27732773
// Create change script that will be used if we need change
2774-
// TODO: pass in scriptChange instead of reservekey so
2774+
// TODO: pass in scriptChange instead of reservedest so
27752775
// change transaction isn't always pay-to-bitcoin-address
27762776
CScript scriptChange;
27772777

@@ -2791,19 +2791,16 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
27912791
strFailReason = _("Can't generate a change-address key. No keys in the internal keypool and can't generate any keys.");
27922792
return false;
27932793
}
2794-
CPubKey vchPubKey;
2795-
bool ret;
2796-
ret = reservekey.GetReservedKey(vchPubKey, true);
2794+
CTxDestination dest;
2795+
const OutputType change_type = TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : m_default_change_type, vecSend);
2796+
bool ret = reservedest.GetReservedDestination(change_type, dest, true);
27972797
if (!ret)
27982798
{
2799-
strFailReason = _("Keypool ran out, please call keypoolrefill first");
2799+
strFailReason = "Keypool ran out, please call keypoolrefill first";
28002800
return false;
28012801
}
28022802

2803-
const OutputType change_type = TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : m_default_change_type, vecSend);
2804-
2805-
LearnRelatedScripts(vchPubKey, change_type);
2806-
scriptChange = GetScriptForDestination(GetDestinationForKey(vchPubKey, change_type));
2803+
scriptChange = GetScriptForDestination(dest);
28072804
}
28082805
CTxOut change_prototype_txout(0, scriptChange);
28092806
coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout);
@@ -3024,7 +3021,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
30243021
}
30253022
}
30263023

3027-
if (nChangePosInOut == -1) reservekey.ReturnKey(); // Return any reserved key if we don't have change
3024+
if (nChangePosInOut == -1) reservedest.ReturnDestination(); // Return any reserved address if we don't have change
30283025

30293026
// Shuffle selected coins and fill in final vin
30303027
txNew.vin.clear();
@@ -3097,7 +3094,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
30973094
/**
30983095
* Call after CreateTransaction unless you want to abort
30993096
*/
3100-
bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, CReserveKey& reservekey, CValidationState& state)
3097+
bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, ReserveDestination& reservedest, CValidationState& state)
31013098
{
31023099
{
31033100
auto locked_chain = chain().lock();
@@ -3112,7 +3109,7 @@ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
31123109
WalletLogPrintf("CommitTransaction:\n%s", wtxNew.tx->ToString()); /* Continued */
31133110
{
31143111
// Take key pair from key pool so it won't be used again
3115-
reservekey.KeepKey();
3112+
reservedest.KeepDestination();
31163113

31173114
// Add tx to wallet, because if it has change it's also ours,
31183115
// otherwise just for transaction history.
@@ -3713,7 +3710,7 @@ std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) co
37133710
return result;
37143711
}
37153712

3716-
bool CReserveKey::GetReservedKey(CPubKey& pubkey, bool internal)
3713+
bool ReserveDestination::GetReservedDestination(const OutputType type, CTxDestination& dest, bool internal)
37173714
{
37183715
if (!pwallet->CanGetAddresses(internal)) {
37193716
return false;
@@ -3729,25 +3726,29 @@ bool CReserveKey::GetReservedKey(CPubKey& pubkey, bool internal)
37293726
fInternal = keypool.fInternal;
37303727
}
37313728
assert(vchPubKey.IsValid());
3732-
pubkey = vchPubKey;
3729+
pwallet->LearnRelatedScripts(vchPubKey, type);
3730+
address = GetDestinationForKey(vchPubKey, type);
3731+
dest = address;
37333732
return true;
37343733
}
37353734

3736-
void CReserveKey::KeepKey()
3735+
void ReserveDestination::KeepDestination()
37373736
{
37383737
if (nIndex != -1)
37393738
pwallet->KeepKey(nIndex);
37403739
nIndex = -1;
37413740
vchPubKey = CPubKey();
3741+
address = CNoDestination();
37423742
}
37433743

3744-
void CReserveKey::ReturnKey()
3744+
void ReserveDestination::ReturnDestination()
37453745
{
37463746
if (nIndex != -1) {
37473747
pwallet->ReturnKey(nIndex, fInternal, vchPubKey);
37483748
}
37493749
nIndex = -1;
37503750
vchPubKey = CPubKey();
3751+
address = CNoDestination();
37513752
}
37523753

37533754
void CWallet::MarkReserveKeysAsUsed(int64_t keypool_id)

‎src/wallet/wallet.h

Lines changed: 30 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -85,11 +85,11 @@ static constexpr size_t DUMMY_NESTED_P2WPKH_INPUT_SIZE = 91;
8585

8686
class CCoinControl;
8787
class COutput;
88-
class CReserveKey;
8988
class CScript;
9089
class CWalletTx;
9190
struct FeeCalculation;
9291
enum class FeeEstimateMode;
92+
class ReserveDestination;
9393

9494
/** (client) version numbers for particular wallet features */
9595
enum WalletFeature
@@ -234,55 +234,57 @@ class CKeyPool
234234
}
235235
};
236236

237-
/** A wrapper to reserve a key from a wallet keypool
237+
/** A wrapper to reserve an address from a wallet
238238
*
239-
* CReserveKey is used to reserve a key from the keypool. It is passed around
239+
* ReserveDestination is used to reserve an address. It is passed around
240240
* during the CreateTransaction/CommitTransaction procedure.
241241
*
242-
* Instantiating a CReserveKey does not reserve a keypool key. To do so,
243-
* GetReservedKey() needs to be called on the object. Once a key has been
244-
* reserved, call KeepKey() on the CReserveKey object to make sure it is not
245-
* returned to the keypool. Call ReturnKey() to return the key to the keypool
246-
* so it can be re-used (for example, if the key was used in a new transaction
242+
* Instantiating a ReserveDestination does not reserve an address. To do so,
243+
* GetReservedDestination() needs to be called on the object. Once an address has been
244+
* reserved, call KeepDestination() on the ReserveDestination object to make sure it is not
245+
* returned. Call ReturnDestination() to return the address so it can be re-used (for
246+
* example, if the address was used in a new transaction
247247
* and that transaction was not completed and needed to be aborted).
248248
*
249-
* If a key is reserved and KeepKey() is not called, then the key will be
250-
* returned to the keypool when the CReserveObject goes out of scope.
249+
* If an address is reserved and KeepDestination() is not called, then the address will be
250+
* returned when the ReserveDestination goes out of scope.
251251
*/
252-
class CReserveKey
252+
class ReserveDestination
253253
{
254254
protected:
255-
//! The wallet to reserve the keypool key from
255+
//! The wallet to reserve from
256256
CWallet* pwallet;
257-
//! The index of the key in the keypool
257+
//! The index of the address's key in the keypool
258258
int64_t nIndex{-1};
259-
//! The public key
259+
//! The public key for the address
260260
CPubKey vchPubKey;
261+
//! The destination
262+
CTxDestination address;
261263
//! Whether this is from the internal (change output) keypool
262264
bool fInternal{false};
263265

264266
public:
265-
//! Construct a CReserveKey object. This does NOT reserve a key from the keypool yet
266-
explicit CReserveKey(CWallet* pwalletIn)
267+
//! Construct a ReserveDestination object. This does NOT reserve an address yet
268+
explicit ReserveDestination(CWallet* pwalletIn)
267269
{
268270
pwallet = pwalletIn;
269271
}
270272

271-
CReserveKey(const CReserveKey&) = delete;
272-
CReserveKey& operator=(const CReserveKey&) = delete;
273+
ReserveDestination(const ReserveDestination&) = delete;
274+
ReserveDestination& operator=(const ReserveDestination&) = delete;
273275

274276
//! Destructor. If a key has been reserved and not KeepKey'ed, it will be returned to the keypool
275-
~CReserveKey()
277+
~ReserveDestination()
276278
{
277-
ReturnKey();
279+
ReturnDestination();
278280
}
279281

280-
//! Reserve a key from the keypool
281-
bool GetReservedKey(CPubKey &pubkey, bool internal = false);
282-
//! Return a key to the keypool
283-
void ReturnKey();
284-
//! Keep the key. Do not return it to the keypool when this object goes out of scope
285-
void KeepKey();
282+
//! Reserve an address
283+
bool GetReservedDestination(const OutputType type, CTxDestination& pubkey, bool internal);
284+
//! Return reserved address
285+
void ReturnDestination();
286+
//! Keep the address. Do not return it's key to the keypool when this object goes out of scope
287+
void KeepDestination();
286288
};
287289

288290
/** Address book data */
@@ -1056,9 +1058,9 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific
10561058
* selected by SelectCoins(); Also create the change output, when needed
10571059
* @note passing nChangePosInOut as -1 will result in setting a random position
10581060
*/
1059-
bool CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CReserveKey& reservekey, CAmount& nFeeRet, int& nChangePosInOut,
1061+
bool CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, ReserveDestination& reservedest, CAmount& nFeeRet, int& nChangePosInOut,
10601062
std::string& strFailReason, const CCoinControl& coin_control, bool sign = true);
1061-
bool CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, CReserveKey& reservekey, CValidationState& state);
1063+
bool CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, ReserveDestination& reservedest, CValidationState& state);
10621064

10631065
bool DummySignTx(CMutableTransaction &txNew, const std::set<CTxOut> &txouts, bool use_max_sig = false) const
10641066
{

0 commit comments

Comments
 (0)