query protocol-parameters: use ledger JSON encoding, not API one #758

smelc · 2024-05-13T09:15:46Z

Changelog

- description: |
    query protocol-parameters: use ledger JSON encoding, not API one
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - refactoring    # QoL changes
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Fixes query protocol-parameters does not show params introduced in Conway #729
Result looked at by @gitmachtl already

How to trust this PR

cardano-node's tests ran on this PR: Test of cardano-cli/pull/758 - CI only cardano-node#5839
Review the diff of golden files on cardano-node's PR: https://github.com/IntersectMBO/cardano-node/pull/5839/files

Checklist

Commit sequence broadly makes sense and commits have useful messages
Self-reviewed the diff

carbolymer · 2024-05-13T12:41:23Z

cardano-cli/src/Cardano/CLI/EraBased/Run/Query.hs

-        $ writeLazyByteStringOutput mOutFile' $ encodePretty $ fromLedgerPParams sbe pparams
+        $ writeLazyByteStringOutput mOutFile'
+          $ shelleyBasedEraConstraints sbe
+            $ encodePretty pparams


What about this TODO: https://github.com/IntersectMBO/cardano-cli/blob/main/cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs#L506
Are we able to round-trip i.e. read the parameters into the transaction build after your change? Afair there was a testnet test for that.

Notes to myself:

On one hand, transaction build doesn't read the parameters by using the CLI's code: it requests them from the node

On the other hand, transaction build-raw does read parameters from the command line.

@carbolymer> actually there are multiple usages of the API JSON encoding:

In build-estimate: https://github.com/IntersectMBO/cardano-cli/blob/main/cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs#L286

In build-raw (the one you pointed out)

In calculate-min-fee: https://github.com/IntersectMBO/cardano-cli/blob/main/cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs#L1261

In calculate-min-value: https://github.com/IntersectMBO/cardano-cli/blob/main/cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs#L1342

Should we change all of them?

@gitmachtl> would you be able to test this change before we merge it? I'm afraid we are not the best ones to test this change exhaustively.

sure, also those are really important. let me compile the cli for testing...

@smelc so first issue i see is, that cli is complaining about the missing parameter key "minUTxOValue" not found

so the parameters with null value that were sorted out are causing an issue here. cli expects at least that the key is in there.

afaik only those parameters are automatically "trimmed" out right now:

"decentralization": null, "extraPraosEntropy": null, "minUTxOValue": null,

not sure if "decentralization" and "extraPraosEntropy" are actually ever read back for some processes? if not, only the "minUTxoValue" one is an issue. that parameter was only used pre babbage era, so would it be an option to only read that in if the used era expects it? utxoCostPerWord is also not present anymore in latest eras. but was used before, that might also be an issue during bootstrap phase to get thru the eras.

@gitmachtl> is there a way I can reproduce your tests myself? So that then I can investigate how to fix the issues in autonomy.

@smelc Sure, you can simply copy&paste those lines on the cli.

dummyTx='{"type":"Unwitnessed Tx ConwayEra","description":"Ledger Cddl Format","cborHex":"84a400d901028182582090ebfd31ec657d52eecf489871ecabefdce3753877aee7ee9e3200bd55fba9b800018282581d6052e63f22c5107ed776b70f7b92248b02552fd08f3e747bc745099441821a002f9d38ac581c01a7db2a2a897c4a84ca4b75b17281df80cc8607413644d21a9e020da14b73746172666f726765313002581c01c2435381c8362192bce939c002a99ce79c7bb7ba399cc55e2af07da14b73746172666f726765373901581c02f917e4ebe2f3474a713ec76e45e51f1ac72d5b053624d3ee124c12a14a73746172666f7267653201581c07e346cdcb11cff8048cf9ae2166a54c3324cf09b04c7554d6e94391a14b73746172666f726765373301581c0b6f1f1220140053fe1a9e44f4d77c12a5909f599970cd9b65e638e6a14b73746172666f726765343301581c0f28da1e6892acb5973ec68aab4e4718fec8761673e9d725343d97a9a14b73746172666f726765393701581c10ccfab9710c68068f3cd2bc86a96420c97b6d363a68b9232097b86aa14b73746172666f726765383801581c11b6c9e65d8e1e1bd51743a8599bfc8d70e1b1ccebb8902e32538c55a14b73746172666f726765313201581c12df514d7cd52225e39a7fe27ee0652dd2f1bfab05139deadda2a7caa14b73746172666f726765383901581c14deabc3a38365004e31a8a865a425e187fe4776e84c626c2c822344a148373536376130363301581cb2e796da6ef0fc4b427ca94f400691f22c949ed09ddd61592611e015a14b73746172666f726765343201581ce6911f8b6bae62fc20b96e55131bbf189334baab80d07f2964ca3b6da14a73746172666f726765300382581d6052e63f22c5107ed776b70f7b92248b02552fd08f3e747bc7450994411b000000058503a08a021a00030d40031a01ba9424a0f5f6"}' protocolParametersJSON=$(cardano-cli conway query protocol-parameters) cardano-cli conway transaction calculate-min-fee --tx-body-file <(echo ${dummyTx}) --protocol-params-file <(echo ${protocolParametersJSON}) --witness-count 1 --reference-script-size 0 Error: "Error in $: key \"extraPraosEntropy\" not found"

