Merge bitcoin/bitcoin#22155: wallet test: Add test for subtract fee from recipient behavior

fe6dc76b7c9c5405f37464a3b19fcf82aaf22861 wallet test: Add test for subtract fee from recipient behavior (Russell Yanofsky)
2565478c813fb7278153b113de4b9338fc186872 wallet test refactor: add CreateSyncedWallet function (Russell Yanofsky)

Pull request description:

  This adds test coverage for wallet subtract from recipient behavior without changing it. Behavior seems to have changed recently in a minor way in #17331 without being noticed.

ACKs for top commit:
  achow101:
    ACK fe6dc76b7c9c5405f37464a3b19fcf82aaf22861
  glozow:
    ACK fe6dc76b7c9c5405f37464a3b19fcf82aaf22861
  promag:
    Code review ACK fe6dc76b7c9c5405f37464a3b19fcf82aaf22861.

Tree-SHA512: e00c5dfe467e4ccef5edb0dd4fff6c53f35a37828a4327bea2e166751e5ef971d519ffca7b8f735b12912bb4a547980626356bc1855981005aed1a6c2a57be0b

6 files changed, 124 insertions, 14 deletions

diff --git a/build_msvc/test_bitcoin/test_bitcoin.vcxproj b/build_msvc/test_bitcoin/test_bitcoin.vcxproj
index 5c4b777d51..bb1a780bfa 100644
--- a/build_msvc/test_bitcoin/test_bitcoin.vcxproj
+++ b/build_msvc/test_bitcoin/test_bitcoin.vcxproj
@@ -16,6 +16,7 @@
     <ClCompile Include="..\..\src\test\util\*.cpp" />
     <ClCompile Include="..\..\src\wallet\test\*_fixture.cpp" />
     <ClCompile Include="..\..\src\wallet\test\*_tests.cpp" />
+    <ClCompile Include="..\..\src\wallet\test\util.cpp" />
   </ItemGroup>
   <ItemGroup>
     <ProjectReference Include="..\libbitcoinconsensus\libbitcoinconsensus.vcxproj">
diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index fc2fd80166..a07a1bb002 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -152,6 +152,7 @@ BITCOIN_TESTS =\
 if ENABLE_WALLET
 BITCOIN_TESTS += \
   wallet/test/psbt_wallet_tests.cpp \
+  wallet/test/spend_tests.cpp \
   wallet/test/wallet_tests.cpp \
   wallet/test/walletdb_tests.cpp \
   wallet/test/wallet_crypto_tests.cpp \
@@ -170,6 +171,8 @@ endif
 
 
 BITCOIN_TEST_SUITE += \
+  wallet/test/util.cpp \
+  wallet/test/util.h \
   wallet/test/wallet_test_fixture.cpp \
   wallet/test/wallet_test_fixture.h \
   wallet/test/init_test_fixture.cpp \
diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp
new file mode 100644
index 0000000000..66e7de4273
--- /dev/null
+++ b/src/wallet/test/spend_tests.cpp
@@ -0,0 +1,61 @@
+// Copyright (c) 2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <policy/fees.h>
+#include <validation.h>
+#include <wallet/coincontrol.h>
+#include <wallet/test/util.h>
+#include <wallet/test/wallet_test_fixture.h>
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_FIXTURE_TEST_SUITE(spend_tests, WalletTestingSetup)
+
+BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
+{
+    CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
+    auto wallet = CreateSyncedWallet(*m_node.chain, m_node.chainman->ActiveChain(), coinbaseKey);
+
+    // Check that a subtract-from-recipient transaction slightly less than the
+    // coinbase input amount does not create a change output (because it would
+    // be uneconomical to add and spend the output), and make sure it pays the
+    // leftover input amount which would have been change to the recipient
+    // instead of the miner.
+    auto check_tx = [&wallet](CAmount leftover_input_amount) {
+        CRecipient recipient{GetScriptForRawPubKey({}), 50 * COIN - leftover_input_amount, true /* subtract fee */};
+        CTransactionRef tx;
+        CAmount fee;
+        int change_pos = -1;
+        bilingual_str error;
+        CCoinControl coin_control;
+        coin_control.m_feerate.emplace(10000);
+        coin_control.fOverrideFeeRate = true;
+        FeeCalculation fee_calc;
+        BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, change_pos, error, coin_control, fee_calc));
+        BOOST_CHECK_EQUAL(tx->vout.size(), 1);
+        BOOST_CHECK_EQUAL(tx->vout[0].nValue, recipient.nAmount + leftover_input_amount - fee);
+        BOOST_CHECK_GT(fee, 0);
+        return fee;
+    };
+
+    // Send full input amount to recipient, check that only nonzero fee is
+    // subtracted (to_reduce == fee).
+    const CAmount fee{check_tx(0)};
+
+    // Send slightly less than full input amount to recipient, check leftover
+    // input amount is paid to recipient not the miner (to_reduce == fee - 123)
+    BOOST_CHECK_EQUAL(fee, check_tx(123));
+
+    // Send full input minus fee amount to recipient, check leftover input
+    // amount is paid to recipient not the miner (to_reduce == 0)
+    BOOST_CHECK_EQUAL(fee, check_tx(fee));
+
+    // Send full input minus more than the fee amount to recipient, check
+    // leftover input amount is paid to recipient not the miner (to_reduce ==
+    // -123). This overpays the recipient instead of overpaying the miner more
+    // than double the neccesary fee.
+    BOOST_CHECK_EQUAL(fee, check_tx(fee + 123));
+}
+
+BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp
new file mode 100644
index 0000000000..c3061b93c0
--- /dev/null
+++ b/src/wallet/test/util.cpp
@@ -0,0 +1,38 @@
+// Copyright (c) 2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <wallet/test/util.h>
+
+#include <chain.h>
+#include <key.h>
+#include <test/util/setup_common.h>
+#include <wallet/wallet.h>
+#include <wallet/walletdb.h>
+
+#include <boost/test/unit_test.hpp>
+
+#include <memory>
+
+std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, const CKey& key)
+{
+    auto wallet = std::make_unique<CWallet>(&chain, "", CreateMockWalletDatabase());
+    {
+        LOCK2(wallet->cs_wallet, ::cs_main);
+        wallet->SetLastBlockProcessed(cchain.Height(), cchain.Tip()->GetBlockHash());
+    }
+    wallet->LoadWallet();
+    {
+        auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan();
+        LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore);
+        spk_man->AddKeyPubKey(key, key.GetPubKey());
+    }
+    WalletRescanReserver reserver(*wallet);
+    reserver.reserve();
+    CWallet::ScanResult result = wallet->ScanForWalletTransactions(cchain.Genesis()->GetBlockHash(), 0 /* start_height */, {} /* max_height */, reserver, false /* update */);
+    BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
+    BOOST_CHECK_EQUAL(result.last_scanned_block, cchain.Tip()->GetBlockHash());
+    BOOST_CHECK_EQUAL(*result.last_scanned_height, cchain.Height());
+    BOOST_CHECK(result.last_failed_block.IsNull());
+    return wallet;
+}
diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h
new file mode 100644
index 0000000000..288c111571
--- /dev/null
+++ b/src/wallet/test/util.h
@@ -0,0 +1,19 @@
+// Copyright (c) 2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_WALLET_TEST_UTIL_H
+#define BITCOIN_WALLET_TEST_UTIL_H
+
+#include <memory>
+
+class CChain;
+class CKey;
+class CWallet;
+namespace interfaces {
+class Chain;
+} // namespace interfaces
+
+std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, const CKey& key);
+
+#endif // BITCOIN_WALLET_TEST_UTIL_H
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index a0070b8dd3..75a08b6f74 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -20,6 +20,7 @@
 #include <util/translation.h>
 #include <validation.h>
 #include <wallet/coincontrol.h>
+#include <wallet/test/util.h>
 #include <wallet/test/wallet_test_fixture.h>
 
 #include <boost/test/unit_test.hpp>
@@ -480,20 +481,7 @@ public:
     ListCoinsTestingSetup()
     {
         CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
-        wallet = std::make_unique<CWallet>(m_node.chain.get(), "", CreateMockWalletDatabase());
-        {
-            LOCK2(wallet->cs_wallet, ::cs_main);
-            wallet->SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
-        }
-        wallet->LoadWallet();
-        AddKey(*wallet, coinbaseKey);
-        WalletRescanReserver reserver(*wallet);
-        reserver.reserve();
-        CWallet::ScanResult result = wallet->ScanForWalletTransactions(m_node.chainman->ActiveChain().Genesis()->GetBlockHash(), 0 /* start_height */, {} /* max_height */, reserver, false /* update */);
-        BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
-        BOOST_CHECK_EQUAL(result.last_scanned_block, m_node.chainman->ActiveChain().Tip()->GetBlockHash());
-        BOOST_CHECK_EQUAL(*result.last_scanned_height, m_node.chainman->ActiveChain().Height());
-        BOOST_CHECK(result.last_failed_block.IsNull());
+        wallet = CreateSyncedWallet(*m_node.chain, m_node.chainman->ActiveChain(), coinbaseKey);
     }
 
     ~ListCoinsTestingSetup()