ADP-1183 (Part 1): Balancing execution units #2952

KtorZ · 2021-10-06T15:48:34Z

📍 Add tx max script execution units to ProtocolParameters
📍 Calculate max script execution cost of transactions instead of relying on redeemers' ex units.
In practice, we want to actually execute a script to set the right value for each redeemer. And not the other way around. So, unless the transaction is fully constructed, the ExUnits value on redeemers is bonkers.
📍 Allow changing ExUnits of redeemers on sealed tx
Doing so, I've also renamed ExtraTxBodyContent -> TxUpdate since (a) it's not only about extra content, (b) it does not only concern the transaction body, but also witnesses now.
📍 Rework balance-tx to always use max execution units.
This is because, we can't actually predict script execution cost
before the full final transaction is constructed. So, as a first step,
we use the biggest approximation we can use which is to always
consider the max execution steps and memory. While this is NOT okay
for production code, it is sufficient to kick off preliminary testing
and integration work.

The goal is to now, in further commits, adjust the ex units of each
redeemers after balancing, changing the change output consequently.
Note that this should not in practice change the cost of execution of
a script, if it does, then we push back on this by arguing that the
underlying script validator is bonkers. Developers ought not to do
this.
📍 Fix typos in comments
Co-authored-by: paweljakubas [email protected]
Co-authored-by: Jonathan Knowles [email protected]
📍 Calculate script integrity hash when updating pre-constructed tx
This is required for any transaction which includes scripts, redeemers or datums as witnesses.
📍 Write e2e scenario using balance+submit on transaction containing Plutus script(s).
📍 Rework tests using pre-constructed Shelley transactions
- Avoid hard-coding names in the test file, but instead, list files in the test directory.
- Use pre-constructed files for all tests, instead of having some inlined hex-binary in the test module.
- Provide more coverage and fix tests actually wrong now that the script integrity hash changes on calls to updateSealedTx
📍 replace plutus pre-generated transactions with newly generated, cbor-only binaries.
📍 Remove redundant (with integration scenarios) compatibility JSON golden spec for scripts.
📍 Cleanup balance endpoint and reduce noise.
There were a lot of things in this handler which were not necessary, as well as many intermediary names which would just add to the noise when reading it. In particular, there's no need to lookup the reward account of the wallet (and it's actually even wrong, because withdrawals may be completely external). Wanting to re-use existing functions is quite flawed in this context because many of the pre-conditions for those functions don't actually hold in the generic balance-tx handler.
📍 Wire collateral requirements into balance endpoint handler
📍 Add feePadding to TransactionCtx, used by calcMinimumCost
📍 Add fee padding to transaction context for balancing
Co-authored-by: Johannes Lund [email protected]
📍 Fix ordering of argument for 'subtract' currently causing underflow.
📍 Rework balance transaction request body to enable passing redeemers.
Because coin-selection will change the order of inputs, we can't construct transactions with redeemers already in place. Redeemers need to be constructed after the facts. Thus, we pass them explicitely in a form that is isomorphic to the ledger's (ScriptPurpose, Data). This way, it's easier to construct transactions once balanced.

Comments

To be done in a next PR:

Calculate the actual execution cost for all redeemers after balancing (using evaluateTransactionExecutionUnits from the cardano-api, or its equivalent from the cardano-ledger-specs).
Replace the execution units of each redeemers with their correspond value.
Adjust the change output's value accordingly.

It should actually not be necessary to re-run the selection at all if we indeed assume that changing the change output's value should not change the execution cost. If it does, then the local node will eventually reject the transaction, and that's none of our business.

Issue Number

ADP-1183

lib/core/src/Cardano/Wallet/Api/Server.hs

paweljakubas · 2021-10-06T18:29:32Z

lib/core/src/Cardano/Wallet/Transaction.hs

@@ -199,25 +201,26 @@ data TransactionLayer k tx = TransactionLayer

    , updateTx
        :: tx
-        -> ExtraTxBodyContent
+        -> TxUpdate


yeah, TxUpdate is definitely better name 😅

paweljakubas · 2021-10-06T18:30:10Z

lib/core/src/Cardano/Wallet/Transaction.hs

    { extraInputs :: [TxIn]
    , extraCollateral :: [TxIn]
       -- ^ Only used in the Alonzo era and later. Will be silently ignored in
       -- previous eras.
    , extraOutputs :: [TxOut]
+    , newExUnits :: ScriptWitnessIndex -> ExecutionUnits -> ExecutionUnits


paweljakubas · 2021-10-06T18:34:18Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

      where
+        modifyRedeemers :: Cardano.TxBodyScriptData era -> Cardano.TxBodyScriptData era


maybe it is absolutely obvious modifyRedeemers is correct, but feel some property test showing the case would not be harmful;-)

What about updating this property https://github.com/input-output-hk/cardano-wallet/blob/master/lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs#L1749 ?

