Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mempool test: generate txs larger than the entire mempool #1230

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

amesgen
Copy link
Member

@amesgen amesgen commented Aug 27, 2024

Closes #1226

In addition to the previously valid/invalid txs (purely based on the UTxO ledger rules), we add an optional per-tx limit to the mock block.

As a second step, we generate very large txs that are larger than an entire mempool, in order to test that we do not block when adding them (just like the other txs), which is important as explained in #1226.


One way to validate this PR is to introduce a bug that would cause us to block on such transactions, and observe that the test now indeed catches that.

For example, retrying when the per-tx limited is not satisfied (this is basically what happened in #1168 and was fixed by #1225) via

diff --git a/ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Mempool/Update.hs b/ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Mempool/Update.hs
index 372ea15c29..31abe25fbf 100644
--- a/ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Mempool/Update.hs
+++ b/ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Mempool/Update.hs
@@ -170,25 +170,8 @@ pureTryAddTx ::
   -> TryAddTx blk
 pureTryAddTx cfg wti tx is =
   case runExcept $ txMeasure cfg (isLedgerState is) tx of
-    Left err ->
-      -- The transaction does not have a valid measure (eg its ExUnits is
-      -- greater than what this ledger state allows for a single transaction).
-      --
-      -- It might seem simpler to remove the failure case from 'txMeasure' and
-      -- simply fully validate the tx before determining whether it'd fit in
-      -- the mempool; that way we could reject invalid txs ASAP. However, for a
-      -- valid tx, we'd pay that validation cost every time the node's
-      -- selection changed, even if the tx wouldn't fit. So it'd very much be
-      -- as if the mempool were effectively over capacity! What's worse, each
-      -- attempt would not be using 'extendVRPrevApplied'.
-      TryAddTx
-        Nothing
-        (MempoolTxRejected tx err)
-        (TraceMempoolRejectedTx
-         tx
-         err
-         (isMempoolSize is)
-        )
+    Left _err ->
+      NotEnoughSpaceLeft
     Right txsz
       -- Check for overflow
       --

or here

diff --git a/ouroboros-consensus/src/unstable-mock-block/Ouroboros/Consensus/Mock/Ledger/Block.hs b/ouroboros-consensus/src/unstable-mock-block/Ouroboros/Consensus/Mock/Ledger/Block.hs
index 743e11bc61..e01d8cfe5a 100644
--- a/ouroboros-consensus/src/unstable-mock-block/Ouroboros/Consensus/Mock/Ledger/Block.hs
+++ b/ouroboros-consensus/src/unstable-mock-block/Ouroboros/Consensus/Mock/Ledger/Block.hs
@@ -452,10 +452,7 @@ instance TxLimits (SimpleBlock c ext) where
   -- But not 'maxbound'!, since the mempool sometimes holds multiple blocks worth.
   blockCapacityTxMeasure _cfg _st = IgnoringOverflow simpleBlockCapacity
 
-  txMeasure cfg _st =
-        fmap IgnoringOverflow
-      . checkTxSize (simpleLedgerMockConfig cfg)
-      . simpleGenTx
+  txMeasure _cfg _st = pure . IgnoringOverflow . txSize . simpleGenTx
 
 simpleBlockCapacity :: ByteSize32
 simpleBlockCapacity = ByteSize32 512

will cause many test failures with FailureDeadlock [Labelled (ThreadId []) (Just "main")]' via io-sim's nice deadlock detection.


Stacked on top of #1175 to avoid boring rebase work

@amesgen

This comment was marked as outdated.

@nfrisby nfrisby force-pushed the nfrisby/consolidate-txlimits branch from f15f603 to a7bd1c5 Compare August 28, 2024 18:07
@amesgen amesgen force-pushed the amesgen/test-empty-mempool-nonblocking branch from 12e99f1 to 7a08ed5 Compare August 30, 2024 15:07
@nfrisby nfrisby force-pushed the nfrisby/consolidate-txlimits branch 4 times, most recently from 703c758 to 2f5f06c Compare September 3, 2024 22:42
@nfrisby

This comment was marked as resolved.

@nfrisby nfrisby force-pushed the nfrisby/consolidate-txlimits branch 2 times, most recently from 6ad371d to 9c022f3 Compare September 4, 2024 13:40
Base automatically changed from nfrisby/consolidate-txlimits to main September 4, 2024 15:26
@amesgen amesgen force-pushed the amesgen/test-empty-mempool-nonblocking branch from 7a08ed5 to cb8d629 Compare September 4, 2024 16:44
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor changes

@@ -492,6 +508,7 @@ genValidTxs = go []
(tx, ledger') <- genValidTx ledger
go (tx:txs) (n - 1) ledger'

-- | Generate a valid transaction (but ignoring any per-tx size limits).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is a bit confusing. Does mustBeValid not check the per-tx size limit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not as we use testLedgerConfigNoSizeLimits in this particular case. The current approach is to first generate "non-large" txs, and then use that to set the mempool size (already before this PR) and the per-tx limit.

@amesgen amesgen force-pushed the amesgen/test-empty-mempool-nonblocking branch from cb8d629 to 5fb0684 Compare September 6, 2024 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
better-tests Ideas to improve the tests component-mempool
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

Test that we never block on adding a tx to an empty mempool
2 participants