Add utxoCostPerByte protocol parameter #4141

newhoggy · 2022-07-06T04:42:38Z

Resolves #4052
See cardano-foundation/CIPs#265

lehins

Thank you for doing this change, that is exactly what I was advocating for. There are two more things need to be done, I believe, to make this change complete:

Json field name needs to be adjusted:

https://github.com/input-output-hk/cardano-node/blob/0cd6878769a10097e7c780e15efb9919fb94f669/cardano-api/src/Cardano/Api/ProtocolParameters.hs#L315

and

https://github.com/input-output-hk/cardano-node/blob/0cd6878769a10097e7c780e15efb9919fb94f669/cardano-api/src/Cardano/Api/ProtocolParameters.hs#L346

I don't know if it is OK to break backwards compatibility when it comes to json representation, but just renaming the field naturally will break it

ProtocolParametersUpdate also needs the rename:

https://github.com/input-output-hk/cardano-node/blob/0cd6878769a10097e7c780e15efb9919fb94f669/cardano-api/src/Cardano/Api/ProtocolParameters.hs#L481

newhoggy · 2022-07-06T12:59:34Z

Thanks @lehins . I did another pass through the code and identified a lot more cases that needed changing.

comments addressed

Jimbo4350

We should make this change backwards compatible

Jimbo4350 · 2022-07-06T13:12:48Z

cardano-api/src/Cardano/Api/ProtocolParameters.hs

@@ -312,7 +312,7 @@ instance FromJSON ProtocolParameters where
        <*> o .: "poolPledgeInfluence"
        <*> o .: "monetaryExpansion"
        <*> o .: "treasuryCut"
-        <*> o .:? "utxoCostPerWord"


So we have a problem. We are breaking backwards compatibility here. We should parse for both "utxoCostPerWord" and "utxoCostPerByte" in the event somebody is running an Alonzo only network. There is also a typo: utxoCostPerBytes

I might be wrong, but I think it could be sufficient to keep backwards compatibility at the JSON deserialization level. Something along the lines:

maybePerWord <- o .:? "utxoCostPerWord" maybePerByte <- o .:? "utxoCostPerByte" utxoCostPerByte <- case maybePerWord of Nothing -> pure maybePerByte Just _ | Just _ <- maybePerByte <- fail "Cannot supply both utxoCostPerWord and utxoCostPerByte" Just perWord -> pure $ coinsPerUTxOWordToCoinsPerUTxOByte perWord

It would work but it would be confusing when looking at the ProtocolParameters type. It's better to make the changes obvious rather than having it only at the JSON deserialization level.

Jimbo4350 · 2022-07-06T13:16:40Z

cardano-api/src/Cardano/Api/ProtocolParameters.hs

       --
-       -- /Introduced in Alonzo/
-       protocolUpdateUTxOCostPerWord :: Maybe Lovelace,


I think the proper way to do this would not be to modify existing Alonzo fields, but to add a new protocolUpdateUTxOCostPerByte field

See my comment, maybe we can get away with renaming. After Babbage is released creating PPUpdates for Alonzo doesn't make much sense, or does it?

In retrospective we should not have renamed it ledger. In particular using the binary flag 17 for perWord and perByte was a mistake, IMHO. But that ship have sailed.

It might for chains other than mainnet if we care about those?

maybe we can get away with renaming.

If we were already in Babbage maybe but keeping backwards compatibility also makes it clear to whoever is looking at the code that this particular field was renamed at the Alonzo -> Babbage boundary which is useful. This has always been the approach in the api.

newhoggy · 2022-07-07T13:32:22Z

cardano-api/gen/Gen/Cardano/Api/Typed.hs

@@ -795,6 +795,7 @@ genProtocolParametersUpdate = do
  protocolUpdateMaxValueSize        <- Gen.maybe genNat
  protocolUpdateCollateralPercent   <- Gen.maybe genNat
  protocolUpdateMaxCollateralInputs <- Gen.maybe genNat