paweljakubas

LGTM! Big shout for the comments in Cardano.Server module! What about adding property test that starts with some SealedTx makes updating of redeemer execution units and as a check we lookup redeemers and see if the execution cost were really properly updated? Redundant or worth having?

Also as now we have TxUpdate we need to change accordingly here:

Anviking

Sounds sane! Will have another look tomorrow with fresh eyes, but feel free to merge before that

Anviking · 2021-10-06T19:19:33Z

lib/core/src/Cardano/Wallet/Api/Server.hs

+        -- transaction considering only the maximum cost, and only after, try to
+        -- adjust the change and ExUnits of each redeemer to something more
+        -- sensible than the max execution cost.
+        let maxCost = maxScriptExecutionCost tl pp sealedTxIncoming


It just occurred to me: actualCost <= maxCost only holds if pp is the protocol parameters active when the tx is accepted, i.e. if pp and the protocol parameter hash match.

Not sure if we, or the client will set the protocol parameter hash, but we should make sure it always matches with pp.

The protocol parameter hash is only needed for validators who depends on it. I'd expect the PAB (or really, anyone constructing the transaction) to set it to what is expected.

Anviking · 2021-10-06T19:35:05Z

lib/core/src/Cardano/Wallet/Transaction.hs

    { extraInputs :: [TxIn]
    , extraCollateral :: [TxIn]
       -- ^ Only used in the Alonzo era and later. Will be silently ignored in
       -- previous eras.
    , extraOutputs :: [TxOut]
+    , newExUnits :: ScriptWitnessIndex -> ExecutionUnits -> ExecutionUnits
+       -- ^ Adjust execution units on existing redeemers.
    , newFee :: Coin -> Coin


I thought I went with modifyFee here, which would make more sense when it's a a -> a. If you feel like renaming to say modifyExUnits / modifyFee, feel free.

lib/core/src/Cardano/Wallet/Transaction.hs

paweljakubas · 2021-10-07T06:15:15Z

@KtorZ maybe it would be good also to have a check like this for max execution unit prices in network's integration test -> https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Network.hs#L78

Anviking

lgtm! (modulo some test would be nice)

Anviking · 2021-10-07T08:34:12Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

        -> Cardano.TxBody era
        -> Either ErrUpdateSealedTx (Cardano.TxBody era)
-    modifyLedgerTxBody ebc (Cardano.ShelleyTxBody shelleyEra bod scripts scriptData aux val) =
-            Right $ Cardano.ShelleyTxBody shelleyEra (adjust ebc shelleyEra bod) scripts scriptData aux val
+    modifyLedgerTx ebc (Cardano.ShelleyTxBody shelleyEra bod scripts scriptData aux val) =


Re the name change… It still takes a TxBody 🤷‍♂️

It does indeed, but that's because the cardano-api has made the choice of including script witnesses inside a type called TxBody 😬 ... which is actually not at all reflecting the TxBody. Script witnesses are, witnesses.

The function is actually called modifyLedgerTx but it's not a "ledger tx" (if reading this as a tx from the cardano-ledger-specs), it's a cardano-api's tx.

KtorZ · 2021-10-07T08:40:35Z

(modulo some test would be nice)

I am working on adding the full end-to-end tests. I thought there was already more tests for the balancing endpoint but I discovered that, not so much 😬 ..

KtorZ · 2021-10-07T08:42:18Z

@KtorZ maybe it would be good also to have a check like this for max execution unit prices in network's integration test -> https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Network.hs#L78

Yes and.. not 😄 ... I haven't added the max execution units to the API yet because that's extra side-work which is really not related to this feature. So I would rather not do it now to not add needless noise.

…g on redeemers' ex units. In practice, we want to actually execute a script to set the right value for each redeemer. And not the other way around. So, unless the transaction is fully constructed, the ExUnits value on redeemers is bonkers.

Doing so, I've also renamed ExtraTxBodyContent -> TxUpdate since (a) it's not only about extra content, (b) it does not only concern the transaction body, but also witnesses now.

This is because, we can't actually predict script execution cost before the full final transaction is constructed. So, as a first step, we use the biggest approximation we can use which is to always consider the max execution steps and memory. While this is NOT okay for production code, it is sufficient to kick off preliminary testing and integration work. The goal is to now, in further commits, adjust the ex units of each redeemers after balancing, changing the change output consequently. Note that this should not in practice change the cost of execution of a script, if it does, then we push back on this by arguing that the underlying script validator is bonkers. Developers ought not to do this.

Co-authored-by: paweljakubas <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]>

This is required for any transaction which includes scripts, redeemers or datums as witnesses.

Anviking · 2021-10-08T13:15:11Z

lib/shelley/test/data/plutus-integration/ping-pong_1-1.json

