AcceptMultipleTransactions: Fix workspace client_maxfeerate

If we do not set the Failure for the workspace when
there is a client_maxfeerate related error, we hit
an Assume() to the contrary. Properly set it.

Author: instagibbs

src/validation.cpp

@@ -1366,7 +1366,9 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
         // Individual modified feerate exceeded caller-defined max; abort
         // N.B. this doesn't take into account CPFPs. Chunk-aware validation may be more robust.
         if (args.m_client_maxfeerate && CFeeRate(ws.m_modified_fees, ws.m_vsize) > args.m_client_maxfeerate.value()) {
-            package_state.Invalid(PackageValidationResult::PCKG_TX, "max feerate exceeded");
+            // Need to set failure here both individually and at package level
+            ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "max feerate exceeded", "");
+            package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed");
             // Exit early to avoid doing pointless work. Update the failed tx result; the rest are unfinished.
             results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
             return PackageMempoolAcceptResult(package_state, std::move(results));

test/functional/rpc_packages.py

@@ -18,6 +18,7 @@
     assert_equal,
     assert_fee_amount,
     assert_raises_rpc_error,
+    fill_mempool,
 )
 from test_framework.wallet import (
     DEFAULT_FEE,
@@ -82,7 +83,8 @@ def run_test(self):
         self.test_conflicting()
         self.test_rbf()
         self.test_submitpackage()
-        self.test_maxfeerate_maxburn_submitpackage()
+        self.test_maxfeerate_submitpackage()
+        self.test_maxburn_submitpackage()
 
     def test_independent(self, coin):
         self.log.info("Test multiple independent transactions in a package")
@@ -358,7 +360,7 @@ def test_submitpackage(self):
         assert_equal(res["tx-results"][sec_wtxid]["error"], "version")
         peer.wait_for_broadcast([first_wtxid])
 
-    def test_maxfeerate_maxburn_submitpackage(self):
+    def test_maxfeerate_submitpackage(self):
         node = self.nodes[0]
         # clear mempool
         deterministic_address = node.get_deterministic_priv_key().address
@@ -369,23 +371,72 @@ def test_maxfeerate_maxburn_submitpackage(self):
         minrate_btc_kvb = min([chained_txn["fee"] / chained_txn["tx"].get_vsize() * 1000 for chained_txn in chained_txns])
         chain_hex = [t["hex"] for t in chained_txns]
         pkg_result = node.submitpackage(chain_hex, maxfeerate=minrate_btc_kvb - Decimal("0.00000001"))
+
+        # First tx failed in single transaction evaluation, so package message is generic
+        assert_equal(pkg_result["package_msg"], "transaction failed")
         assert_equal(pkg_result["tx-results"][chained_txns[0]["wtxid"]]["error"], "max feerate exceeded")
         assert_equal(pkg_result["tx-results"][chained_txns[1]["wtxid"]]["error"], "bad-txns-inputs-missingorspent")
         assert_equal(node.getrawmempool(), [])
 
+        # Make chain of two transactions where parent doesn't make minfee threshold
+        # but child is too high fee
+        # Lower mempool limit to make it easier to fill_mempool
+        self.restart_node(0, extra_args=[
+            "-datacarriersize=100000",
+            "-maxmempool=5",
+            "-persistmempool=0",
+        ])
+
+        fill_mempool(self, node, self.wallet)
+
+        minrelay = node.getmempoolinfo()["minrelaytxfee"]
+        parent = self.wallet.create_self_transfer(
+            fee_rate=minrelay,
+        )
+
+        child = self.wallet.create_self_transfer(
+            fee_rate=DEFAULT_FEE,
+            utxo_to_spend=parent["new_utxo"],
+        )
+
+        pkg_result = node.submitpackage([parent["hex"], child["hex"]], maxfeerate=DEFAULT_FEE - Decimal("0.00000001"))
+
+        # Child is connected even though parent is invalid and still reports fee exceeded
+        # this implies sub-package evaluation of both entries together.
+        assert_equal(pkg_result["package_msg"], "transaction failed")
+        assert "mempool min fee not met" in pkg_result["tx-results"][parent["wtxid"]]["error"]
+        assert_equal(pkg_result["tx-results"][child["wtxid"]]["error"], "max feerate exceeded")
+        assert parent["txid"] not in node.getrawmempool()
+        assert child["txid"] not in node.getrawmempool()
+
+        # Reset maxmempool, datacarriersize, reset dynamic mempool minimum feerate, and empty mempool.
+        self.restart_node(0)
+
+        assert_equal(node.getrawmempool(), [])
+
+    def test_maxburn_submitpackage(self):
+        node = self.nodes[0]
+
+        assert_equal(node.getrawmempool(), [])
+
         self.log.info("Submitpackage maxburnamount arg testing")
-        tx = tx_from_hex(chain_hex[1])
+        chained_txns_burn = self.wallet.create_self_transfer_chain(chain_length=2)
+        chained_burn_hex = [t["hex"] for t in chained_txns_burn]
+
+        tx = tx_from_hex(chained_burn_hex[1])
         tx.vout[-1].scriptPubKey = b'a' * 10001 # scriptPubKey bigger than 10k IsUnspendable
-        chain_hex = [chain_hex[0], tx.serialize().hex()]
+        chained_burn_hex = [chained_burn_hex[0], tx.serialize().hex()]
         # burn test is run before any package evaluation; nothing makes it in and we get broader exception
-        assert_raises_rpc_error(-25, "Unspendable output exceeds maximum configured by user", node.submitpackage, chain_hex, 0, chained_txns[1]["new_utxo"]["value"] - Decimal("0.00000001"))
+        assert_raises_rpc_error(-25, "Unspendable output exceeds maximum configured by user", node.submitpackage, chained_burn_hex, 0, chained_txns_burn[1]["new_utxo"]["value"] - Decimal("0.00000001"))
         assert_equal(node.getrawmempool(), [])
 
+        minrate_btc_kvb_burn = min([chained_txn_burn["fee"] / chained_txn_burn["tx"].get_vsize() * 1000 for chained_txn_burn in chained_txns_burn])
+
         # Relax the restrictions for both and send it; parent gets through as own subpackage
-        pkg_result = node.submitpackage(chain_hex, maxfeerate=minrate_btc_kvb, maxburnamount=chained_txns[1]["new_utxo"]["value"])
-        assert "error" not in pkg_result["tx-results"][chained_txns[0]["wtxid"]]
+        pkg_result = node.submitpackage(chained_burn_hex, maxfeerate=minrate_btc_kvb_burn, maxburnamount=chained_txns_burn[1]["new_utxo"]["value"])
+        assert "error" not in pkg_result["tx-results"][chained_txns_burn[0]["wtxid"]]
         assert_equal(pkg_result["tx-results"][tx.getwtxid()]["error"], "scriptpubkey")
-        assert_equal(node.getrawmempool(), [chained_txns[0]["txid"]])
+        assert_equal(node.getrawmempool(), [chained_txns_burn[0]["txid"]])
 
 if __name__ == "__main__":
     RPCPackagesTest().main()