+  protocolUpdateUTxOCostPerByte     <- Gen.maybe genLovelace


Does it make sense to generate both the protocolUpdateUTxOCostPerWord and the protocolUpdateUTxOCostPerByte in the same update?

For now yeah

Comments addressed

newhoggy · 2022-07-07T14:50:02Z

cardano-api/src/Cardano/Api/Fees.hs

+       -- | The 'ProtocolParameters' must provide the value for the cost per
+       -- byte parameter, for eras that use this parameter.
+     | TxBodyErrorMissingParamCostPerByte
+


Both TxBodyErrorMissingParamCostPerWord and TxBodyErrorMissingParamCostPerByte appear to be unused. Should I delete @Jimbo4350? Or is a change needed to ensure they are used?

newhoggy · 2022-07-07T14:50:33Z

cardano-api/src/Cardano/Api/Fees.hs

@@ -1260,6 +1267,7 @@ calculateMinimumUTxO era txout@(TxOut _ v _ _) pparams' =
 data MinimumUTxOError =
    PParamsMinUTxOMissing
  | PParamsUTxOCostPerWordMissing
+  | PParamsUTxOCostPerByteMissing


Both PParamsUTxOCostPerWordMissing and PParamsUTxOCostPerByteMissing appear to be unused. Should I delete @Jimbo4350? Or is a change needed to ensure they are used?

You can remove them both 👍

Jimbo4350 · 2022-07-07T19:46:02Z

cardano-api/src/Cardano/Api/ProtocolParameters.hs