@@ -0,0 +1,8 @@
+{


Did you mean to have this in the lib/core-integration/test/ instead?

I couldn't run the ping-pong test without copying this from lib/shelley/test/data/plutus-integration to lib/core-integration/test/data/plutus-integration.

Yes. I moved it before pushing the PR, but didn't commit :|

…tus script(s).

- Avoid hard-coding names in the test file, but instead, list files in the test directory. - Use pre-constructed files for all tests, instead of having some inlined hex-binary in the test module. - Provide more coverage and fix tests actually wrong now that the script integrity hash changes on calls to updateSealedTx

…only binaries.

…en spec for scripts.

There were a lot of things in this handler which were not necessary, as well as many intermediary names which would just add to the noise when reading it. In particular, there's no need to lookup the reward account of the wallet (and it's actually even wrong, because withdrawals may be completely external). Wanting to re-use existing functions is quite flawed in this context because many of the pre-conditions for those functions don't actually hold in the generic balance-tx handler.

iohk-bors · 2021-10-10T12:32:49Z

Build failed:

ci/hydra-build:required

KtorZ · 2021-10-10T18:49:00Z

bors r+

2952: ADP-1183 (Part 1): Balancing execution units r=KtorZ a=KtorZ  - 📍 **Add tx max script execution units to ProtocolParameters** - 📍 **Calculate max script execution cost of transactions instead of relying on redeemers' ex units.** In practice, we want to actually execute a script to set the right value for each redeemer. And not the other way around. So, unless the transaction is fully constructed, the ExUnits value on redeemers is bonkers. - 📍 **Allow changing ExUnits of redeemers on sealed tx** Doing so, I've also renamed ExtraTxBodyContent -> TxUpdate since (a) it's not only about extra content, (b) it does not only concern the transaction body, but also witnesses now. - 📍 **Rework balance-tx to always use max execution units.** This is because, we can't actually predict script execution cost before the full final transaction is constructed. So, as a first step, we use the biggest approximation we can use which is to always consider the max execution steps and memory. While this is NOT okay for production code, it is sufficient to kick off preliminary testing and integration work. The goal is to now, in further commits, adjust the ex units of each redeemers after balancing, changing the change output consequently. Note that this should not in practice change the cost of execution of a script, if it does, then we push back on this by arguing that the underlying script validator is bonkers. Developers ought not to do this. - 📍 **Fix typos in comments** Co-authored-by: paweljakubas <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]> - 📍 **Calculate script integrity hash when updating pre-constructed tx** This is required for any transaction which includes scripts, redeemers or datums as witnesses. - 📍 **Write e2e scenario using balance+submit on transaction containing Plutus script(s).** - 📍 **Rework tests using pre-constructed Shelley transactions** - Avoid hard-coding names in the test file, but instead, list files in the test directory. - Use pre-constructed files for all tests, instead of having some inlined hex-binary in the test module. - Provide more coverage and fix tests actually wrong now that the script integrity hash changes on calls to updateSealedTx - 📍 **replace plutus pre-generated transactions with newly generated, cbor-only binaries.** - 📍 **Remove redundant (with integration scenarios) compatibility JSON golden spec for scripts.** - 📍 **Cleanup balance endpoint and reduce noise.** There were a lot of things in this handler which were not necessary, as well as many intermediary names which would just add to the noise when reading it. In particular, there's no need to lookup the reward account of the wallet (and it's actually even wrong, because withdrawals may be completely external). Wanting to re-use existing functions is quite flawed in this context because many of the pre-conditions for those functions don't actually hold in the generic balance-tx handler. - 📍 **Wire collateral requirements into balance endpoint handler** - 📍 **Add feePadding to TransactionCtx, used by calcMinimumCost** - 📍 **Add fee padding to transaction context for balancing** Co-authored-by: Johannes Lund <[email protected]> - 📍 **Fix ordering of argument for 'subtract' currently causing underflow.** - 📍 **Rework balance transaction request body to enable passing redeemers.** Because coin-selection will change the order of inputs, we can't construct transactions with redeemers already in place. Redeemers need to be constructed after the facts. Thus, we pass them explicitely in a form that is isomorphic to the ledger's (ScriptPurpose, Data). This way, it's easier to construct transactions once balanced. ### Comments  To be done in a next PR: - Calculate the actual execution cost for all redeemers after balancing (using [evaluateTransactionExecutionUnits](https://input-output-hk.github.io/cardano-node/cardano-api/lib/Cardano-Api-Fees.html#v:evaluateTransactionExecutionUnits) from the `cardano-api`, or its equivalent from the `cardano-ledger-specs`). - Replace the execution units of each redeemers with their correspond value. - Adjust the change output's value accordingly. It should actually not be necessary to re-run the selection at all if we indeed assume that changing the change output's value should not change the execution cost. If it does, then the local node will eventually reject the transaction, and that's none of our business. ### Issue Number  ADP-1183 Co-authored-by: KtorZ <[email protected]> Co-authored-by: Matthias Benkort <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: IOHK <[email protected]>

