Made MEMPOOL call LEDGER #4880

Soupstraw · 2025-02-07T14:26:43Z

Description

Checklist

Commits in meaningful sequence and with useful messages
Tests added or updated when needed
CHANGELOG.md files updated for packages with externally visible changes

New section is never added with the code changes. (See RELEASING.md)
Versions updated in .cabal and CHANGELOG.md files when necessary, according to the
versioning process.
Version bounds in .cabal files updated when necessary

If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
Code formatted (use scripts/fourmolize.sh)
Cabal files formatted (use scripts/cabal-format.sh)
hie.yaml updated (use scripts/gen-hie.sh)
Self-reviewed the diff

Soupstraw · 2025-02-10T16:21:00Z

eras/conway/impl/testlib/Test/Cardano/Ledger/Conway/Binary/RoundTrip.hs

@@ -84,7 +84,6 @@ instance RuleListEra ConwayEra where
       , "GOV"
       , "LEDGER"
       , "LEDGERS"
-       , "MEMPOOL"


If I leave this here, I get a compiler error that says that it can't find an EraRuleFailure instance for the MEMPOOL predicate failures. However I can't add EraRuleFailure type instance for MEMPOOL because that violates type injectivity since MEMPOOL shares the predicate failure with LEDGER.

teodanciu

I understand that applyTxOpts calls directly MEMPOOL rule, which calls LEDGER.
Some things I don't understand:

Is it necessary to stop emitting events from the MEMPOOL rule? If yes, then probably the mempoolevents-related tests in LedgerSpec should be deleted
Why do we need the ApplyTxRule associated type in ApplyTx?

I'm sure it's because I'm missing some details, we can discuss it in the meeting if it's easier!

teodanciu · 2025-02-12T13:30:50Z

eras/conway/impl/testlib/Test/Cardano/Ledger/Conway/Imp/LedgerSpec.hs

@@ -256,7 +254,7 @@ spec = do
        Left e ->
          assertFailure $ "Unexpected failure while applyingTx: " <> show tx <> ": " <> show e
        Right (_, evs) ->
-          length [ev | ev@(MempoolEvent (ConwayMempoolEvent _)) <- evs] `shouldBe` 1
+          length [ev | ev <- evs] `shouldBe` 1


I think the list comprehension is unnecessary

Suggested change

length [ev | ev <- evs] `shouldBe` 1

length evs `shouldBe` 1

teodanciu · 2025-02-12T13:41:09Z

eras/shelley/impl/src/Cardano/Ledger/Shelley/API/Mempool.hs

  ) =>
  ApplyTx era
  where
+  type ApplyTxRule era :: Type


Does this associated type improve on the version without it, or is it necessary for the solution to work?

teodanciu · 2025-02-12T13:54:11Z

eras/conway/impl/testlib/Test/Cardano/Ledger/Conway/Imp/LedgerSpec.hs

@@ -235,7 +233,7 @@ spec = do
          (LedgersEnv slotNo epochNo pp account)
          ls
          (Seq.singleton tx)
-      let mempoolEvents = [ev | LedgerEvent ev@(MempoolEvent (ConwayMempoolEvent _)) <- evs]
+      let mempoolEvents = [ev | LedgerEvent ev <- evs]


If Mempool is no longer emitting events, then I'm not sure if Mempool events test makes sense at all? It just counts all the events emitted from Ledger, nothing to do with the Mempool rule. The count is also wrong right now and the tests should be failing

teodanciu · 2025-02-12T13:56:14Z

eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Ledger.hs

@@ -286,22 +281,19 @@ data ConwayLedgerEvent era
  = UtxowEvent (Event (EraRule "UTXOW" era))
  | CertsEvent (Event (EraRule "CERTS" era))
  | GovEvent (Event (EraRule "GOV" era))
-  | MempoolEvent (Event (EraRule "MEMPOOL" era))


Can we remove this without breaking node-to-client protocol?
If so, we should probably reflect it in the CHANGELOG

Yeah we can, ledger events aren't part of the node-to-client protocol.
However, after giving more thought to how actual mempool uses ledger, made me realize there is no point in making events available in ApplyTx, since they will never be used.
Adding a changelog about it the removal is a good idea.

