Fix virtual size limit enforcement in transaction package context

instagibbs commented at 5:34 pm on September 13, 2023: member

(Alternative) Minimal subset of #28345 to:

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.
pass correct vsize to chain limit evaluations in package context
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.

DrahtBot commented at 5:34 pm on September 13, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ariard, glozow, achow101
Concept ACK	darosior

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

in doc/policy/packages.md:21 in 45b71639ae outdated

17@@ -18,7 +18,7 @@ tip or some preceding transaction in the package.
18 
19 The following rules are enforced for all packages:
20 
21-* Packages cannot exceed `MAX_PACKAGE_COUNT=25` count and `MAX_PACKAGE_SIZE=101KvB` total size
22+* Packages cannot exceed `MAX_PACKAGE_COUNT=25` count and `MAX_PACKAGE_SIZE=101KB` total size

glozow commented at 8:38 am on September 14, 2023:

Would it be simpler / easier to understand if we just said there’s a maximum weight MAX_PACKAGE_WEIGHT=404000?

instagibbs commented at 8:35 pm on September 14, 2023:

did MAX_PACKAGE_KWEIGHT=404

glozow commented at 8:40 am on September 14, 2023: member

Concept ACK, thanks for taking this over!

in src/txmempool.cpp:166 in e4640da4bc outdated

162@@ -163,6 +163,17 @@ util::Result<CTxMemPool::setEntries> CTxMemPool::CalculateAncestorsAndCheckLimit
163     int64_t totalSizeWithAncestors = entry_size;
164     setEntries ancestors;
165 
166+    // Package itself is busting mempool limits; should be rejected even if no staged_ancestors exist

glozow commented at 8:42 am on September 14, 2023:

Needs test coverage, maybe in test/functional/mempool_sigoplimit.py? Or a unit test for CheckPackageLimits?

instagibbs commented at 8:34 pm on September 14, 2023:

Added in mempool_sigoplimit.py since this change threads through a few layers

in src/txmempool.cpp:1 in e4640da4bc

glozow commented at 8:58 am on September 14, 2023:

Perhaps include a note in the e4640da4bcacb513c7b602ff3127f65963c3bba4 commit message that you’re specifically trying to catch when total package sigop-adjusted vsize. Might be helpful to a future git-blamer who may think this code is redundant with the other package size checks.

instagibbs commented at 8:35 pm on September 14, 2023:

done

dergoegge commented at 10:59 am on September 14, 2023: member

Can you add tests for the bug fixes?

instagibbs force-pushed on Sep 14, 2023

instagibbs commented at 8:34 pm on September 14, 2023: member

Can you add tests for the bug fixes?

Added.

in src/policy/packages.h:22 in 86eb12aec1 outdated

19-static constexpr uint32_t MAX_PACKAGE_SIZE{101};
20+/** Default maximum total weight of transactions in a package in KWu and (non-virtual) KB
21+    to allow for context-less checks. */
22+static constexpr uint32_t MAX_PACKAGE_KWEIGHT = 404;
23+static constexpr uint32_t MAX_PACKAGE_SIZE{MAX_PACKAGE_KWEIGHT / WITNESS_SCALE_FACTOR};
24+static_assert(MAX_PACKAGE_SIZE == 101);

glozow commented at 9:21 am on September 15, 2023:

86eb12aec1110dcf506846d1dbe4dc58b8254612

Might it be better to not define a MAX_PACKAGE_SIZE and just do weight instead? I’d be worried about rounding errors as they can add up as we round for each transaction’s GetVirtualTransactionSize().

I suggest just doing a MAX_PACKAGE_WEIGHT_KWU and changing packages.cpp to sum GetTransactionWeight instead.

instagibbs commented at 1:26 pm on September 15, 2023:

done.

in test/functional/mempool_sigoplimit.py:165 in 71930556bc outdated

160+        tx_child.calc_sha256()
161+        tx_child_utxo["txid"] = tx_child.hash
162+        tx_child_utxo["value"] -= Decimal("0.00005000")
163+
164+        # Separately, the parent tx is ok
165+        assert self.nodes[0].testmempoolaccept([tx_parent.serialize().hex()])[0]["allowed"]

glozow commented at 9:43 am on September 15, 2023:

71930556bc30659e02a88b3a115c0d36dacd9545

Worth checking + explaining the numbers?

0        parent_individual_testres = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex()])[0]
1        assert parent_individual_testres["allowed"]
2        # Multisig is counted as MAX_PUBKEYS_PER_MULTISIG = 20 sigops
3        assert_equal(parent_individual_testres["vsize"], 5000 * 20)

instagibbs commented at 1:26 pm on September 15, 2023:

done

in test/functional/mempool_sigoplimit.py:173 in 71930556bc outdated

168+        packet_test = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex(), tx_child.serialize().hex()])
169+        assert_equal([x["package-error"] for x in packet_test], ["package-mempool-limits", "package-mempool-limits"])
170+        assert_raises_rpc_error(-26, "too-long-mempool-chain", self.nodes[0].submitpackage, [tx_parent.serialize().hex(), tx_child.serialize().hex()])
171+
172+        # Transactions are tiny in weight
173+        assert_greater_than(500, len(tx_parent.serialize()) + len(tx_child.serialize()))

glozow commented at 9:45 am on September 15, 2023:

71930556bc30659e02a88b3a115c0d36dacd9545 maybe use the actual weight getter instead

0        assert_greater_than(2000, tx_parent.get_weight() + tx_child.get_weight())

instagibbs commented at 1:26 pm on September 15, 2023:

done

in test/functional/mempool_sigoplimit.py:172 in 71930556bc outdated

165+        assert self.nodes[0].testmempoolaccept([tx_parent.serialize().hex()])[0]["allowed"]
166+
167+        # But together, it's exceeding limits
168+        packet_test = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex(), tx_child.serialize().hex()])
169+        assert_equal([x["package-error"] for x in packet_test], ["package-mempool-limits", "package-mempool-limits"])
170+        assert_raises_rpc_error(-26, "too-long-mempool-chain", self.nodes[0].submitpackage, [tx_parent.serialize().hex(), tx_child.serialize().hex()])

glozow commented at 9:49 am on September 15, 2023:

71930556bc30659e02a88b3a115c0d36dacd9545

Add comment explaining why the error message is different between testmempoolaccept and submitpackage

0        # When we actually try to submit, the parent makes it into the mempool, but the child would exceed ancestor vsize limits
1       assert_raises_rpc_error(-26, "too-long-mempool-chain", self.nodes[0].submitpackage, [tx_parent.serialize().hex(), tx_child.serialize().hex()])
2       assert tx_parent.rehash() in self.nodes[0].getrawmempool()

instagibbs commented at 1:26 pm on September 15, 2023:

done

in src/txmempool.cpp:174 in 8a8a549a33 outdated

