Redesign of Tx/TxInBlock #2379

nc6 · 2021-07-12T13:21:42Z

This comes out of discussions with the consensus team. Rather than sending basic Shelley transactions around and computing the IsValidating field at the node level, we instead require consumers to compute their own IsValidating field. This is then verified as usual, and the node is required to verify this. Depending on the configuration, the node may choose to do different things with the transaction if the flag is set incorrectly.

This may be reviewed commit by commit; they should all stand alone.

nfrisby

This looks good to me. I'll integrate against it today on my branch and report back.

In one of the commit messages:

Drop 'TxInBlock' and use Core.Tx family.

Since Tx will now be what is submitted by the wallet/node, ...

Is there a typo in that sentence fragment? It reads like "now that X is true" but X already was true. I was expecting something like "now that Tx (ie what the wallet/node submits) contains the same data as TxInBlock, ...`

nfrisby · 2021-07-12T14:59:14Z

alonzo/impl/src/Cardano/Ledger/Alonzo/Rules/Bbody.hs

    -- Conditions to define the rule in this Era
    HasField "_d" (Core.PParams era) UnitInterval,
    HasField "_maxBlockExUnits" (Core.PParams era) ExUnits,
    Era era, -- supplies WellFormed HasField, and Crypto constraints
    Era.TxSeq era ~ Alonzo.TxSeq era,
-    Era.TxInBlock era ~ Alonzo.ValidatedTx era,
+    Core.Tx era ~ Alonzo.ValidatedTx era,


I think we discussed this on a call, but I'll risk repeating my opinion. ValidatedTx seems like a misnomer. EG Core.Tx is the type of a transaction that we just deserialized from the network; we haven't had a chancee to validate it beyond "does it deserialize".

On the other hand, it does carry a flag, so at least someone is claiming to have validated it.

No particular action requested, just pointing out an awkwardness/opportunity for confusion.

Edit: looking ahead, I see that we'll indirectly have Validated Alonzo.ValidatedTx in the new mempool API, eg.

Yes, sorry, the thing I forgot to do in this branch was rename ValidatedTx, I agree that it's now a bad name!

nfrisby

I have a branch on which the non-testing code compiles against this PR's branch. I'm requesting changes based on a couple discovered requirements re the Validated newtype.

shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/API/Mempool.hs

nc6 · 2021-07-19T06:45:25Z

@nfrisby Is this ok for you now?

nfrisby · 2021-07-19T13:26:38Z

@nfrisby Is this ok for you now?

I think so. I'll be able to confirm that today.

JaredCorduan · 2021-07-19T20:13:14Z

cardano-ledger-test/src/Test/Cardano/Ledger/Examples/TwoPhaseValidation.hs

-trustMe :: Bool -> Core.Tx A -> ValidatedTx A
-trustMe v (Core.Tx b w a) = ValidatedTx b w (IsValidating v) a
+trustMe :: Bool -> ValidatedTx A -> ValidatedTx A
+trustMe iv' (ValidatedTx b w _ m) = ValidatedTx b w (IsValidating iv') m


I think you do not need this function anymore, the tests can just stamp the correct IsValidating field directly.

JaredCorduan

From my end, I think this looks great! It's a nice simplification.

nfrisby · 2021-07-20T01:33:02Z

OK, I have my downstream PR working with this PR right now, except for one last hurdle.

One of my tests isn't compiling, and I think the fix would be to change the type of sleTx to Core.Tx. IE the sleTx for Alonzo should have the flag in it.

https://github.com/input-output-hk/cardano-ledger-specs/blob/ea07de488ca85f1be17e7de7a1d12d4c7eeb8496/shelley/chain-and-ledger/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Examples/Consensus.hs#L78-L81

https://github.com/input-output-hk/cardano-ledger-specs/blob/810298f87d91a8ab6573d10710baedf35825a3b5/alonzo/test/lib/Test/Cardano/Ledger/Alonzo/Examples/Consensus.hs#L59

(There is already an exampleTransactionInBlock in that module, and I think the sleTx field should now be set to that.)

Edit: if the module hierarchy makes this awkward (is cardano-ledger-core a build-dep of shelley-ledger-spec?), then I suppose you could just tell me how to convert from the current type of sleTx to Core.Tx (AlonzoEra c) and I could update my test code to do that.

The theme behind this change is that consensus (and the node, and components above this) will regard an Alonzo Tx to be the full Tx, including the `IsValidating` tag. Where previously we exposed the unvarnished `Tx` and kept the `TxInBlock` as a ledger-internal detail, we will now be doing the opposite - exposing the full `TxInBlock`.

Since `Tx` will now be what is submitted by the wallet/node, there is no need for a separate `TxInBlock` concept. Instead, `Tx` becomes a type family (since it _will_ vary between eras) and everything is defined in those terms. As part of this, we alter the `SupportsSegWit` type class to reflect now the isomorphism between `TxSeq` and `Seq Tx`.

This serialisation is only for use when transmitting Tx from node to node, or wallet to node.

Previously, we used the Tx/TxInBlock distinction to also embody the fact that a Tx had been validated, and hence future applications of that Tx could skip a certain number of checks, an important performance concern. Now we lack this distinction (which in any case was somewhat a conflation of concerns), so we introduce the `Validated` newtype to reflect this, and rewrite the Mempool API to reflect this.

We will drop this when downstream projects are updated.

Most of this is trivial. We modify `EraGen` to include a `constructTx`, which creates a transaction from its constituent parts.

Approximately, for the 'TranslateEra' instance.

nfrisby · 2021-07-20T19:26:40Z

~~I can add flags to get around it, but currently my source-repo-driven-nix-build of cardano-ledger-alonzo is failing with~~

<no location info>: warning: [-Wunused-packages]
    The following packages were specified via -package or -package-id flags,
    but were not needed for compilation:
      - mtl-2.2.2
      - array-0.5.4.0

Edit: Nevermind, I hadn't noticed that the Plutus source-repo dep needed to be updated. All clear now.

nfrisby

This now works for me. Thanks! Approved.

The only failures I'm now seeing are the result of the new serialization of Alonzo txs, totally expected.

nc6 requested review from jasagredo, nfrisby and TimSheard July 12, 2021 13:21

nc6 force-pushed the nc/alonzo-tx branch from 5a777ad to 08d7780 Compare July 12, 2021 14:17

nfrisby reviewed Jul 12, 2021

View reviewed changes

nfrisby requested changes Jul 14, 2021

View reviewed changes

shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/API/Mempool.hs Show resolved Hide resolved

nc6 force-pushed the nc/alonzo-tx branch 2 times, most recently from b3d337d to 0b8fae3 Compare July 15, 2021 12:08

nfrisby reviewed Jul 16, 2021

View reviewed changes

shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/API/Mempool.hs Show resolved Hide resolved

nc6 force-pushed the nc/alonzo-tx branch 3 times, most recently from 8dff99f to a6ab504 Compare July 16, 2021 09:13

nc6 requested a review from JaredCorduan July 19, 2021 12:43

JaredCorduan reviewed Jul 19, 2021

View reviewed changes

JaredCorduan approved these changes Jul 19, 2021

View reviewed changes

nc6 added 5 commits July 20, 2021 16:01

Add serialisation code for ValidatedTx.

0ab8d78

This serialisation is only for use when transmitting Tx from node to node, or wallet to node.

Re-introduce a TxInBlock alias for compatibility.

4999e67

We will drop this when downstream projects are updated.

nc6 force-pushed the nc/alonzo-tx branch from a6ab504 to 1e0a395 Compare July 20, 2021 14:02

nc6 added 2 commits July 20, 2021 17:16

Fixup tests.

3da864b

Most of this is trivial. We modify `EraGen` to include a `constructTx`, which creates a transaction from its constituent parts.

Add 'NoThunks' and 'TranslateEra' for Validated

712bdb0

Approximately, for the 'TranslateEra' instance.

nc6 force-pushed the nc/alonzo-tx branch from 1e0a395 to 712bdb0 Compare July 20, 2021 15:16

nfrisby approved these changes Jul 20, 2021

View reviewed changes

nfrisby mentioned this pull request Jul 21, 2021

Reject invalid scripts from our wallet IntersectMBO/ouroboros-network#3230

Merged

nc6 merged commit 136967c into master Jul 21, 2021

iohk-bors bot deleted the nc/alonzo-tx branch July 21, 2021 06:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign of Tx/TxInBlock #2379

Redesign of Tx/TxInBlock #2379

nc6 commented Jul 12, 2021 •

edited

nfrisby left a comment

nfrisby Jul 12, 2021

nc6 Jul 13, 2021

nfrisby left a comment

nc6 commented Jul 19, 2021

nfrisby commented Jul 19, 2021

JaredCorduan Jul 19, 2021

JaredCorduan left a comment

nfrisby commented Jul 20, 2021 •

edited

nfrisby commented Jul 20, 2021 •

edited

nfrisby left a comment

Redesign of Tx/TxInBlock #2379

Redesign of Tx/TxInBlock #2379

Conversation

nc6 commented Jul 12, 2021 • edited

nfrisby left a comment

Choose a reason for hiding this comment

nfrisby Jul 12, 2021

Choose a reason for hiding this comment

nc6 Jul 13, 2021

Choose a reason for hiding this comment

nfrisby left a comment

Choose a reason for hiding this comment

nc6 commented Jul 19, 2021

nfrisby commented Jul 19, 2021

JaredCorduan Jul 19, 2021

Choose a reason for hiding this comment

JaredCorduan left a comment

Choose a reason for hiding this comment

nfrisby commented Jul 20, 2021 • edited

nfrisby commented Jul 20, 2021 • edited

nfrisby left a comment

Choose a reason for hiding this comment

nc6 commented Jul 12, 2021 •

edited

nfrisby commented Jul 20, 2021 •

edited

nfrisby commented Jul 20, 2021 •

edited