Merge bitcoin#29722: 28950 followups

7b29119 use const ref for client_maxfeerate (Greg Sanders)
f10fd07 scripted-diff: Rename max_sane_feerate to client_maxfeerate (Greg Sanders)

Pull request description:

  Follow-ups to bitcoin#28950

ACKs for top commit:
  glozow:
    utACK 7b29119
  stickies-v:
    ACK 7b29119

Tree-SHA512: b9e13509c6e9d7c08aa9d4e759f9707004c1c7b8f3e521fe2ec0037160b87c7fb02528966b9f26eaca6291621df9300e84b5aec66dbc2e97d13bf2f3cd7f979c

Author: glozow

src/rpc/mempool.cpp

@@ -181,7 +181,7 @@ static RPCHelpMan testmempoolaccept()
             Chainstate& chainstate = chainman.ActiveChainstate();
             const PackageMempoolAcceptResult package_result = [&] {
                 LOCK(::cs_main);
-                if (txns.size() > 1) return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/true, /*max_sane_feerate=*/{});
+                if (txns.size() > 1) return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/true, /*client_maxfeerate=*/{});
                 return PackageMempoolAcceptResult(txns[0]->GetWitnessHash(),
                                                   chainman.ProcessTransaction(txns[0], /*test_accept=*/true));
             }();
@@ -873,10 +873,10 @@ static RPCHelpMan submitpackage()
 
             // Fee check needs to be run with chainstate and package context
             const CFeeRate max_raw_tx_fee_rate = ParseFeeRate(self.Arg<UniValue>(1));
-            std::optional<CFeeRate> max_sane_feerate{max_raw_tx_fee_rate};
+            std::optional<CFeeRate> client_maxfeerate{max_raw_tx_fee_rate};
             // 0-value is special; it's mapped to no sanity check
             if (max_raw_tx_fee_rate == CFeeRate(0)) {
-                max_sane_feerate = std::nullopt;
+                client_maxfeerate = std::nullopt;
             }
 
             // Burn sanity check is run with no context
@@ -906,7 +906,7 @@ static RPCHelpMan submitpackage()
             NodeContext& node = EnsureAnyNodeContext(request.context);
             CTxMemPool& mempool = EnsureMemPool(node);
             Chainstate& chainstate = EnsureChainman(node).ActiveChainstate();
-            const auto package_result = WITH_LOCK(::cs_main, return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/ false, max_sane_feerate));
+            const auto package_result = WITH_LOCK(::cs_main, return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/ false, client_maxfeerate));
 
             std::string package_msg = "success";
 

src/test/fuzz/package_eval.cpp

@@ -277,7 +277,7 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
         auto single_submit = txs.size() == 1 && fuzzed_data_provider.ConsumeBool();
 
         const auto result_package = WITH_LOCK(::cs_main,
-                                    return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/single_submit, /*max_sane_feerate=*/{}));
+                                    return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/single_submit, /*client_maxfeerate=*/{}));
 
         // Always set bypass_limits to false because it is not supported in ProcessNewPackage and
         // can be a source of divergence.

src/test/fuzz/tx_pool.cpp

@@ -291,7 +291,7 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
         // 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, /*max_sane_feerate=*/{}));
+                                    return ProcessNewPackage(chainstate, tx_pool, {tx}, true, /*client_maxfeerate=*/{}));
         // 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) {

src/test/txpackage_tests.cpp

@@ -132,7 +132,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup)
                                                    /*output_amount=*/CAmount(48 * COIN), /*submit=*/false);
     CTransactionRef tx_child = MakeTransactionRef(mtx_child);
     Package package_parent_child{tx_parent, tx_child};
