highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-04-20 11:49:07 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge #16208: wallet: Consume ReserveDestination on successful CreateTransaction

Browse Source 
e10e1e8db0 Restrict lifetime of ReserveDestination to CWallet::CreateTransaction (Gregory Sanders)
d9ff862f2d CreateTransaction calls KeepDestination on ReserveDestination before success (Gregory Sanders)

Pull request description:

  The typical usage pattern of `ReserveDestination` is to explicitly `KeepDestination`, or `ReturnDestination` when it's detected it will not be used.

  Implementers such as myself may fail to complete this pattern, and could result in key re-use: https://github.com/bitcoin/bitcoin/pull/15557#discussion_r271956393

  Since ReserveDestination is currently only used directly in the `CreateTransaction`/`CommitTransaction` flow(or fee bumping where it's just used in `CreateTransaction`), I instead make the assumption that if a transaction is returned by `CreateTransaction` it's highly likely that it will be accepted by the caller, and the `ReserveDestination` kept. This simplifies the API as well. There are very few cases where this would not be the case which may result in keys being burned.

  Those failure cases appear to be:
  `CommitTransaction` failing to get the transaction into the mempool
  Belt and suspenders check in `WalletModel::prepareTransaction`

  Alternative to https://github.com/bitcoin/bitcoin/pull/15796

ACKs for top commit:
  achow101:
    ACK e10e1e8db0 Reviewed the diff
  stevenroose:
    utACK e10e1e8db0
  meshcollider:
    utACK e10e1e8db0

Tree-SHA512: 78d047a00f39ab41cfa297052cc1e9c224d5f47d3d2299face650d71827635de077ac33fb4ab9f7dc6fc5a27f4a68415a1bc9ca33a3cb09a78f4f15b2a48411b
This commit is contained in:
MeshCollider
2019-07-17 19:45:14 +12:00
parent 24dbcf3808 e10e1e8db0
commit 459baa1756
6 changed files with 23 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
src/wallet/wallet.cpp
View File

@@ -2779,17 +2779,13 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
auto locked_chain = chain().lock();
LOCK(cs_wallet);
ReserveDestination reservedest(this);
CTransactionRef tx_new;
if (!CreateTransaction(*locked_chain, vecSend, tx_new, reservedest, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) {
if (!CreateTransaction(*locked_chain, vecSend, tx_new, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) {
return false;
}
if (nChangePosInOut != -1) {
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.
reservedest.KeepDestination();
}
// Copy output sizes from new transaction; they may have had the fee
@@ -2900,10 +2896,11 @@ 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<CRecipient>& vecSend, CTransactionRef& tx, ReserveDestination& reservedest, CAmount& nFeeRet,
bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet,
int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign)
{
CAmount nValue = 0;
ReserveDestination reservedest(this);
int nChangePosRequest = nChangePosInOut;
unsigned int nSubtractFeeFromAmount = 0;
for (const auto& recipient : vecSend)
@@ -3183,8 +3180,6 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
}
}
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();
std::vector<CInputCoin> selected_coins(setCoins.begin(), setCoins.end());
@@ -3247,6 +3242,10 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
}
}
// Before we return success, we assume any change key will be used to prevent
// accidental re-use.
reservedest.KeepDestination();
WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u Needed:%d Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",
nFeeRet, nBytes, nFeeNeeded, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay,
feeCalc.est.pass.start, feeCalc.est.pass.end,
@@ -3261,7 +3260,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<std::pair<std::string, std::string>> orderForm, ReserveDestination& reservedest, CValidationState& state)
bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, CValidationState& state)
{
{
auto locked_chain = chain().lock();
@@ -3275,8 +3274,6 @@ 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
reservedest.KeepDestination();
// Add tx to wallet, because if it has change it's also ours,
// otherwise just for transaction history.