From 0501a11c879cace62226b183531924903478cc6c Mon Sep 17 00:00:00 2001 From: xdustinface Date: Fri, 21 Aug 2020 15:39:02 +0200 Subject: [PATCH 1/8] test: Implement unit tests for CWallet::CreateTransaction --- src/wallet/test/wallet_tests.cpp | 344 +++++++++++++++++++++++++++++++ 1 file changed, 344 insertions(+) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index c1950fd299ab..5258b5998d99 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -4,6 +4,7 @@ #include +#include #include #include #include @@ -23,6 +24,7 @@ extern UniValue importmulti(const JSONRPCRequest& request); extern UniValue dumpwallet(const JSONRPCRequest& request); extern UniValue importwallet(const JSONRPCRequest& request); +extern UniValue getnewaddress(const JSONRPCRequest& request); // how many times to run all the tests to have a chance to catch errors that only show up with particular random shuffles #define RUN_TESTS 100 @@ -698,4 +700,346 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) BOOST_CHECK_EQUAL(list.begin()->second.size(), 2); } +class CreateTransactionTestSetup : public TestChain100Setup +{ +public: + enum ChangeTest { + Skip, + NoChangeExpected, + ChangeExpected, + }; + + CreateTransactionTestSetup() + { + CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + wallet = MakeUnique("mock", WalletDatabase::CreateMock()); + bool firstRun; + wallet->LoadWallet(firstRun); + AddWallet(wallet.get()); + AddKey(*wallet, coinbaseKey); + WalletRescanReserver reserver(wallet.get()); + reserver.reserve(); + wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver); + } + + ~CreateTransactionTestSetup() + { + RemoveWallet(wallet.get()); + } + + std::unique_ptr wallet; + CCoinControl coinControl; + + bool CreateTransaction(const std::vector>& vecEntries, bool fCreateShouldSucceed = true, ChangeTest changeTest = ChangeTest::Skip) + { + return CreateTransaction(vecEntries, -1, fCreateShouldSucceed, changeTest); + } + + bool CreateTransaction(const std::vector>& vecEntries, int nChangePosRequest = -1, bool fCreateShouldSucceed = true, ChangeTest changeTest = ChangeTest::Skip) + { + CWalletTx wtx; + CReserveKey reservekey(wallet.get()); + CAmount nFeeRet; + int nChangePos = nChangePosRequest; + std::string strError; + + auto checkEqual = [&](bool l, bool r) -> bool { + BOOST_CHECK_EQUAL(l, r); + return l == r; + }; + + // Verify the creation succeeds if expected and fails if not. + if (!checkEqual(fCreateShouldSucceed, wallet->CreateTransaction(GetRecipients(vecEntries), wtx, reservekey, nFeeRet, nChangePos, strError, coinControl))) { + return false; + } + if (!fCreateShouldSucceed) { + // No need to evaluate the following if the creation should have failed. + return true; + } + // Verify there is no change output if there wasn't any expected + bool fChangeTestPassed = changeTest == ChangeTest::Skip || + (changeTest == ChangeTest::ChangeExpected && nChangePos != -1) || + (changeTest == ChangeTest::NoChangeExpected && nChangePos == -1); + BOOST_CHECK(fChangeTestPassed); + if (!fChangeTestPassed) { + return false; + } + // Verify the change is at the requested position if there was a request + if (nChangePosRequest != -1 && !checkEqual(nChangePosRequest, nChangePos)) { + return false; + } + // Verify the number of requested outputs does match the resulting outputs + checkEqual(wtx.tx->vout.size() - (nChangePos != -1 ? 1 : 0), vecEntries.size()); + return true; + } + + std::vector GetRecipients(const std::vector>& vecEntries) + { + std::vector vecRecipients; + for (auto entry : vecEntries) { + vecRecipients.push_back({GetScriptForDestination(DecodeDestination(getnewaddress(JSONRPCRequest()).get_str())), entry.first, entry.second}); + } + return vecRecipients; + } + + std::vector GetCoins(const std::vector>& vecEntries) + { + CWalletTx wtx; + CReserveKey reserveKey(wallet.get()); + CAmount nFeeRet; + int nChangePosRet = -1; + std::string strError; + CCoinControl coinControl; + BOOST_CHECK(wallet->CreateTransaction(GetRecipients(vecEntries), wtx, reserveKey, nFeeRet, nChangePosRet, strError, coinControl)); + CValidationState state; + BOOST_CHECK(wallet->CommitTransaction(wtx, reserveKey, nullptr, state)); + CMutableTransaction blocktx; + { + LOCK(wallet->cs_wallet); + blocktx = CMutableTransaction(*wallet->mapWallet.at(wtx.tx->GetHash()).tx); + } + CreateAndProcessBlock({CMutableTransaction(blocktx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + LOCK(cs_main); + LOCK(wallet->cs_wallet); + auto it = wallet->mapWallet.find(wtx.GetHash()); + BOOST_CHECK(it != wallet->mapWallet.end()); + it->second.SetMerkleBranch(chainActive.Tip(), 1); + wtx = it->second; + + std::vector vecOutpoints; + size_t n; + for (n = 0; n < wtx.tx->vout.size(); ++n) { + if (nChangePosRet != -1 && n == nChangePosRet) { + // Skip the change output to only return the requested coins + continue; + } + vecOutpoints.push_back(COutPoint(wtx.GetHash(), n)); + } + assert(vecOutpoints.size() == vecEntries.size()); + return vecOutpoints; + } +}; + +BOOST_FIXTURE_TEST_CASE(CreateTransactionTest, CreateTransactionTestSetup) +{ + // Try to send dust + BOOST_CHECK(CreateTransaction({{10, false}}, false)); + BOOST_CHECK(CreateTransaction({{10, true}}, false)); + BOOST_CHECK(CreateTransaction({{100, false}}, false)); + BOOST_CHECK(CreateTransaction({{100, true}}, false)); + BOOST_CHECK(CreateTransaction({{10000, true}, {100, true}}, false)); + BOOST_CHECK(CreateTransaction({{10000, false}, {100, false}}, false)); + + auto runTest = [&](const int nTestId, const CAmount nFeeRate, const std::map>& mapExpected) { + coinControl.m_feerate = CFeeRate(nFeeRate); + const std::map>> mapTestCases{ + {0, {{1000, false}}}, + {1, {{1000, true}}}, + {2, {{10000, false}}}, + {3, {{10000, true}}}, + {4, {{34000, false}, {40000, false}}}, + {5, {{37000, false}, {40000, false}}}, + {6, {{50000, false}, {50000, false}}}, + {7, {{50000, true}, {50000, false}}}, + {8, {{50000, false}, {50001, false}}}, + {9, {{50000, true}, {50001, true}}}, + {10, {{100000, false}}}, + {11, {{100000, true}}}, + {12, {{100001, false}}}, + {13, {{100001, true}}} + }; + assert(mapTestCases.size() == mapExpected.size()); + for (int i = 0; i < mapTestCases.size(); ++i) { + if (!CreateTransaction(mapTestCases.at(i), mapExpected.at(i).first, mapExpected.at(i).second)) { + std::cout << strprintf("CreateTransactionTest failed at: %d - %d\n", nTestId, i) << std::endl; + } + } + }; + + // First run the tests with only one input containing 100k duffs + { + coinControl.SetNull(); + coinControl.Select(GetCoins({{100000, false}})[0]); + + // Start with fallback feerate + runTest(1, DEFAULT_FALLBACK_FEE, { + {0, {true, ChangeTest::ChangeExpected}}, + {1, {true, ChangeTest::ChangeExpected}}, + {2, {true, ChangeTest::ChangeExpected}}, + {3, {true, ChangeTest::ChangeExpected}}, + {4, {true, ChangeTest::ChangeExpected}}, + {5, {true, ChangeTest::ChangeExpected}}, + {6, {false, ChangeTest::Skip}}, + {7, {true, ChangeTest::NoChangeExpected}}, + {8, {false, ChangeTest::Skip}}, + {9, {true, ChangeTest::NoChangeExpected}}, + {10, {false, ChangeTest::Skip}}, + {11, {true, ChangeTest::NoChangeExpected}}, + {12, {false, ChangeTest::Skip}}, + {13, {true, ChangeTest::NoChangeExpected}} + }); + // Now with 100x fallback feerate + runTest(2, DEFAULT_FALLBACK_FEE * 100, { + {0, {true, ChangeTest::ChangeExpected}}, + {1, {false, ChangeTest::Skip}}, + {2, {true, ChangeTest::ChangeExpected}}, + {3, {false, ChangeTest::Skip}}, + {4, {true, ChangeTest::NoChangeExpected}}, + {5, {false, ChangeTest::Skip}}, + {6, {false, ChangeTest::Skip}}, + {7, {true, ChangeTest::NoChangeExpected}}, + {8, {false, ChangeTest::Skip}}, + {9, {true, ChangeTest::NoChangeExpected}}, + {10, {false, ChangeTest::Skip}}, + {11, {true, ChangeTest::NoChangeExpected}}, + {12, {false, ChangeTest::Skip}}, + {13, {true, ChangeTest::NoChangeExpected}} + }); + } + // Now use 4 different inputs with a total of 100k duff + { + coinControl.SetNull(); + auto setCoins = GetCoins({{1000, false}, {5000, false}, {10000, false}, {84000, false}}); + for (auto coin : setCoins) { + coinControl.Select(coin); + } + + // Start with fallback feerate + runTest(3, DEFAULT_FALLBACK_FEE, { + {0, {true, ChangeTest::ChangeExpected}}, + {1, {false, ChangeTest::Skip}}, + {2, {true, ChangeTest::ChangeExpected}}, + {3, {true, ChangeTest::ChangeExpected}}, + {4, {true, ChangeTest::ChangeExpected}}, + {5, {true, ChangeTest::ChangeExpected}}, + {6, {false, ChangeTest::Skip}}, + {7, {true, ChangeTest::NoChangeExpected}}, + {8, {false, ChangeTest::Skip}}, + {9, {true, ChangeTest::NoChangeExpected}}, + {10, {false, ChangeTest::Skip}}, + {11, {true, ChangeTest::NoChangeExpected}}, + {12, {false, ChangeTest::Skip}}, + {13, {true, ChangeTest::NoChangeExpected}} + }); + // Now with 100x fallback feerate + runTest(4, DEFAULT_FALLBACK_FEE * 100, { + {0, {true, ChangeTest::ChangeExpected}}, + {1, {false, ChangeTest::Skip}}, + {2, {true, ChangeTest::ChangeExpected}}, + {3, {false, ChangeTest::Skip}}, + {4, {false, ChangeTest::Skip}}, + {5, {false, ChangeTest::Skip}}, + {6, {false, ChangeTest::Skip}}, + {7, {false, ChangeTest::Skip}}, + {8, {false, ChangeTest::Skip}}, + {9, {true, ChangeTest::NoChangeExpected}}, + {10, {false, ChangeTest::Skip}}, + {11, {true, ChangeTest::NoChangeExpected}}, + {12, {false, ChangeTest::Skip}}, + {13, {true, ChangeTest::NoChangeExpected}} + }); + } + + // Last use 10 equal inputs with a total of 100k duff + { + coinControl.SetNull(); + auto setCoins = GetCoins({{10000, false}, {10000, false}, {10000, false}, {10000, false}, {10000, false}, + {10000, false}, {10000, false}, {10000, false}, {10000, false}, {10000, false}}); + + for (auto coin : setCoins) { + coinControl.Select(coin); + } + + // Start with fallback feerate + runTest(5, DEFAULT_FALLBACK_FEE, { + {0, {true, ChangeTest::ChangeExpected}}, + {1, {false, ChangeTest::Skip}}, + {2, {true, ChangeTest::ChangeExpected}}, + {3, {true, ChangeTest::ChangeExpected}}, + {4, {true, ChangeTest::ChangeExpected}}, + {5, {true, ChangeTest::ChangeExpected}}, + {6, {false, ChangeTest::Skip}}, + {7, {true, ChangeTest::NoChangeExpected}}, + {8, {false, ChangeTest::Skip}}, + {9, {true, ChangeTest::NoChangeExpected}}, + {10, {false, ChangeTest::Skip}}, + {11, {true, ChangeTest::NoChangeExpected}}, + {12, {false, ChangeTest::Skip}}, + {13, {true, ChangeTest::NoChangeExpected}} + }); + // Now with 100x fallback feerate + runTest(6, DEFAULT_FALLBACK_FEE * 100, { + {0, {false, ChangeTest::Skip}}, + {1, {false, ChangeTest::Skip}}, + {2, {false, ChangeTest::Skip}}, + {3, {false, ChangeTest::Skip}}, + {4, {false, ChangeTest::Skip}}, + {5, {false, ChangeTest::Skip}}, + {6, {false, ChangeTest::Skip}}, + {7, {false, ChangeTest::Skip}}, + {8, {false, ChangeTest::Skip}}, + {9, {false, ChangeTest::Skip}}, + {10, {false, ChangeTest::Skip}}, + {11, {false, ChangeTest::Skip}}, + {12, {false, ChangeTest::Skip}}, + {13, {false, ChangeTest::Skip}} + }); + } + // Some tests without selected coins in coinControl, let the wallet decide + // which inputs to use + { + coinControl.SetNull(); + auto setCoins = GetCoins({{1000, false}, {1000, false}, {1000, false}, {1000, false}, {1000, false}, + {1100, false}, {1200, false}, {1300, false}, {1400, false}, {1500, false}, + {3000, false}, {3000, false}, {2000, false}, {2000, false}, {1000, false}}); + // Lock all other coins which were already in the wallet + std::vector vecAvailable; + wallet->AvailableCoins(vecAvailable); + for (auto coin : vecAvailable) { + auto out = COutPoint(coin.tx->GetHash(), coin.i); + if (std::find(setCoins.begin(), setCoins.end(), out) == setCoins.end()) { + wallet->LockCoin(out); + } + } + + BOOST_CHECK(CreateTransaction({{100, false}}, false)); + BOOST_CHECK(CreateTransaction({{1000, true}}, true)); + BOOST_CHECK(CreateTransaction({{1100, false}}, true)); + BOOST_CHECK(CreateTransaction({{1100, true}}, true)); + BOOST_CHECK(CreateTransaction({{2200, false}}, true)); + BOOST_CHECK(CreateTransaction({{3300, false}}, true)); + BOOST_CHECK(CreateTransaction({{4400, false}}, true)); + BOOST_CHECK(CreateTransaction({{5500, false}}, true)); + BOOST_CHECK(CreateTransaction({{5500, true}}, true)); + BOOST_CHECK(CreateTransaction({{6600, false}}, true)); + BOOST_CHECK(CreateTransaction({{7700, false}}, true)); + BOOST_CHECK(CreateTransaction({{8800, false}}, true)); + BOOST_CHECK(CreateTransaction({{9900, false}}, true)); + BOOST_CHECK(CreateTransaction({{9900, true}}, true)); + BOOST_CHECK(CreateTransaction({{10000, false}}, true)); + BOOST_CHECK(CreateTransaction({{10000, false}, {10000, false}}, false)); + BOOST_CHECK(CreateTransaction({{10000, false}, {12500, true}}, true)); + BOOST_CHECK(CreateTransaction({{10000, true}, {10000, true}}, true)); + BOOST_CHECK(CreateTransaction({{1000, false}, {2000, false}, {3000, false}, {4000, false}}, true)); + BOOST_CHECK(CreateTransaction({{1234, false}}, true)); + BOOST_CHECK(CreateTransaction({{1234, false}, {4321, false}}, true)); + BOOST_CHECK(CreateTransaction({{1234, false}, {4321, false}, {5678, false}}, true)); + BOOST_CHECK(CreateTransaction({{1234, false}, {4321, false}, {5678, false}, {8765, false}}, false)); + BOOST_CHECK(CreateTransaction({{1234, false}, {4321, false}, {5678, false}, {8765, true}}, true)); + BOOST_CHECK(CreateTransaction({{1000000, false}}, false)); + + wallet->UnlockAllCoins(); + } + // Test if the change output ends up at the requested position + { + coinControl.SetNull(); + coinControl.Select(GetCoins({{100000, false}})[0]); + + BOOST_CHECK(CreateTransaction({{25000, false}, {25000, false}, {25000, false}}, 0, true, ChangeTest::ChangeExpected)); + BOOST_CHECK(CreateTransaction({{25000, false}, {25000, false}, {25000, false}}, 1, true, ChangeTest::ChangeExpected)); + BOOST_CHECK(CreateTransaction({{25000, false}, {25000, false}, {25000, false}}, 2, true, ChangeTest::ChangeExpected)); + BOOST_CHECK(CreateTransaction({{25000, false}, {25000, false}, {25000, false}}, 3, true, ChangeTest::ChangeExpected)); + } +} + BOOST_AUTO_TEST_SUITE_END() From 1182df75b8847300ece12bc5335b2177443c4ea1 Mon Sep 17 00:00:00 2001 From: xdustinface Date: Sat, 22 Aug 2020 20:37:29 +0200 Subject: [PATCH 2/8] test: Reset minRelayTxFee in CreateTransactionTest At this point its modified from an other test when running all unit tests which lets this test fail unexpectedly. --- src/wallet/test/wallet_tests.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 5258b5998d99..122c52740c53 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -822,6 +822,7 @@ class CreateTransactionTestSetup : public TestChain100Setup BOOST_FIXTURE_TEST_CASE(CreateTransactionTest, CreateTransactionTestSetup) { + minRelayTxFee = CFeeRate(DEFAULT_MIN_RELAY_TX_FEE); // Try to send dust BOOST_CHECK(CreateTransaction({{10, false}}, false)); BOOST_CHECK(CreateTransaction({{10, true}}, false)); From cfe6b77a1d7cd030d6990ae81d342f29366e072b Mon Sep 17 00:00:00 2001 From: xdustinface Date: Sun, 23 Aug 2020 01:31:54 +0200 Subject: [PATCH 3/8] test: Lock cs_main/cs_wallet when required in CreateTransactionTest --- src/wallet/test/wallet_tests.cpp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 122c52740c53..4296d1885171 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -995,11 +995,14 @@ BOOST_FIXTURE_TEST_CASE(CreateTransactionTest, CreateTransactionTestSetup) {3000, false}, {3000, false}, {2000, false}, {2000, false}, {1000, false}}); // Lock all other coins which were already in the wallet std::vector vecAvailable; - wallet->AvailableCoins(vecAvailable); - for (auto coin : vecAvailable) { - auto out = COutPoint(coin.tx->GetHash(), coin.i); - if (std::find(setCoins.begin(), setCoins.end(), out) == setCoins.end()) { - wallet->LockCoin(out); + { + LOCK2(cs_main, wallet->cs_wallet); + wallet->AvailableCoins(vecAvailable); + for (auto coin : vecAvailable) { + auto out = COutPoint(coin.tx->GetHash(), coin.i); + if (std::find(setCoins.begin(), setCoins.end(), out) == setCoins.end()) { + wallet->LockCoin(out); + } } } @@ -1029,6 +1032,7 @@ BOOST_FIXTURE_TEST_CASE(CreateTransactionTest, CreateTransactionTestSetup) BOOST_CHECK(CreateTransaction({{1234, false}, {4321, false}, {5678, false}, {8765, true}}, true)); BOOST_CHECK(CreateTransaction({{1000000, false}}, false)); + LOCK(wallet->cs_wallet); wallet->UnlockAllCoins(); } // Test if the change output ends up at the requested position From 9ea70b778a70322921bad338754e1f64f5254aca Mon Sep 17 00:00:00 2001 From: xdustinface Date: Sun, 23 Aug 2020 17:08:32 +0200 Subject: [PATCH 4/8] test: Add new test in CreateTransactionTest Test if the wallet creation fails properly with the correct error messages. --- src/wallet/test/wallet_tests.cpp | 118 +++++++++++++++++++++++++++---- 1 file changed, 104 insertions(+), 14 deletions(-) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 4296d1885171..5db0d634c0af 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -709,6 +709,19 @@ class CreateTransactionTestSetup : public TestChain100Setup ChangeExpected, }; + // Result strings to test + const std::string strInsufficientFunds = "Insufficient funds."; + const std::string strAmountNotNegative = "Transaction amounts must not be negative"; + const std::string strAtLeastOneRecipient = "Transaction must have at least one recipient"; + const std::string strTooSmallToPayFee = "The transaction amount is too small to pay the fee"; + const std::string strTooSmallAfterFee = "The transaction amount is too small to send after the fee has been deducted"; + const std::string strTooSmall = "Transaction amount too small"; + const std::string strUnableToLocatePrivateSend1 = "Unable to locate enough PrivateSend non-denominated funds for this transaction."; + const std::string strUnableToLocatePrivateSend2 = "Unable to locate enough PrivateSend denominated funds for this transaction. PrivateSend uses exact denominated amounts to send funds, you might simply need to mix some more coins."; + const std::string strTransactionTooLarge = "Transaction too large"; + const std::string strTransactionTooLargeForFeePolicy = "Transaction too large for fee policy"; + const std::string strChangeIndexOutOfRange = "Change index out of range"; + CreateTransactionTestSetup() { CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); @@ -730,12 +743,23 @@ class CreateTransactionTestSetup : public TestChain100Setup std::unique_ptr wallet; CCoinControl coinControl; + template + bool CheckEqual(const T expected, const T actual) + { + BOOST_CHECK_EQUAL(expected, actual); + return expected == actual; + } + bool CreateTransaction(const std::vector>& vecEntries, bool fCreateShouldSucceed = true, ChangeTest changeTest = ChangeTest::Skip) { - return CreateTransaction(vecEntries, -1, fCreateShouldSucceed, changeTest); + return CreateTransaction(vecEntries, {}, -1, fCreateShouldSucceed, changeTest); + } + bool CreateTransaction(const std::vector>& vecEntries, std::string strErrorExpected, bool fCreateShouldSucceed = true, ChangeTest changeTest = ChangeTest::Skip) + { + return CreateTransaction(vecEntries, strErrorExpected, -1, fCreateShouldSucceed, changeTest); } - bool CreateTransaction(const std::vector>& vecEntries, int nChangePosRequest = -1, bool fCreateShouldSucceed = true, ChangeTest changeTest = ChangeTest::Skip) + bool CreateTransaction(const std::vector>& vecEntries, std::string strErrorExpected, int nChangePosRequest = -1, bool fCreateShouldSucceed = true, ChangeTest changeTest = ChangeTest::Skip) { CWalletTx wtx; CReserveKey reservekey(wallet.get()); @@ -743,13 +767,12 @@ class CreateTransactionTestSetup : public TestChain100Setup int nChangePos = nChangePosRequest; std::string strError; - auto checkEqual = [&](bool l, bool r) -> bool { - BOOST_CHECK_EQUAL(l, r); - return l == r; - }; - // Verify the creation succeeds if expected and fails if not. - if (!checkEqual(fCreateShouldSucceed, wallet->CreateTransaction(GetRecipients(vecEntries), wtx, reservekey, nFeeRet, nChangePos, strError, coinControl))) { + if (!CheckEqual(fCreateShouldSucceed, wallet->CreateTransaction(GetRecipients(vecEntries), wtx, reservekey, nFeeRet, nChangePos, strError, coinControl))) { + return false; + } + // Verify the expected error string if there is one provided + if (strErrorExpected.size() && !CheckEqual(strErrorExpected, strError)) { return false; } if (!fCreateShouldSucceed) { @@ -765,11 +788,11 @@ class CreateTransactionTestSetup : public TestChain100Setup return false; } // Verify the change is at the requested position if there was a request - if (nChangePosRequest != -1 && !checkEqual(nChangePosRequest, nChangePos)) { + if (nChangePosRequest != -1 && !CheckEqual(nChangePosRequest, nChangePos)) { return false; } // Verify the number of requested outputs does match the resulting outputs - checkEqual(wtx.tx->vout.size() - (nChangePos != -1 ? 1 : 0), vecEntries.size()); + CheckEqual(vecEntries.size(), wtx.tx->vout.size() - (nChangePos != -1 ? 1 : 0)); return true; } @@ -1040,10 +1063,77 @@ BOOST_FIXTURE_TEST_CASE(CreateTransactionTest, CreateTransactionTestSetup) coinControl.SetNull(); coinControl.Select(GetCoins({{100000, false}})[0]); - BOOST_CHECK(CreateTransaction({{25000, false}, {25000, false}, {25000, false}}, 0, true, ChangeTest::ChangeExpected)); - BOOST_CHECK(CreateTransaction({{25000, false}, {25000, false}, {25000, false}}, 1, true, ChangeTest::ChangeExpected)); - BOOST_CHECK(CreateTransaction({{25000, false}, {25000, false}, {25000, false}}, 2, true, ChangeTest::ChangeExpected)); - BOOST_CHECK(CreateTransaction({{25000, false}, {25000, false}, {25000, false}}, 3, true, ChangeTest::ChangeExpected)); + BOOST_CHECK(CreateTransaction({{25000, false}, {25000, false}, {25000, false}}, {}, 0, true, ChangeTest::ChangeExpected)); + BOOST_CHECK(CreateTransaction({{25000, false}, {25000, false}, {25000, false}}, {}, 1, true, ChangeTest::ChangeExpected)); + BOOST_CHECK(CreateTransaction({{25000, false}, {25000, false}, {25000, false}}, {}, 2, true, ChangeTest::ChangeExpected)); + BOOST_CHECK(CreateTransaction({{25000, false}, {25000, false}, {25000, false}}, {}, 3, true, ChangeTest::ChangeExpected)); + } + // Test error cases + { + coinControl.SetNull(); + // First try to send something without any coins available + { + // Lock all other coins + std::vector vecAvailable; + { + LOCK2(cs_main, wallet->cs_wallet); + wallet->AvailableCoins(vecAvailable); + for (auto coin : vecAvailable) { + wallet->LockCoin(COutPoint(coin.tx->GetHash(), coin.i)); + } + } + + BOOST_CHECK(CreateTransaction({{1000, false}}, strInsufficientFunds, false)); + BOOST_CHECK(CreateTransaction({{1000, true}}, strInsufficientFunds, false)); + coinControl.nCoinType = CoinType::ONLY_NONDENOMINATED; + BOOST_CHECK(CreateTransaction({{1000, true}}, strUnableToLocatePrivateSend1, false)); + coinControl.nCoinType = CoinType::ONLY_FULLY_MIXED; + BOOST_CHECK(CreateTransaction({{1000, true}}, strUnableToLocatePrivateSend2, false)); + + LOCK(wallet->cs_wallet); + wallet->UnlockAllCoins(); + } + + // Just to create nCount output recipes to use in tests below + std::vector> vecOutputEntries{{5000, false}}; + auto createOutputEntries = [&](int nCount) { + while (vecOutputEntries.size() <= nCount) { + vecOutputEntries.push_back(vecOutputEntries.back()); + } + if (vecOutputEntries.size() > nCount) { + int nDiff = vecOutputEntries.size() - nCount; + vecOutputEntries.erase(vecOutputEntries.begin(), vecOutputEntries.begin() + nDiff); + } + }; + + coinControl.SetNull(); + coinControl.Select(GetCoins({{100 * COIN, false}})[0]); + + BOOST_CHECK(CreateTransaction({{-5000, false}}, strAmountNotNegative, false)); + BOOST_CHECK(CreateTransaction({}, strAtLeastOneRecipient, false)); + BOOST_CHECK(CreateTransaction({{545, false}}, strTooSmall, false)); + BOOST_CHECK(CreateTransaction({{545, true}}, strTooSmall, false)); + BOOST_CHECK(CreateTransaction({{546, true}}, strTooSmallAfterFee, false)); + + createOutputEntries(100); + vecOutputEntries.push_back({600, true}); + BOOST_CHECK(CreateTransaction(vecOutputEntries, strTooSmallToPayFee, false)); + vecOutputEntries.pop_back(); + + createOutputEntries(2934); + BOOST_CHECK(CreateTransaction(vecOutputEntries, {}, true)); + createOutputEntries(2935); + BOOST_CHECK(CreateTransaction(vecOutputEntries, strTransactionTooLarge, false)); + + auto prevRate = minRelayTxFee; + coinControl.m_feerate = prevRate; + coinControl.fOverrideFeeRate = true; + minRelayTxFee = CFeeRate(prevRate.GetFeePerK() * 10); + BOOST_CHECK(CreateTransaction({{5000, false}}, strTransactionTooLargeForFeePolicy, false)); + coinControl.m_feerate.reset(); + minRelayTxFee = prevRate; + + BOOST_CHECK(CreateTransaction({{5000, false}, {5000, false}, {5000, false}}, strChangeIndexOutOfRange, 4, false)); } } From 0919549ce48bfb547af164a93061fddcb8a50692 Mon Sep 17 00:00:00 2001 From: xdustinface Date: Mon, 24 Aug 2020 13:04:15 +0200 Subject: [PATCH 5/8] test: Fixed expected test results for some CreateTransactionTest cases --- src/wallet/test/wallet_tests.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 5db0d634c0af..e0ef38fbc3a5 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -896,11 +896,11 @@ BOOST_FIXTURE_TEST_CASE(CreateTransactionTest, CreateTransactionTestSetup) {6, {false, ChangeTest::Skip}}, {7, {true, ChangeTest::NoChangeExpected}}, {8, {false, ChangeTest::Skip}}, - {9, {true, ChangeTest::NoChangeExpected}}, + {9, {false, ChangeTest::Skip}}, {10, {false, ChangeTest::Skip}}, {11, {true, ChangeTest::NoChangeExpected}}, {12, {false, ChangeTest::Skip}}, - {13, {true, ChangeTest::NoChangeExpected}} + {13, {false, ChangeTest::Skip}} }); // Now with 100x fallback feerate runTest(2, DEFAULT_FALLBACK_FEE * 100, { @@ -913,11 +913,11 @@ BOOST_FIXTURE_TEST_CASE(CreateTransactionTest, CreateTransactionTestSetup) {6, {false, ChangeTest::Skip}}, {7, {true, ChangeTest::NoChangeExpected}}, {8, {false, ChangeTest::Skip}}, - {9, {true, ChangeTest::NoChangeExpected}}, + {9, {false, ChangeTest::Skip}}, {10, {false, ChangeTest::Skip}}, {11, {true, ChangeTest::NoChangeExpected}}, {12, {false, ChangeTest::Skip}}, - {13, {true, ChangeTest::NoChangeExpected}} + {13, {false, ChangeTest::Skip}} }); } // Now use 4 different inputs with a total of 100k duff @@ -939,11 +939,11 @@ BOOST_FIXTURE_TEST_CASE(CreateTransactionTest, CreateTransactionTestSetup) {6, {false, ChangeTest::Skip}}, {7, {true, ChangeTest::NoChangeExpected}}, {8, {false, ChangeTest::Skip}}, - {9, {true, ChangeTest::NoChangeExpected}}, + {9, {false, ChangeTest::Skip}}, {10, {false, ChangeTest::Skip}}, {11, {true, ChangeTest::NoChangeExpected}}, {12, {false, ChangeTest::Skip}}, - {13, {true, ChangeTest::NoChangeExpected}} + {13, {false, ChangeTest::Skip}} }); // Now with 100x fallback feerate runTest(4, DEFAULT_FALLBACK_FEE * 100, { @@ -956,11 +956,11 @@ BOOST_FIXTURE_TEST_CASE(CreateTransactionTest, CreateTransactionTestSetup) {6, {false, ChangeTest::Skip}}, {7, {false, ChangeTest::Skip}}, {8, {false, ChangeTest::Skip}}, - {9, {true, ChangeTest::NoChangeExpected}}, + {9, {false, ChangeTest::Skip}}, {10, {false, ChangeTest::Skip}}, {11, {true, ChangeTest::NoChangeExpected}}, {12, {false, ChangeTest::Skip}}, - {13, {true, ChangeTest::NoChangeExpected}} + {13, {false, ChangeTest::Skip}} }); } @@ -985,11 +985,11 @@ BOOST_FIXTURE_TEST_CASE(CreateTransactionTest, CreateTransactionTestSetup) {6, {false, ChangeTest::Skip}}, {7, {true, ChangeTest::NoChangeExpected}}, {8, {false, ChangeTest::Skip}}, - {9, {true, ChangeTest::NoChangeExpected}}, + {9, {false, ChangeTest::Skip}}, {10, {false, ChangeTest::Skip}}, {11, {true, ChangeTest::NoChangeExpected}}, {12, {false, ChangeTest::Skip}}, - {13, {true, ChangeTest::NoChangeExpected}} + {13, {false, ChangeTest::Skip}} }); // Now with 100x fallback feerate runTest(6, DEFAULT_FALLBACK_FEE * 100, { From 613a33da2e8672ca03af6b761680c681c6146dcc Mon Sep 17 00:00:00 2001 From: xdustinface Date: Mon, 24 Aug 2020 19:46:07 +0200 Subject: [PATCH 6/8] test: Fail if CreateTransaction runs into "Exceed max tries" case --- src/wallet/test/wallet_tests.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index e0ef38fbc3a5..6a9ec47f92fb 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -721,6 +721,7 @@ class CreateTransactionTestSetup : public TestChain100Setup const std::string strTransactionTooLarge = "Transaction too large"; const std::string strTransactionTooLargeForFeePolicy = "Transaction too large for fee policy"; const std::string strChangeIndexOutOfRange = "Change index out of range"; + const std::string strExceededMaxTries = "Exceeded max tries."; CreateTransactionTestSetup() { @@ -767,8 +768,15 @@ class CreateTransactionTestSetup : public TestChain100Setup int nChangePos = nChangePosRequest; std::string strError; + bool fCreationSucceeded = wallet->CreateTransaction(GetRecipients(vecEntries), wtx, reservekey, nFeeRet, nChangePos, strError, coinControl); + bool fHitMaxTries = strError == strExceededMaxTries; + // This should never happen. + if (fHitMaxTries) { + BOOST_CHECK(!fHitMaxTries); + return false; + } // Verify the creation succeeds if expected and fails if not. - if (!CheckEqual(fCreateShouldSucceed, wallet->CreateTransaction(GetRecipients(vecEntries), wtx, reservekey, nFeeRet, nChangePos, strError, coinControl))) { + if (!CheckEqual(fCreateShouldSucceed, fCreationSucceeded)) { return false; } // Verify the expected error string if there is one provided From cdea578caf1061d4b1db8085908d4e0d8545d65e Mon Sep 17 00:00:00 2001 From: xdustinface Date: Fri, 4 Sep 2020 00:30:16 +0200 Subject: [PATCH 7/8] test: Adjust last return in CreateTransaction --- src/wallet/test/wallet_tests.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 6a9ec47f92fb..92e5b2df3f99 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -800,8 +800,7 @@ class CreateTransactionTestSetup : public TestChain100Setup return false; } // Verify the number of requested outputs does match the resulting outputs - CheckEqual(vecEntries.size(), wtx.tx->vout.size() - (nChangePos != -1 ? 1 : 0)); - return true; + return CheckEqual(vecEntries.size(), wtx.tx->vout.size() - (nChangePos != -1 ? 1 : 0)); } std::vector GetRecipients(const std::vector>& vecEntries) From 2190f80143669bbf4e2aadb592afba24f68515fc Mon Sep 17 00:00:00 2001 From: xdustinface Date: Fri, 4 Sep 2020 00:33:47 +0200 Subject: [PATCH 8/8] test: Drop dust tests --- src/wallet/test/wallet_tests.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 92e5b2df3f99..56f354106143 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -853,13 +853,6 @@ class CreateTransactionTestSetup : public TestChain100Setup BOOST_FIXTURE_TEST_CASE(CreateTransactionTest, CreateTransactionTestSetup) { minRelayTxFee = CFeeRate(DEFAULT_MIN_RELAY_TX_FEE); - // Try to send dust - BOOST_CHECK(CreateTransaction({{10, false}}, false)); - BOOST_CHECK(CreateTransaction({{10, true}}, false)); - BOOST_CHECK(CreateTransaction({{100, false}}, false)); - BOOST_CHECK(CreateTransaction({{100, true}}, false)); - BOOST_CHECK(CreateTransaction({{10000, true}, {100, true}}, false)); - BOOST_CHECK(CreateTransaction({{10000, false}, {100, false}}, false)); auto runTest = [&](const int nTestId, const CAmount nFeeRate, const std::map>& mapExpected) { coinControl.m_feerate = CFeeRate(nFeeRate);