@@ -1460,7 +1483,8 @@ toAlonzoPParams ProtocolParameters {
                               (Ledger.boundRational protocolParamTreasuryCut)

      -- New params in Alonzo:
-    , Alonzo._coinsPerUTxOWord  = toShelleyLovelace utxoCostPerWord
+    , Alonzo._coinsPerUTxOWord  = -- adapted from babbage protocol parameter field


Why the comment? I don't understand.

Left over comment from when I previously did the byte to work conversion. Deleting.

Jimbo4350 · 2022-07-07T19:46:56Z

cardano-api/src/Cardano/Api/ProtocolParameters.hs

@@ -1506,7 +1530,7 @@ toBabbagePParams ProtocolParameters {
                   protocolParamPoolPledgeInfluence,
                   protocolParamMonetaryExpansion,
                   protocolParamTreasuryCut,
-                   protocolParamUTxOCostPerWord = Just utxoCostPerWord,
+                   protocolParamUTxOCostPerByte = Just utxoCostPerByte,


Jimbo4350 · 2022-07-07T19:48:10Z

cardano-api/src/Cardano/Api/ProtocolParameters.hs

@@ -1693,6 +1717,7 @@ fromAlonzoPParams
    , protocolParamMaxValueSize        = Just _maxValSize
    , protocolParamCollateralPercent   = Just _collateralPercentage
    , protocolParamMaxCollateralInputs = Just _maxCollateralInputs
+    , protocolParamUTxOCostPerByte     = Nothing


Adding a comment here explaining this is deprecated in Babbage would be useful

Added comment.

Jimbo4350 · 2022-07-07T19:52:09Z

cardano-cli/src/Cardano/CLI/Run/Friendly.hs

@@ -262,14 +263,16 @@ friendlyProtocolParametersUpdate
        ("monetary expansion" .=) . friendlyRational
    , protocolUpdateTreasuryCut <&> ("treasury expansion" .=) . friendlyRational
    , protocolUpdateUTxOCostPerWord <&>
-        ("UTxO storage cost per unit" .=) . friendlyLovelace
+        ("UTxO storage cost per word" .=) . friendlyLovelace


Jimbo4350 · 2022-07-07T19:59:26Z

cardano-api/src/Cardano/Api/Fees.hs

@@ -1260,6 +1267,7 @@ calculateMinimumUTxO era txout@(TxOut _ v _ _) pparams' =
 data MinimumUTxOError =
    PParamsMinUTxOMissing
  | PParamsUTxOCostPerWordMissing
+  | PParamsUTxOCostPerByteMissing


You can remove them both 👍

Jimbo4350 · 2022-07-07T19:59:51Z

LGTM, minor suggestions

newhoggy · 2022-07-07T22:37:57Z

cardano-cli/test/data/golden/alonzo/transaction-view.out

@@ -18,7 +18,7 @@ update proposal:
  updates:
  - genesis key hash: 1bafa294233a5a7ffbf539ae798da0943aa83d2a19398c2d0e5af114
    update:
-      UTxO storage cost per unit: 194 Lovelace
+      UTxO storage cost per word: 194 Lovelace


Note this golden file has changed.

See #4052 See cardano-foundation/CIPs#265

newhoggy · 2022-07-08T03:31:41Z

bors r+

4141: Add utxoCostPerByte protocol parameter r=newhoggy a=newhoggy Resolves #4052 See cardano-foundation/CIPs#265 Co-authored-by: John Ky <[email protected]>

iohk-bors · 2022-07-08T05:35:17Z

Timed out.

newhoggy · 2022-07-08T12:09:45Z

bors r+

iohk-bors · 2022-07-08T12:39:54Z

Build succeeded:

gitmachtl · 2022-07-08T18:22:05Z

tested latest master. ok so the protocol parameters query will introduce the utxoCostPerWord parameter on the switch to alonzo-era like

...
    "txFeeFixed": 155381,
    "txFeePerByte": 44,
    "utxoCostPerByte": null,
    "utxoCostPerWord": 34482
...

and will remove or set the parameter again in babbage-era like

...
    "txFeeFixed": 155381,
    "txFeePerByte": 44,
    "utxoCostPerByte": 4310,
    "utxoCostPerWord": null
...

Will this behaviour stay for a while? Because Tools are looking out for those parameters in the protocol to do the minUtxo calculation based on it (not using the inbuilt cli function).

gitmachtl · 2022-07-24T12:55:55Z

@Jimbo4350 why was this PR not included in the tag 1.35.2 ?

whats the plan for the release version? we have external tools depending on the cli protocol-parameters output. with "utxoCostPerByte" present its clear that the minOutUtxo must be calculated with the new perByte method. or is the plan to not show this parameter at all and use the new method if a protocolVersion.major>=7 is present?

thx

lehins · 2022-07-24T13:14:28Z

@gitmachtl From what I understand 1.35.2 only includes an important bug fix that is necessary for the upcoming hard fork. All other minor bug fixes and features, which applies to utxoCostPerByte param, will be included in the subsequent minor release.

newhoggy marked this pull request as ready for review July 6, 2022 04:42

newhoggy requested review from dcoutts, Jimbo4350, erikd and JaredCorduan as code owners July 6, 2022 04:42

newhoggy force-pushed the newhoggy/rename-utxoCostPerWord-to-utxoCostPerByte branch from 10c8fbb to 26176ec Compare July 6, 2022 05:02

lehins previously requested changes Jul 6, 2022

View reviewed changes

newhoggy force-pushed the newhoggy/rename-utxoCostPerWord-to-utxoCostPerByte branch 2 times, most recently from eb9ff82 to c0cfc27 Compare July 6, 2022 12:56

newhoggy force-pushed the newhoggy/rename-utxoCostPerWord-to-utxoCostPerByte branch from c0cfc27 to a66e638 Compare July 6, 2022 13:02

newhoggy requested a review from lehins July 6, 2022 13:02

newhoggy force-pushed the newhoggy/rename-utxoCostPerWord-to-utxoCostPerByte branch 4 times, most recently from 5f4fb59 to d1eb9ea Compare July 6, 2022 13:06

Jimbo4350 previously requested changes Jul 6, 2022

View reviewed changes

Jimbo4350 reviewed Jul 6, 2022

View reviewed changes

newhoggy commented Jul 7, 2022

View reviewed changes

newhoggy force-pushed the newhoggy/rename-utxoCostPerWord-to-utxoCostPerByte branch from 435ae33 to 66ce20d Compare July 7, 2022 14:26

newhoggy requested review from deepfire, denisshevchenko and jutaro as code owners July 7, 2022 14:26

newhoggy force-pushed the newhoggy/rename-utxoCostPerWord-to-utxoCostPerByte branch 2 times, most recently from 305e182 to 8138352 Compare July 7, 2022 14:45

newhoggy requested a review from Jimbo4350 July 7, 2022 14:46

newhoggy changed the title ~~Rename utxoCostPerWord to utxoCostPerByte~~ Add utxoCostPerByte protocol parameter Jul 7, 2022

newhoggy force-pushed the newhoggy/rename-utxoCostPerWord-to-utxoCostPerByte branch from 8138352 to 705fc3c Compare July 7, 2022 14:47

newhoggy force-pushed the newhoggy/rename-utxoCostPerWord-to-utxoCostPerByte branch from 705fc3c to 79df3c5 Compare July 7, 2022 14:52

newhoggy commented Jul 7, 2022

View reviewed changes

newhoggy force-pushed the newhoggy/rename-utxoCostPerWord-to-utxoCostPerByte branch 4 times, most recently from 45801c4 to 2343b86 Compare July 7, 2022 15:09

Jimbo4350 approved these changes Jul 7, 2022

View reviewed changes

newhoggy force-pushed the newhoggy/rename-utxoCostPerWord-to-utxoCostPerByte branch from 2343b86 to f6d5ada Compare July 7, 2022 22:36

newhoggy commented Jul 7, 2022

View reviewed changes

Add utxoCostPerByte parameter.

7193afe

See #4052 See cardano-foundation/CIPs#265

newhoggy force-pushed the newhoggy/rename-utxoCostPerWord-to-utxoCostPerByte branch from f6d5ada to 7193afe Compare July 8, 2022 01:19

iohk-bors bot added a commit that referenced this pull request Jul 8, 2022

Merge #4141

e926778

4141: Add utxoCostPerByte protocol parameter r=newhoggy a=newhoggy Resolves #4052 See cardano-foundation/CIPs#265 Co-authored-by: John Ky <[email protected]>

iohk-bors bot merged commit 9c217ae into master Jul 8, 2022

iohk-bors bot deleted the newhoggy/rename-utxoCostPerWord-to-utxoCostPerByte branch July 8, 2022 12:39

mkoura mentioned this pull request Jul 11, 2022

[BUG] - building Alonzo-era Plutus tx on Babbage fails due to missing protocolParamUTxOCostPerWord #4164

Closed

CarlosLopezDeLara mentioned this pull request Aug 2, 2022

1.35.3 SCOPE #4257

Open

12 tasks

caike mentioned this pull request May 2, 2024

Fix Kupmios provider errors on protocol params Anastasia-Labs/lucid-evolution#62

Closed

Add utxoCostPerByte protocol parameter #4141

Add utxoCostPerByte protocol parameter #4141

Conversation

newhoggy commented Jul 6, 2022 • edited Loading

lehins left a comment

Choose a reason for hiding this comment

newhoggy commented Jul 6, 2022

Jimbo4350 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

Jimbo4350 Jul 7, 2022 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 commented Jul 7, 2022

Choose a reason for hiding this comment

newhoggy commented Jul 8, 2022

iohk-bors bot commented Jul 8, 2022

newhoggy commented Jul 8, 2022

iohk-bors bot commented Jul 8, 2022

gitmachtl commented Jul 8, 2022 • edited Loading

gitmachtl commented Jul 24, 2022 • edited Loading

lehins commented Jul 24, 2022

newhoggy commented Jul 6, 2022 •

edited

Loading

Jimbo4350 Jul 7, 2022 •

edited

Loading

gitmachtl commented Jul 8, 2022 •

edited

Loading

gitmachtl commented Jul 24, 2022 •

edited

Loading