iohk-bors · 2021-10-10T18:59:51Z

Build failed:

buildkite/cardano-wallet

KtorZ · 2021-10-10T19:41:25Z

bors retry

2952: ADP-1183 (Part 1): Balancing execution units r=KtorZ a=KtorZ  - 📍 **Add tx max script execution units to ProtocolParameters** - 📍 **Calculate max script execution cost of transactions instead of relying on redeemers' ex units.** In practice, we want to actually execute a script to set the right value for each redeemer. And not the other way around. So, unless the transaction is fully constructed, the ExUnits value on redeemers is bonkers. - 📍 **Allow changing ExUnits of redeemers on sealed tx** Doing so, I've also renamed ExtraTxBodyContent -> TxUpdate since (a) it's not only about extra content, (b) it does not only concern the transaction body, but also witnesses now. - 📍 **Rework balance-tx to always use max execution units.** This is because, we can't actually predict script execution cost before the full final transaction is constructed. So, as a first step, we use the biggest approximation we can use which is to always consider the max execution steps and memory. While this is NOT okay for production code, it is sufficient to kick off preliminary testing and integration work. The goal is to now, in further commits, adjust the ex units of each redeemers after balancing, changing the change output consequently. Note that this should not in practice change the cost of execution of a script, if it does, then we push back on this by arguing that the underlying script validator is bonkers. Developers ought not to do this. - 📍 **Fix typos in comments** Co-authored-by: paweljakubas <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]> - 📍 **Calculate script integrity hash when updating pre-constructed tx** This is required for any transaction which includes scripts, redeemers or datums as witnesses. - 📍 **Write e2e scenario using balance+submit on transaction containing Plutus script(s).** - 📍 **Rework tests using pre-constructed Shelley transactions** - Avoid hard-coding names in the test file, but instead, list files in the test directory. - Use pre-constructed files for all tests, instead of having some inlined hex-binary in the test module. - Provide more coverage and fix tests actually wrong now that the script integrity hash changes on calls to updateSealedTx - 📍 **replace plutus pre-generated transactions with newly generated, cbor-only binaries.** - 📍 **Remove redundant (with integration scenarios) compatibility JSON golden spec for scripts.** - 📍 **Cleanup balance endpoint and reduce noise.** There were a lot of things in this handler which were not necessary, as well as many intermediary names which would just add to the noise when reading it. In particular, there's no need to lookup the reward account of the wallet (and it's actually even wrong, because withdrawals may be completely external). Wanting to re-use existing functions is quite flawed in this context because many of the pre-conditions for those functions don't actually hold in the generic balance-tx handler. - 📍 **Wire collateral requirements into balance endpoint handler** - 📍 **Add feePadding to TransactionCtx, used by calcMinimumCost** - 📍 **Add fee padding to transaction context for balancing** Co-authored-by: Johannes Lund <[email protected]> - 📍 **Fix ordering of argument for 'subtract' currently causing underflow.** - 📍 **Rework balance transaction request body to enable passing redeemers.** Because coin-selection will change the order of inputs, we can't construct transactions with redeemers already in place. Redeemers need to be constructed after the facts. Thus, we pass them explicitely in a form that is isomorphic to the ledger's (ScriptPurpose, Data). This way, it's easier to construct transactions once balanced. ### Comments  To be done in a next PR: - Calculate the actual execution cost for all redeemers after balancing (using [evaluateTransactionExecutionUnits](https://input-output-hk.github.io/cardano-node/cardano-api/lib/Cardano-Api-Fees.html#v:evaluateTransactionExecutionUnits) from the `cardano-api`, or its equivalent from the `cardano-ledger-specs`). - Replace the execution units of each redeemers with their correspond value. - Adjust the change output's value accordingly. It should actually not be necessary to re-run the selection at all if we indeed assume that changing the change output's value should not change the execution cost. If it does, then the local node will eventually reject the transaction, and that's none of our business. ### Issue Number  ADP-1183 Co-authored-by: KtorZ <[email protected]> Co-authored-by: Matthias Benkort <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: IOHK <[email protected]>

iohk-bors · 2021-10-10T20:02:28Z

Build failed:

ci/hydra-build:required

edit (KtorZ): I don't get ☝️ , does the nix-scripts need something additional to figure out the existence of the test folder?

KtorZ · 2021-10-11T08:09:37Z

bors retry

