Merge bitcoin#24152: policy / validation: CPFP fee bumping within packages

9bebf35 [validation] don't package validate if not policy or missing inputs (glozow)
51edcff [unit test] package feerate and package cpfp (glozow)
1b93748 [validation] try individual validation before package validation (glozow)
17a8ffd [packages/policy] use package feerate in package validation (glozow)
09f32cf [docs] package feerate (glozow)

Pull request description:

  Part of bitcoin#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 bitcoin@9bebf35
  mzumsande:
    Code review ACK 9bebf35
  t-bast:
    ACK bitcoin@9bebf35

Tree-SHA512: 5117cfcc3ce55c00384d9e8003a0589ceac1e6f738b1c299007d9cd9cdd2d7c530d31cfd23658b041a6604d39073bcc6e81f0639a300082a92097682a6ea8c8f

Author: fanquake

doc/policy/packages.md

@@ -72,3 +72,48 @@ test accepts):
      a competing package or transaction with a mutated witness, even though the two
      same-txid-different-witness transactions are conflicting and cannot replace each other, 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 virtual 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*: Packages are intended for incentive-compatible fee-bumping: transaction B is a
+"legitimate" fee-bump for transaction A only if B is a descendant of A and has a *higher* feerate
+than A. We want to prevent "parents pay for children" behavior; fees of parents should not help
+their children, since the parents can be mined without the child.  More generally, if transaction A
+is not needed in order for transaction B to be mined, A's fees cannot help B. In a
+child-with-parents package, simply excluding any parent transactions that meet feerate requirements
+individually is sufficient to ensure this.
+
+*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.

src/test/fuzz/tx_pool.cpp

@@ -234,14 +234,18 @@ FUZZ_TARGET_INIT(tx_pool_standard, 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->GetWitnessHash());
-        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->GetWitnessHash());
+            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;