diff options
author | fanquake <fanquake@gmail.com> | 2023-11-03 14:35:30 +0000 |
---|---|---|
committer | fanquake <fanquake@gmail.com> | 2023-11-03 14:41:17 +0000 |
commit | 5d9f45082bda426340dada89b03d0bee96dd2c42 (patch) | |
tree | e84cdf038f48039a9d664c41289e75a3a4ed411d /src/test | |
parent | f23ac10ca5a322e87664b58233dccc4cb74c0570 (diff) | |
parent | b5a60abe8783852f5b31bc1e63b5836530410e65 (diff) | |
download | bitcoin-5d9f45082bda426340dada89b03d0bee96dd2c42.tar.xz |
Merge bitcoin/bitcoin#28758: refactors for subpackage evaluation
b5a60abe8783852f5b31bc1e63b5836530410e65 MOVEONLY: CleanupTemporaryCoins into its own function (glozow)
10c0a8678cd28e7f0715e6cfa3e651903e4ad4aa [test util] CreateValidTransaction multi-in/out, configurable feerate, signal BIP125 (glozow)
6ff647a7e0d85040a6033047c5cf84f8f22b1c65 scripted-diff: rename CheckPackage to IsWellFormedPackage (glozow)
da9aceba217bbded6909f06144eaa1e1a4ebcb69 [refactor] move package checks into helper functions (glozow)
Pull request description:
This is part of #27463. It splits off the more trivial changes from #26711 for ease of review, as requested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253.
- Split package sanitization in policy/packages.h into helper functions
- Add some tests for its quirks (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1340521597)
- Rename `CheckPackage` to `IsPackageWellFormed`
- Improve the `CreateValidTransaction` unit test utility to:
- Configure the target feerate and return the fee paid
- Signal BIP125 on transactions to enable RBF tests
- Allow the specification of multiple inputs and outputs
- Move `CleanupTemporaryCoins` into its own function to be reused later without duplication
ACKs for top commit:
dergoegge:
Code review ACK b5a60abe8783852f5b31bc1e63b5836530410e65
instagibbs:
ACK b5a60abe8783852f5b31bc1e63b5836530410e65
Tree-SHA512: 39d67a5f0041e381f0d0f802a98ccffbff11e44daa3a49611189d6306b03f18613d5ff16c618898d490c97a216753e99e0db231ff14d327f92c17ae4d269cfec
Diffstat (limited to 'src/test')
-rw-r--r-- | src/test/txpackage_tests.cpp | 47 | ||||
-rw-r--r-- | src/test/util/setup_common.cpp | 103 | ||||
-rw-r--r-- | src/test/util/setup_common.h | 41 |
3 files changed, 155 insertions, 36 deletions
diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 4d9a5ef7f3..debefa7f93 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -9,6 +9,7 @@ #include <primitives/transaction.h> #include <script/script.h> #include <test/util/random.h> +#include <test/util/script.h> #include <test/util/setup_common.h> #include <validation.h> @@ -47,7 +48,7 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup) package_too_many.emplace_back(create_placeholder_tx(1, 1)); } PackageValidationState state_too_many; - BOOST_CHECK(!CheckPackage(package_too_many, state_too_many)); + BOOST_CHECK(!IsWellFormedPackage(package_too_many, state_too_many, /*require_sorted=*/true)); BOOST_CHECK_EQUAL(state_too_many.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state_too_many.GetRejectReason(), "package-too-many-transactions"); @@ -62,7 +63,7 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup) } BOOST_CHECK(package_too_large.size() <= MAX_PACKAGE_COUNT); PackageValidationState state_too_large; - BOOST_CHECK(!CheckPackage(package_too_large, state_too_large)); + BOOST_CHECK(!IsWellFormedPackage(package_too_large, state_too_large, /*require_sorted=*/true)); BOOST_CHECK_EQUAL(state_too_large.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state_too_large.GetRejectReason(), "package-too-large"); @@ -73,9 +74,39 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup) package_duplicate_txids_empty.emplace_back(MakeTransactionRef(empty_tx)); } PackageValidationState state_duplicates; - BOOST_CHECK(!CheckPackage(package_duplicate_txids_empty, state_duplicates)); + BOOST_CHECK(!IsWellFormedPackage(package_duplicate_txids_empty, state_duplicates, /*require_sorted=*/true)); BOOST_CHECK_EQUAL(state_duplicates.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state_duplicates.GetRejectReason(), "package-contains-duplicates"); + BOOST_CHECK(!IsConsistentPackage(package_duplicate_txids_empty)); + + // Packages can't have transactions spending the same prevout + CMutableTransaction tx_zero_1; + CMutableTransaction tx_zero_2; + COutPoint same_prevout{InsecureRand256(), 0}; + tx_zero_1.vin.emplace_back(same_prevout); + tx_zero_2.vin.emplace_back(same_prevout); + // Different vouts (not the same tx) + tx_zero_1.vout.emplace_back(CENT, P2WSH_OP_TRUE); + tx_zero_2.vout.emplace_back(2 * CENT, P2WSH_OP_TRUE); + Package package_conflicts{MakeTransactionRef(tx_zero_1), MakeTransactionRef(tx_zero_2)}; + BOOST_CHECK(!IsConsistentPackage(package_conflicts)); + // Transactions are considered sorted when they have no dependencies. + BOOST_CHECK(IsTopoSortedPackage(package_conflicts)); + PackageValidationState state_conflicts; + BOOST_CHECK(!IsWellFormedPackage(package_conflicts, state_conflicts, /*require_sorted=*/true)); + BOOST_CHECK_EQUAL(state_conflicts.GetResult(), PackageValidationResult::PCKG_POLICY); + BOOST_CHECK_EQUAL(state_conflicts.GetRejectReason(), "conflict-in-package"); + + // IsConsistentPackage only cares about conflicts between transactions, not about a transaction + // conflicting with itself (i.e. duplicate prevouts in vin). + CMutableTransaction dup_tx; + const COutPoint rand_prevout{InsecureRand256(), 0}; + dup_tx.vin.emplace_back(rand_prevout); + dup_tx.vin.emplace_back(rand_prevout); + Package package_with_dup_tx{MakeTransactionRef(dup_tx)}; + BOOST_CHECK(IsConsistentPackage(package_with_dup_tx)); + package_with_dup_tx.emplace_back(create_placeholder_tx(1, 1)); + BOOST_CHECK(IsConsistentPackage(package_with_dup_tx)); } BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) @@ -157,8 +188,8 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup) CTransactionRef tx_child = MakeTransactionRef(mtx_child); PackageValidationState state; - BOOST_CHECK(CheckPackage({tx_parent, tx_child}, state)); - BOOST_CHECK(!CheckPackage({tx_child, tx_parent}, state)); + BOOST_CHECK(IsWellFormedPackage({tx_parent, tx_child}, state, /*require_sorted=*/true)); + BOOST_CHECK(!IsWellFormedPackage({tx_child, tx_parent}, state, /*require_sorted=*/true)); BOOST_CHECK_EQUAL(state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted"); BOOST_CHECK(IsChildWithParents({tx_parent, tx_child})); @@ -186,7 +217,7 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup) package.push_back(MakeTransactionRef(child)); PackageValidationState state; - BOOST_CHECK(CheckPackage(package, state)); + BOOST_CHECK(IsWellFormedPackage(package, state, /*require_sorted=*/true)); BOOST_CHECK(IsChildWithParents(package)); BOOST_CHECK(IsChildWithParentsTree(package)); @@ -224,8 +255,8 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup) BOOST_CHECK(!IsChildWithParentsTree({tx_parent, tx_parent_also_child, tx_child})); // IsChildWithParents does not detect unsorted parents. BOOST_CHECK(IsChildWithParents({tx_parent_also_child, tx_parent, tx_child})); - BOOST_CHECK(CheckPackage({tx_parent, tx_parent_also_child, tx_child}, state)); - BOOST_CHECK(!CheckPackage({tx_parent_also_child, tx_parent, tx_child}, state)); + BOOST_CHECK(IsWellFormedPackage({tx_parent, tx_parent_also_child, tx_child}, state, /*require_sorted=*/true)); + BOOST_CHECK(!IsWellFormedPackage({tx_parent_also_child, tx_parent, tx_child}, state, /*require_sorted=*/true)); BOOST_CHECK_EQUAL(state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted"); } diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index e70c105c8a..6ae94a8101 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -49,6 +49,7 @@ #include <txdb.h> #include <txmempool.h> #include <util/chaintype.h> +#include <util/rbf.h> #include <util/strencodings.h> #include <util/string.h> #include <util/thread.h> @@ -336,56 +337,106 @@ CBlock TestChain100Setup::CreateAndProcessBlock( return block; } - -CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactionRef input_transaction, - int input_vout, - int input_height, - CKey input_signing_key, - CScript output_destination, - CAmount output_amount, - bool submit) +std::pair<CMutableTransaction, CAmount> TestChain100Setup::CreateValidTransaction(const std::vector<CTransactionRef>& input_transactions, + const std::vector<COutPoint>& inputs, + int input_height, + const std::vector<CKey>& input_signing_keys, + const std::vector<CTxOut>& outputs, + const std::optional<CFeeRate>& feerate, + const std::optional<uint32_t>& fee_output) { - // Transaction we will submit to the mempool CMutableTransaction mempool_txn; + mempool_txn.vin.reserve(inputs.size()); + mempool_txn.vout.reserve(outputs.size()); - // Create an input - COutPoint outpoint_to_spend(input_transaction->GetHash(), input_vout); - CTxIn input(outpoint_to_spend); - mempool_txn.vin.push_back(input); - - // Create an output - CTxOut output(output_amount, output_destination); - mempool_txn.vout.push_back(output); + for (const auto& outpoint : inputs) { + mempool_txn.vin.emplace_back(outpoint, CScript(), MAX_BIP125_RBF_SEQUENCE); + } + mempool_txn.vout = outputs; - // Sign the transaction // - Add the signing key to a keystore FillableSigningProvider keystore; - keystore.AddKey(input_signing_key); + for (const auto& input_signing_key : input_signing_keys) { + keystore.AddKey(input_signing_key); + } // - Populate a CoinsViewCache with the unspent output CCoinsView coins_view; CCoinsViewCache coins_cache(&coins_view); - AddCoins(coins_cache, *input_transaction.get(), input_height); - // - Use GetCoin to properly populate utxo_to_spend, - Coin utxo_to_spend; - assert(coins_cache.GetCoin(outpoint_to_spend, utxo_to_spend)); - // - Then add it to a map to pass in to SignTransaction + for (const auto& input_transaction : input_transactions) { + AddCoins(coins_cache, *input_transaction.get(), input_height); + } + // Build Outpoint to Coin map for SignTransaction std::map<COutPoint, Coin> input_coins; - input_coins.insert({outpoint_to_spend, utxo_to_spend}); + CAmount inputs_amount{0}; + for (const auto& outpoint_to_spend : inputs) { + // - Use GetCoin to properly populate utxo_to_spend, + Coin utxo_to_spend; + assert(coins_cache.GetCoin(outpoint_to_spend, utxo_to_spend)); + input_coins.insert({outpoint_to_spend, utxo_to_spend}); + inputs_amount += utxo_to_spend.out.nValue; + } // - Default signature hashing type int nHashType = SIGHASH_ALL; std::map<int, bilingual_str> input_errors; assert(SignTransaction(mempool_txn, &keystore, input_coins, nHashType, input_errors)); + CAmount current_fee = inputs_amount - std::accumulate(outputs.begin(), outputs.end(), CAmount(0), + [](const CAmount& acc, const CTxOut& out) { + return acc + out.nValue; + }); + // Deduct fees from fee_output to meet feerate if set + if (feerate.has_value()) { + assert(fee_output.has_value()); + assert(fee_output.value() < mempool_txn.vout.size()); + CAmount target_fee = feerate.value().GetFee(GetVirtualTransactionSize(CTransaction{mempool_txn})); + CAmount deduction = target_fee - current_fee; + if (deduction > 0) { + // Only deduct fee if there's anything to deduct. If the caller has put more fees than + // the target feerate, don't change the fee. + mempool_txn.vout[fee_output.value()].nValue -= deduction; + // Re-sign since an output has changed + input_errors.clear(); + assert(SignTransaction(mempool_txn, &keystore, input_coins, nHashType, input_errors)); + current_fee = target_fee; + } + } + return {mempool_txn, current_fee}; +} +CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(const std::vector<CTransactionRef>& input_transactions, + const std::vector<COutPoint>& inputs, + int input_height, + const std::vector<CKey>& input_signing_keys, + const std::vector<CTxOut>& outputs, + bool submit) +{ + CMutableTransaction mempool_txn = CreateValidTransaction(input_transactions, inputs, input_height, input_signing_keys, outputs, std::nullopt, std::nullopt).first; // If submit=true, add transaction to the mempool. if (submit) { LOCK(cs_main); const MempoolAcceptResult result = m_node.chainman->ProcessTransaction(MakeTransactionRef(mempool_txn)); assert(result.m_result_type == MempoolAcceptResult::ResultType::VALID); } - return mempool_txn; } +CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactionRef input_transaction, + uint32_t input_vout, + int input_height, + CKey input_signing_key, + CScript output_destination, + CAmount output_amount, + bool submit) +{ + COutPoint input{input_transaction->GetHash(), input_vout}; + CTxOut output{output_amount, output_destination}; + return CreateValidMempoolTransaction(/*input_transactions=*/{input_transaction}, + /*inputs=*/{input}, + /*input_height=*/input_height, + /*input_signing_keys=*/{input_signing_key}, + /*outputs=*/{output}, + /*submit=*/submit); +} + std::vector<CTransactionRef> TestChain100Setup::PopulateMempool(FastRandomContext& det_rand, size_t num_transactions, bool submit) { std::vector<CTransactionRef> mempool_transactions; diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index 7cd4fdb417..4e1a26f303 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -124,7 +124,44 @@ struct TestChain100Setup : public TestingSetup { void mineBlocks(int num_blocks); /** - * Create a transaction and submit to the mempool. + * Create a transaction, optionally setting the fee based on the feerate. + * Note: The feerate may not be met exactly depending on whether the signatures can have different sizes. + * + * @param input_transactions The transactions to spend + * @param inputs Outpoints with which to construct transaction vin. + * @param input_height The height of the block that included the input transactions. + * @param input_signing_keys The keys to spend the input transactions. + * @param outputs Transaction vout. + * @param feerate The feerate the transaction should pay. + * @param fee_output The index of the output to take the fee from. + * @return The transaction and the fee it pays + */ + std::pair<CMutableTransaction, CAmount> CreateValidTransaction(const std::vector<CTransactionRef>& input_transactions, + const std::vector<COutPoint>& inputs, + int input_height, + const std::vector<CKey>& input_signing_keys, + const std::vector<CTxOut>& outputs, + const std::optional<CFeeRate>& feerate, + const std::optional<uint32_t>& fee_output); + /** + * Create a transaction and, optionally, submit to the mempool. + * + * @param input_transactions The transactions to spend + * @param inputs Outpoints with which to construct transaction vin. + * @param input_height The height of the block that included the input transaction(s). + * @param input_signing_keys The keys to spend inputs. + * @param outputs Transaction vout. + * @param submit Whether or not to submit to mempool + */ + CMutableTransaction CreateValidMempoolTransaction(const std::vector<CTransactionRef>& input_transactions, + const std::vector<COutPoint>& inputs, + int input_height, + const std::vector<CKey>& input_signing_keys, + const std::vector<CTxOut>& outputs, + bool submit = true); + + /** + * Create a 1-in-1-out transaction and, optionally, submit to the mempool. * * @param input_transaction The transaction to spend * @param input_vout The vout to spend from the input_transaction @@ -135,7 +172,7 @@ struct TestChain100Setup : public TestingSetup { * @param submit Whether or not to submit to mempool */ CMutableTransaction CreateValidMempoolTransaction(CTransactionRef input_transaction, - int input_vout, + uint32_t input_vout, int input_height, CKey input_signing_key, CScript output_destination, |