2952: ADP-1183 (Part 1): Balancing execution units r=KtorZ a=KtorZ  - 📍 **Add tx max script execution units to ProtocolParameters** - 📍 **Calculate max script execution cost of transactions instead of relying on redeemers' ex units.** In practice, we want to actually execute a script to set the right value for each redeemer. And not the other way around. So, unless the transaction is fully constructed, the ExUnits value on redeemers is bonkers. - 📍 **Allow changing ExUnits of redeemers on sealed tx** Doing so, I've also renamed ExtraTxBodyContent -> TxUpdate since (a) it's not only about extra content, (b) it does not only concern the transaction body, but also witnesses now. - 📍 **Rework balance-tx to always use max execution units.** This is because, we can't actually predict script execution cost before the full final transaction is constructed. So, as a first step, we use the biggest approximation we can use which is to always consider the max execution steps and memory. While this is NOT okay for production code, it is sufficient to kick off preliminary testing and integration work. The goal is to now, in further commits, adjust the ex units of each redeemers after balancing, changing the change output consequently. Note that this should not in practice change the cost of execution of a script, if it does, then we push back on this by arguing that the underlying script validator is bonkers. Developers ought not to do this. - 📍 **Fix typos in comments** Co-authored-by: paweljakubas <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]> - 📍 **Calculate script integrity hash when updating pre-constructed tx** This is required for any transaction which includes scripts, redeemers or datums as witnesses. - 📍 **Write e2e scenario using balance+submit on transaction containing Plutus script(s).** - 📍 **Rework tests using pre-constructed Shelley transactions** - Avoid hard-coding names in the test file, but instead, list files in the test directory. - Use pre-constructed files for all tests, instead of having some inlined hex-binary in the test module. - Provide more coverage and fix tests actually wrong now that the script integrity hash changes on calls to updateSealedTx - 📍 **replace plutus pre-generated transactions with newly generated, cbor-only binaries.** - 📍 **Remove redundant (with integration scenarios) compatibility JSON golden spec for scripts.** - 📍 **Cleanup balance endpoint and reduce noise.** There were a lot of things in this handler which were not necessary, as well as many intermediary names which would just add to the noise when reading it. In particular, there's no need to lookup the reward account of the wallet (and it's actually even wrong, because withdrawals may be completely external). Wanting to re-use existing functions is quite flawed in this context because many of the pre-conditions for those functions don't actually hold in the generic balance-tx handler. - 📍 **Wire collateral requirements into balance endpoint handler** - 📍 **Add feePadding to TransactionCtx, used by calcMinimumCost** - 📍 **Add fee padding to transaction context for balancing** Co-authored-by: Johannes Lund <[email protected]> - 📍 **Fix ordering of argument for 'subtract' currently causing underflow.** - 📍 **Rework balance transaction request body to enable passing redeemers.** Because coin-selection will change the order of inputs, we can't construct transactions with redeemers already in place. Redeemers need to be constructed after the facts. Thus, we pass them explicitely in a form that is isomorphic to the ledger's (ScriptPurpose, Data). This way, it's easier to construct transactions once balanced. ### Comments  To be done in a next PR: - Calculate the actual execution cost for all redeemers after balancing (using [evaluateTransactionExecutionUnits](https://input-output-hk.github.io/cardano-node/cardano-api/lib/Cardano-Api-Fees.html#v:evaluateTransactionExecutionUnits) from the `cardano-api`, or its equivalent from the `cardano-ledger-specs`). - Replace the execution units of each redeemers with their correspond value. - Adjust the change output's value accordingly. It should actually not be necessary to re-run the selection at all if we indeed assume that changing the change output's value should not change the execution cost. If it does, then the local node will eventually reject the transaction, and that's none of our business. ### Issue Number  ADP-1183 Co-authored-by: KtorZ <[email protected]> Co-authored-by: Matthias Benkort <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: IOHK <[email protected]>

iohk-bors · 2021-10-11T09:03:48Z

Canceled.

KtorZ · 2021-10-11T09:03:51Z

bors r+