teodanciu · 2025-02-12T13:59:50Z

eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Ledger.hs

  , BaseM (someLEDGER era) ~ ShelleyBase
  , STS (someLEDGER era)
  ) =>
  TransitionRule (someLEDGER era)
 ledgerTransition = do
  TRC
-    ( le@(LedgerEnv slot mbCurEpochNo _txIx pp account mempool)
-      , ls@(LedgerState utxoState certState)
+    ( LedgerEnv slot mbCurEpochNo _txIx pp account _mempool


There is no point to keep this ledgerMempool Bool around in LedgerEnv, no?

Indeed there is no point.

Fixed in #4890

lehins

As I've mentioned in the meeting on Monday, I didn't think that we need this whole complication with a type family. I went ahead and experimented with it today and indeed I converged on a much simpler approach that avoids whole lot of unnecessary nonsense that didn't need to be there to begin with.
See my PR #4890 It uses the first commit from this PR.

lehins · 2025-02-13T00:19:19Z

eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Ledger.hs

@@ -286,22 +281,19 @@ data ConwayLedgerEvent era
  = UtxowEvent (Event (EraRule "UTXOW" era))
  | CertsEvent (Event (EraRule "CERTS" era))
  | GovEvent (Event (EraRule "GOV" era))
-  | MempoolEvent (Event (EraRule "MEMPOOL" era))


Yeah we can, ledger events aren't part of the node-to-client protocol.
However, after giving more thought to how actual mempool uses ledger, made me realize there is no point in making events available in ApplyTx, since they will never be used.
Adding a changelog about it the removal is a good idea.

lehins · 2025-02-13T05:43:34Z

eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Ledger.hs

  , BaseM (someLEDGER era) ~ ShelleyBase
  , STS (someLEDGER era)
  ) =>
  TransitionRule (someLEDGER era)
 ledgerTransition = do
  TRC
-    ( le@(LedgerEnv slot mbCurEpochNo _txIx pp account mempool)
-      , ls@(LedgerState utxoState certState)
+    ( LedgerEnv slot mbCurEpochNo _txIx pp account _mempool


Indeed there is no point.

Soupstraw · 2025-02-13T11:34:22Z

Closing in favor of #4890

Soupstraw force-pushed the jj/invert-mempool branch from b71a576 to 45c690e Compare February 10, 2025 14:19

Soupstraw marked this pull request as ready for review February 10, 2025 14:27

Soupstraw requested a review from a team as a code owner February 10, 2025 14:27

Made MEMPOOL call LEDGER

afba950

Soupstraw force-pushed the jj/invert-mempool branch from 45c690e to afba950 Compare February 10, 2025 14:27

Soupstraw commented Feb 10, 2025

View reviewed changes

Soupstraw force-pushed the jj/invert-mempool branch from c9901fd to fb73f98 Compare February 11, 2025 12:15

associated type approach

02dba29

Soupstraw force-pushed the jj/invert-mempool branch from fb73f98 to 02dba29 Compare February 11, 2025 12:20

teodanciu reviewed Feb 12, 2025

View reviewed changes

lehins mentioned this pull request Feb 13, 2025

Invert mempool #4890

Open

9 tasks

lehins requested changes Feb 13, 2025

View reviewed changes

Soupstraw closed this Feb 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made MEMPOOL call LEDGER #4880

Made MEMPOOL call LEDGER #4880

Soupstraw commented Feb 7, 2025

Soupstraw Feb 10, 2025 •

edited

Loading

teodanciu left a comment

teodanciu Feb 12, 2025

teodanciu Feb 12, 2025

teodanciu Feb 12, 2025

teodanciu Feb 12, 2025

lehins Feb 13, 2025

teodanciu Feb 12, 2025

lehins Feb 13, 2025

lehins Feb 13, 2025

lehins left a comment

lehins Feb 13, 2025

lehins Feb 13, 2025

Soupstraw commented Feb 13, 2025

Made MEMPOOL call LEDGER #4880

Made MEMPOOL call LEDGER #4880

Conversation

Soupstraw commented Feb 7, 2025

Description

Checklist

Soupstraw Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

teodanciu 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

lehins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Soupstraw commented Feb 13, 2025

Soupstraw Feb 10, 2025 •

edited

Loading