169+    } else if (entry_count > static_cast<uint64_t>(limits.descendant_count)) {
170+        return util::Error{Untranslated(strprintf("too many descendants in package %u [limit: %u]", entry_count, limits.descendant_count))};
171+    } else if (entry_size > limits.ancestor_size_vbytes) {
172+        return util::Error{Untranslated(strprintf("exceeds ancestor size limit %u [limit: %u]", entry_size, limits.ancestor_size_vbytes))};
173+    } else if (entry_size > limits.descendant_size_vbytes) {
174+        return util::Error{Untranslated(strprintf("exceeds descendant size limit for tx %u [limit: %u]", entry_size, limits.descendant_size_vbytes))};

glozow commented at 10:07 am on September 15, 2023:

8a8a549a330ef3de99a6a3d7c150a6e2ab5ff11e

Why is this in CalculateAncestorsAndCheckLimits instead of CheckPackageLimits? Would be faster to check this before calculating staged_ancestors.

Also I wonder if this could be a confusing error string due to how we’re checking. e.g. descendant limit < ancestor limit, so a child-with-24-parents package hits “too many descendants” even if everyone has a descendant count of 1 or 2.

Perhaps more clear is “package count exceeds ancestor count limit,” “package vsize exceeds ancestor count limit,” etc.

instagibbs commented at 1:54 pm on September 15, 2023:

done

glozow commented at 10:10 am on September 15, 2023: member

LGTM except worried about rounding and error strings. Also a few nits on the test.

instagibbs force-pushed on Sep 15, 2023

in src/policy/packages.h:51 in 0774c39e86 outdated

47@@ -47,7 +48,7 @@ class PackageValidationState : public ValidationState<PackageValidationResult> {
48 
49 /** Context-free package policy checks:
50  * 1. The number of transactions cannot exceed MAX_PACKAGE_COUNT.
51- * 2. The total virtual size cannot exceed MAX_PACKAGE_SIZE.
52+ * 2. The total (non-virtual) size cannot exceed MAX_PACKAGE_KWU / WITNESS_SCALE_FACTOR.

glozow commented at 1:29 pm on September 15, 2023:

nit 0774c39e86ca52b747ace02f367617f4d6d88305

total weight

instagibbs commented at 1:40 pm on September 15, 2023:

pushed cleanups

in src/test/txpackage_tests.cpp:125 in 0774c39e86 outdated

121@@ -122,7 +122,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup)
122 
123     // A single, giant transaction submitted through ProcessNewPackage fails on single tx policy.
124     CTransactionRef giant_ptx = create_placeholder_tx(999, 999);
125-    BOOST_CHECK(GetVirtualTransactionSize(*giant_ptx) > MAX_PACKAGE_SIZE * 1000);
126+    BOOST_CHECK(GetVirtualTransactionSize(*giant_ptx) > DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1000);

glozow commented at 1:30 pm on September 15, 2023:

nit 0774c39e86ca52b747ace02f367617f4d6d88305

should this be checking against MAX_PACKAGE_KWU?

instagibbs commented at 1:33 pm on September 15, 2023:

fails on single tx policy

it’s a single tx test check, not a package limit check? :shrug:

instagibbs force-pushed on Sep 15, 2023

in doc/policy/packages.md:21 in 66f6868c43 outdated

17@@ -18,7 +18,7 @@ tip or some preceding transaction in the package.
18 
19 The following rules are enforced for all packages:
20 
21-* Packages cannot exceed `MAX_PACKAGE_COUNT=25` count and `MAX_PACKAGE_SIZE=101KvB` total size
22+* Packages cannot exceed `MAX_PACKAGE_COUNT=25` count and `MAX_PACKAGE_WEIGHT_KWU=404` total weight

glozow commented at 1:49 pm on September 15, 2023:

66f6868c4331d3c289221441130e6c1fb61bc60c

this says MAX_PACKAGE_WEIGHT_KWU but the variable is named MAX_PACKAGE_KWU

instagibbs commented at 1:57 pm on September 15, 2023:

fixed

glozow commented at 1:56 pm on September 15, 2023: member

ACK 3a4adfbfa6a947721bf140a534fb2aefdf0922fd with a doc nit

instagibbs force-pushed on Sep 15, 2023

glozow commented at 1:59 pm on September 15, 2023: member

Could we get a review from maybe @luke-jr or @ariard?

glozow added the label TX fees and policy on Sep 15, 2023

in doc/policy/packages.md:24 in 204249bb72 outdated

17@@ -18,7 +18,7 @@ tip or some preceding transaction in the package.
18 
19 The following rules are enforced for all packages:
20 
21-* Packages cannot exceed `MAX_PACKAGE_COUNT=25` count and `MAX_PACKAGE_SIZE=101KvB` total size
22+* Packages cannot exceed `MAX_PACKAGE_COUNT=25` count and `MAX_PACKAGE_KWU=404` total weight
23    (#20833)
24 
25    - *Rationale*: This is already enforced as mempool ancestor/descendant limits. If

glozow commented at 2:25 pm on September 15, 2023:

Ah hold on, avoiding rounding is good but it does mean that this limit is effectively a few Wu smaller than the default ancestor size limit. (https://github.com/bitcoin/bitcoin/pull/22097#issuecomment-859901930 and #22097 (comment)) so this statement is untrue.

I think having this limit denominated in Wu is good and it should be distinct from the ancestor package limits anyway. But I agree with those comments that maybe it’s best to not make this limit more restrictive than normal ancestor limits.

We can bake in the rounding so this limit is 404000Wu + MAX_PACKAGE_COUNT * (WITNESS_SCALE_FACTOR -1) = 404075Wu.

Also I think the primary Rationale (if you could edit this bullet point please) should be “We want package size to be as small as possible to mitigate DoS via package validation. However, we want to make sure that the limit does not restrict ancestor packages that would be allowed if submitted individually.”

instagibbs commented at 3:05 pm on September 15, 2023:

Fixed up as suggested, and fixed some inverted static asserts.

I thought about calling it 405KWU and calling it a day, but I think that would make code history more difficult, so instead stayed precisely as calculated.

glozow commented at 9:37 am on September 20, 2023:

Oops, those comments are incorrect. The rounding does mean that this weight limit across multiple transactions is different from the ancestor size limits, but it makes the package limits less restrictive, not more restrictive. That’s totally fine. No change needed, we can just keep 404,000Wu.

glozow added the label Bug on Sep 15, 2023

instagibbs force-pushed on Sep 15, 2023

DrahtBot added the label CI failed on Sep 15, 2023

DrahtBot removed the label CI failed on Sep 15, 2023

glozow requested review from darosior on Sep 19, 2023

glozow requested review from ariard on Sep 19, 2023

ariard commented at 5:58 pm on September 19, 2023: member

Concept ACK on replacing MAX_PACKAGE_SIZE to MAX_PACKAGE_WEIGHT and other changes sounds good to me, will code review more soon.

in src/policy/packages.h:22 in 1f25d0e629 outdated

20-static_assert(MAX_PACKAGE_SIZE * WITNESS_SCALE_FACTOR * 1000 >= MAX_STANDARD_TX_WEIGHT);
21+/** Default maximum total weight of transactions in a package in weight
22+    to allow for context-less checks. This includes room for weight to vsize rounding error
23+    for all included package transactions to not block transactions that would have all
24+    been accepted individually. */
25+static constexpr uint32_t MAX_PACKAGE_WEIGHT = 404'000 +  MAX_PACKAGE_COUNT * (WITNESS_SCALE_FACTOR - 1);

achow101 commented at 9:29 am on September 20, 2023:

In 1f25d0e629820a885a9425d8e0115087f8183cb3 “Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion”

Since weight to vsize is rounded up, this limit does not need to move up. The total weight will never be more than vsize * WITNESS_SCALE_FACTOR. So 404,000 weight units is the correct conversion here.

instagibbs commented at 12:35 pm on September 20, 2023:

done

in src/policy/packages.h:30 in 1f25d0e629 outdated

31+// If a package is to be evaluated, it must be at least as large as the mempool's ancestor/descendant limits,
32+// otherwise transactions that would be individually accepted may be rejected in a package erroneously.
33+// Since a submitted package must be child-with-unconfirmed-parents (all of the transactions are an ancestor
34 // of the child), package limits are ultimately bounded by mempool package limits. Ensure that the
35-// defaults reflect this constraint.
36+// defaults reflect this constraint, including weight to vbyte rounding down.

achow101 commented at 9:37 am on September 20, 2023:

In 1f25d0e629820a885a9425d8e0115087f8183cb3 “Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion”

Weight to vsize rounds up, not down.

instagibbs commented at 12:35 pm on September 20, 2023:

done

in src/policy/packages.h:36 in 1f25d0e629 outdated

39-static_assert(DEFAULT_ANCESTOR_SIZE_LIMIT_KVB >= MAX_PACKAGE_SIZE);
40-static_assert(DEFAULT_DESCENDANT_SIZE_LIMIT_KVB >= MAX_PACKAGE_SIZE);
41+static_assert(MAX_PACKAGE_WEIGHT >=
42+    DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * WITNESS_SCALE_FACTOR * 1000 + MAX_PACKAGE_COUNT * (WITNESS_SCALE_FACTOR - 1));
43+static_assert(MAX_PACKAGE_WEIGHT >=
44+    DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * WITNESS_SCALE_FACTOR * 1000 + MAX_PACKAGE_COUNT * (WITNESS_SCALE_FACTOR - 1));

achow101 commented at 9:38 am on September 20, 2023:

In 1f25d0e629820a885a9425d8e0115087f8183cb3 “Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion”

I think these will be wrong with the weight corrected. Converting vsize to weight may overshoot the weight limit even if the actual weight limit is correct. I think the descendant and ancestor size limits will need to be changed to weight as well?

instagibbs commented at 12:35 pm on September 20, 2023:

done

in test/functional/mempool_sigoplimit.py:172 in 97d53ff4c3 outdated

170+        # But together, it's exceeding limits
171+        packet_test = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex(), tx_child.serialize().hex()])
172+        assert_equal([x["package-error"] for x in packet_test], ["package-mempool-limits", "package-mempool-limits"])
173+
174+        # When we actually try to submit, the parent makes it into the mempool, but the child would exceed ancestor vsize limits
175+        assert_raises_rpc_error(-26, "too-long-mempool-chain", self.nodes[0].submitpackage, [tx_parent.serialize().hex(), tx_child.serialize().hex()])

achow101 commented at 10:00 am on September 20, 2023:

In 97d53ff4c3ede1d1fdc6d93386f14c230b03b98d “Add functional test to catch too large vsize packages”

Can you add a comment explaining what this test is actually testing for? It’s not clear to me why testmempoolaccept would behave differently from submitpackage, and how observing this difference in behavior is useful in a test.

instagibbs commented at 12:35 pm on September 20, 2023:

done

in src/policy/packages.h:57 in 97d53ff4c3 outdated

53@@ -47,7 +54,7 @@ class PackageValidationState : public ValidationState<PackageValidationResult> {
54 
55 /** Context-free package policy checks:
56  * 1. The number of transactions cannot exceed MAX_PACKAGE_COUNT.
57- * 2. The total virtual size cannot exceed MAX_PACKAGE_SIZE.
58+ * 2. The total weight cannot exceed MAX_PACKAGE_KWU.

darosior commented at 11:02 am on September 20, 2023:

nit: MAX_PACKAGE_KWU doesn’t exist anymore

ariard commented at 12:04 pm on September 20, 2023:

should say MAX_PACKAGE_WEIGHT

instagibbs commented at 12:35 pm on September 20, 2023:

done

instagibbs commented at 12:43 pm on September 20, 2023:

done

darosior commented at 11:23 am on September 20, 2023: member

Concept ACK

Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion

While allowing submitted packages to be slightly larger than what
may be allowed in the mempool to allow simpler reasoning
about contextual-less checks vs chain limits.

533660c58a

Bugfix: Pass correct virtual size to CheckPackageLimits bc013fe8e3

in src/txmempool.cpp:207 in 97d53ff4c3 outdated

202                                     std::string &errString) const
203 {
204+    size_t pack_size = package.size();
205+
206+    // Package itself is busting mempool limits; should be rejected even if no staged_ancestors exist
207+    if (pack_size > static_cast<uint64_t>(limits.ancestor_count)) {

ariard commented at 12:21 pm on September 20, 2023:

I think those new package limits are debatable in themselves. If a node operator is setting lower limitancestorcount / limitdescendantcount (e.g 12), a reasonable behavior when we’re accepting over-size package (e.g 25 elements) could be to accept the 12th first component of the package.

I think those new checks are preventing this or at the very least we should document mempool options on the new constraint they’re bringing on package evaluation from my understanding.

instagibbs commented at 12:40 pm on September 20, 2023:

CheckPackageLimits is only called in “AcceptMultipleTransactions”, in other words, we already will try to submit these first parts when they make economic sense.

We should probably do something about the naming of functions at some point to delineate better when they are called, but I’ll consider that future work.

ariard commented at 1:57 pm on September 20, 2023:

From my understanding of offline discussions with PR author, those limits will be only considered when the individual transactions are not satisfying the mempool min fee on their own and they serve as an upper bound check on the max number of elements in the transaction.

Yes documentation and naming of functions can be improved in the future.

instagibbs force-pushed on Sep 20, 2023

instagibbs commented at 12:35 pm on September 20, 2023: member

rebased on latest master with all suggestions included (will discuss @ariard note)

in test/functional/mempool_sigoplimit.py:162 in 5c48501e9f outdated

157+        tx_child = tx_child_dict["tx"]
158+        tx_child.vout.append(CTxOut(amount_for_bare, keys_to_multisig_script([pubkey], k=1)))
159+        tx_child.vout[0].nValue -= amount_for_bare
160+        tx_child.calc_sha256()
161+        tx_child_utxo["txid"] = tx_child.hash
162+        tx_child_utxo["value"] -= Decimal("0.00005000")

glozow commented at 1:37 pm on September 20, 2023:

nit 5c48501e9f1a3da5307b1bee08396598398311e8: it would be cleaner to move some of this into a create_multisig_tx helper to deduplicate the multisig insertion code and make it more difficult to accidentally use a variable from before the tx changes.

instagibbs commented at 3:22 pm on September 20, 2023:

done

in test/functional/mempool_sigoplimit.py:152 in 5c48501e9f outdated

147+        tx_parent_utxo = tx_parent_dict["new_utxo"]
148+        tx_parent = tx_parent_dict["tx"]
149+        tx_parent.vout.append(CTxOut(amount_for_bare, keys_to_multisig_script([pubkey], k=1)))
150+        tx_parent.vout[0].nValue -= amount_for_bare
151+        tx_parent.calc_sha256()
152+        tx_parent_utxo["txid"] = tx_parent.hash

glozow commented at 1:38 pm on September 20, 2023:

nit 5c48501e9f1a3da5307b1bee08396598398311e8: you can also just do this

0        tx_parent_utxo["txid"] = tx_parent.rehash()

instagibbs commented at 3:22 pm on September 20, 2023:

done

in test/functional/mempool_sigoplimit.py:171 in 5c48501e9f outdated

166+        assert parent_individual_testres["allowed"]
167+        # Multisig is counted as MAX_PUBKEYS_PER_MULTISIG = 20 sigops
168+        assert_equal(parent_individual_testres["vsize"], 5000 * 20)
169+
170+        # But together, it's exceeding limits in the *package* context. If sigops adjusted vsize wasn't being checked
171+        # here, it would get farther in validation and give too-long-mempool-chain error instead.

glozow commented at 1:39 pm on September 20, 2023:

nit 5c48501e9f1a3da5307b1bee08396598398311e8

0        # here, it would get further in validation and give too-long-mempool-chain error instead.

instagibbs commented at 3:22 pm on September 20, 2023:

done

glozow commented at 1:39 pm on September 20, 2023: member

ACK 5c48501e9f1a3da5307b1bee08396598398311e8

in src/policy/packages.h:22 in 5c48501e9f outdated

20-static_assert(MAX_PACKAGE_SIZE * WITNESS_SCALE_FACTOR * 1000 >= MAX_STANDARD_TX_WEIGHT);
21+/** Default maximum total weight of transactions in a package in weight
22+    to allow for context-less checks. This must allow a superset of sigops
23+    weighted vsize limited transactions to not disallow transactions we would
24+    have otherwise accepted individually. */
25+static constexpr uint32_t MAX_PACKAGE_WEIGHT = 404'000;

ariard commented at 2:19 pm on September 20, 2023:

Playing with the boundary values 403’999 or 404’001 doesn’t break package_sanitization_tests as one could expect.

instagibbs commented at 3:25 pm on September 20, 2023:

should break if you make it significantly larger, I think, since it’s making txns that aren’t 1 WU. leaving as is for now

in src/txmempool.cpp:208 in 5c48501e9f outdated

203+    size_t pack_size = package.size();
204+
205+    // Package itself is busting mempool limits; should be rejected even if no staged_ancestors exist
206+    if (pack_size > static_cast<uint64_t>(m_limits.ancestor_count)) {
207+        errString = strprintf("package count %u exceeds ancestor count limit [limit: %u]", pack_size, m_limits.ancestor_count);
208+        return false;

ariard commented at 2:21 pm on September 20, 2023:

Following diff doesn’t sound to break functional test mempool_sigoplimit.py.

 0diff --git a/src/txmempool.cpp b/src/txmempool.cpp
 1index 324292c055..21571b9e8a 100644
 2--- a/src/txmempool.cpp
 3+++ b/src/txmempool.cpp
 4@@ -205,7 +205,7 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
 5     // Package itself is busting mempool limits; should be rejected even if no staged_ancestors exist
 6     if (pack_size > static_cast<uint64_t>(m_limits.ancestor_count)) {
 7         errString = strprintf("package count %u exceeds ancestor count limit [limit: %u]", pack_size, m_limits.ancestor_count);
 8-        return false;
 9+        return true;
10     } else if (pack_size > static_cast<uint64_t>(m_limits.descendant_count)) {
11         errString = strprintf("package count %u exceeds descendant count limit [limit: %u]", pack_size, m_limits.descendant_count);
12         return false;

instagibbs commented at 3:34 pm on September 20, 2023:

going to reserve for follow-up test writing

in src/txmempool.cpp:211 in 5c48501e9f outdated

206+    if (pack_size > static_cast<uint64_t>(m_limits.ancestor_count)) {
207+        errString = strprintf("package count %u exceeds ancestor count limit [limit: %u]", pack_size, m_limits.ancestor_count);
208+        return false;
209+    } else if (pack_size > static_cast<uint64_t>(m_limits.descendant_count)) {
210+        errString = strprintf("package count %u exceeds descendant count limit [limit: %u]", pack_size, m_limits.descendant_count);
211+        return false;

ariard commented at 2:25 pm on September 20, 2023:

Same here

 0diff --git a/src/txmempool.cpp b/src/txmempool.cpp
 1index 324292c055..a9d8355aa3 100644
 2--- a/src/txmempool.cpp
 3+++ b/src/txmempool.cpp
 4@@ -208,7 +208,7 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
 5         return false;
 6     } else if (pack_size > static_cast<uint64_t>(m_limits.descendant_count)) {
 7         errString = strprintf("package count %u exceeds descendant count limit [limit: %u]", pack_size, m_limits.descendant_count);
 8-        return false;
 9+        return true;
10     } else if (total_vsize > m_limits.ancestor_size_vbytes) {
11         errString = strprintf("package size %u exceeds ancestor size limit [limit: %u]", total_vsize, m_limits.ancestor_size_vbytes);
12         return false;

instagibbs commented at 3:34 pm on September 20, 2023:

going to reserve for follow-up test writing

in src/txmempool.cpp:217 in 5c48501e9f outdated

212+    } else if (total_vsize > m_limits.ancestor_size_vbytes) {
213+        errString = strprintf("package size %u exceeds ancestor size limit [limit: %u]", total_vsize, m_limits.ancestor_size_vbytes);
214+        return false;
215+    } else if (total_vsize > m_limits.descendant_size_vbytes) {
216+        errString = strprintf("package size %u exceeds descendant size limit [limit: %u]", total_vsize, m_limits.descendant_size_vbytes);
217+        return false;

ariard commented at 2:27 pm on September 20, 2023:

Same here

 0diff --git a/src/txmempool.cpp b/src/txmempool.cpp
 1index 324292c055..20456e250c 100644
 2--- a/src/txmempool.cpp
 3+++ b/src/txmempool.cpp
 4@@ -214,7 +214,7 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
 5         return false;
 6     } else if (total_vsize > m_limits.descendant_size_vbytes) {
 7         errString = strprintf("package size %u exceeds descendant size limit [limit: %u]", total_vsize, m_limits.descendant_size_vbytes);
 8-        return false;
 9+        return true;
10     }

(Mutating the third check on ancestor size vbytes break the functional test)

instagibbs commented at 3:34 pm on September 20, 2023:

going to reserve for follow-up test writing

in test/functional/mempool_sigoplimit.py:143 in 5c48501e9f outdated

135@@ -133,6 +136,49 @@ def test_sigops_limit(self, bytes_per_sigop, num_sigops):
136         assert_equal(entry_parent['descendantcount'], 2)
137         assert_equal(entry_parent['descendantsize'], parent_tx.get_vsize() + sigop_equivalent_vsize)
138 
139+    def test_sigops_package(self):
140+        # Make a 2-transaction package which fails vbyte checks even though
141+        # separately they would work.
142+        self.restart_node(0, extra_args=["-bytespersigop=5000"] + self.extra_args[0])

ariard commented at 2:28 pm on September 20, 2023:

A log info is nice like

 0diff --git a/test/functional/mempool_sigoplimit.py b/test/functional/mempool_sigoplimit.py
 1index aef4602c16..f94af9da73 100755
 2--- a/test/functional/mempool_sigoplimit.py
 3+++ b/test/functional/mempool_sigoplimit.py
 4@@ -140,6 +140,7 @@ class BytesPerSigOpTest(BitcoinTestFramework):
 5         # Make a 2-transaction package which fails vbyte checks even though
 6         # separately they would work.
 7         self.restart_node(0, extra_args=["-bytespersigop=5000"] + self.extra_args[0])
 8+        self.log.info("Test sigops package")
 9 
10         _, pubkey = generate_keypair()
11         amount_for_bare = 50000

instagibbs commented at 3:22 pm on September 20, 2023:

fixed

ariard approved

ariard commented at 2:28 pm on September 20, 2023: member

Code Review ACK 5c48501e9f

Test coverage sounds it can be better.

Handle over-sized (in virtual bytes) packages with no in-mempool ancestors 1a579f9d01

instagibbs force-pushed on Sep 20, 2023

Add functional test to catch too large vsize packages eb8f58f5e4

instagibbs force-pushed on Sep 20, 2023

DrahtBot added the label CI failed on Sep 20, 2023

instagibbs commented at 5:34 pm on September 20, 2023: member

seems to be a spurious failure

DrahtBot removed the label CI failed on Sep 20, 2023

hebasto commented at 8:31 pm on September 20, 2023: member

seems to be a spurious failure

Yeap. The failed job has been restarted.

The failure itself is fixed in #28509.

ariard approved

ariard commented at 12:04 pm on September 21, 2023: member

Re-Code ACK eb8f58f5e

DrahtBot requested review from glozow on Sep 21, 2023

in test/functional/mempool_sigoplimit.py:140 in eb8f58f5e4

135@@ -133,6 +136,45 @@ def test_sigops_limit(self, bytes_per_sigop, num_sigops):
136         assert_equal(entry_parent['descendantcount'], 2)
137         assert_equal(entry_parent['descendantsize'], parent_tx.get_vsize() + sigop_equivalent_vsize)
138 
139+    def test_sigops_package(self):
140+        self.log.info("Test a overly-large sigops-vbyte hits package limits")

glozow commented at 3:27 pm on September 21, 2023:

nit eb8f58f5e4a249d55338304099e8238896d2316d

0       self.log.info("Test a package whose sigop-adjusted vsize hits ancestor size limits")

in test/functional/mempool_sigoplimit.py:158 in eb8f58f5e4

153+            tx_utxo["txid"] = tx.rehash()
154+            tx_utxo["value"] -= Decimal("0.00005000")
155+            return (tx_utxo, tx)
156+
157+        tx_parent_utxo, tx_parent = create_bare_multisig_tx()
158+        tx_child_utxo, tx_child = create_bare_multisig_tx(tx_parent_utxo)

glozow commented at 3:28 pm on September 21, 2023:

nit eb8f58f5e4a249d55338304099e8238896d2316d: unused

0       _, tx_child = create_bare_multisig_tx(tx_parent_utxo)

glozow commented at 3:28 pm on September 21, 2023: member

reACK eb8f58f5e4a249d55338304099e8238896d2316d

glozow requested review from achow101 on Sep 21, 2023

glozow requested review from darosior on Sep 21, 2023

achow101 commented at 3:54 pm on September 21, 2023: member

ACK eb8f58f5e4a249d55338304099e8238896d2316d

DrahtBot removed review request from achow101 on Sep 21, 2023

achow101 merged this on Sep 21, 2023

achow101 closed this on Sep 21, 2023

darosior commented at 4:50 pm on September 21, 2023: member

post-merge light ACK eb8f58f5e4a249d55338304099e8238896d2316d. Still need to catchup with the past 2 years of package relay but this makes sense to me.

luke-jr commented at 3:40 am on September 22, 2023: member

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.

This just seems to make the bug harder to fix? How does it account for sigops?

darosior commented at 11:14 am on September 22, 2023: member

This PR changed the first context-less size check use weight units to make it clear it doesn’t check sigops at this stage. As you know (since it’s your commit), virtual size adjusted for sigops is later accounted for in PackageMempoolChecks: https://github.com/bitcoin/bitcoin/blob/b66f6dcb26906ca8187c7e54735e21168b8101c7/src/validation.cpp#L1302-L1305

What bug are you talking about which is made harder to fix?

Fix virtual size limit enforcement in transaction package context #28471

Reviews

Conflicts