From aa638131293897c8b04510899671227c8c3973a1 Mon Sep 17 00:00:00 2001 From: xdustinface Date: Wed, 26 Aug 2020 00:41:41 +0200 Subject: [PATCH 1/7] makefile.test.include: Let privatesend_tests.cpp depend on ENABLE_WALLET --- src/Makefile.test.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 031d835be2d4..f7ef2ce8fa08 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -72,7 +72,6 @@ BITCOIN_TESTS =\ test/policyestimator_tests.cpp \ test/pow_tests.cpp \ test/prevector_tests.cpp \ - test/privatesend_tests.cpp \ test/raii_event_tests.cpp \ test/random_tests.cpp \ test/ratecheck_tests.cpp \ @@ -106,6 +105,7 @@ BITCOIN_TESTS =\ if ENABLE_WALLET BITCOIN_TESTS += \ + test/privatesend_tests.cpp \ wallet/test/wallet_test_fixture.cpp \ wallet/test/wallet_test_fixture.h \ wallet/test/accounting_tests.cpp \ From 43d20ecda50bba4aa39284317ea716ee044be304 Mon Sep 17 00:00:00 2001 From: xdustinface Date: Wed, 26 Aug 2020 00:03:19 +0200 Subject: [PATCH 2/7] test: Implement unit tests for CTransactionBuilder --- src/test/privatesend_tests.cpp | 146 +++++++++++++++++++++++++++++++++ 1 file changed, 146 insertions(+) diff --git a/src/test/privatesend_tests.cpp b/src/test/privatesend_tests.cpp index 64decdc3575d..e60b8c9e347b 100644 --- a/src/test/privatesend_tests.cpp +++ b/src/test/privatesend_tests.cpp @@ -5,7 +5,11 @@ #include #include +#include +#include #include +#include +#include #include @@ -26,4 +30,146 @@ BOOST_AUTO_TEST_CASE(ps_collatoral_tests) BOOST_CHECK(!CPrivateSend::IsCollateralAmount(0.00100001 * COIN)); } +class CTransactionBuilderTestSetup : public TestChain100Setup +{ +public: + CTransactionBuilderTestSetup() + { + CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + wallet = MakeUnique("mock", WalletDatabase::CreateMock()); + bool firstRun; + wallet->LoadWallet(firstRun); + AddWallet(wallet.get()); + { + LOCK(wallet->cs_wallet); + wallet->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey()); + } + WalletRescanReserver reserver(wallet.get()); + reserver.reserve(); + wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver); + } + + ~CTransactionBuilderTestSetup() + { + RemoveWallet(wallet.get()); + } + + std::unique_ptr wallet; + + CWalletTx& AddTxToChain(uint256 nTxHash) + { + assert(wallet->mapWallet.find(nTxHash) != wallet->mapWallet.end()); + CMutableTransaction blocktx; + { + LOCK(wallet->cs_wallet); + blocktx = CMutableTransaction(*wallet->mapWallet.at(nTxHash).tx); + } + CreateAndProcessBlock({blocktx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + LOCK(cs_main); + LOCK(wallet->cs_wallet); + auto it = wallet->mapWallet.find(blocktx.GetHash()); + BOOST_CHECK(it != wallet->mapWallet.end()); + it->second.SetMerkleBranch(chainActive.Tip(), 1); + return it->second; + } + CompactTallyItem GetTallyItem(const std::vector& vecAmounts) + { + CompactTallyItem tallyItem; + CWalletTx tx; + CReserveKey destKey(wallet.get()); + CReserveKey reserveKey(wallet.get()); + CAmount nFeeRet; + int nChangePosRet = -1; + std::string strError; + CCoinControl coinControl; + CPubKey pubKey; + BOOST_CHECK(destKey.GetReservedKey(pubKey, false)); + tallyItem.txdest = pubKey.GetID(); + + for (CAmount nAmount : vecAmounts) { + BOOST_CHECK(wallet->CreateTransaction({{GetScriptForDestination(tallyItem.txdest), nAmount, false}}, tx, reserveKey, nFeeRet, nChangePosRet, strError, coinControl)); + CValidationState state; + BOOST_CHECK(wallet->CommitTransaction(tx, reserveKey, nullptr, state)); + CWalletTx& wtx = AddTxToChain(tx.GetHash()); + for (size_t n = 0; n < wtx.tx->vout.size(); ++n) { + if (nChangePosRet != -1 && n == nChangePosRet) { + // Skip the change output to only return the requested coins + continue; + } + tallyItem.vecOutPoints.push_back(COutPoint(wtx.GetHash(), n)); + tallyItem.nAmount += wtx.tx->vout[n].nValue; + } + } + assert(tallyItem.vecOutPoints.size() == vecAmounts.size()); + destKey.KeepKey(); + return tallyItem; + } +}; + +BOOST_FIXTURE_TEST_CASE(CTransactionBuilderTest, CTransactionBuilderTestSetup) +{ + minRelayTxFee = CFeeRate(DEFAULT_MIN_RELAY_TX_FEE); + // Tests with single outpoint tallyItem + { + CompactTallyItem tallyItem = GetTallyItem({5000}); + CTransactionBuilder txBuilder(wallet.get(), tallyItem); + + BOOST_CHECK_EQUAL(txBuilder.CountOutputs(), 0); + BOOST_CHECK_EQUAL(tallyItem.nAmount, txBuilder.GetAmountInitial()); + BOOST_CHECK_EQUAL(txBuilder.GetAmountLeft(), 4810); + + BOOST_CHECK(txBuilder.CouldAddOutput(4776)); + BOOST_CHECK(!txBuilder.CouldAddOutput(4777)); + + BOOST_CHECK(txBuilder.CouldAddOutput(0)); + BOOST_CHECK(!txBuilder.CouldAddOutput(-1)); + + BOOST_CHECK(txBuilder.CouldAddOutputs({1000, 1000, 2708})); + BOOST_CHECK(!txBuilder.CouldAddOutputs({1000, 1000, 2709})); + + BOOST_CHECK_EQUAL(txBuilder.AddOutput(5000), nullptr); + BOOST_CHECK_EQUAL(txBuilder.AddOutput(-1), nullptr); + + CTransactionBuilderOutput* output = txBuilder.AddOutput(); + BOOST_CHECK(output->UpdateAmount(txBuilder.GetAmountLeft())); + BOOST_CHECK(!output->UpdateAmount(output->GetAmount() + 1)); + BOOST_CHECK(!output->UpdateAmount(0)); + BOOST_CHECK(!output->UpdateAmount(-1)); + BOOST_CHECK_EQUAL(txBuilder.CountOutputs(), 1); + + std::string strResult; + BOOST_CHECK(txBuilder.Commit(strResult)); + CWalletTx& wtx = AddTxToChain(uint256S(strResult)); + BOOST_CHECK_EQUAL(wtx.tx->vout[0].nValue, output->GetAmount()); + BOOST_CHECK(wtx.tx->vout[0].scriptPubKey == output->GetScript()); + } + // Tests with multiple outpoint tallyItem + { + CompactTallyItem tallyItem = GetTallyItem({10000, 20000, 30000, 40000, 50000}); + CTransactionBuilder txBuilder(wallet.get(), tallyItem); + std::vector vecOutputs; + std::string strResult; + + vecOutputs.push_back(txBuilder.AddOutput(100)); + BOOST_CHECK(!txBuilder.Commit(strResult)); + + vecOutputs.back()->UpdateAmount(1000); + while (vecOutputs.size() < 100) { + vecOutputs.push_back(txBuilder.AddOutput(1000 + vecOutputs.size())); + } + BOOST_CHECK_EQUAL(vecOutputs.size(), 100); + BOOST_CHECK(txBuilder.Commit(strResult)); + CWalletTx& wtx = AddTxToChain(uint256S(strResult)); + for (const auto& out : wtx.tx->vout) { + auto it = std::find_if(vecOutputs.begin(), vecOutputs.end(), [&](CTransactionBuilderOutput* output) -> bool { + return output->GetAmount() == out.nValue && output->GetScript() == out.scriptPubKey; + }); + if (it != vecOutputs.end()) { + vecOutputs.erase(it); + } + } + BOOST_CHECK(vecOutputs.size() == 0); + } +} + BOOST_AUTO_TEST_SUITE_END() From 6f7caa22d73a88f1ebedd0bf93e73afb234b6e7b Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Thu, 27 Aug 2020 03:55:14 +0300 Subject: [PATCH 3/7] Check that we can decrease the amount and GetAmountLeft() is updated accordingly --- src/test/privatesend_tests.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/privatesend_tests.cpp b/src/test/privatesend_tests.cpp index e60b8c9e347b..3e5b349145af 100644 --- a/src/test/privatesend_tests.cpp +++ b/src/test/privatesend_tests.cpp @@ -132,6 +132,8 @@ BOOST_FIXTURE_TEST_CASE(CTransactionBuilderTest, CTransactionBuilderTestSetup) CTransactionBuilderOutput* output = txBuilder.AddOutput(); BOOST_CHECK(output->UpdateAmount(txBuilder.GetAmountLeft())); + BOOST_CHECK(output->UpdateAmount(1)); + BOOST_CHECK(output->UpdateAmount(output->GetAmount() + txBuilder.GetAmountLeft())); BOOST_CHECK(!output->UpdateAmount(output->GetAmount() + 1)); BOOST_CHECK(!output->UpdateAmount(0)); BOOST_CHECK(!output->UpdateAmount(-1)); From 7d5e94f2a2d0b9fc0b412d458e804c54213e4698 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Thu, 27 Aug 2020 04:13:32 +0300 Subject: [PATCH 4/7] Check if resulting tx has a change output when expected --- src/test/privatesend_tests.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/test/privatesend_tests.cpp b/src/test/privatesend_tests.cpp index 3e5b349145af..3f355af6437b 100644 --- a/src/test/privatesend_tests.cpp +++ b/src/test/privatesend_tests.cpp @@ -142,6 +142,7 @@ BOOST_FIXTURE_TEST_CASE(CTransactionBuilderTest, CTransactionBuilderTestSetup) std::string strResult; BOOST_CHECK(txBuilder.Commit(strResult)); CWalletTx& wtx = AddTxToChain(uint256S(strResult)); + BOOST_CHECK_EQUAL(wtx.tx->vout.size(), txBuilder.CountOutputs()); // should have no change output BOOST_CHECK_EQUAL(wtx.tx->vout[0].nValue, output->GetAmount()); BOOST_CHECK(wtx.tx->vout[0].scriptPubKey == output->GetScript()); } @@ -162,12 +163,16 @@ BOOST_FIXTURE_TEST_CASE(CTransactionBuilderTest, CTransactionBuilderTestSetup) BOOST_CHECK_EQUAL(vecOutputs.size(), 100); BOOST_CHECK(txBuilder.Commit(strResult)); CWalletTx& wtx = AddTxToChain(uint256S(strResult)); + BOOST_CHECK_EQUAL(wtx.tx->vout.size(), txBuilder.CountOutputs() + 1); // should have change output for (const auto& out : wtx.tx->vout) { auto it = std::find_if(vecOutputs.begin(), vecOutputs.end(), [&](CTransactionBuilderOutput* output) -> bool { return output->GetAmount() == out.nValue && output->GetScript() == out.scriptPubKey; }); if (it != vecOutputs.end()) { vecOutputs.erase(it); + } else { + // change output + BOOST_CHECK_EQUAL(txBuilder.GetAmountLeft() - 34, out.nValue); } } BOOST_CHECK(vecOutputs.size() == 0); From 63042615cccb7118ff1048a325209708685bc2f6 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Thu, 27 Aug 2020 04:14:32 +0300 Subject: [PATCH 5/7] Avoid pushing nullptr into vecOutputs --- src/test/privatesend_tests.cpp | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/test/privatesend_tests.cpp b/src/test/privatesend_tests.cpp index 3f355af6437b..f962a17d9086 100644 --- a/src/test/privatesend_tests.cpp +++ b/src/test/privatesend_tests.cpp @@ -153,14 +153,23 @@ BOOST_FIXTURE_TEST_CASE(CTransactionBuilderTest, CTransactionBuilderTestSetup) std::vector vecOutputs; std::string strResult; - vecOutputs.push_back(txBuilder.AddOutput(100)); + auto output = txBuilder.AddOutput(100); + BOOST_CHECK(output != nullptr); BOOST_CHECK(!txBuilder.Commit(strResult)); - vecOutputs.back()->UpdateAmount(1000); + if (output != nullptr) { + output->UpdateAmount(1000); + vecOutputs.push_back(output); + } while (vecOutputs.size() < 100) { - vecOutputs.push_back(txBuilder.AddOutput(1000 + vecOutputs.size())); + output = txBuilder.AddOutput(1000 + vecOutputs.size()); + if (output == nullptr) { + break; + } + vecOutputs.push_back(output); } BOOST_CHECK_EQUAL(vecOutputs.size(), 100); + BOOST_CHECK_EQUAL(txBuilder.CountOutputs(), vecOutputs.size()); BOOST_CHECK(txBuilder.Commit(strResult)); CWalletTx& wtx = AddTxToChain(uint256S(strResult)); BOOST_CHECK_EQUAL(wtx.tx->vout.size(), txBuilder.CountOutputs() + 1); // should have change output From 9d3b88218b84a7eb4f30b07d749042ecf9a25c37 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Thu, 27 Aug 2020 04:17:31 +0300 Subject: [PATCH 6/7] Add few notes about size calculations --- src/test/privatesend_tests.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/test/privatesend_tests.cpp b/src/test/privatesend_tests.cpp index f962a17d9086..107db33f310c 100644 --- a/src/test/privatesend_tests.cpp +++ b/src/test/privatesend_tests.cpp @@ -108,6 +108,12 @@ class CTransactionBuilderTestSetup : public TestChain100Setup BOOST_FIXTURE_TEST_CASE(CTransactionBuilderTest, CTransactionBuilderTestSetup) { + // NOTE: Mock wallet version is FEATURE_BASE which means that it uses uncompressed pubkeys + // (65 bytes instead of 33 bytes), so CTxIn size is 180 bytes, not 148 bytes as one might expect. + // Each output is 34 bytes, vin and vout compact sizes are 1 byte each. + // Therefore base size (i.e. for a tx with 1 input, 0 outputs) is expected to be + // 4(n32bitVersion) + 1(vin size) + 180(vin[0]) + 1(vout size) + 4(nLockTime) = 190 bytes. + minRelayTxFee = CFeeRate(DEFAULT_MIN_RELAY_TX_FEE); // Tests with single outpoint tallyItem { @@ -116,15 +122,15 @@ BOOST_FIXTURE_TEST_CASE(CTransactionBuilderTest, CTransactionBuilderTestSetup) BOOST_CHECK_EQUAL(txBuilder.CountOutputs(), 0); BOOST_CHECK_EQUAL(tallyItem.nAmount, txBuilder.GetAmountInitial()); - BOOST_CHECK_EQUAL(txBuilder.GetAmountLeft(), 4810); + BOOST_CHECK_EQUAL(txBuilder.GetAmountLeft(), 4810); // 5000 - 190 - BOOST_CHECK(txBuilder.CouldAddOutput(4776)); + BOOST_CHECK(txBuilder.CouldAddOutput(4776)); // 4810 - 34 BOOST_CHECK(!txBuilder.CouldAddOutput(4777)); BOOST_CHECK(txBuilder.CouldAddOutput(0)); BOOST_CHECK(!txBuilder.CouldAddOutput(-1)); - BOOST_CHECK(txBuilder.CouldAddOutputs({1000, 1000, 2708})); + BOOST_CHECK(txBuilder.CouldAddOutputs({1000, 1000, 2708})); // (4810 - 34 * 3) split in 3 outputs BOOST_CHECK(!txBuilder.CouldAddOutputs({1000, 1000, 2709})); BOOST_CHECK_EQUAL(txBuilder.AddOutput(5000), nullptr); From dde64e3a7712e882c238a9f09fb155c868f780eb Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Thu, 27 Aug 2020 04:18:26 +0300 Subject: [PATCH 7/7] nit: better readability (imo) --- src/test/privatesend_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/privatesend_tests.cpp b/src/test/privatesend_tests.cpp index 107db33f310c..0275e0905357 100644 --- a/src/test/privatesend_tests.cpp +++ b/src/test/privatesend_tests.cpp @@ -121,7 +121,7 @@ BOOST_FIXTURE_TEST_CASE(CTransactionBuilderTest, CTransactionBuilderTestSetup) CTransactionBuilder txBuilder(wallet.get(), tallyItem); BOOST_CHECK_EQUAL(txBuilder.CountOutputs(), 0); - BOOST_CHECK_EQUAL(tallyItem.nAmount, txBuilder.GetAmountInitial()); + BOOST_CHECK_EQUAL(txBuilder.GetAmountInitial(), tallyItem.nAmount); BOOST_CHECK_EQUAL(txBuilder.GetAmountLeft(), 4810); // 5000 - 190 BOOST_CHECK(txBuilder.CouldAddOutput(4776)); // 4810 - 34