Conway: also enable mempool ref script size limit #1168
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amesgen
requested review from
nfrisby,
jasagredo,
fraser-iohk,
dnadales and
RenateEilers
as code owners
amesgen
force-pushed
the
amesgen/conway-mempool-refscript-limit
branch
from
9a3d23b to
0d61086
Compare
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 1, 2024
Making sure that we never forge blocks that violate the new max ref scripts size limit per block that is enforced at the ledger level starting in Conway: IntersectMBO/cardano-ledger#4450 Also see #1168 for restoring a similar limit for the size of the total mempool in Conway.
Merged
jasagredo
approved these changes
Jul 3, 2024
|
FTR: This PR will very likely be closed in favor of #1175 |
amesgen
force-pushed
the
amesgen/conway-mempool-refscript-limit
branch
from
0d61086 to
2227d76
Compare
nfrisby
approved these changes
Aug 20, 2024
There was a problem hiding this comment.
Reviewed in detail in a Slack conversation. It's particularly helpful to note that the benefit mentioned in the changelog entry is actually implemented by code not present in the diff.
PR #1175 will supercede this, but likely not before Chang; it has too big of a footprint.
amesgen
force-pushed
the
amesgen/conway-mempool-refscript-limit
branch
from
2227d76 to
c6ba2a0
Compare
This was referenced Aug 21, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 23, 2024
Follow-up to #1168 that makes sure that adding a tx exceeding the per-tx limit does not cause a deadlock which prevents txs from being added to the mempool until the node is restarted. We accomplish this by validating such transactions and relying on the per-tx limit to reject them.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 4, 2024
Closes #1177. Beyond that Issue, this PR also changes the mempool's definition of full. The mempool will accepte transactions until the tx chosen to next enter the mempool would cause any component of the capacity vector (byte size, exunit steps, exunit mems, ref script byte size) to exceed its limit. The default capacity is chosen as twice the capacity of a block. Such a capacity can admit more txs than could fit into two back-to-back blocks, but no single component (eg ExUnits) of the mempool's contents' size could exceed what could fit in two blocks. The mempool capacity has two consequences. - It implements _latency hiding_: txs can cover the network a little in advance, so that two blocks on a chain whose slots are very close together could still both be full (if there are enough submitted txs). - It bounds the potential burst of CPU load necessary to fill an empty mempool, which otherwise could be a DoS vector. Also closes #1168 by superseding the "ad-hoc" mempool reference scripts capacity limit. This PR also moves the logic for choosing which prefix of the mempool to put in the block out of the forging functions and into the NodeKernel forging thread --- the mempool snapshot is the perfect place to do that, since tx measurements (each using the correct ledger state) are already determined.
github-merge-queue bot
pushed a commit
that referenced
this pull request
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also implementing #1166 (comment), but in a more fine-grained fashion
The insight here is that
txRefScriptSizeis implemented usinggetBabbageTxDictfor Shelley-based blocks, see the usage sites of that function inOuroboros.Consensus.Mempool.Impl.CommonandOuroboros.Consensus.Mempool.Update.