Fix virtual size limit enforcement in transaction package context #28471

pull instagibbs wants to merge 4 commits into bitcoin:master from instagibbs:package_eval_sigops changing 8 files +99 −28
  1. instagibbs commented at 5:34 pm on September 13, 2023: member

    (Alternative) Minimal subset of #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.

  2. 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.

  3. 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
  4. glozow commented at 8:40 am on September 14, 2023: member
    Concept ACK, thanks for taking this over!
  5. 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
  6. 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
  7. dergoegge commented at 10:59 am on September 14, 2023: member
    Can you add tests for the bug fixes?
  8. instagibbs force-pushed on Sep 14, 2023
  9. instagibbs commented at 8:34 pm on September 14, 2023: member

    Can you add tests for the bug fixes?

    Added.

  10. 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.
  11. 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
  12. 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
  13. 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
  14. 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
  15. 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.
  16. instagibbs force-pushed on Sep 15, 2023
  17. 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
  18. 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:

  19. instagibbs force-pushed on Sep 15, 2023
  20. 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
  21. glozow commented at 1:56 pm on September 15, 2023: member
    ACK 3a4adfbfa6a947721bf140a534fb2aefdf0922fd with a doc nit
  22. instagibbs force-pushed on Sep 15, 2023
  23. glozow commented at 1:59 pm on September 15, 2023: member
    Could we get a review from maybe @luke-jr or @ariard?
  24. glozow added the label TX fees and policy on Sep 15, 2023
  25. 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.
  26. glozow added the label Bug on Sep 15, 2023
  27. instagibbs force-pushed on Sep 15, 2023
  28. instagibbs force-pushed on Sep 15, 2023
  29. DrahtBot added the label CI failed on Sep 15, 2023
  30. DrahtBot removed the label CI failed on Sep 15, 2023
  31. glozow requested review from darosior on Sep 19, 2023
  32. glozow requested review from ariard on Sep 19, 2023
  33. ariard commented at 5:58 pm on September 19, 2023: contributor
    Concept ACK on replacing MAX_PACKAGE_SIZE to MAX_PACKAGE_WEIGHT and other changes sounds good to me, will code review more soon.
  34. 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
  35. 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
  36. 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
  37. 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
  38. 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
  39. darosior commented at 11:23 am on September 20, 2023: member
    Concept ACK
  40. 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
  41. Bugfix: Pass correct virtual size to CheckPackageLimits bc013fe8e3
  42. 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.

  43. instagibbs force-pushed on Sep 20, 2023
  44. instagibbs commented at 12:35 pm on September 20, 2023: member
    rebased on latest master with all suggestions included (will discuss @ariard note)
  45. 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
  46. 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
  47. 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
  48. glozow commented at 1:39 pm on September 20, 2023: member
    ACK 5c48501e9f1a3da5307b1bee08396598398311e8
  49. 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
  50. 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
  51. 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
  52. 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
  53. 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
  54. ariard approved
  55. ariard commented at 2:28 pm on September 20, 2023: contributor

    Code Review ACK 5c48501e9f

    Test coverage sounds it can be better.

  56. Handle over-sized (in virtual bytes) packages with no in-mempool ancestors 1a579f9d01
  57. instagibbs force-pushed on Sep 20, 2023
  58. Add functional test to catch too large vsize packages eb8f58f5e4
  59. instagibbs force-pushed on Sep 20, 2023
  60. DrahtBot added the label CI failed on Sep 20, 2023
  61. instagibbs commented at 5:34 pm on September 20, 2023: member
    seems to be a spurious failure
  62. DrahtBot removed the label CI failed on Sep 20, 2023
  63. 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.

  64. ariard approved
  65. ariard commented at 12:04 pm on September 21, 2023: contributor
    Re-Code ACK eb8f58f5e
  66. DrahtBot requested review from glozow on Sep 21, 2023
  67. 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")
  68. 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)
  69. glozow commented at 3:28 pm on September 21, 2023: member
    reACK eb8f58f5e4a249d55338304099e8238896d2316d
  70. glozow requested review from achow101 on Sep 21, 2023
  71. glozow requested review from darosior on Sep 21, 2023
  72. achow101 commented at 3:54 pm on September 21, 2023: member
    ACK eb8f58f5e4a249d55338304099e8238896d2316d
  73. DrahtBot removed review request from achow101 on Sep 21, 2023
  74. achow101 merged this on Sep 21, 2023
  75. achow101 closed this on Sep 21, 2023
  76. 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.
  77. 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?

  78. 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?

  79. Frank-GER referenced this in commit da842f271a on Sep 25, 2023
  80. sidhujag referenced this in commit 12b1a34dce on Sep 26, 2023
  81. luke-jr referenced this in commit 189f0bff22 on Oct 20, 2023
  82. luke-jr referenced this in commit 1d8b2d966c on Oct 20, 2023
  83. luke-jr referenced this in commit 8f0bc19767 on Oct 20, 2023
  84. luke-jr referenced this in commit c6dfebd149 on Oct 20, 2023
  85. bitcoin locked this on Sep 21, 2024


instagibbs DrahtBot glozow dergoegge ariard achow101 darosior hebasto luke-jr


darosior

Labels
Bug TX fees and policy

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me