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

Test that we never block on adding a tx to an empty mempool #1226

Closed
amesgen opened this issue Aug 22, 2024 · 3 comments · Fixed by #1230
Closed

Test that we never block on adding a tx to an empty mempool #1226

amesgen opened this issue Aug 22, 2024 · 3 comments · Fixed by #1230
Assignees
Labels
better-tests Ideas to improve the tests component-mempool

Comments

@amesgen
Copy link
Member

amesgen commented Aug 22, 2024

Since #49, adding a tx to the mempool is guarded by a lock


Therefore, while a tx is waiting to be added to the mempool, no other tx can enter the mempool. In order to avoid a deadlock, it must be guaranteed that adding a tx can not block forever. Such a deadlock is problematic both for the local node (as a restart is necessary), as well as for the entire network, as only empty blocks might otherwise be minted.

This requirement can be deduced from the following two properties:

  1. Transactions are removed from the mempool over time due to transactions becoming invalid (most commonly, due to being included in a minted block).
  2. Add a transaction to an empty mempool never blocks.

As the mempool will eventually become empty, no transaction can block forever.

The goal of this issue is to test the second property.


At the moment (6a8def9), this property is true for the block size capacity due to #21: As the capacity of the mempool is positive, the conditional mempoolsize < mempoolcapacity is always true when adding a tx to an empty mempool.

See #1225 for a recent change restoring this behavior for the ad-hoc reference scripts size limit (added in #1166) after #1168.

As of #1175, this property will be guaranteed by performing the per-tx size checks as part of the check whether to add the tx to the mempool, as the per-tx limit is smaller than the per-block limit, and the mempool capacity encompasses at least one block.


One strategy to test this would be to enrich the TestBlock used in Test.Consensus.Mempool with a per-tx size limit, and then add a simple test which tries to add overly large txs to an empty mempool.

@amesgen amesgen added component-mempool better-tests Ideas to improve the tests labels Aug 22, 2024
@amesgen
Copy link
Member Author

amesgen commented Aug 22, 2024

A refinement of this issue would be to check that adding a tx to a nearly empty mempool can not block. Otherwise, minted blocks might not actually make much use of the block capacity despite high tx load. See #1225 (comment) for a concrete example.

#1175 implicitly has this property as the per-tx limits are significantly smaller than the per-block limits.

@nfrisby
Copy link
Contributor

nfrisby commented Aug 22, 2024

Add a transaction to an empty mempool never block.

adding a tx to a nearly empty mempool can not block

I agree both of those policies are desireable. Which plausible mechanisms are we aware of?

  • PR Consolidate TxLimits #1175 enforces a per-tx limit before blocking. As you stated, that's the key ingredient, assuming the per-tx limit << the block limit.

  • What other mechanisms are possible? I suppose PR Consolidate TxLimits #1175 does that check explicitly, whereas PR Mempool: reject txs that don't fit in an empty mempool #1225 does the same check slightly indirectly, assuming that the Ledger Rules for applying a tx will fail if it exceeds the per-tx limit. (It seems undesireable to simply begin by validating the tx; that could be wasteful for a valid tx that doesn't fit --- it would be validated again everytime the selection changes without making sufficient space in the mempool... which shouldn't happen more than once or so for some tx? PR Mempool: reject txs that don't fit in an empty mempool #1225 avoids that pitfall by checking the size first and only invoking validation if the size indicates the tx will be rejected.)

@amesgen
Copy link
Member Author

amesgen commented Aug 22, 2024

Another mechanism is using the mempoolsize < mempoolcapacity conditional for deciding whether to try adding the tx, as is the case for the tx byte size on current main

@amesgen amesgen self-assigned this Aug 22, 2024
github-merge-queue bot pushed a commit that referenced this issue Sep 24, 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
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
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
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: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants