Align protocol parameters JSON #1226

v0d1ch · 2023-12-29T12:49:41Z

This is a reproduction of the issue we found where cardano-cli query protocol-parameters is missing to return certain fields (like minFeeA, minFeeB) which are present in the Json instance for PParams in cardano-ledger. Since cardano-cli is just calling the cardano-api query to obtain PParams perhaps the bug is somewhere in the fromLedgerPParams or similar functions that convert back and forth between ledger and api representation of PParams.

@ch1bo / @locallycompact adopted this PR:

In fact, the cardano-cli was not broken, but the hydra-node was expecting cardano-ledger PParams on the --ledger-protocol-parameters input file .. which is wrong and this PR fixes this now. This was a hidden breaking change on master so far!

👾 Load --ledger-protocol-parameters using the cardano-api ProtocolParameters instance, which is the one the cardano-cli also uses.
👾 Adds a test that asserts the Hydra.Ledger.Cardano.Configuration.pparamsFromJson function can read the JSON returned by cardano-cli query protocol-parameters

CHANGELOG updated or not needed
Documentation updated or not needed
Haddocks updated or not needed
No new TODOs introduced or explained herafter
- One TODO about the fact that we should add (somewhere) and test against a JSON schema to ensure pparamsFromJson works as expected in hydra-node -> follow-up PR

github-actions · 2023-12-29T13:00:52Z

Transactions Costs

Sizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using arbitrary values and results are not fully deterministic and comparable to previous runs.

Metadata
Generated at	2024-01-05 18:03:42.349993309 UTC
Max. memory units	14000000
Max. CPU units	10000000000
Max. tx size (kB)	16384

Script summary

Name	Hash	Size (Bytes)
νInitial	985245919fcc6c0c5cd116023cd2c947c43e80dcbb5075fe12433fbb	4072
νCommit	7cb20fa71eb4c563ca283566ebe0aa65859d96c3f8cba35c52c181fd	2043
νHead	7a36661f5c15e9f1783aeaab890812c59b7286cbbc6de762d3110772	8816
μHead	8b111ac12274e46314769295a1c5dcab1d260096fc469fd698065463*	3851

The minting policy hash is only usable for comparison. As the script is parameterized, the actual script is unique per Head.

Cost of Init Transaction

Parties	Tx size	% max Mem	% max CPU	Min fee ₳
1	4375	10.25	3.95	0.45
2	4576	12.72	4.90	0.49
3	4774	15.02	5.77	0.52
5	5180	19.22	7.35	0.59
10	6184	30.30	11.55	0.75
41	12418	99.15	37.65	1.77

Cost of Commit Transaction

This is using ada-only outputs for better comparability.

UTxO	Tx size	% max Mem	% max CPU	Min fee ₳
1	531	11.37	4.44	0.30
2	723	15.04	6.07	0.35
3	912	18.85	7.75	0.40
5	1280	26.90	11.27	0.51
10	2216	49.55	20.97	0.80
19	3906	99.43	41.75	1.43

Cost of CollectCom Transaction

Parties	UTxO (bytes)	Tx size	% max Mem	% max CPU	Min fee ₳
1	57	480	21.12	8.27	0.40
2	113	590	33.21	13.10	0.54
3	170	700	45.87	18.26	0.68
4	227	810	63.32	25.28	0.88
5	283	924	74.40	30.08	1.01

Cost of Close Transaction

Parties	Tx size	% max Mem	% max CPU	Min fee ₳
1	544	16.72	7.69	0.36
2	722	18.41	9.29	0.40
3	913	20.18	10.93	0.43
5	1215	23.23	13.80	0.49
10	2196	32.49	22.30	0.67
50	8853	98.12	83.29	1.93

Cost of Contest Transaction

Parties	Tx size	% max Mem	% max CPU	Min fee ₳
1	569	20.18	8.94	0.40
2	714	21.72	10.30	0.43
3	908	24.19	12.35	0.47
5	1294	27.98	15.70	0.55
10	2103	36.71	23.34	0.71
43	7670	97.72	76.36	1.83

Cost of Abort Transaction

Some variation because of random mixture of still initial and already committed outputs.

Parties	Tx size	% max Mem	% max CPU	Min fee ₳
1	4307	18.85	8.11	0.55
2	4499	32.06	13.97	0.71
3	4661	47.34	20.79	0.89
4	4819	66.19	29.13	1.11
5	4999	87.85	38.74	1.36

Cost of FanOut Transaction

Involves spending head output and burning head tokens. Uses ada-only UTxO for better comparability.