2952: ADP-1183 (Part 1): Balancing execution units r=KtorZ a=KtorZ  - 📍 **Add tx max script execution units to ProtocolParameters** - 📍 **Calculate max script execution cost of transactions instead of relying on redeemers' ex units.** In practice, we want to actually execute a script to set the right value for each redeemer. And not the other way around. So, unless the transaction is fully constructed, the ExUnits value on redeemers is bonkers. - 📍 **Allow changing ExUnits of redeemers on sealed tx** Doing so, I've also renamed ExtraTxBodyContent -> TxUpdate since (a) it's not only about extra content, (b) it does not only concern the transaction body, but also witnesses now. - 📍 **Rework balance-tx to always use max execution units.** This is because, we can't actually predict script execution cost before the full final transaction is constructed. So, as a first step, we use the biggest approximation we can use which is to always consider the max execution steps and memory. While this is NOT okay for production code, it is sufficient to kick off preliminary testing and integration work. The goal is to now, in further commits, adjust the ex units of each redeemers after balancing, changing the change output consequently. Note that this should not in practice change the cost of execution of a script, if it does, then we push back on this by arguing that the underlying script validator is bonkers. Developers ought not to do this. - 📍 **Fix typos in comments** Co-authored-by: paweljakubas <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]> - 📍 **Calculate script integrity hash when updating pre-constructed tx** This is required for any transaction which includes scripts, redeemers or datums as witnesses. - 📍 **Write e2e scenario using balance+submit on transaction containing Plutus script(s).** - 📍 **Rework tests using pre-constructed Shelley transactions** - Avoid hard-coding names in the test file, but instead, list files in the test directory. - Use pre-constructed files for all tests, instead of having some inlined hex-binary in the test module. - Provide more coverage and fix tests actually wrong now that the script integrity hash changes on calls to updateSealedTx - 📍 **replace plutus pre-generated transactions with newly generated, cbor-only binaries.** - 📍 **Remove redundant (with integration scenarios) compatibility JSON golden spec for scripts.** - 📍 **Cleanup balance endpoint and reduce noise.** There were a lot of things in this handler which were not necessary, as well as many intermediary names which would just add to the noise when reading it. In particular, there's no need to lookup the reward account of the wallet (and it's actually even wrong, because withdrawals may be completely external). Wanting to re-use existing functions is quite flawed in this context because many of the pre-conditions for those functions don't actually hold in the generic balance-tx handler. - 📍 **Wire collateral requirements into balance endpoint handler** - 📍 **Add feePadding to TransactionCtx, used by calcMinimumCost** - 📍 **Add fee padding to transaction context for balancing** Co-authored-by: Johannes Lund <[email protected]> - 📍 **Fix ordering of argument for 'subtract' currently causing underflow.** - 📍 **Rework balance transaction request body to enable passing redeemers.** Because coin-selection will change the order of inputs, we can't construct transactions with redeemers already in place. Redeemers need to be constructed after the facts. Thus, we pass them explicitely in a form that is isomorphic to the ledger's (ScriptPurpose, Data). This way, it's easier to construct transactions once balanced. ### Comments  To be done in a next PR: - Calculate the actual execution cost for all redeemers after balancing (using [evaluateTransactionExecutionUnits](https://input-output-hk.github.io/cardano-node/cardano-api/lib/Cardano-Api-Fees.html#v:evaluateTransactionExecutionUnits) from the `cardano-api`, or its equivalent from the `cardano-ledger-specs`). - Replace the execution units of each redeemers with their correspond value. - Adjust the change output's value accordingly. It should actually not be necessary to re-run the selection at all if we indeed assume that changing the change output's value should not change the execution cost. If it does, then the local node will eventually reject the transaction, and that's none of our business. ### Issue Number  ADP-1183 Co-authored-by: KtorZ <[email protected]> Co-authored-by: Matthias Benkort <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: IOHK <[email protected]>

iohk-bors · 2021-10-11T09:42:43Z

Build failed:

buildkite/cardano-wallet

For some misterious reason, this doesn't work under nix (reproducible) builds. *sigh*. So, trying this out in hope.

KtorZ · 2021-10-11T10:32:49Z

bors r+

