From f557dc48e55969e835dd6172c4d1cf31672808f3 Mon Sep 17 00:00:00 2001 From: MacroFake Date: Mon, 16 May 2022 10:49:40 +0200 Subject: [PATCH 01/12] Merge bitcoin/bitcoin#25067: validationinterface: make MainSignalsInstance() a class, drop unused forward declarations ca1ac1f0e0fbabbe97666aca94024afb8104da06 scripted-diff: Rename MainSignalsInstance() class to MainSignalsImpl() (Jon Atack) 2aaec2352d14753e1f2d4bf4b11bbe6ad42edf5c refactor: remove unused forward declarations in validationinterface.h (Jon Atack) 23854f84024c292164ce49b3fefee2a9b4fe0831 refactor: make MainSignalsInstance() a class (Jon Atack) Pull request description: Make MainSignalsInstance a class, rename it to MainSignalsImpl, use Doxygen documentation for it, and remove no longer used forward declarations in src/validationinterface.h. ---- MainSignalsInstance was created in 3a19fed9db5 and originally was a collection of boost::signals methods moved to validationinterface.cpp, in order to no longer need to include boost/signals in validationinterface.h. MainSignalsInstance then evolved in d6815a23131 to become class-like: - [C.8: Use class rather than struct if any member is non-public](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-class) - [C.2: Use class if the class has an invariant; use struct if the data members can vary independently](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c2-use-class-if-the-class-has-an-invariant-use-struct-if-the-data-members-can-vary-independently) - A class has the advantage of default private access, as opposed to public for a struct. ACKs for top commit: furszy: Code ACK ca1ac1f promag: Code review ACK ca1ac1f0e0fbabbe97666aca94024afb8104da06. danielabrozzoni: Code review ACK ca1ac1f0e0fbabbe97666aca94024afb8104da06 Tree-SHA512: 25f85e2b582fe8c269e6cf384a4aa29350b97ea6477edf3c63591e4f68a97364f7fb2fc4ad2764f61ff86b27353e31d2f12eed7a23368a247e4cf271c7e133ea --- src/masternode/node.h | 1 + src/validationinterface.cpp | 22 ++++++++++++---------- src/validationinterface.h | 6 ++---- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/masternode/node.h b/src/masternode/node.h index 931233c42f72f..5e4d94bc352a9 100644 --- a/src/masternode/node.h +++ b/src/masternode/node.h @@ -11,6 +11,7 @@ #include #include +class CConnman; class CDeterministicMNManager; struct CActiveMasternodeInfo { diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 88668ea79bbee..a71c7ab1f3c59 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -19,14 +19,16 @@ std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept; -//! The MainSignalsInstance manages a list of shared_ptr -//! callbacks. -//! -//! A std::unordered_map is used to track what callbacks are currently -//! registered, and a std::list is to used to store the callbacks that are -//! currently registered as well as any callbacks that are just unregistered -//! and about to be deleted when they are done executing. -struct MainSignalsInstance { +/** + * MainSignalsImpl manages a list of shared_ptr callbacks. + * + * A std::unordered_map is used to track what callbacks are currently + * registered, and a std::list is used to store the callbacks that are + * currently registered as well as any callbacks that are just unregistered + * and about to be deleted when they are done executing. + */ +class MainSignalsImpl +{ private: Mutex m_mutex; //! List entries consist of a callback pointer and reference count. The @@ -43,7 +45,7 @@ struct MainSignalsInstance { // our own queue here :( SingleThreadedSchedulerClient m_schedulerClient; - explicit MainSignalsInstance(CScheduler& scheduler LIFETIMEBOUND) : m_schedulerClient(scheduler) {} + explicit MainSignalsImpl(CScheduler& scheduler LIFETIMEBOUND) : m_schedulerClient(scheduler) {} void Register(std::shared_ptr callbacks) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) { @@ -95,7 +97,7 @@ static CMainSignals g_signals; void CMainSignals::RegisterBackgroundSignalScheduler(CScheduler& scheduler) { assert(!m_internals); - m_internals = std::make_unique(scheduler); + m_internals = std::make_unique(scheduler); } void CMainSignals::UnregisterBackgroundSignalScheduler() diff --git a/src/validationinterface.h b/src/validationinterface.h index dc20388ddfc62..271c76f88fa54 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -17,12 +17,10 @@ class BlockValidationState; class CBlock; class CBlockIndex; struct CBlockLocator; -class CConnman; class CValidationInterface; class CGovernanceVote; class CDeterministicMNList; class CDeterministicMNListDiff; -class uint256; class CScheduler; enum class MemPoolRemovalReason; namespace chainlock { @@ -204,10 +202,10 @@ class CValidationInterface { friend class ValidationInterfaceTest; }; -struct MainSignalsInstance; +class MainSignalsImpl; class CMainSignals { private: - std::unique_ptr m_internals; + std::unique_ptr m_internals; friend void ::RegisterSharedValidationInterface(std::shared_ptr); friend void ::UnregisterValidationInterface(CValidationInterface*); From 796fea71743aa69872d3701c22c174dff88f32e4 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 16 May 2022 14:28:31 -0400 Subject: [PATCH 02/12] Merge bitcoin/bitcoin#23662: rpc: improve `getreceivedby{address,label}` performance f336ff7f213564909cf5f9742618cc6ec87600fd rpc: avoid expensive `IsMine` calls in `GetReceived` tally (Sebastian Falbesoner) a7b65af2a4450663d4a20a617c59dac87b34fb36 rpc: avoid scriptPubKey<->CTxDestination conversions in `GetReceived` tally (Sebastian Falbesoner) Pull request description: The RPC calls `getreceivedbyaddress`/`getreceivedbylabel` both use the internal helper function `GetReceived` which was introduced in PR #17579 to deduplicate tallying code. For every wallet-related transaction output, the following unnecessary operations are currently performed in the tally loop, leading to a quite bad performance (as reported in #23645): - converting from CScript -> TxDestination (`ExtractDestination(...)`), converting from TxDestination -> CScript (`CWallet::IsMine(const CTxDestination& dest)`); this can be avoided by directly using output scripts in the search set instead of addresses (first commit) - checking if the iterated output script belongs to the wallet by calling `IsMine`; this can be avoided by only adding addresses to the search set which fulfil `IsMine` in the first place (second commit) ### Benchmark results The functional test [wallet_pr23662.py](https://github.com/theStack/bitcoin/blob/pr23662_benchmarks/test/functional/wallet_pr23662.py) (not part of this PR) creates transactions with 15000 different addresses: - 5000 outputs (500 txs with 10 outputs each) with label set, IsMine set (received) - 5000 outputs (500 txs with 10 outputs each) with label set, IsMine not set (sent) - 5000 outputs (500 txs with 10 outputs each) without label set, IsMine not set (sent) Then, the time is measured for calling `getreceivedbyaddress` and `getreceivedbylabel`, the latter with two variants. Results on my machine: | branch | `getreceivedbyaddress` (single) | `getreceivedbylabel` (single) | `getreceivedbylabel` (10000) | |--------------------|---------------------------------|-------------------------------|------------------------------| | master | 406.13ms | 425.33ms | 446.58ms | | PR (first commit) | 367.18ms | 365.81ms | 426.33ms | | PR (second commit) | 3.96ms | 4.83ms | 339.69ms | Fixes #23645. ACKs for top commit: achow101: ACK f336ff7f213564909cf5f9742618cc6ec87600fd w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/23662/commits/f336ff7f213564909cf5f9742618cc6ec87600fd furszy: Code ACK f336ff7f Tree-SHA512: 9cbf402b9e269713bd3feda9e31719d9dca8a0dfd526de12fd3d561711589195d0c50143432c65dae279c4eab90a4fc3f99e29fbc0452fefe05113e92d129b8f --- src/wallet/rpc/coins.cpp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 84accf15ce1ba..369b506c4e568 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -17,12 +17,17 @@ namespace wallet { static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { - std::vector address_vector; + std::vector output_scripts; if (by_label) { // Get the set of addresses assigned to label std::string label = LabelFromValue(params[0]); - address_vector = wallet.ListAddrBookAddresses(CWallet::AddrBookFilter{label}); + for (const auto& address : wallet.ListAddrBookAddresses(CWallet::AddrBookFilter{label})) { + auto output_script{GetScriptForDestination(address)}; + if (wallet.IsMine(output_script)) { + output_scripts.emplace_back(output_script); + } + } } else { // Get the address CTxDestination dest = DecodeDestination(params[0].get_str()); @@ -33,7 +38,7 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b if (!wallet.IsMine(script_pub_key)) { throw JSONRPCError(RPC_WALLET_ERROR, "Address not found in wallet"); } - address_vector.emplace_back(dest); + output_scripts.emplace_back(script_pub_key); } // Minimum confirmations @@ -66,8 +71,7 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b if (depth < min_depth && !(fAddLocked && wallet.IsTxLockedByInstantSend(wtx))) continue; for (const CTxOut& txout : wtx.tx->vout) { - CTxDestination address; - if (ExtractDestination(txout.scriptPubKey, address) && wallet.IsMine(address) && std::find(address_vector.begin(), address_vector.end(), address) != address_vector.end()) { + if (std::find(output_scripts.begin(), output_scripts.end(), txout.scriptPubKey) != output_scripts.end()) { amount += txout.nValue; } } From 844eaafc9474032d812fd4622f872a0fc0c27310 Mon Sep 17 00:00:00 2001 From: fanquake Date: Thu, 7 Apr 2022 09:43:25 +0100 Subject: [PATCH 03/12] Merge bitcoin/bitcoin#24152: policy / validation: CPFP fee bumping within packages BACKPORT NOTE: This PR is backported to minimize conflicts in future even if Dash Core has no bumpfee feature 9bebf35e269b2a918df27708565ecd0c5bd3f116 [validation] don't package validate if not policy or missing inputs (glozow) 51edcffa0e156dba06191a8d5c636ba01fa5b65f [unit test] package feerate and package cpfp (glozow) 1b93748c937e870e7574a8e120a85bee6f9013ff [validation] try individual validation before package validation (glozow) 17a8ffd8020375d60428695858558f2be264aa36 [packages/policy] use package feerate in package validation (glozow) 09f32cffa6c3e8b2d77281a5983ffe8f482a5945 [docs] package feerate (glozow) Pull request description: Part of #22290, aka [Package Mempool Accept](https://gist.github.com/glozow/dc4e9d5c5b14ade7cdfac40f43adb18a). This enables CPFP fee bumping in child-with-unconfirmed-parents packages by introducing [package feerate](https://gist.github.com/glozow/dc4e9d5c5b14ade7cdfac40f43adb18a#fee-related-checks-use-package-feerate) (total modified fees divided by total virtual size) and using it in place of individual feerate. We also always [validate individual transactions first](https://gist.github.com/glozow/dc4e9d5c5b14ade7cdfac40f43adb18a#always-try-individual-submission-first) to avoid incentive-incompatible policies like "parents pay for children" or "siblings pay for siblings" behavior. ACKs for top commit: instagibbs: reACK https://github.com/bitcoin/bitcoin/pull/24152/commits/9bebf35e269b2a918df27708565ecd0c5bd3f116 mzumsande: Code review ACK 9bebf35e269b2a918df27708565ecd0c5bd3f116 t-bast: ACK https://github.com/bitcoin/bitcoin/pull/24152/commits/9bebf35e269b2a918df27708565ecd0c5bd3f116 Tree-SHA512: 5117cfcc3ce55c00384d9e8003a0589ceac1e6f738b1c299007d9cd9cdd2d7c530d31cfd23658b041a6604d39073bcc6e81f0639a300082a92097682a6ea8c8f --- doc/policy/packages.md | 37 +++++++ src/test/fuzz/tx_pool.cpp | 14 ++- src/test/txpackage_tests.cpp | 209 ++++++++++++++++++++++++++++++++++- src/validation.cpp | 88 +++++++++++++-- src/validation.h | 8 ++ 5 files changed, 337 insertions(+), 19 deletions(-) diff --git a/doc/policy/packages.md b/doc/policy/packages.md index 1e2ddbd82f964..aa720050588ca 100644 --- a/doc/policy/packages.md +++ b/doc/policy/packages.md @@ -60,3 +60,40 @@ test accepts): - *Rationale*: We want to prevent potential censorship vectors. We should not reject entire packages because we already have one of the transactions. Also, if an attacker first broadcasts a competing package, the honest package should still be considered for acceptance. + +### Package Fees and Feerate + +*Package Feerate* is the total modified fees (base fees + any fee delta from +`prioritisetransaction`) divided by the total size of all transactions in the package. +If any transactions in the package are already in the mempool, they are not submitted again +("deduplicated") and are thus excluded from this calculation. + +To meet the two feerate requirements of a mempool, i.e., the pre-configured minimum relay feerate +(`minRelayTxFee`) and the dynamic mempool minimum feerate, the total package feerate is used instead +of the individual feerate. The individual transactions are allowed to be below the feerate +requirements if the package meets the feerate requirements. For example, the parent(s) in the +package can pay no fees but be paid for by the child. + +*Rationale*: This can be thought of as "CPFP within a package," solving the issue of a parent not +meeting minimum fees on its own. This would allow contracting applications to adjust their fees at +broadcast time instead of overshooting or risking becoming stuck or pinned. + +*Rationale*: It would be incorrect to use the fees of transactions that are already in the mempool, as +we do not want a transaction's fees to be double-counted. + +Implementation Note: Transactions within a package are always validated individually first, and +package validation is used for the transactions that failed. Since package feerate is only +calculated using transactions that are not in the mempool, this implementation detail affects the +outcome of package validation. + +*Rationale*: We must not allow a low-feerate child to prevent its parent from being accepted; fees +of children should not negatively impact their parents, since they are not necessary for the parents +to be mined. More generally, if transaction B is not needed in order for transaction A to be mined, +B's fees cannot harm A. In a child-with-parents package, simply validating parents individually +first is sufficient to ensure this. + +*Rationale*: As a principle, we want to avoid accidentally restricting policy in order to be +backward-compatible for users and applications that rely on p2p transaction relay. Concretely, +package validation should not prevent the acceptance of a transaction that would otherwise be +policy-valid on its own. By always accepting a transaction that passes individual validation before +trying package validation, we prevent any unintentional restriction of policy. diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index adb1a170440df..bb7718dbc70f8 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -233,14 +233,18 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool) const bool bypass_limits = fuzzed_data_provider.ConsumeBool(); ::fRequireStandard = fuzzed_data_provider.ConsumeBool(); - // Make sure ProcessNewPackage on one transaction works and always fully validates the transaction. + // Make sure ProcessNewPackage on one transaction works. // The result is not guaranteed to be the same as what is returned by ATMP. const auto result_package = WITH_LOCK(::cs_main, return ProcessNewPackage(chainstate, tx_pool, {tx}, true)); - auto it = result_package.m_tx_results.find(tx->GetHash()); - Assert(it != result_package.m_tx_results.end()); - Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID || - it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID); + // If something went wrong due to a package-specific policy, it might not return a + // validation result for the transaction. + if (result_package.m_state.GetResult() != PackageValidationResult::PCKG_POLICY) { + auto it = result_package.m_tx_results.find(tx->GetHash()); + Assert(it != result_package.m_tx_results.end()); + Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID || + it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID); + } const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx, GetTime(), bypass_limits, /*test_accept=*/false)); const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID; diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index e645a71776f72..20b30657d6662 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -81,7 +81,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100NoDIP0001Setup) auto mtx_parent = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[0], /*input_vout=*/0, /*input_height=*/0, /*input_signing_key=*/coinbaseKey, /*output_destination=*/parent_locking_script, - /*output_amount=*/CAmount(49 * COIN), /*submit=*/false); + /*output_amount=*/CAmount(499 * COIN), /*submit=*/false); CTransactionRef tx_parent = MakeTransactionRef(mtx_parent); CKey child_key; @@ -90,7 +90,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100NoDIP0001Setup) auto mtx_child = CreateValidMempoolTransaction(/*input_transaction=*/tx_parent, /*input_vout=*/0, /*input_height=*/101, /*input_signing_key=*/parent_key, /*output_destination=*/child_locking_script, - /*output_amount=*/CAmount(48 * COIN), /*submit=*/false); + /*output_amount=*/CAmount(498 * COIN), /*submit=*/false); CTransactionRef tx_child = MakeTransactionRef(mtx_child); const auto result_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, {tx_parent, tx_child}, /*test_accept=*/true); BOOST_CHECK_MESSAGE(result_parent_child.m_state.IsValid(), @@ -103,6 +103,9 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100NoDIP0001Setup) BOOST_CHECK(it_child != result_parent_child.m_tx_results.end()); BOOST_CHECK_MESSAGE(it_child->second.m_state.IsValid(), "Package validation unexpectedly failed: " << it_child->second.m_state.GetRejectReason()); + BOOST_CHECK(result_parent_child.m_package_feerate.has_value()); + BOOST_CHECK(result_parent_child.m_package_feerate.value() == + CFeeRate(2 * COIN, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child))); // A single, giant transaction submitted through ProcessNewPackage fails on single tx policy. CTransactionRef giant_ptx = create_placeholder_tx(999, 999); @@ -114,6 +117,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100NoDIP0001Setup) auto it_giant_tx = result_single_large.m_tx_results.find(giant_ptx->GetHash()); BOOST_CHECK(it_giant_tx != result_single_large.m_tx_results.end()); BOOST_CHECK_EQUAL(it_giant_tx->second.m_state.GetRejectReason(), "tx-size"); + BOOST_CHECK(result_single_large.m_package_feerate == std::nullopt); // Check that mempool size hasn't changed. BOOST_CHECK_EQUAL(m_node.mempool->size(), initialPoolSize); @@ -234,6 +238,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100NoDIP0001Setup) BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetRejectReason(), "package-not-child-with-parents"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + BOOST_CHECK(result_unrelated_submit.m_package_feerate == std::nullopt); // Parent and Child (and Grandchild) Package Package package_parent_child; @@ -275,6 +280,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100NoDIP0001Setup) BOOST_CHECK_EQUAL(result_3gen_submit.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(result_3gen_submit.m_state.GetRejectReason(), "package-not-child-with-parents"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + BOOST_CHECK(result_3gen_submit.m_package_feerate == std::nullopt); } // Child with missing parent. @@ -290,6 +296,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100NoDIP0001Setup) BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetRejectReason(), "package-not-child-with-unconfirmed-parents"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + BOOST_CHECK(result_missing_parent.m_package_feerate == std::nullopt); } // Submit package with parent + child. @@ -309,6 +316,10 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100NoDIP0001Setup) BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); BOOST_CHECK(m_node.mempool->exists(tx_parent->GetHash())); BOOST_CHECK(m_node.mempool->exists(tx_child->GetHash())); + + // Since both transactions have high feerates, they each passed validation individually. + // Package validation was unnecessary, so there is no package feerate. + BOOST_CHECK(submit_parent_child.m_package_feerate == std::nullopt); } // Already-in-mempool transactions should be detected and de-duplicated. @@ -329,6 +340,200 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100NoDIP0001Setup) BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); BOOST_CHECK(m_node.mempool->exists(tx_parent->GetHash())); BOOST_CHECK(m_node.mempool->exists(tx_child->GetHash())); + + BOOST_CHECK(submit_deduped.m_package_feerate == std::nullopt); + } +} + +BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) +{ + mineBlocks(5); + LOCK(::cs_main); + size_t expected_pool_size = m_node.mempool->size(); + CKey child_key; + child_key.MakeNewKey(true); + CScript parent_spk = GetScriptForDestination(PKHash(child_key.GetPubKey())); + CKey grandchild_key; + grandchild_key.MakeNewKey(true); + CScript child_spk = GetScriptForDestination(PKHash(grandchild_key.GetPubKey())); + + // zero-fee parent and high-fee child package + const CAmount coinbase_value{500 * COIN}; + const CAmount parent_value{coinbase_value - 0}; + const CAmount child_value{parent_value - COIN}; + + Package package_cpfp; + auto mtx_parent = CreateValidMempoolTransaction(/*input_transaction=*/ m_coinbase_txns[0], /*vout=*/ 0, + /*input_height=*/ 0, /*input_signing_key=*/ coinbaseKey, + /*output_destination=*/ parent_spk, + /*output_amount=*/ parent_value, /*submit=*/ false); + CTransactionRef tx_parent = MakeTransactionRef(mtx_parent); + package_cpfp.push_back(tx_parent); + + auto mtx_child = CreateValidMempoolTransaction(/*input_transaction=*/ tx_parent, /*vout=*/ 0, + /*input_height=*/ 101, /*input_signing_key=*/ child_key, + /*output_destination=*/ child_spk, + /*output_amount=*/ child_value, /*submit=*/ false); + CTransactionRef tx_child = MakeTransactionRef(mtx_child); + package_cpfp.push_back(tx_child); + + // Package feerate is calculated using modified fees, and prioritisetransaction accepts negative + // fee deltas. This should be taken into account. De-prioritise the parent transaction by -1BTC, + // bringing the package feerate to 0. + m_node.mempool->PrioritiseTransaction(tx_parent->GetHash(), -1 * COIN); + { + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + const auto submit_cpfp_deprio = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + package_cpfp, /*test_accept=*/ false); + BOOST_CHECK_MESSAGE(submit_cpfp_deprio.m_state.IsInvalid(), + "Package validation unexpectedly succeeded: " << submit_cpfp_deprio.m_state.GetRejectReason()); + BOOST_CHECK(submit_cpfp_deprio.m_tx_results.empty()); + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + const CFeeRate expected_feerate(0, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child)); + BOOST_CHECK(submit_cpfp_deprio.m_package_feerate.has_value()); + BOOST_CHECK(submit_cpfp_deprio.m_package_feerate.value() == CFeeRate{0}); + BOOST_CHECK_MESSAGE(submit_cpfp_deprio.m_package_feerate.value() == expected_feerate, + strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), + submit_cpfp_deprio.m_package_feerate.value().ToString())); + } + + // Clear the prioritisation of the parent transaction. + WITH_LOCK(m_node.mempool->cs, m_node.mempool->ClearPrioritisation(tx_parent->GetHash())); + + // Package CPFP: Even though the parent pays 0 absolute fees, the child pays 1 BTC which is + // enough for the package feerate to meet the threshold. + { + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + const auto submit_cpfp = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + package_cpfp, /*test_accept=*/ false); + expected_pool_size += 2; + BOOST_CHECK_MESSAGE(submit_cpfp.m_state.IsValid(), + "Package validation unexpectedly failed: " << submit_cpfp.m_state.GetRejectReason()); + auto it_parent = submit_cpfp.m_tx_results.find(tx_parent->GetHash()); + auto it_child = submit_cpfp.m_tx_results.find(tx_child->GetHash()); + BOOST_CHECK(it_parent != submit_cpfp.m_tx_results.end()); + BOOST_CHECK(it_parent->second.m_result_type == MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK(it_parent->second.m_base_fees.value() == 0); + BOOST_CHECK(it_child != submit_cpfp.m_tx_results.end()); + BOOST_CHECK(it_child->second.m_result_type == MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK(it_child->second.m_base_fees.value() == COIN); + + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + BOOST_CHECK(m_node.mempool->exists(tx_parent->GetHash())); + BOOST_CHECK(m_node.mempool->exists(tx_child->GetHash())); + + const CFeeRate expected_feerate(coinbase_value - child_value, + GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child)); + BOOST_CHECK(expected_feerate.GetFeePerK() > 1000); + BOOST_CHECK(submit_cpfp.m_package_feerate.has_value()); + BOOST_CHECK_MESSAGE(submit_cpfp.m_package_feerate.value() == expected_feerate, + strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), + submit_cpfp.m_package_feerate.value().ToString())); + } + + // Just because we allow low-fee parents doesn't mean we allow low-feerate packages. + // This package just pays 200 satoshis total. This would be enough to pay for the child alone, + // but isn't enough for the entire package to meet the 1sat/vbyte minimum. + Package package_still_too_low; + auto mtx_parent_cheap = CreateValidMempoolTransaction(/*input_transaction=*/ m_coinbase_txns[1], /*vout=*/ 0, + /*input_height=*/ 0, /*input_signing_key=*/ coinbaseKey, + /*output_destination=*/ parent_spk, + /*output_amount=*/ coinbase_value, /*submit=*/ false); + CTransactionRef tx_parent_cheap = MakeTransactionRef(mtx_parent_cheap); + package_still_too_low.push_back(tx_parent_cheap); + + auto mtx_child_cheap = CreateValidMempoolTransaction(/*input_transaction=*/ tx_parent_cheap, /*vout=*/ 0, + /*input_height=*/ 101, /* input_signing_key */ child_key, + /*output_destination=*/ child_spk, + /*output_amount=*/ coinbase_value - 200, /*submit=*/ false); + CTransactionRef tx_child_cheap = MakeTransactionRef(mtx_child_cheap); + package_still_too_low.push_back(tx_child_cheap); + + // Cheap package should fail with package-fee-too-low. + { + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + const auto submit_package_too_low = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + package_still_too_low, /* test_accept */ false); + BOOST_CHECK_MESSAGE(submit_package_too_low.m_state.IsInvalid(), "Package validation unexpectedly succeeded"); + BOOST_CHECK_EQUAL(submit_package_too_low.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); + BOOST_CHECK_EQUAL(submit_package_too_low.m_state.GetRejectReason(), "package-fee-too-low"); + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + const CFeeRate child_feerate(200, GetVirtualTransactionSize(*tx_child_cheap)); + BOOST_CHECK(child_feerate.GetFeePerK() > 1000); + const CFeeRate expected_feerate(200, + GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap)); + BOOST_CHECK(expected_feerate.GetFeePerK() < 1000); + BOOST_CHECK(submit_package_too_low.m_package_feerate.has_value()); + BOOST_CHECK_MESSAGE(submit_package_too_low.m_package_feerate.value() == expected_feerate, + strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), + submit_package_too_low.m_package_feerate.value().ToString())); + } + + // Package feerate includes the modified fees of the transactions. + // This means a child with its fee delta from prioritisetransaction can pay for a parent. + m_node.mempool->PrioritiseTransaction(tx_child_cheap->GetHash(), 1 * COIN); + // Now that the child's fees have "increased" by 1 BTC, the cheap package should succeed. + { + const auto submit_prioritised_package = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + package_still_too_low, /*test_accept=*/ false); + expected_pool_size += 2; + BOOST_CHECK_MESSAGE(submit_prioritised_package.m_state.IsValid(), + "Package validation unexpectedly failed" << submit_prioritised_package.m_state.GetRejectReason()); + const CFeeRate expected_feerate(1 * COIN + 200, + GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap)); + BOOST_CHECK(submit_prioritised_package.m_package_feerate.has_value()); + BOOST_CHECK_MESSAGE(submit_prioritised_package.m_package_feerate.value() == expected_feerate, + strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), + submit_prioritised_package.m_package_feerate.value().ToString())); + } + + // Package feerate is calculated without topology in mind; it's just aggregating fees and sizes. + // However, this should not allow parents to pay for children. Each transaction should be + // validated individually first, eliminating sufficient-feerate parents before they are unfairly + // included in the package feerate. It's also important that the low-fee child doesn't prevent + // the parent from being accepted. + Package package_rich_parent; + const CAmount high_parent_fee{1 * COIN}; + auto mtx_parent_rich = CreateValidMempoolTransaction(/*input_transaction=*/ m_coinbase_txns[2], /*vout=*/ 0, + /*input_height=*/ 0, /*input_signing_key=*/ coinbaseKey, + /*output_destination=*/ parent_spk, + /*output_amount=*/ coinbase_value - high_parent_fee, /*submit=*/ false); + CTransactionRef tx_parent_rich = MakeTransactionRef(mtx_parent_rich); + package_rich_parent.push_back(tx_parent_rich); + + auto mtx_child_poor = CreateValidMempoolTransaction(/*input_transaction=*/ tx_parent_rich, /*vout=*/ 0, + /*input_height=*/ 101, /*input_signing_key=*/ child_key, + /*output_destination=*/ child_spk, + /*output_amount=*/ coinbase_value - high_parent_fee, /*submit=*/ false); + CTransactionRef tx_child_poor = MakeTransactionRef(mtx_child_poor); + package_rich_parent.push_back(tx_child_poor); + + // Parent pays 1 BTC and child pays none. The parent should be accepted without the child. + { + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + const auto submit_rich_parent = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + package_rich_parent, /* test_accept */ false); + expected_pool_size += 1; + BOOST_CHECK_MESSAGE(submit_rich_parent.m_state.IsInvalid(), "Package validation unexpectedly succeeded"); + + // The child would have been validated on its own and failed, then submitted as a "package" of 1. + // The package feerate is just the child's feerate, which is 0sat/vb. + BOOST_CHECK(submit_rich_parent.m_package_feerate.has_value()); + BOOST_CHECK_MESSAGE(submit_rich_parent.m_package_feerate.value() == CFeeRate(), + "expected 0, got " << submit_rich_parent.m_package_feerate.value().ToString()); + BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); + BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetRejectReason(), "package-fee-too-low"); + + auto it_parent = submit_rich_parent.m_tx_results.find(tx_parent_rich->GetHash()); + BOOST_CHECK(it_parent != submit_rich_parent.m_tx_results.end()); + BOOST_CHECK(it_parent->second.m_result_type == MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK(it_parent->second.m_state.GetRejectReason() == ""); + BOOST_CHECK_MESSAGE(it_parent->second.m_base_fees.value() == high_parent_fee, + strprintf("rich parent: expected fee %s, got %s", high_parent_fee, it_parent->second.m_base_fees.value())); + + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + BOOST_CHECK(m_node.mempool->exists(tx_parent_rich->GetHash())); + BOOST_CHECK(!m_node.mempool->exists(tx_child_poor->GetHash())); } } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/validation.cpp b/src/validation.cpp index f0292f45ff6e0..7a505d13135e3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -559,6 +559,10 @@ class MemPoolAccept * partially submitted. */ const bool m_package_submission; + /** When true, use package feerates instead of individual transaction feerates for fee-based + * policies such as mempool min fee and min relay fee. + */ + const bool m_package_feerates; /** Parameters for single transaction mempool validation. */ static ATMPArgs SingleAccept(const CChainParams& chainparams, int64_t accept_time, @@ -570,6 +574,7 @@ class MemPoolAccept /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ test_accept, /* m_package_submission */ false, + /* m_package_feerates */ false, }; } @@ -582,6 +587,7 @@ class MemPoolAccept /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ true, /* m_package_submission */ false, // not submitting to mempool + /* m_package_feerates */ false, }; } @@ -594,6 +600,19 @@ class MemPoolAccept /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ false, /* m_package_submission */ true, + /* m_package_feerates */ true, + }; + } + + /** Parameters for a single transaction within a package. */ + static ATMPArgs SingleInPackageAccept(const ATMPArgs& package_args) { + return ATMPArgs{/* m_chainparams */ package_args.m_chainparams, + /* m_accept_time */ package_args.m_accept_time, + /* m_bypass_limits */ false, + /* m_coins_to_uncache */ package_args.m_coins_to_uncache, + /* m_test_accept */ package_args.m_test_accept, + /* m_package_submission */ false, + /* m_package_feerates */ false, // only 1 transaction }; } @@ -605,13 +624,15 @@ class MemPoolAccept bool bypass_limits, std::vector& coins_to_uncache, bool test_accept, - bool package_submission) + bool package_submission, + bool package_feerates) : m_chainparams{chainparams}, m_accept_time{accept_time}, m_bypass_limits{bypass_limits}, m_coins_to_uncache{coins_to_uncache}, m_test_accept{test_accept}, - m_package_submission{package_submission} + m_package_submission{package_submission}, + m_package_feerates{package_feerates} { } }; @@ -892,12 +913,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "bad-txns-too-many-sigops", strprintf("%d", nSigOps)); - // No transactions are allowed below minRelayTxFee except from disconnected - // blocks + // No individual transactions are allowed below minRelayTxFee and mempool min fee except from + // disconnected blocks and transactions in a package. Package transactions will be checked using + // package feerate later. // Checking of fee for MNHF_SIGNAL should be skipped: mnhf does not have // inputs, outputs, or fee if (!tx.IsSpecialTxVersion() || tx.nType != TRANSACTION_MNHF_SIGNAL) { - if (!bypass_limits && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, state)) return false; + if (!bypass_limits && !args.m_package_feerates && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, state)) return false; } // Calculate in-mempool ancestors, up to a limit. @@ -1201,12 +1223,27 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: m_viewmempool.PackageAddTransaction(ws.m_ptx); } + // Transactions must meet two minimum feerates: the mempool minimum fee and min relay fee. + // For transactions consisting of exactly one child and its parents, it suffices to use the + // package feerate (total modified fees / total virtual size) to check this requirement. + const auto m_total_vsize = std::accumulate(workspaces.cbegin(), workspaces.cend(), int64_t{0}, + [](int64_t sum, auto& ws) { return sum + ws.m_vsize; }); + const auto m_total_modified_fees = std::accumulate(workspaces.cbegin(), workspaces.cend(), CAmount{0}, + [](CAmount sum, auto& ws) { return sum + ws.m_modified_fees; }); + const CFeeRate package_feerate(m_total_modified_fees, m_total_vsize); + TxValidationState placeholder_state; + if (args.m_package_feerates && + !CheckFeeRate(m_total_vsize, m_total_modified_fees, placeholder_state)) { + package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-fee-too-low"); + return PackageMempoolAcceptResult(package_state, package_feerate, {}); + } + // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction, // because it's unnecessary. Also, CPFP carve out can increase the limit for individual // transactions, but this exemption is not extended to packages in CheckPackageLimits(). std::string err_string; if (txns.size() > 1 && !PackageMempoolChecks(txns, package_state)) { - return PackageMempoolAcceptResult(package_state, std::move(results)); + return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); } for (Workspace& ws : workspaces) { @@ -1214,7 +1251,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // Exit early to avoid doing pointless work. Update the failed tx result; the rest are unfinished. package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); results.emplace(ws.m_ptx -> GetHash(), MempoolAcceptResult::Failure(ws.m_state)); - return PackageMempoolAcceptResult(package_state, std::move(results)); + return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); } if (args.m_test_accept) { // When test_accept=true, transactions that pass PolicyScriptChecks are valid because there are @@ -1224,14 +1261,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: } } - if (args.m_test_accept) return PackageMempoolAcceptResult(package_state, std::move(results)); + if (args.m_test_accept) return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); if (!SubmitPackage(args, workspaces, package_state, results)) { // PackageValidationState filled in by SubmitPackage(). - return PackageMempoolAcceptResult(package_state, std::move(results)); + return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); } - return PackageMempoolAcceptResult(package_state, std::move(results)); + return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); } PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, ATMPArgs& args) @@ -1298,6 +1335,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // transactions that are already in the mempool, and only call AcceptMultipleTransactions() with // the new transactions. This ensures we don't double-count transaction counts and sizes when // checking ancestor/descendant limits, or double-count transaction fees for fee-related policy. + ATMPArgs single_args = ATMPArgs::SingleInPackageAccept(args); + bool quit_early{false}; std::vector txns_new; for (const auto& tx : package) { const auto& txid = tx->GetHash(); @@ -1310,18 +1349,43 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, results.emplace(txid, MempoolAcceptResult::MempoolTx(iter.value()->GetTxSize(), iter.value()->GetFee())); } else { // Transaction does not already exist in the mempool. - txns_new.push_back(tx); + // Try submitting the transaction on its own. + const auto single_res = AcceptSingleTransaction(tx, single_args); + if (single_res.m_result_type == MempoolAcceptResult::ResultType::VALID) { + // The transaction succeeded on its own and is now in the mempool. Don't include it + // in package validation, because its fees should only be "used" once. + assert(m_pool.exists(txid)); + results.emplace(txid, single_res); + } else if (single_res.m_state.GetResult() != TxValidationResult::TX_MEMPOOL_POLICY && + single_res.m_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { + // Package validation policy only differs from individual policy in its evaluation + // of feerate. For example, if a transaction fails here due to violation of a + // consensus rule, the result will not change when it is submitted as part of a + // package. To minimize the amount of repeated work, unless the transaction fails + // due to feerate or missing inputs (its parent is a previous transaction in the + // package that failed due to feerate), don't run package validation. Note that this + // decision might not make sense if different types of packages are allowed in the + // future. Continue individually validating the rest of the transactions, because + // some of them may still be valid. + quit_early = true; + } else { + txns_new.push_back(tx); + } } } // Nothing to do if the entire package has already been submitted. - if (txns_new.empty()) return PackageMempoolAcceptResult(package_state, std::move(results)); + if (quit_early || txns_new.empty()) { + // No package feerate when no package validation was done. + return PackageMempoolAcceptResult(package_state, std::move(results)); + } // Validate the (deduplicated) transactions as a package. auto submission_result = AcceptMultipleTransactions(txns_new, args); // Include already-in-mempool transaction results in the final result. for (const auto& [txid, mempoolaccept_res] : results) { submission_result.m_tx_results.emplace(txid, mempoolaccept_res); } + if (submission_result.m_state.IsValid()) assert(submission_result.m_package_feerate.has_value()); return submission_result; } diff --git a/src/validation.h b/src/validation.h index 31318029cf8e7..72769a0562dc9 100644 --- a/src/validation.h +++ b/src/validation.h @@ -231,11 +231,19 @@ struct PackageMempoolAcceptResult * was a package-wide error (see result in m_state), m_tx_results will be empty. */ std::map m_tx_results; + /** Package feerate, defined as the aggregated modified fees divided by the total size + * of all transactions in the package. May be unavailable if some inputs were not available or + * a transaction failure caused validation to terminate early. */ + std::optional m_package_feerate; explicit PackageMempoolAcceptResult(PackageValidationState state, std::map&& results) : m_state{state}, m_tx_results(std::move(results)) {} + explicit PackageMempoolAcceptResult(PackageValidationState state, CFeeRate feerate, + std::map&& results) + : m_state{state}, m_tx_results(std::move(results)), m_package_feerate{feerate} {} + /** Constructor to create a PackageMempoolAcceptResult from a single MempoolAcceptResult */ explicit PackageMempoolAcceptResult(const uint256 &txid, const MempoolAcceptResult& result) : m_tx_results{ {txid, result} } {} From fcfb96ea421fcc0f0fe8c149087dc9ab12457a00 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Fri, 3 Oct 2025 03:44:08 +0700 Subject: [PATCH 04/12] fix: remove mentioning of `virtual` from validation.h which are irrelevant --- src/validation.cpp | 4 ++-- src/validation.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 7a505d13135e3..8f99f5240e5a0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -664,7 +664,7 @@ class MemPoolAccept * inserted into the mempool until Finalize(). */ std::unique_ptr m_entry; - /** Virtual size of the transaction as used by the mempool, calculated using serialized size + /** Size of the transaction as used by the mempool, calculated using serialized size * of the transaction and sigops. */ int64_t m_vsize; /** Fees paid by this transaction: total input amounts subtracted by total output amounts. */ @@ -1225,7 +1225,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // Transactions must meet two minimum feerates: the mempool minimum fee and min relay fee. // For transactions consisting of exactly one child and its parents, it suffices to use the - // package feerate (total modified fees / total virtual size) to check this requirement. + // package feerate (total modified fees / total size) to check this requirement. const auto m_total_vsize = std::accumulate(workspaces.cbegin(), workspaces.cend(), int64_t{0}, [](int64_t sum, auto& ws) { return sum + ws.m_vsize; }); const auto m_total_modified_fees = std::accumulate(workspaces.cbegin(), workspaces.cend(), CAmount{0}, diff --git a/src/validation.h b/src/validation.h index 72769a0562dc9..f53f098b9a3b3 100644 --- a/src/validation.h +++ b/src/validation.h @@ -180,7 +180,7 @@ struct MempoolAcceptResult { const TxValidationState m_state; // The following fields are only present when m_result_type = ResultType::VALID or MEMPOOL_ENTRY - /** Virtual size as used by the mempool, calculated using serialized size and sigops. */ + /** Size as used by the mempool, calculated using serialized size and sigops. */ const std::optional m_vsize; /** Raw base fees in satoshis. */ const std::optional m_base_fees; From 5867f716c45c491a5a24ba5901f03e3d0dd2eafe Mon Sep 17 00:00:00 2001 From: laanwj <126646+laanwj@users.noreply.github.com> Date: Tue, 5 Apr 2022 13:02:27 +0200 Subject: [PATCH 05/12] Merge bitcoin/bitcoin#24147: Miniscript integration 2da94a4c6f55f7a3621f4a6f70902c52f735c868 fuzz: add a fuzz target for Miniscript decoding from Script (Antoine Poinsot) f8369996e76dbc41a12f7b7eea14a7e7990a81c1 Miniscript: ops limit and stack size computation (Pieter Wuille) 2e55e88f86d0dd49b35d04af3f57e863498aabae Miniscript: conversion from script (Pieter Wuille) 1ddaa66eae67b102f5e37d212d366a5dcad4aa26 Miniscript: type system, script creation, text notation, tests (Pieter Wuille) 4fe29368c0ded0e62f437cab3a7c904f7fd3ad67 script: expose getter for CScriptNum, add a BuildScript helper (Antoine Poinsot) f4e289f384efdda6c3f56e1e1c30820a91ac2612 script: move CheckMinimalPush from interpreter to script.h (Antoine Poinsot) 31ec6ae92a5d9910a26d90a6ff20bab27dee5826 script: make IsPushdataOp non-static (Antoine Poinsot) Pull request description: Miniscript is a language for writing (a subset of) Bitcoin Scripts in a structured way. Miniscript permits: - To safely extend the Output Descriptor language to many more scripting features thanks to the typing system (composition). - Statical analysis of spending conditions, maximum spending cost of each branch, security properties, third-party malleability. - General satisfaction of any correctly typed ("valid" [0]) Miniscript. The satisfaction itself is also analyzable. - To extend the possibilities of external signers, because of all of the above and since it carries enough metadata. Miniscript guarantees: - That for any statically-analyzed as "safe" [0] Script, a witness can be constructed in the bounds of the consensus and standardness rules (standardness complete). - That unless the conditions of the Miniscript are met, no witness can be created for the Script (consensus sound). - Third-party malleability protection for the satisfaction of a sane Miniscript, which is too complex to summarize here. For more details around Miniscript (including the specifications), please refer to the [website](https://bitcoin.sipa.be/miniscript/). Miniscript was designed by Pieter Wuille, Andrew Poelstra and Sanket Kanjalkar. This PR is an updated and rebased version of #16800. See [the commit history of the Miniscript repository](https://github.com/sipa/miniscript/commits/master) for details about the changes made since September 2019 (TL;DR: bugfixes, introduction of timelock conflicts in the type system, `pk()` and `pkh()` aliases, `thresh_m` renamed to `multi`, all recursive algorithms were made non-recursive). This PR is also the first in a series of 3: - The first one (here) integrates the backbone of Miniscript. - The second one (#24148) introduces support for Miniscript in Output Descriptors, allowing for watch-only support of Miniscript Descriptors in the wallet. - The third one (#24149) implements signing for these Miniscript Descriptors, using Miniscript's satisfaction algorithm. Note to reviewers: - Miniscript is currently defined only for P2WSH. No Taproot yet. - Miniscript is different from the policy language (a high-level logical representation of a spending policy). A policy->Miniscript compiler is not included here. - The fuzz target included here is more interestingly extended in the 3rd PR to check a script's satisfaction against `VerifyScript`. I think it could be further improved by having custom mutators as we now have for multisig (see https://github.com/bitcoin/bitcoin/issues/23105). A minified corpus of Miniscript Scripts is available at https://github.com/bitcoin-core/qa-assets/pull/85. [0] We call "valid" any correctly-typed Miniscript. And "safe" any sane Miniscript, ie one whose satisfaction isn't malleable, which requires a key for any spending path, etc.. ACKs for top commit: jb55: ACK 2da94a4c6f55f7a3621f4a6f70902c52f735c868 laanwj: Light code review ACK 2da94a4c6f55f7a3621f4a6f70902c52f735c868 (mostly reviewed the changes to the existing code and build system) Tree-SHA512: d3ef558436cfcc699a50ad13caf1e776f7d0addddb433ee28ef38f66ea5c3e581382d8c748ccac9b51768e4b95712ed7a6112b0e3281a6551e0f325331de9167 --- src/Makefile.am | 2 + src/Makefile.test.include | 2 + src/script/interpreter.cpp | 25 - src/script/interpreter.h | 2 - src/script/miniscript.cpp | 348 ++++++ src/script/miniscript.h | 1649 +++++++++++++++++++++++++++ src/script/script.cpp | 25 + src/script/script.h | 29 + src/script/standard.cpp | 5 - src/script/standard.h | 5 + src/test/fuzz/miniscript_decode.cpp | 72 ++ src/test/miniscript_tests.cpp | 284 +++++ 12 files changed, 2416 insertions(+), 32 deletions(-) create mode 100644 src/script/miniscript.cpp create mode 100644 src/script/miniscript.h create mode 100644 src/test/fuzz/miniscript_decode.cpp create mode 100644 src/test/miniscript_tests.cpp diff --git a/src/Makefile.am b/src/Makefile.am index 9ad66b87a276d..cacc53d622ec7 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -345,6 +345,7 @@ BITCOIN_CORE_H = \ scheduler.h \ script/descriptor.h \ script/keyorigin.h \ + script/miniscript.h \ script/sigcache.h \ script/sign.h \ script/signingprovider.h \ @@ -923,6 +924,7 @@ libbitcoin_common_a_SOURCES = \ saltedhasher.cpp \ scheduler.cpp \ script/descriptor.cpp \ + script/miniscript.cpp \ script/sign.cpp \ script/signingprovider.cpp \ script/standard.cpp \ diff --git a/src/Makefile.test.include b/src/Makefile.test.include index b05116eaee526..d4107750889d6 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -145,6 +145,7 @@ BITCOIN_TESTS =\ test/mempool_tests.cpp \ test/merkle_tests.cpp \ test/merkleblock_tests.cpp \ + test/miniscript_tests.cpp \ test/minisketch_tests.cpp \ test/miner_tests.cpp \ test/multisig_tests.cpp \ @@ -320,6 +321,7 @@ test_fuzz_fuzz_SOURCES = \ test/fuzz/locale.cpp \ test/fuzz/merkleblock.cpp \ test/fuzz/message.cpp \ + test/fuzz/miniscript_decode.cpp \ test/fuzz/minisketch.cpp \ test/fuzz/muhash.cpp \ test/fuzz/multiplication_overflow.cpp \ diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 0d7f63a82a644..7d2c42f2fe6ac 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -221,31 +221,6 @@ bool static CheckPubKeyEncoding(const valtype &vchPubKey, unsigned int flags, co return true; } -bool CheckMinimalPush(const valtype& data, opcodetype opcode) { - // Excludes OP_1NEGATE, OP_1-16 since they are by definition minimal - assert(0 <= opcode && opcode <= OP_PUSHDATA4); - if (data.size() == 0) { - // Should have used OP_0. - return opcode == OP_0; - } else if (data.size() == 1 && data[0] >= 1 && data[0] <= 16) { - // Should have used OP_1 .. OP_16. - return false; - } else if (data.size() == 1 && data[0] == 0x81) { - // Should have used OP_1NEGATE. - return false; - } else if (data.size() <= 75) { - // Must have used a direct push (opcode indicating number of bytes pushed + those bytes). - return opcode == data.size(); - } else if (data.size() <= 255) { - // Must have used OP_PUSHDATA. - return opcode == OP_PUSHDATA1; - } else if (data.size() <= 65535) { - // Must have used OP_PUSHDATA2. - return opcode == OP_PUSHDATA2; - } - return true; -} - int FindAndDelete(CScript& script, const CScript& b) { int nFound = 0; diff --git a/src/script/interpreter.h b/src/script/interpreter.h index d8809f386c711..f2678e4b748fe 100644 --- a/src/script/interpreter.h +++ b/src/script/interpreter.h @@ -214,8 +214,6 @@ class DeferringSignatureChecker : public BaseSignatureChecker bool EvalScript(std::vector >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker, SigVersion sigversion, ScriptError* error = nullptr); bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* error = nullptr); -bool CheckMinimalPush(const std::vector& data, opcodetype opcode); - int FindAndDelete(CScript& script, const CScript& b); #endif // BITCOIN_SCRIPT_INTERPRETER_H diff --git a/src/script/miniscript.cpp b/src/script/miniscript.cpp new file mode 100644 index 0000000000000..d0bb937885248 --- /dev/null +++ b/src/script/miniscript.cpp @@ -0,0 +1,348 @@ +// Copyright (c) 2019 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 +#include +#include