Parties	UTxO	UTxO (bytes)	Tx size	% max Mem	% max CPU	Min fee ₳
5	0	0	4205	8.26	3.46	0.43
5	1	56	4238	9.49	4.22	0.44
5	5	285	4375	13.98	7.09	0.51
5	10	570	4546	20.13	10.90	0.59
5	20	1139	4885	32.43	18.53	0.76
5	30	1709	5226	45.15	26.34	0.93
5	40	2278	5565	57.46	33.97	1.10
5	50	2847	5905	69.56	41.52	1.27
5	74	4214	6719	99.34	59.96	1.68

End-To-End Benchmark Results

This page is intended to collect the latest end-to-end benchmarks results produced by Hydra's Continuous Integration system from the latest master code.

Please take those results with a grain of salt as they are currently produced from very limited cloud VMs and not controlled hardware. Instead of focusing on the absolute results, the emphasis should be on relative results, eg. how the timings for a scenario evolve as the code changes.

Generated at 2024-01-05 18:06:43.034545384 UTC

Baseline Scenario

Number of nodes	3
Number of txs	9000
Avg. Confirmation Time (ms)	23.563835508
P99	116.76609833000005ms
P95	33.556395ms
P50	20.6950895ms
Number of Invalid txs	0

Baseline Scenario

Number of nodes	1
Number of txs	3000
Avg. Confirmation Time (ms)	4.205986084
P99	6.009112069999992ms
P95	4.993861949999998ms
P50	4.10217ms
Number of Invalid txs	0

github-actions · 2024-01-03T13:32:32Z

Test Results

393 tests +3 385 ✅ +2 18m 18s ⏱️ - 2m 48s
132 suites ±0 8 💤 +1
5 files ±0 0 ❌ ±0

Results for commit 191cb0c. ± Comparison against base commit bd8a113.

♻️ This comment has been updated with latest results.

abailly-iohk

The fact a single change to the withHydraNode function ripples all over the place because we need to modify how it's called appears to me as a want for refactoring and encapsulating the parameters into some structure with default values

abailly-iohk · 2024-01-05T14:30:46Z

hydra-cluster/src/HydraNode.hs

-    let cardanoLedgerProtocolParametersFile = dir </> "pparams.json"
-    writeFileBS cardanoLedgerProtocolParametersFile (writeZeroedPParams pparams)
+    let cardanoLedgerProtocolParametersFile = dir </> "protocol-parameters.json"
+    case chainConfig of


This logic is starting to be quite large, I would suggest to extract a function eg. makeProtocolParametersFile or something

.github/workflows/ci-nix.yaml

hydra-cluster/bench/Bench/EndToEnd.hs

hydra-cluster/bench/Main.hs

hydra-cluster/src/CardanoClient.hs

hydra-cluster/src/CardanoNode.hs

This is needed until the upstram bug fix is released.

Move one related cardano-cli test to appropriate module

The removed test is pointless as it does compare the output of using cardano-api with using cardano-cli. We do not care whether those are consistent, but rather that the output matches our expectations.

This deliberately uses the cardano-cli to query protocol parameters as JSON.

These are a documented starting point to come up with parameters for the hydra-node and are also used for the offline mode 'withHydraNode' in hydra-cluster.

This also improves the parse error message on hydra-node --contestation-period as it helped us to debug this.

It's not a good idea to keep protocol parameters in the RunningNode handle as they could change throughout our scenarios.

We were missing the cardano-cli from the shell environment running the benchmark in CI. The `withFile` from `base` is actively rewriting the ioe_location from "posix_spawnp" to "withFile" and thus obfuscates any errors raised within the wrapped action.

Rebased the whole PR on top of #1236 and so we do not need this workaround anymore.

v0d1ch self-assigned this Dec 29, 2023

ch1bo added this to the 0.15.0 milestone Jan 2, 2024

ch1bo assigned locallycompact and ch1bo and unassigned v0d1ch Jan 3, 2024

ch1bo changed the title ~~Check protocol parameters roundtrip~~ Align protocol parameters JSON Jan 3, 2024

ch1bo added the bug 🐛 Something isn't working label Jan 3, 2024

ch1bo force-pushed the check-protocol-parameters-roundtrip branch from 890e634 to 4a69ba4 Compare January 3, 2024 14:07

locallycompact force-pushed the check-protocol-parameters-roundtrip branch from 6a2fb64 to c7ec699 Compare January 3, 2024 14:37

ch1bo force-pushed the check-protocol-parameters-roundtrip branch 2 times, most recently from ab50449 to dcfc5e3 Compare January 4, 2024 10:14

locallycompact force-pushed the check-protocol-parameters-roundtrip branch from 2c83d56 to 59e70b3 Compare January 4, 2024 12:08

ch1bo requested review from ffakenz and abailly-iohk January 4, 2024 12:21

ch1bo force-pushed the check-protocol-parameters-roundtrip branch 3 times, most recently from dea7363 to 91f5d83 Compare January 5, 2024 12:39

abailly-iohk approved these changes Jan 5, 2024

View reviewed changes

ch1bo force-pushed the check-protocol-parameters-roundtrip branch from 945d500 to 1bd94d3 Compare January 5, 2024 14:57

ffakenz approved these changes Jan 5, 2024

View reviewed changes

v0d1ch and others added 8 commits January 5, 2024 18:57

Check cardano-cli version in scope and pparams roundtrip

8d7e1b7

Fix the failing test by setting the protocolVersion to optional field

33ccefd

This is needed until the upstram bug fix is released.

Query PParams using cardano-cli in scope and check compatibility

68d9070

Move one related cardano-cli test to appropriate module

Contain the changes only in CardanoCliSpec

6ffce46

Remove redundant cardano-cli test and add diagnostic output

ff2681b

The removed test is pointless as it does compare the output of using cardano-api with using cardano-cli. We do not care whether those are consistent, but rather that the output matches our expectations.

CardanoCliSpec: Only test decoding to ProtocolParameters

afa5994

Draft a cliQueryProtocolParameters in withHydraNode'

c38930f

This deliberately uses the cardano-cli to query protocol parameters as JSON.

match on ChainConfig in withHydraNode

711d603

ch1bo and others added 22 commits January 5, 2024 18:57

Fix invocation of cardano-cli and pin-point the root cause

a44c621

Add example protocol-parameters.json to hydra-cluster/config

bfc3e78

These are a documented starting point to come up with parameters for the hydra-node and are also used for the offline mode 'withHydraNode' in hydra-cluster.

Remove PParams from withHydraNode calls

7b0b66e

Add blockTime to RunningNode to be flexible in e2e tests

4991bba

withHydraNode': Add Tracer argument

fed7d0f

Use 1s contestation period on full e2e test

535f22c

This also improves the parse error message on hydra-node --contestation-period as it helped us to debug this.

Fix a typo

edf1380

withCardanoNode: callback with RunningNode instead of SocketPath

deca67d

Remove pparams from RunningNode

237d20a

It's not a good idea to keep protocol parameters in the RunningNode handle as they could change throughout our scenarios.

Query genesis parameters to compute blockTime

244c5df

Infer blockTime properly

75e15dc

Explicitly test pparamsFromJson in CardanoCliSpec

a11ddb7

simplify cardano-cli pparams test

55a15e8

Upload benchmark logs

ed505a1

Minor refactor while waiting for CI failures

9a30287

Debug benchmark failures in CI

a142014

Use a dedicated work directory per dataset

6d90a17

Add more trace output in bench

d3ca66b

Minor refactor: Use readProcess and re-use cmd

f3091af

Not work around, but rely on withFile being fixed

802c064

Rebased the whole PR on top of #1236 and so we do not need this workaround anymore.

Print where benchmark logs are saved

191cb0c

ch1bo force-pushed the check-protocol-parameters-roundtrip branch from 1bd94d3 to 191cb0c Compare January 5, 2024 18:01

ch1bo merged commit a1b4e90 into master Jan 8, 2024
21 of 25 checks passed

ch1bo deleted the check-protocol-parameters-roundtrip branch January 8, 2024 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align protocol parameters JSON #1226

Align protocol parameters JSON #1226

v0d1ch commented Dec 29, 2023 •

edited by ch1bo

Loading

github-actions bot commented Dec 29, 2023 •

edited

Loading

github-actions bot commented Jan 3, 2024 •

edited

Loading

abailly-iohk left a comment

abailly-iohk Jan 5, 2024

Align protocol parameters JSON #1226

Align protocol parameters JSON #1226

Conversation

v0d1ch commented Dec 29, 2023 • edited by ch1bo Loading

github-actions bot commented Dec 29, 2023 • edited Loading

Transactions Costs

Script summary

Cost of Init Transaction

Cost of Commit Transaction

Cost of CollectCom Transaction

Cost of Close Transaction

Cost of Contest Transaction

Cost of Abort Transaction

Cost of FanOut Transaction

End-To-End Benchmark Results

Baseline Scenario

Baseline Scenario

github-actions bot commented Jan 3, 2024 • edited Loading

Test Results

abailly-iohk left a comment

Choose a reason for hiding this comment

abailly-iohk Jan 5, 2024

Choose a reason for hiding this comment

v0d1ch commented Dec 29, 2023 •

edited by ch1bo

Loading

github-actions bot commented Dec 29, 2023 •

edited

Loading

github-actions bot commented Jan 3, 2024 •

edited

Loading