2952: ADP-1183 (Part 1): Balancing execution units r=KtorZ a=KtorZ  - 📍 **Add tx max script execution units to ProtocolParameters** - 📍 **Calculate max script execution cost of transactions instead of relying on redeemers' ex units.** In practice, we want to actually execute a script to set the right value for each redeemer. And not the other way around. So, unless the transaction is fully constructed, the ExUnits value on redeemers is bonkers. - 📍 **Allow changing ExUnits of redeemers on sealed tx** Doing so, I've also renamed ExtraTxBodyContent -> TxUpdate since (a) it's not only about extra content, (b) it does not only concern the transaction body, but also witnesses now. - 📍 **Rework balance-tx to always use max execution units.** This is because, we can't actually predict script execution cost before the full final transaction is constructed. So, as a first step, we use the biggest approximation we can use which is to always consider the max execution steps and memory. While this is NOT okay for production code, it is sufficient to kick off preliminary testing and integration work. The goal is to now, in further commits, adjust the ex units of each redeemers after balancing, changing the change output consequently. Note that this should not in practice change the cost of execution of a script, if it does, then we push back on this by arguing that the underlying script validator is bonkers. Developers ought not to do this. - 📍 **Fix typos in comments** Co-authored-by: paweljakubas <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]> - 📍 **Calculate script integrity hash when updating pre-constructed tx** This is required for any transaction which includes scripts, redeemers or datums as witnesses. - 📍 **Write e2e scenario using balance+submit on transaction containing Plutus script(s).** - 📍 **Rework tests using pre-constructed Shelley transactions** - Avoid hard-coding names in the test file, but instead, list files in the test directory. - Use pre-constructed files for all tests, instead of having some inlined hex-binary in the test module. - Provide more coverage and fix tests actually wrong now that the script integrity hash changes on calls to updateSealedTx - 📍 **replace plutus pre-generated transactions with newly generated, cbor-only binaries.** - 📍 **Remove redundant (with integration scenarios) compatibility JSON golden spec for scripts.** - 📍 **Cleanup balance endpoint and reduce noise.** There were a lot of things in this handler which were not necessary, as well as many intermediary names which would just add to the noise when reading it. In particular, there's no need to lookup the reward account of the wallet (and it's actually even wrong, because withdrawals may be completely external). Wanting to re-use existing functions is quite flawed in this context because many of the pre-conditions for those functions don't actually hold in the generic balance-tx handler. - 📍 **Wire collateral requirements into balance endpoint handler** - 📍 **Add feePadding to TransactionCtx, used by calcMinimumCost** - 📍 **Add fee padding to transaction context for balancing** Co-authored-by: Johannes Lund <[email protected]> - 📍 **Fix ordering of argument for 'subtract' currently causing underflow.** - 📍 **Rework balance transaction request body to enable passing redeemers.** Because coin-selection will change the order of inputs, we can't construct transactions with redeemers already in place. Redeemers need to be constructed after the facts. Thus, we pass them explicitely in a form that is isomorphic to the ledger's (ScriptPurpose, Data). This way, it's easier to construct transactions once balanced. ### Comments  To be done in a next PR: - Calculate the actual execution cost for all redeemers after balancing (using [evaluateTransactionExecutionUnits](https://input-output-hk.github.io/cardano-node/cardano-api/lib/Cardano-Api-Fees.html#v:evaluateTransactionExecutionUnits) from the `cardano-api`, or its equivalent from the `cardano-ledger-specs`). - Replace the execution units of each redeemers with their correspond value. - Adjust the change output's value accordingly. It should actually not be necessary to re-run the selection at all if we indeed assume that changing the change output's value should not change the execution cost. If it does, then the local node will eventually reject the transaction, and that's none of our business. ### Issue Number  ADP-1183 Co-authored-by: KtorZ <[email protected]> Co-authored-by: Matthias Benkort <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: IOHK <[email protected]>

iohk-bors · 2021-10-11T10:58:33Z

Build failed:

ci/hydra-build:required

I am done with Nix. This tool is causing more problems than it gives any benefits.

KtorZ · 2021-10-11T11:11:40Z

bors r+

iohk-bors · 2021-10-11T11:54:31Z

Build succeeded:

2964: Remove 'coin_selection' field from the balance API's response r=KtorZ a=KtorZ  - 📍 **Remove 'coin_selection' field from the Balance API's response** (a) the implementation of this field was wrong, and getting it right is quite involved as it requires revising several parts of the core wallet. It is perhaps not desirable / out-of-scope for this endpoint actually. (b) the coin_selection result is only truly useful when constructing a transaction which is under control of the wallet. Balancing may be done on any arbitrary foreign transaction and as such, breaks several invariants of the wallet (only one withdrawal, only one delegation certificate at a time etc..). ### Comments  Based off [`KtorZ/ADP-1183/balancing-execution-units`](#2952) ### Issue Number  ADP-1183 Co-authored-by: KtorZ <[email protected]>

KtorZ requested review from Anviking and paweljakubas October 6, 2021 15:48

KtorZ self-assigned this Oct 6, 2021

paweljakubas reviewed Oct 6, 2021

View reviewed changes

lib/core/src/Cardano/Wallet/Api/Server.hs Outdated Show resolved Hide resolved

paweljakubas reviewed Oct 6, 2021

View reviewed changes

lib/core/src/Cardano/Wallet/Api/Server.hs Outdated Show resolved Hide resolved

paweljakubas reviewed Oct 6, 2021

View reviewed changes

lib/core/src/Cardano/Wallet/Api/Server.hs Outdated Show resolved Hide resolved

paweljakubas reviewed Oct 6, 2021

View reviewed changes

paweljakubas approved these changes Oct 6, 2021

View reviewed changes

Anviking reviewed Oct 6, 2021

View reviewed changes

jonathanknowles reviewed Oct 7, 2021

View reviewed changes

lib/core/src/Cardano/Wallet/Transaction.hs Outdated Show resolved Hide resolved

Anviking approved these changes Oct 7, 2021

View reviewed changes

KtorZ force-pushed the KtorZ/ADP-1183/balancing-execution-units branch from 58b790d to dfc8688 Compare October 7, 2021 18:40

KtorZ mentioned this pull request Oct 8, 2021

Add feePadding for use in balanceTransaction #2957

Closed

1 task

KtorZ and others added 6 commits October 8, 2021 13:50

Add tx max script execution units to ProtocolParameters

ba3886d

Allow changing ExUnits of redeemers on sealed tx

e458765

Doing so, I've also renamed ExtraTxBodyContent -> TxUpdate since (a) it's not only about extra content, (b) it does not only concern the transaction body, but also witnesses now.

Fix typos in comments

