Merge #16208: wallet: Consume ReserveDestination on successful CreateTransaction

e10e1e8db043e9b7c113e07faf408f337c1b732d Restrict lifetime of ReserveDestination to CWallet::CreateTransaction (Gregory Sanders)
d9ff862f2d24784ee081a8f62a76ffdfe409c10a 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 e10e1e8db043e9b7c113e07faf408f337c1b732d Reviewed the diff
  stevenroose:
    utACK e10e1e8db043e9b7c113e07faf408f337c1b732d
  meshcollider:
    utACK e10e1e8db043e9b7c113e07faf408f337c1b732d

Tree-SHA512: 78d047a00f39ab41cfa297052cc1e9c224d5f47d3d2299face650d71827635de077ac33fb4ab9f7dc6fc5a27f4a68415a1bc9ca33a3cb09a78f4f15b2a48411b

1 files changed, 3 insertions, 4 deletions

diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp
index 8982c70fd2..deb1618ceb 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_dest(&wallet) {}
+    explicit PendingWalletTxImpl(CWallet& wallet) : m_wallet(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_dest, state)) {
+        if (!m_wallet.CommitTransaction(m_tx, std::move(value_map), std::move(order_form), state)) {
             reject_reason = state.GetRejectReason();
             return false;
         }
@@ -56,7 +56,6 @@ public:
 
     CTransactionRef m_tx;
     CWallet& m_wallet;
-    ReserveDestination m_dest;
 };
 
 //! Construct wallet tx struct.
@@ -238,7 +237,7 @@ public:
         auto locked_chain = m_wallet->chain().lock();
         LOCK(m_wallet->cs_wallet);
         auto pending = MakeUnique<PendingWalletTxImpl>(*m_wallet);
-        if (!m_wallet->CreateTransaction(*locked_chain, recipients, pending->m_tx, pending->m_dest, fee, change_pos,
+        if (!m_wallet->CreateTransaction(*locked_chain, recipients, pending->m_tx, fee, change_pos,
                 fail_reason, coin_control, sign)) {
             return {};
         }