-    const auto result_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_parent_child, /*test_accept=*/true, /*max_sane_feerate=*/{});
+    const auto result_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_parent_child, /*test_accept=*/true, /*client_maxfeerate=*/{});
     if (auto err_parent_child{CheckPackageMempoolAcceptResult(package_parent_child, result_parent_child, /*expect_valid=*/true, nullptr)}) {
         BOOST_ERROR(err_parent_child.value());
     } else {
@@ -151,7 +151,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup)
     CTransactionRef giant_ptx = create_placeholder_tx(999, 999);
     BOOST_CHECK(GetVirtualTransactionSize(*giant_ptx) > DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1000);
     Package package_single_giant{giant_ptx};
-    auto result_single_large = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_single_giant, /*test_accept=*/true, /*max_sane_feerate=*/{});
+    auto result_single_large = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_single_giant, /*test_accept=*/true, /*client_maxfeerate=*/{});
     if (auto err_single_large{CheckPackageMempoolAcceptResult(package_single_giant, result_single_large, /*expect_valid=*/false, nullptr)}) {
         BOOST_ERROR(err_single_large.value());
     } else {
@@ -275,7 +275,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup)
         package_unrelated.emplace_back(MakeTransactionRef(mtx));
     }
     auto result_unrelated_submit = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
-                                                     package_unrelated, /*test_accept=*/false, /*max_sane_feerate=*/{});
+                                                     package_unrelated, /*test_accept=*/false, /*client_maxfeerate=*/{});
     // We don't expect m_tx_results for each transaction when basic sanity checks haven't passed.
     BOOST_CHECK(result_unrelated_submit.m_state.IsInvalid());
     BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetResult(), PackageValidationResult::PCKG_POLICY);
@@ -315,7 +315,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup)
     // 3 Generations is not allowed.
     {
         auto result_3gen_submit = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
-                                                    package_3gen, /*test_accept=*/false, /*max_sane_feerate=*/{});
+                                                    package_3gen, /*test_accept=*/false, /*client_maxfeerate=*/{});
         BOOST_CHECK(result_3gen_submit.m_state.IsInvalid());
         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");
@@ -332,7 +332,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup)
         CTransactionRef tx_parent_invalid = MakeTransactionRef(mtx_parent_invalid);
         Package package_invalid_parent{tx_parent_invalid, tx_child};
         auto result_quit_early = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
-                                                   package_invalid_parent, /*test_accept=*/ false, /*max_sane_feerate=*/{});
+                                                   package_invalid_parent, /*test_accept=*/ false, /*client_maxfeerate=*/{});
         if (auto err_parent_invalid{CheckPackageMempoolAcceptResult(package_invalid_parent, result_quit_early, /*expect_valid=*/false, m_node.mempool.get())}) {
             BOOST_ERROR(err_parent_invalid.value());
         } else {
@@ -353,7 +353,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup)
     package_missing_parent.push_back(MakeTransactionRef(mtx_child));
     {
         const auto result_missing_parent = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
-                                                             package_missing_parent, /*test_accept=*/false, /*max_sane_feerate=*/{});
+                                                             package_missing_parent, /*test_accept=*/false, /*client_maxfeerate=*/{});
         BOOST_CHECK(result_missing_parent.m_state.IsInvalid());
         BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetResult(), PackageValidationResult::PCKG_POLICY);
         BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetRejectReason(), "package-not-child-with-unconfirmed-parents");
@@ -363,7 +363,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup)
     // Submit package with parent + child.
     {
         const auto submit_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
-                                                           package_parent_child, /*test_accept=*/false, /*max_sane_feerate=*/{});
+                                                           package_parent_child, /*test_accept=*/false, /*client_maxfeerate=*/{});
         expected_pool_size += 2;
         BOOST_CHECK_MESSAGE(submit_parent_child.m_state.IsValid(),
                             "Package validation unexpectedly failed: " << submit_parent_child.m_state.GetRejectReason());
