diff options
author | fanquake <fanquake@gmail.com> | 2022-04-07 09:43:25 +0100 |
---|---|---|
committer | fanquake <fanquake@gmail.com> | 2022-04-07 10:05:43 +0100 |
commit | d844b5e7999419de82bfe6cbc7c9f0b7848248ef (patch) | |
tree | 61b64158628d692f9012a2b93d5e9e556540feb9 /src/validation.cpp | |
parent | 41720a1f540ef3c16a283a6cce6f0a63552a4937 (diff) | |
parent | 9bebf35e269b2a918df27708565ecd0c5bd3f116 (diff) | |
download | bitcoin-d844b5e7999419de82bfe6cbc7c9f0b7848248ef.tar.xz |
Merge bitcoin/bitcoin#24152: policy / validation: CPFP fee bumping within packages
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
Diffstat (limited to 'src/validation.cpp')
-rw-r--r-- | src/validation.cpp | 89 |
1 files changed, 77 insertions, 12 deletions
diff --git a/src/validation.cpp b/src/validation.cpp index c971b020ae..f4b316f67a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -460,6 +460,10 @@ public: * 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, @@ -472,6 +476,7 @@ public: /* m_test_accept */ test_accept, /* m_allow_bip125_replacement */ true, /* m_package_submission */ false, + /* m_package_feerates */ false, }; } @@ -485,6 +490,7 @@ public: /* m_test_accept */ true, /* m_allow_bip125_replacement */ false, /* m_package_submission */ false, // not submitting to mempool + /* m_package_feerates */ false, }; } @@ -498,6 +504,20 @@ public: /* m_test_accept */ false, /* m_allow_bip125_replacement */ 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_allow_bip125_replacement */ true, + /* m_package_submission */ false, + /* m_package_feerates */ false, // only 1 transaction }; } @@ -510,14 +530,16 @@ public: std::vector<COutPoint>& coins_to_uncache, bool test_accept, bool allow_bip125_replacement, - 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_allow_bip125_replacement{allow_bip125_replacement}, - m_package_submission{package_submission} + m_package_submission{package_submission}, + m_package_feerates{package_feerates} { } }; @@ -819,9 +841,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "bad-txns-too-many-sigops", strprintf("%d", nSigOpsCost)); - // No transactions are allowed below minRelayTxFee except from disconnected - // blocks - if (!bypass_limits && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, state)) return false; + // 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. + if (!bypass_limits && !args.m_package_feerates && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, state)) return false; ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts); // Calculate in-mempool ancestors, up to a limit. @@ -1205,12 +1228,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) { @@ -1218,7 +1256,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->GetWitnessHash(), 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 @@ -1229,14 +1267,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) @@ -1303,6 +1341,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<CTransactionRef> txns_new; for (const auto& tx : package) { const auto& wtxid = tx->GetWitnessHash(); @@ -1329,18 +1369,43 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, results.emplace(wtxid, MempoolAcceptResult::MempoolTxDifferentWitness(iter.value()->GetTx().GetWitnessHash())); } 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(GenTxid::Wtxid(wtxid))); + results.emplace(wtxid, 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& [wtxid, mempoolaccept_res] : results) { submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res); } + if (submission_result.m_state.IsValid()) assert(submission_result.m_package_feerate.has_value()); return submission_result; } |