b03be04

Co-authored-by: paweljakubas <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]>

Calculate script integrity hash when updating pre-constructed tx

88c3f71

This is required for any transaction which includes scripts, redeemers or datums as witnesses.

Anviking reviewed Oct 8, 2021

View reviewed changes

KtorZ added 5 commits October 8, 2021 20:54

Write e2e scenario using balance+submit on transaction containing Plu…

0da96c8

…tus script(s).

replace plutus pre-generated transactions with newly generated, cbor-…

f29c8db

…only binaries.

Remove redundant (with integration scenarios) compatibility JSON gold…

6e2eb43

…en spec for scripts.

KtorZ mentioned this pull request Oct 11, 2021

Remove 'coin_selection' field from the balance API's response #2964

Merged

Move plutus integration test files from core-integration to shelley/

9a8265c

For some misterious reason, this doesn't work under nix (reproducible) builds. *sigh*. So, trying this out in hope.

KtorZ force-pushed the KtorZ/ADP-1183/balancing-execution-units branch from e561c0f to edb6311 Compare October 11, 2021 10:29

KtorZ and others added 2 commits October 11, 2021 13:11

Inline Plutus JSON scenarios into a Haskell module.

d9d24e2

I am done with Nix. This tool is causing more problems than it gives any benefits.

Regenerate nix

8305b23

KtorZ force-pushed the KtorZ/ADP-1183/balancing-execution-units branch from 45d239c to 8305b23 Compare October 11, 2021 11:11

iohk-bors bot merged commit 63df46a into master Oct 11, 2021

iohk-bors bot deleted the KtorZ/ADP-1183/balancing-execution-units branch October 11, 2021 11:54

WilliamKingNoel-Bot pushed a commit that referenced this pull request Oct 11, 2021

docs: Merge #2952

5731f36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADP-1183 (Part 1): Balancing execution units #2952

ADP-1183 (Part 1): Balancing execution units #2952

KtorZ commented Oct 6, 2021 •

edited

Loading

paweljakubas Oct 6, 2021

paweljakubas Oct 6, 2021

paweljakubas Oct 6, 2021

paweljakubas Oct 6, 2021

paweljakubas Oct 6, 2021

paweljakubas left a comment •

edited

Loading

Anviking left a comment •

edited

Loading

Anviking Oct 6, 2021

KtorZ Oct 7, 2021

Anviking Oct 6, 2021

paweljakubas commented Oct 7, 2021

Anviking left a comment

Anviking Oct 7, 2021

KtorZ Oct 7, 2021

KtorZ Oct 7, 2021

KtorZ commented Oct 7, 2021

KtorZ commented Oct 7, 2021

Anviking Oct 8, 2021

KtorZ Oct 8, 2021

iohk-bors bot commented Oct 10, 2021

KtorZ commented Oct 10, 2021

iohk-bors bot commented Oct 10, 2021

KtorZ commented Oct 10, 2021

iohk-bors bot commented Oct 10, 2021 •

edited by KtorZ

Loading

KtorZ commented Oct 11, 2021

iohk-bors bot commented Oct 11, 2021

KtorZ commented Oct 11, 2021

iohk-bors bot commented Oct 11, 2021

KtorZ commented Oct 11, 2021

iohk-bors bot commented Oct 11, 2021

KtorZ commented Oct 11, 2021

iohk-bors bot commented Oct 11, 2021

		where
		modifyRedeemers :: Cardano.TxBodyScriptData era -> Cardano.TxBodyScriptData era

ADP-1183 (Part 1): Balancing execution units #2952

ADP-1183 (Part 1): Balancing execution units #2952

Conversation

KtorZ commented Oct 6, 2021 • edited Loading

Comments

Issue Number

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

paweljakubas left a comment • edited Loading

Choose a reason for hiding this comment

Anviking left a comment • edited Loading

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

paweljakubas commented Oct 7, 2021

Anviking 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

KtorZ commented Oct 7, 2021

KtorZ commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iohk-bors bot commented Oct 10, 2021

KtorZ commented Oct 10, 2021

iohk-bors bot commented Oct 10, 2021

KtorZ commented Oct 10, 2021

iohk-bors bot commented Oct 10, 2021 • edited by KtorZ Loading

KtorZ commented Oct 11, 2021

iohk-bors bot commented Oct 11, 2021

KtorZ commented Oct 11, 2021

iohk-bors bot commented Oct 11, 2021

KtorZ commented Oct 11, 2021

iohk-bors bot commented Oct 11, 2021

KtorZ commented Oct 11, 2021

iohk-bors bot commented Oct 11, 2021

KtorZ commented Oct 6, 2021 •

edited

Loading

paweljakubas left a comment •

edited

Loading

Anviking left a comment •

edited

Loading

iohk-bors bot commented Oct 10, 2021 •

edited by KtorZ

Loading