@@ -385,7 +385,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup)
     // Already-in-mempool transactions should be detected and de-duplicated.
     {
         const auto submit_deduped = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
-                                                      package_parent_child, /*test_accept=*/false, /*max_sane_feerate=*/{});
+                                                      package_parent_child, /*test_accept=*/false, /*client_maxfeerate=*/{});
         if (auto err_deduped{CheckPackageMempoolAcceptResult(package_parent_child, submit_deduped, /*expect_valid=*/true, m_node.mempool.get())}) {
             BOOST_ERROR(err_deduped.value());
         } else {
@@ -456,15 +456,15 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup)
     {
         Package package_parent_child1{ptx_parent, ptx_child1};
         const auto submit_witness1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
-                                                       package_parent_child1, /*test_accept=*/false, /*max_sane_feerate=*/{});
+                                                       package_parent_child1, /*test_accept=*/false, /*client_maxfeerate=*/{});
         if (auto err_witness1{CheckPackageMempoolAcceptResult(package_parent_child1, submit_witness1, /*expect_valid=*/true, m_node.mempool.get())}) {
             BOOST_ERROR(err_witness1.value());
         }
 
         // Child2 would have been validated individually.
         Package package_parent_child2{ptx_parent, ptx_child2};
         const auto submit_witness2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
-                                                       package_parent_child2, /*test_accept=*/false, /*max_sane_feerate=*/{});
+                                                       package_parent_child2, /*test_accept=*/false, /*client_maxfeerate=*/{});
         if (auto err_witness2{CheckPackageMempoolAcceptResult(package_parent_child2, submit_witness2, /*expect_valid=*/true, m_node.mempool.get())}) {
             BOOST_ERROR(err_witness2.value());
         } else {
@@ -478,7 +478,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup)
         // Deduplication should work when wtxid != txid. Submit package with the already-in-mempool
         // transactions again, which should not fail.
         const auto submit_segwit_dedup = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