So the cli complains about the missing extrapraosEntropy parameter. To simulate the presence of the extraPraosEntropy we can do:

protocolParametersJSON=$(./cardano-cli conway query protocol-parameters | jq -r ". += {\"extraPraosEntropy\": null}") cardano-cli conway transaction calculate-min-fee --tx-body-file <(echo ${dummyTx}) --protocol-params-file <(echo ${protocolParametersJSON}) --witness-count 1 --reference-script-size 0 Error: "Error in $: key \"minUTxOValue\" not found"

Now its complaining about the missing minUTxOVallue

If we also add this with the null value again, the fee calculation works:

protocolParametersJSON=$(./cardano-cli conway query protocol-parameters | jq -r ". += {\"extraPraosEntropy\": null, \"minUTxOValue\": null}") cardano-cli conway transaction calculate-min-fee --tx-body-file <(echo ${dummyTx}) --protocol-params-file <(echo ${protocolParametersJSON}) --witness-count 1 --reference-script-size 0 189173 Lovelace

Same errors are for the calculate-min-required-utxo function.

@smelc yes, we should only use one encoding, so in this case ledger's.

smelc · 2024-05-15T16:26:07Z

Work happening at https://github.com/IntersectMBO/cardano-cli/commits/smelc/8.22.0.0-backport-write-protocol-parameters-use-ledger-encoding/

I will bring the commits to this PR when I'm done. I need to work on 8.22.0.0 to more easily test that with cardano-node master.

carbolymer · 2024-05-16T14:47:56Z

@smelc check afterwards with @mkoura if it's not breaking cardano-node-tests too.

smelc · 2024-05-17T10:17:58Z

@gitmachtl> can you give this PR a new round of testing?

gitmachtl · 2024-05-17T12:37:28Z

@gitmachtl> can you give this PR a new round of testing?

@smelc yea, will do it over the weekend, thx.

gitmachtl · 2024-05-19T22:17:15Z

@gitmachtl> can you give this PR a new round of testing?

@smelc looking good. did test the min-fee calculation, min-utxo calculation, made transactions via build-raw and ran the new build-estimate command with the new protocol-parameter version. 👍 but i only tested in conway era. have to redo it also in babbage era.

gitmachtl · 2024-05-20T18:51:45Z

@smelc ok, babbage era also looks good 👍

smelc · 2024-05-21T09:10:48Z

Thanks @gitmachtl for the feedback 🙏

Jimbo4350

LGTM! Feel free to merge as long as @gitmachtl is happy.

gitmachtl · 2024-05-28T11:48:55Z

LGTM! Feel free to merge as long as @gitmachtl is happy.

@Jimbo4350 i am happy 😄

smelc mentioned this pull request May 13, 2024

Test of cardano-cli/pull/758 - CI only IntersectMBO/cardano-node#5839

Closed

smelc force-pushed the smelc/write-protocol-parameters-use-ledger-encoding branch from 3486a52 to 3e7aae3 Compare May 13, 2024 12:01

smelc marked this pull request as ready for review May 13, 2024 12:02

smelc requested review from Jimbo4350, newhoggy, carbolymer, palas, kevinhammond and disassembler as code owners May 13, 2024 12:02

carbolymer reviewed May 13, 2024

View reviewed changes

smelc mentioned this pull request May 13, 2024

Add Conway parameters in ProtocolParameters IntersectMBO/cardano-api#532

Closed

3 tasks

gitmachtl mentioned this pull request May 14, 2024

query protocol-parameters does not show params introduced in Conway #729

