Merge bitcoin/bitcoin#28471: Fix virtual size limit enforcement in transaction package context

eb8f58f5e4a249d55338304099e8238896d2316d Add functional test to catch too large vsize packages (Greg Sanders)
1a579f9d01256b0b2570168496d129a8b2009b35 Handle over-sized (in virtual bytes) packages with no in-mempool ancestors (Greg Sanders)
bc013fe8e3d0bae2ab766a8248219aa3c9e344df Bugfix: Pass correct virtual size to CheckPackageLimits (Luke Dashjr)
533660c58ad5a218671a9bc1537299b1d67bb55d Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion (Greg Sanders)

Pull request description:

  (Alternative) Minimal subset of https://github.com/bitcoin/bitcoin/pull/28345 to:

  1) Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT which accounts for additional WU necessary to not exclude default chain limit transactions that would have been accepted individually. Avoids sigops vbyte confusion.
  2) pass correct vsize to chain limit evaluations in package context
  3) stop overly-large packages that have no existing mempool ancestors (also a bugfix by itself if someone sets non-standard chain limits)

  This should fix the known issues while not blocking additional refactoring later.

ACKs for top commit:
  achow101:
    ACK eb8f58f5e4a249d55338304099e8238896d2316d
  ariard:
    Re-Code ACK eb8f58f5e
  glozow:
    reACK eb8f58f5e4a249d55338304099e8238896d2316d

Tree-SHA512: 1b5cca1a526207e25d387fcc29a776a3198c3a013dc2b35c6275b9d5a64db2476c154ebf52e3a1aed0b9924c75613f21a946577aa760de28cadf0c9c7f68dc39

1 files changed, 45 insertions, 1 deletions

diff --git a/test/functional/mempool_sigoplimit.py b/test/functional/mempool_sigoplimit.py
index 962b2b19bd..fbec6d0dc8 100755
--- a/test/functional/mempool_sigoplimit.py
+++ b/test/functional/mempool_sigoplimit.py
@@ -3,6 +3,7 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test sigop limit mempool policy (`-bytespersigop` parameter)"""
+from decimal import Decimal
 from math import ceil
 
 from test_framework.messages import (
@@ -25,6 +26,7 @@ from test_framework.script import (
     OP_TRUE,
 )
 from test_framework.script_util import (
+    keys_to_multisig_script,
     script_to_p2wsh_script,
 )
 from test_framework.test_framework import BitcoinTestFramework
@@ -32,9 +34,10 @@ from test_framework.util import (
     assert_equal,
     assert_greater_than,
     assert_greater_than_or_equal,
+    assert_raises_rpc_error,
 )
 from test_framework.wallet import MiniWallet
-
+from test_framework.wallet_util import generate_keypair
 
 DEFAULT_BYTES_PER_SIGOP = 20  # default setting
 
@@ -133,6 +136,45 @@ class BytesPerSigOpTest(BitcoinTestFramework):
         assert_equal(entry_parent['descendantcount'], 2)
         assert_equal(entry_parent['descendantsize'], parent_tx.get_vsize() + sigop_equivalent_vsize)
 
+    def test_sigops_package(self):
+        self.log.info("Test a overly-large sigops-vbyte hits package limits")
+        # Make a 2-transaction package which fails vbyte checks even though
+        # separately they would work.
+        self.restart_node(0, extra_args=["-bytespersigop=5000"] + self.extra_args[0])
+
+        def create_bare_multisig_tx(utxo_to_spend=None):
+            _, pubkey = generate_keypair()
+            amount_for_bare = 50000
+            tx_dict = self.wallet.create_self_transfer(fee=Decimal("3"), utxo_to_spend=utxo_to_spend)
+            tx_utxo = tx_dict["new_utxo"]
+            tx = tx_dict["tx"]
+            tx.vout.append(CTxOut(amount_for_bare, keys_to_multisig_script([pubkey], k=1)))
+            tx.vout[0].nValue -= amount_for_bare
+            tx_utxo["txid"] = tx.rehash()
+            tx_utxo["value"] -= Decimal("0.00005000")
+            return (tx_utxo, tx)
+
+        tx_parent_utxo, tx_parent = create_bare_multisig_tx()
+        tx_child_utxo, tx_child = create_bare_multisig_tx(tx_parent_utxo)
+
+        # Separately, the parent tx is ok
+        parent_individual_testres = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex()])[0]
+        assert parent_individual_testres["allowed"]
+        # Multisig is counted as MAX_PUBKEYS_PER_MULTISIG = 20 sigops
+        assert_equal(parent_individual_testres["vsize"], 5000 * 20)
+
+        # But together, it's exceeding limits in the *package* context. If sigops adjusted vsize wasn't being checked
+        # here, it would get further in validation and give too-long-mempool-chain error instead.
+        packet_test = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex(), tx_child.serialize().hex()])
+        assert_equal([x["package-error"] for x in packet_test], ["package-mempool-limits", "package-mempool-limits"])
+
+        # When we actually try to submit, the parent makes it into the mempool, but the child would exceed ancestor vsize limits
+        assert_raises_rpc_error(-26, "too-long-mempool-chain", self.nodes[0].submitpackage, [tx_parent.serialize().hex(), tx_child.serialize().hex()])
+        assert tx_parent.rehash() in self.nodes[0].getrawmempool()
+
+        # Transactions are tiny in weight
+        assert_greater_than(2000, tx_parent.get_weight() + tx_child.get_weight())
+
     def run_test(self):
         self.wallet = MiniWallet(self.nodes[0])
 
@@ -149,6 +191,8 @@ class BytesPerSigOpTest(BitcoinTestFramework):
 
             self.generate(self.wallet, 1)
 
+        self.test_sigops_package()
+
 
 if __name__ == '__main__':
     BytesPerSigOpTest().main()