-                                                           package_parent_child1, /*test_accept=*/false, /*max_sane_feerate=*/{});
+                                                           package_parent_child1, /*test_accept=*/false, /*client_maxfeerate=*/{});
         if (auto err_segwit_dedup{CheckPackageMempoolAcceptResult(package_parent_child1, submit_segwit_dedup, /*expect_valid=*/true, m_node.mempool.get())}) {
             BOOST_ERROR(err_segwit_dedup.value());
         } else {
@@ -508,7 +508,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup)
     {
         Package package_child2_grandchild{ptx_child2, ptx_grandchild};
         const auto submit_spend_ignored = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
-                                                            package_child2_grandchild, /*test_accept=*/false, /*max_sane_feerate=*/{});
+                                                            package_child2_grandchild, /*test_accept=*/false, /*client_maxfeerate=*/{});
         if (auto err_spend_ignored{CheckPackageMempoolAcceptResult(package_child2_grandchild, submit_spend_ignored, /*expect_valid=*/true, m_node.mempool.get())}) {
             BOOST_ERROR(err_spend_ignored.value());
         } else {
@@ -606,7 +606,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup)
     // parent3 should be accepted
     // child should be accepted
     {
-        const auto mixed_result = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_mixed, false, /*max_sane_feerate=*/{});
+        const auto mixed_result = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_mixed, false, /*client_maxfeerate=*/{});
         if (auto err_mixed{CheckPackageMempoolAcceptResult(package_mixed, mixed_result, /*expect_valid=*/true, m_node.mempool.get())}) {
             BOOST_ERROR(err_mixed.value());
         } else {
@@ -670,7 +670,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
     {
         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, /*max_sane_feerate=*/{});
+                                                   package_cpfp, /*test_accept=*/ false, /*client_maxfeerate=*/{});
         if (auto err_cpfp_deprio{CheckPackageMempoolAcceptResult(package_cpfp, submit_cpfp_deprio, /*expect_valid=*/false, m_node.mempool.get())}) {
             BOOST_ERROR(err_cpfp_deprio.value());
         } else {
@@ -692,7 +692,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
     {
         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, /*max_sane_feerate=*/{});
+                                                   package_cpfp, /*test_accept=*/ false, /*client_maxfeerate=*/{});
         if (auto err_cpfp{CheckPackageMempoolAcceptResult(package_cpfp, submit_cpfp, /*expect_valid=*/true, m_node.mempool.get())}) {
             BOOST_ERROR(err_cpfp.value());
         } else {
@@ -744,7 +744,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
     // Cheap package should fail for being too low fee.
     {
         const auto submit_package_too_low = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
-                                                   package_still_too_low, /*test_accept=*/false, /*max_sane_feerate=*/{});
+                                                   package_still_too_low, /*test_accept=*/false, /*client_maxfeerate=*/{});
         if (auto err_package_too_low{CheckPackageMempoolAcceptResult(package_still_too_low, submit_package_too_low, /*expect_valid=*/false, m_node.mempool.get())}) {
             BOOST_ERROR(err_package_too_low.value());
         } else {
@@ -770,7 +770,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
     // 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, /*max_sane_feerate=*/{});
+                                                                  package_still_too_low, /*test_accept=*/false, /*client_maxfeerate=*/{});
         if (auto err_prioritised{CheckPackageMempoolAcceptResult(package_still_too_low, submit_prioritised_package, /*expect_valid=*/true, m_node.mempool.get())}) {
             BOOST_ERROR(err_prioritised.value());
         } else {
@@ -818,7 +818,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
     {
         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, /*max_sane_feerate=*/{});
+                                                          package_rich_parent, /*test_accept=*/false, /*client_maxfeerate=*/{});
         if (auto err_rich_parent{CheckPackageMempoolAcceptResult(package_rich_parent, submit_rich_parent, /*expect_valid=*/false, m_node.mempool.get())}) {
             BOOST_ERROR(err_rich_parent.value());
         } else {

src/validation.cpp

@@ -511,7 +511,7 @@ class MemPoolAccept
 
         /** Parameters for child-with-unconfirmed-parents package validation. */
         static ATMPArgs PackageChildWithParents(const CChainParams& chainparams, int64_t accept_time,
-                                                std::vector<COutPoint>& coins_to_uncache, std::optional<CFeeRate>& client_maxfeerate) {
+                                                std::vector<COutPoint>& coins_to_uncache, const std::optional<CFeeRate>& client_maxfeerate) {
             return ATMPArgs{/* m_chainparams */ chainparams,
                             /* m_accept_time */ accept_time,
                             /* m_bypass_limits */ false,
@@ -1716,7 +1716,7 @@ MempoolAcceptResult AcceptToMemoryPool(Chainstate& active_chainstate, const CTra
 }
 
 PackageMempoolAcceptResult ProcessNewPackage(Chainstate& active_chainstate, CTxMemPool& pool,
-                                                   const Package& package, bool test_accept, std::optional<CFeeRate> client_maxfeerate)
+                                                   const Package& package, bool test_accept, const std::optional<CFeeRate>& client_maxfeerate)
 {
     AssertLockHeld(cs_main);
     assert(!package.empty());

src/validation.h

@@ -275,14 +275,14 @@ MempoolAcceptResult AcceptToMemoryPool(Chainstate& active_chainstate, const CTra
 * Validate (and maybe submit) a package to the mempool. See doc/policy/packages.md for full details
 * on package validation rules.
 * @param[in]    test_accept         When true, run validation checks but don't submit to mempool.
-* @param[in]    max_sane_feerate    If exceeded by an individual transaction, rest of (sub)package evalution is aborted.
+* @param[in]    client_maxfeerate    If exceeded by an individual transaction, rest of (sub)package evalution is aborted.
 *                                   Only for sanity checks against local submission of transactions.
 * @returns a PackageMempoolAcceptResult which includes a MempoolAcceptResult for each transaction.
 * If a transaction fails, validation will exit early and some results may be missing. It is also
 * possible for the package to be partially submitted.
 */
 PackageMempoolAcceptResult ProcessNewPackage(Chainstate& active_chainstate, CTxMemPool& pool,
-                                                   const Package& txns, bool test_accept, std::optional<CFeeRate> max_sane_feerate)
+                                                   const Package& txns, bool test_accept, const std::optional<CFeeRate>& client_maxfeerate)
                                                    EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
 /* Mempool validation helper functions */