Closed

smelc marked this pull request as draft May 15, 2024 15:51

smelc mentioned this pull request May 16, 2024

Remove UnwitnessedCliFormattedTxBody constructor #707

Merged

3 tasks

smelc changed the base branch from main to smelc/remove-UnwitnessedCliFormattedTxBody-constructor-v2 May 16, 2024 15:45

smelc force-pushed the smelc/write-protocol-parameters-use-ledger-encoding branch from d073319 to f54073c Compare May 16, 2024 15:45

Base automatically changed from smelc/remove-UnwitnessedCliFormattedTxBody-constructor-v2 to main May 17, 2024 06:55

smelc force-pushed the smelc/write-protocol-parameters-use-ledger-encoding branch 2 times, most recently from 3931011 to 1bad372 Compare May 17, 2024 09:42

smelc force-pushed the smelc/write-protocol-parameters-use-ledger-encoding branch from 1bad372 to c9a7471 Compare May 17, 2024 12:40

smelc marked this pull request as ready for review May 17, 2024 14:10

smelc force-pushed the smelc/write-protocol-parameters-use-ledger-encoding branch from c9a7471 to f71d11b Compare May 21, 2024 09:12

Jimbo4350 approved these changes May 28, 2024

View reviewed changes

smelc added 2 commits May 28, 2024 13:19

query protocol parameters: use ledger JSON encoding, not API one

fa7044c

readProtocolParameters: return Ledger PParams instead of API ones

87134c3

smelc force-pushed the smelc/write-protocol-parameters-use-ledger-encoding branch from f71d11b to 87134c3 Compare May 28, 2024 11:19

smelc enabled auto-merge May 28, 2024 12:21

smelc disabled auto-merge May 28, 2024 12:21

smelc merged commit b08f10c into main May 28, 2024
14 of 15 checks passed

smelc deleted the smelc/write-protocol-parameters-use-ledger-encoding branch May 28, 2024 12:21

smelc mentioned this pull request May 30, 2024

protocol parameters: do not depend on API encoding anymore #779

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

query protocol-parameters: use ledger JSON encoding, not API one #758

query protocol-parameters: use ledger JSON encoding, not API one #758

smelc commented May 13, 2024 •

edited

Loading

carbolymer May 13, 2024

smelc May 13, 2024

smelc May 13, 2024

gitmachtl May 13, 2024

gitmachtl May 13, 2024 •

edited

Loading

smelc May 14, 2024

gitmachtl May 14, 2024 •

edited

Loading

carbolymer May 16, 2024

smelc commented May 15, 2024

carbolymer commented May 16, 2024 •

edited

Loading

smelc commented May 17, 2024

gitmachtl commented May 17, 2024

gitmachtl commented May 19, 2024 •

edited

Loading

gitmachtl commented May 20, 2024

smelc commented May 21, 2024

Jimbo4350 left a comment

gitmachtl commented May 28, 2024

query protocol-parameters: use ledger JSON encoding, not API one #758

query protocol-parameters: use ledger JSON encoding, not API one #758

Conversation

smelc commented May 13, 2024 • edited Loading

Changelog

Context

How to trust this PR

Checklist

carbolymer May 13, 2024

Choose a reason for hiding this comment

smelc May 13, 2024

Choose a reason for hiding this comment

smelc May 13, 2024

Choose a reason for hiding this comment

gitmachtl May 13, 2024

Choose a reason for hiding this comment

gitmachtl May 13, 2024 • edited Loading

Choose a reason for hiding this comment

smelc May 14, 2024

Choose a reason for hiding this comment

gitmachtl May 14, 2024 • edited Loading

Choose a reason for hiding this comment

carbolymer May 16, 2024

Choose a reason for hiding this comment

smelc commented May 15, 2024

carbolymer commented May 16, 2024 • edited Loading

smelc commented May 17, 2024

gitmachtl commented May 17, 2024

gitmachtl commented May 19, 2024 • edited Loading

gitmachtl commented May 20, 2024

smelc commented May 21, 2024

Jimbo4350 left a comment

Choose a reason for hiding this comment

gitmachtl commented May 28, 2024

smelc commented May 13, 2024 •

edited

Loading

gitmachtl May 13, 2024 •

edited

Loading

gitmachtl May 14, 2024 •

edited

Loading

carbolymer commented May 16, 2024 •

edited

Loading

gitmachtl commented May 19, 2024 •

edited

Loading