search

IntersectMBO / cardano-node

The core component that is used to participate in a Cardano decentralised blockchain.
https://cardano.org
Apache License 2.0
3.04k stars 722 forks source link

[BUG] - `makeTransactionBody` duplicates scripts #3650

Open chessai opened 2 years ago

chessai commented 2 years ago

Internal/External External

Area Plutus Related to Plutus Scripts (Alonzo).

Summary makeTransactionBody duplicates scripts. This is obviously undesirable due to size and execution unit limits. I have written a version which does not duplicate scripts, but this is not ideal - it has to do a lot of unsafe and gnarly things with Generics to get around the fact that the true constructors for TxBody are not exported (which they should be, please!). Because the constructors aren't exposed, I can't just deduplicate after the fact.

Steps to reproduce Steps to reproduce the behavior: I don't have a minimal repro right now. What I would suggest is to create a TxBodyContent with some scripts of a chosen size (say 2000 bytes), spending multiple different inputs from the same script. Call makeTransactionBody, then serialiseToCBOR, then inspect in the CBOR Playground. You should see multiple appearances of the script with the size you chose.

Expected behavior makeTransactionBody should not duplicate scripts.

System info (please complete the following information):

Screenshots and attachments Here is the cbor of a transaction with duplicates: https://gist.github.com/chessai/12f9795ab322cc633b1b403995473c55 Note that the scripts are 0'd out. You can load this into the CBOR Playground and you should see that there are two instances of a script with size 2121.

Additional context I would suggest exporting the constructors of TxBody from an unsafe module, and keeping the (complete) pattern synonym as the default export.

JaredCorduan commented 2 years ago

@chessai makeTransactionBody uses an intermediate type for the transaction body, a type which is somewhere between what the ledger code calls a transaction body and a full transaction. And indeed the intermediate type does contain duplicates. When the CLI creates a full transaction from this intermediate body type, however, it removes the duplication and creates a proper transaction according to the ledger wire specification.

chessai commented 2 years ago

@chessai makeTransactionBody uses an intermediate type for the transaction body, a type which is somewhere between what the ledger code calls a transaction body and a full transaction. And indeed the intermediate type does contain duplicates. When the CLI creates a full transaction from this intermediate body type, however, it removes the duplication and creates a proper transaction according to the ledger wire specification.

Even if things are fine (i.e. deduplicated) when it comes time to submit a transaction, I want to be able to test my Plutus applications off-chain and make sure that I don't exceed execution units/size such that the test conditions match the real world. Currently I am using a variant of makeTransactionBody to do this. What is the recommended way to do this, if one exists?

JaredCorduan commented 2 years ago

I don't honestly know what all tooling exists for testing Plutus scripts off-chain first. Perhaps @Jimbo4350 or @michaelpj knows about something that I am unaware of.

I did write a function in the ledger repo, evaluateTransactionExecutionUnits, which is used by the CLI to count the execution units needed for a given transaction:

https://github.com/input-output-hk/cardano-ledger/blob/f26eea2bcde15fde726d0289f716cc3617715b81/eras/alonzo/impl/src/Cardano/Ledger/Alonzo/Tools.hs#L127-L132

That function is probably not convenient for you, though, given that it needs the protocol parameters, the UTxO (or at least the relevant subset for the transaction), and some time related information.

Jimbo4350 commented 2 years ago

@chessai Currently the transaction build has a convenient flag --calculate-plutus-script-cost that outputs the execution units and cost in lovelace to run a script as a JSON file. As for duplicate scripts in the tx body, I will need some time to investigate and adjust this.

chessai commented 2 years ago

I don't honestly know what all tooling exists for testing Plutus scripts off-chain first. Perhaps @Jimbo4350 or @michaelpj knows about something that I am unaware of.

I did write a function in the ledger repo, evaluateTransactionExecutionUnits, which is used by the CLI to count the execution units needed for a given transaction:

https://github.com/input-output-hk/cardano-ledger/blob/f26eea2bcde15fde726d0289f716cc3617715b81/eras/alonzo/impl/src/Cardano/Ledger/Alonzo/Tools.hs#L127-L132

That function is probably not convenient for you, though, given that it needs the protocol parameters, the UTxO (or at least the relevant subset for the transaction), and some time related information.

I have a lot of my own tooling for doing this. It seems like a killer feature of Cardano to be able to test off-chain, but this isn't really well-supported by anything open source. I am working toward things of this nature which are general and open source.

For now, does the intermediate representation need those duplicates? Could a fix be as simple as calling some variant of nub? I don't see why not, plus any call to nub will be cheap (at least for now), because tx size/cpu/mem requirements make it so that too many scripts can't exist anyway.