Fix virtual size limit enforcement in transaction package context #28471

instagibbs · 2023-09-13T17:34:31Z

(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 · 2023-09-13T17:34:34Z

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.

glozow

Concept ACK, thanks for taking this over!

doc/policy/packages.md

src/txmempool.cpp

dergoegge · 2023-09-14T10:59:02Z

Can you add tests for the bug fixes?

instagibbs · 2023-09-14T20:34:32Z

Can you add tests for the bug fixes?

Added.

glozow

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

src/policy/packages.h

test/functional/mempool_sigoplimit.py

src/txmempool.cpp

src/policy/packages.h

src/test/txpackage_tests.cpp

glozow

ACK 3a4adfb with a doc nit

doc/policy/packages.md

glozow · 2023-09-15T13:59:14Z

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

glozow · 2023-09-15T14:25:48Z

doc/policy/packages.md

@@ -18,7 +18,7 @@ tip or some preceding transaction in the package.

 The following rules are enforced for all packages:

-* Packages cannot exceed `MAX_PACKAGE_COUNT=25` count and `MAX_PACKAGE_SIZE=101KvB` total size
+* Packages cannot exceed `MAX_PACKAGE_COUNT=25` count and `MAX_PACKAGE_KWU=404` total weight
   (#20833)

   - *Rationale*: This is already enforced as mempool ancestor/descendant limits. If


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. (#22097 (comment) 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."

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.

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.

ariard · 2023-09-19T17:58:13Z

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

src/policy/packages.h

instagibbs · 2023-09-20T12:35:52Z

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

glozow

ACK 5c48501

test/functional/mempool_sigoplimit.py

ariard

Code Review ACK 5c48501

Test coverage sounds it can be better.

ariard · 2023-09-20T14:19:02Z

src/policy/packages.h

+    to allow for context-less checks. This must allow a superset of sigops
+    weighted vsize limited transactions to not disallow transactions we would
+    have otherwise accepted individually. */
+static constexpr uint32_t MAX_PACKAGE_WEIGHT = 404'000;


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

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

ariard · 2023-09-20T14:21:08Z

src/txmempool.cpp

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


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

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

going to reserve for follow-up test writing

ariard · 2023-09-20T14:25:35Z

src/txmempool.cpp

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


Same here

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

going to reserve for follow-up test writing

ariard · 2023-09-20T14:27:20Z

src/txmempool.cpp

+        return false;
+    } else if (total_vsize > m_limits.descendant_size_vbytes) {
+        errString = strprintf("package size %u exceeds descendant size limit [limit: %u]", total_vsize, m_limits.descendant_size_vbytes);
+        return false;


Same here

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

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

going to reserve for follow-up test writing

test/functional/mempool_sigoplimit.py

…stors

instagibbs · 2023-09-20T17:34:51Z

seems to be a spurious failure

hebasto · 2023-09-20T20:31:49Z

seems to be a spurious failure

Yeap. The failed job has been restarted.

The failure itself is fixed in #28509.

ariard

Re-Code ACK eb8f58f

glozow

reACK eb8f58f

glozow · 2023-09-21T15:27:49Z

test/functional/mempool_sigoplimit.py

@@ -133,6 +136,45 @@ def test_sigops_limit(self, bytes_per_sigop, num_sigops):
        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")


nit eb8f58f

Suggested change

self.log.info("Test a overly-large sigops-vbyte hits package limits")

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

glozow · 2023-09-21T15:28:14Z

test/functional/mempool_sigoplimit.py

+            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)


nit eb8f58f: unused

Suggested change

tx_child_utxo, tx_child = create_bare_multisig_tx(tx_parent_utxo)

_, tx_child = create_bare_multisig_tx(tx_parent_utxo)

achow101 · 2023-09-21T15:54:31Z

ACK eb8f58f

darosior · 2023-09-21T16:50:30Z

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

luke-jr · 2023-09-22T03:40:27Z

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 · 2023-09-22T11:14:04Z

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:

bitcoin/src/validation.cpp

Lines 1302 to 1305 in b66f6dc

    
           std::string err_string; 
        
           if (txns.size() > 1 && !PackageMempoolChecks(txns, m_total_vsize, package_state)) { 
        
               return PackageMempoolAcceptResult(package_state, std::move(results)); 
        
           }

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

…n package context eb8f58f Add functional test to catch too large vsize packages (Greg Sanders) 1a579f9 Handle over-sized (in virtual bytes) packages with no in-mempool ancestors (Greg Sanders) bc013fe Bugfix: Pass correct virtual size to CheckPackageLimits (Luke Dashjr) 533660c Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion (Greg Sanders) Pull request description: (Alternative) Minimal subset of bitcoin#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 eb8f58f ariard: Re-Code ACK eb8f58f glozow: reACK eb8f58f Tree-SHA512: 1b5cca1a526207e25d387fcc29a776a3198c3a013dc2b35c6275b9d5a64db2476c154ebf52e3a1aed0b9924c75613f21a946577aa760de28cadf0c9c7f68dc39

…sion 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. Github-Pull: bitcoin#28471 Rebased-From: 533660c

Github-Pull: bitcoin#28471 Rebased-From: bc013fe

…stors Github-Pull: bitcoin#28471 Rebased-From: 1a579f9

Github-Pull: bitcoin#28471 Rebased-From: eb8f58f

instagibbs mentioned this pull request Sep 13, 2023

Add package evaluation fuzzer #28450

Merged

glozow reviewed Sep 14, 2023

View reviewed changes

doc/policy/packages.md Outdated Show resolved Hide resolved

glozow reviewed Sep 14, 2023

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

glozow mentioned this pull request Sep 14, 2023

Package Relay Project Tracking #27463

Open

53 tasks

glozow reviewed Sep 14, 2023

View reviewed changes

src/txmempool.cpp Show resolved Hide resolved

glozow mentioned this pull request Sep 14, 2023

validate package transactions with their in-package ancestor sets #26711

Closed

DrahtBot mentioned this pull request Sep 14, 2023

Remove MemPoolAccept::m_limits to avoid mutating it in package evaluation #28472

Merged

instagibbs force-pushed the package_eval_sigops branch from e4640da to 7193055 Compare September 14, 2023 20:32

glozow reviewed Sep 15, 2023

View reviewed changes

instagibbs force-pushed the package_eval_sigops branch from 7193055 to 0ad8369 Compare September 15, 2023 13:26

glozow reviewed Sep 15, 2023

View reviewed changes

src/policy/packages.h Outdated Show resolved Hide resolved

src/test/txpackage_tests.cpp Show resolved Hide resolved

instagibbs force-pushed the package_eval_sigops branch from 0ad8369 to 3a4adfb Compare September 15, 2023 13:40

glozow reviewed Sep 15, 2023

View reviewed changes

doc/policy/packages.md Outdated Show resolved Hide resolved

instagibbs force-pushed the package_eval_sigops branch from 3a4adfb to 09b64e5 Compare September 15, 2023 13:57

glozow mentioned this pull request Sep 15, 2023

validation: Move package acceptance size limit from KvB to WU #22097

Closed

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

glozow reviewed Sep 15, 2023

View reviewed changes

glozow added the Bug label Sep 15, 2023

instagibbs force-pushed the package_eval_sigops branch 2 times, most recently from b6679c0 to 97d53ff Compare September 15, 2023 15:09

DrahtBot added CI failed and removed CI failed labels Sep 15, 2023

glozow requested review from darosior and ariard September 19, 2023 16:19

achow101 reviewed Sep 20, 2023

View reviewed changes

src/policy/packages.h Outdated Show resolved Hide resolved

src/policy/packages.h Outdated Show resolved Hide resolved

src/policy/packages.h Outdated Show resolved Hide resolved

glozow reviewed Sep 20, 2023

View reviewed changes

test/functional/mempool_sigoplimit.py Outdated Show resolved Hide resolved

test/functional/mempool_sigoplimit.py Outdated Show resolved Hide resolved

test/functional/mempool_sigoplimit.py Outdated Show resolved Hide resolved

ariard approved these changes Sep 20, 2023

View reviewed changes

Handle over-sized (in virtual bytes) packages with no in-mempool ance…

1a579f9

…stors

instagibbs force-pushed the package_eval_sigops branch from 5c48501 to ee31e72 Compare September 20, 2023 15:22

Add functional test to catch too large vsize packages

eb8f58f

instagibbs force-pushed the package_eval_sigops branch from ee31e72 to eb8f58f Compare September 20, 2023 15:34

DrahtBot added the CI failed label Sep 20, 2023

DrahtBot removed the CI failed label Sep 20, 2023

ariard approved these changes Sep 21, 2023

View reviewed changes

DrahtBot requested a review from glozow September 21, 2023 12:04

glozow reviewed Sep 21, 2023

View reviewed changes

glozow requested review from achow101 and darosior September 21, 2023 15:28

DrahtBot removed the request for review from achow101 September 21, 2023 15:54

achow101 merged commit 2303fd2 into bitcoin:master Sep 21, 2023
16 checks passed

luke-jr mentioned this pull request Sep 22, 2023

Bugfix: Package relay / bytespersigop checks #28345

Closed

glozow mentioned this pull request Sep 28, 2023

rpc: allow submitpackage to be called outside of regtest #27609

Merged

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2023

Bugfix: Pass correct virtual size to CheckPackageLimits

1d8b2d9

Github-Pull: bitcoin#28471 Rebased-From: bc013fe

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2023

Handle over-sized (in virtual bytes) packages with no in-mempool ance…

8f0bc19

…stors Github-Pull: bitcoin#28471 Rebased-From: 1a579f9

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2023

Add functional test to catch too large vsize packages

c6dfebd

Github-Pull: bitcoin#28471 Rebased-From: eb8f58f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix virtual size limit enforcement in transaction package context #28471

Fix virtual size limit enforcement in transaction package context #28471

instagibbs commented Sep 13, 2023 •

edited

DrahtBot commented Sep 13, 2023 •

edited

glozow left a comment

dergoegge commented Sep 14, 2023

instagibbs commented Sep 14, 2023

glozow left a comment

glozow left a comment

glozow commented Sep 15, 2023

glozow Sep 15, 2023 •

edited

instagibbs Sep 15, 2023

glozow Sep 20, 2023

ariard commented Sep 19, 2023

instagibbs commented Sep 20, 2023

glozow left a comment

ariard left a comment

ariard Sep 20, 2023

instagibbs Sep 20, 2023

ariard Sep 20, 2023

instagibbs Sep 20, 2023

ariard Sep 20, 2023

instagibbs Sep 20, 2023

ariard Sep 20, 2023

instagibbs Sep 20, 2023

instagibbs commented Sep 20, 2023

hebasto commented Sep 20, 2023

ariard left a comment

glozow left a comment

glozow Sep 21, 2023

glozow Sep 21, 2023

achow101 commented Sep 21, 2023

darosior commented Sep 21, 2023

luke-jr commented Sep 22, 2023

darosior commented Sep 22, 2023

	self.log.info("Test a overly-large sigops-vbyte hits package limits")
	self.log.info("Test a package whose sigop-adjusted vsize hits ancestor size limits")

	tx_child_utxo, tx_child = create_bare_multisig_tx(tx_parent_utxo)
	_, tx_child = create_bare_multisig_tx(tx_parent_utxo)

Fix virtual size limit enforcement in transaction package context #28471

Fix virtual size limit enforcement in transaction package context #28471

Conversation

instagibbs commented Sep 13, 2023 • edited

DrahtBot commented Sep 13, 2023 • edited

Reviews

Conflicts

glozow left a comment

Choose a reason for hiding this comment

dergoegge commented Sep 14, 2023

instagibbs commented Sep 14, 2023

glozow left a comment

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

glozow commented Sep 15, 2023

glozow Sep 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented Sep 19, 2023

instagibbs commented Sep 20, 2023

glozow left a comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Sep 20, 2023

hebasto commented Sep 20, 2023

ariard left a comment

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Sep 21, 2023

darosior commented Sep 21, 2023

luke-jr commented Sep 22, 2023

darosior commented Sep 22, 2023

instagibbs commented Sep 13, 2023 •

edited

DrahtBot commented Sep 13, 2023 •

edited

glozow Sep 15, 2023 •

edited