ST in output not checked #658

v0d1ch · 2022-12-22T16:15:09Z

Why

We want to make sure that the ST token is present in the tx output of the head validator and that it has correct CurrencySymbol that uniquely identifies a Hydra Head instance. We can't tell if some arbitrary NFT with the valid TokenName is indeed our ST token we expect so to tackle this security hole we are enforcing the check for the actual CurrencySymbol we expect.

What

Add CurrencySymbol to the datum and check the tx output for its existence in the head validators check for collectCom, close and contest.
Add the same check into commit and initial validator scripts in the ViaAbort case
Add Mutation tests for each of the affected validator checks

Note: This PR removes some of the tests that did exist previously. TxSpec ones are outdated, highy specialized and we have a coverage for the same properties already. It also removes the mutation tests tied to MutateHeadScriptInput since we are not using the head script output to check the datum contents.

To check before merging:

CHANGELOG is up to date
Up to date with master

github-actions · 2022-12-27T15:23:09Z

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	2023-01-11 15:54:43.472582258 UTC
Max. memory units	14000000
Max. CPU units	10000000000
Max. tx size (kB)	16384

Cost of Init Transaction

Parties	Tx size	% max Mem	% max CPU	Min fee ₳
1	4986	10.07	3.98	0.48
2	5189	13.97	5.54	0.53
3	5394	13.20	5.17	0.53
5	5802	18.76	7.35	0.61
10	6828	28.89	11.26	0.77
46	14210	98.68	38.11	1.85

Cost of Commit Transaction

Currently only one UTxO per commit allowed (this is about to change soon)

UTxO	Tx size	% max Mem	% max CPU	Min fee ₳
1	629	20.50	8.27	0.40

Cost of CollectCom Transaction

Parties	Tx size	% max Mem	% max CPU	Min fee ₳
1	12785	23.44	9.22	0.97
2	13209	44.03	17.38	1.21
3	13347	63.00	24.95	1.43
4	13841	90.05	35.99	1.75

Cost of Close Transaction

Parties	Tx size	% max Mem	% max CPU	Min fee ₳
1	9355	7.88	3.27	0.65
2	9520	8.67	3.73	0.67
3	9719	9.83	4.33	0.69
5	10011	11.04	5.10	0.72
10	10943	16.12	7.84	0.82
30	14208	31.50	16.82	1.15
69	16138	40.94	15.70	1.30

Cost of Contest Transaction

Parties	Tx size	% max Mem	% max CPU	Min fee ₳
1	9352	7.69	3.19	0.65
2	9548	8.81	3.78	0.67
3	9683	9.27	4.10	0.68
5	10012	10.85	5.02	0.71
10	10868	15.18	7.46	0.80
30	14232	31.79	16.93	1.16
42	16221	41.37	22.47	1.36

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	13612	21.24	8.91	0.99
2	14039	36.96	15.96	1.18
3	14313	54.49	24.12	1.39
4	14423	71.66	31.57	1.59

Cost of FanOut Transaction

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

UTxO	Tx size	% max Mem	% max CPU	Min fee ₳
1	13505	9.90	4.32	0.86
2	13607	12.10	5.47	0.89
3	13708	13.96	6.48	0.91
5	13844	17.13	8.28	0.96
10	13894	23.98	12.39	1.04
50	15458	85.96	48.02	1.87
59	15589	98.50	55.49	2.03

github-actions · 2022-12-28T15:55:31Z

Test Results

268 tests - 2 262 ✔️ - 2 12m 21s ⏱️ - 3m 26s
  91 suites - 1     6 💤 ±0
    4 files ±0     0 ❌ ±0

Results for commit e780c01. ± Comparison against base commit 5ed00df.

This pull request removes 4 and adds 2 tests. Note that renamed tests count towards both.

Hydra.Chain.Direct.Tx/abortTx ‑ Ignore InitTx with wrong contestation period
Hydra.Chain.Direct.Tx/abortTx ‑ cover fee correctly handles redeemers
Hydra.Chain.Direct.Tx/abortTx ‑ validates
Hydra.Chain.Direct.Tx/collectComTx ‑ validates

Hydra.Chain.Direct.Tx/collectComTx ‑ Ignore InitTx with wrong contestation period
Hydra.Chain.Direct.Tx/collectComTx ‑ cover fee correctly handles redeemers

♻️ This comment has been updated with latest results.

hydra-node/test/Hydra/Chain/Direct/Contract/Mutation.hs

hydra-node/test/Hydra/Chain/Direct/StateSpec.hs

hydra-node/src/Hydra/Chain/Direct/Tx.hs

hydra-node/src/Hydra/Options.hs

hydra-plutus/src/Hydra/Contract/HeadState.hs

hydra-node/test/Hydra/Chain/Direct/Contract/CollectCom.hs

hydra-node/test/Hydra/Chain/Direct/Contract/Abort.hs

ch1bo

I think we are missing the datum check in the commit validator. Other than that, some minor stuff in the mutations and I don't see where this is now enabling 8 parties to collect/abort a head?

CHANGELOG.md

hydra-cardano-api/src/Hydra/Cardano/Api/Value.hs

hydra-plutus/src/Hydra/Contract/Commit.hs

hydra-plutus/src/Hydra/Contract/HeadState.hs

hydra-plutus/src/Hydra/Contract/Initial.hs

hydra-node/test/Hydra/Chain/Direct/Contract/CollectCom.hs

hydra-node/test/Hydra/Chain/Direct/Contract/Commit.hs

hydra-node/test/Hydra/Chain/Direct/Contract/Contest.hs

…ommit

We use the healthyXXX prefix for our starting values to identify them as good values to mutate "away" from.

ch1bo

We are still missing a lot of error traces in the scripts.

This is required before I can approve.

I already addressed some of the inconsistencies and oversights. In general, the PR really shows that it's open too long already. Messy.

CHANGELOG.md

hydra-node/src/Hydra/Chain/Direct/Tx.hs

hydra-node/test/Hydra/Chain/Direct/Contract/Abort.hs

hydra-node/test/Hydra/Chain/Direct/TxSpec.hs

hydra-node/test/Hydra/Chain/Direct/StateSpec.hs

hydra-node/test/Hydra/Chain/Direct/Contract/Mutation.hs

hydra-node/test/Hydra/Chain/Direct/Contract/Contest.hs

Also refactor couple functions to move them to hydra-plutus. This change is larger than it should be.

Hydra.Chain.Direct.Tx was not directly including the required orphan instance, but Hydra.Ledger.Cardano is.

ch1bo · 2023-01-11T14:03:13Z

hydra-node/src/Hydra/Chain/Direct/Tx.hs

@@ -194,7 +190,7 @@ commitTx scriptRegistry networkId party utxo (initialInput, out, vkh) =
  initialScriptRef =
    fst (initialReference scriptRegistry)
  initialDatum =
-    mkScriptDatum $ Initial.datum ()
+    mkScriptDatum $ Initial.datum (headIdToCurrencySymbol headId)


hydra-node/src/Hydra/Chain/Direct/Tx.hs

ch1bo · 2023-01-11T14:07:59Z

hydra-plutus/src/Hydra/Contract/Head.hs

  mustBurnAllHeadTokens =
-    traceIfFalse "number of inputs do not match number of parties" $


ch1bo · 2023-01-11T14:08:09Z

hydra-plutus/src/Hydra/Contract/Head.hs

  everyoneHasCommitted =
-    traceIfFalse "not everyone committed" $


ch1bo · 2023-01-11T14:08:48Z

hydra-plutus/src/Hydra/Contract/Head.hs

@@ -372,10 +350,10 @@ checkHeadOutputDatum ctx d =
    NoOutputDatum ->
      traceError "missing datum"
    OutputDatumHash actualHash ->
-      traceIfFalse "output datum hash mismatch" $


ch1bo

Re-added some last traces in da1cdd9.. now it's good to go :)

v0d1ch · 2023-01-11T15:20:04Z

Re-added some last traces in da1cdd9.. now it's good to go :)

Nice but now our tx sizes grew bigger :) Shall I increase the limit or revert?

v0d1ch · 2023-01-11T16:06:20Z

We reduced the tx traces and hitting the button now!

v0d1ch self-assigned this Dec 22, 2022

v0d1ch marked this pull request as draft December 22, 2022 16:15

v0d1ch force-pushed the ensemble/st-in-output-not-checked branch from a0fbacc to 76949e0 Compare December 27, 2022 08:18

ffakenz reviewed Dec 28, 2022

View reviewed changes

hydra-node/test/Hydra/Chain/Direct/Contract/Mutation.hs Show resolved Hide resolved

ffakenz reviewed Dec 28, 2022

View reviewed changes

hydra-node/test/Hydra/Chain/Direct/StateSpec.hs Outdated Show resolved Hide resolved

v0d1ch force-pushed the ensemble/st-in-output-not-checked branch from c2f5832 to f650d86 Compare December 29, 2022 10:52

v0d1ch marked this pull request as ready for review December 30, 2022 08:12

ffakenz reviewed Dec 30, 2022

View reviewed changes

ffakenz approved these changes Dec 30, 2022

View reviewed changes

pgrange reviewed Jan 3, 2023

View reviewed changes

hydra-node/test/Hydra/Chain/Direct/Contract/Abort.hs Show resolved Hide resolved

pgrange approved these changes Jan 3, 2023

View reviewed changes

ch1bo self-requested a review January 3, 2023 10:42

ch1bo requested changes Jan 4, 2023

View reviewed changes

ffakenz force-pushed the ensemble/st-in-output-not-checked branch from ef6add8 to cb38604 Compare January 5, 2023 10:59

v0d1ch force-pushed the ensemble/st-in-output-not-checked branch 2 times, most recently from ada2638 to 8957d10 Compare January 5, 2023 15:05

pgrange mentioned this pull request Jan 5, 2023

Align implementation with HeadV1 protocol specification #452

Closed

20 tasks

cardano-scaling deleted a comment from pgrange Jan 5, 2023

v0d1ch force-pushed the ensemble/st-in-output-not-checked branch 10 times, most recently from 2c519f8 to fa01315 Compare January 10, 2023 10:17

v0d1ch added 2 commits January 11, 2023 08:29

Remove collectCom tx-spec test

d1b9fc0

Remove MutateHeadScriptInput since it is not relevant anymore for v_c…

6ff7ab4

…ommit

ch1bo force-pushed the ensemble/st-in-output-not-checked branch from 62023b6 to 6ff7ab4 Compare January 11, 2023 07:29

ch1bo added 4 commits January 11, 2023 08:45

Rename variables in Close mutation test

5c9c9a1

We use the healthyXXX prefix for our starting values to identify them as good values to mutate "away" from.

Fix haddock mentions of ST/PT

82c0e75

Use a dedicatd TxIn for Contest mutation tests

fbd2da5

Follow naming scheme in Commit mutation test

89f77b5

ch1bo requested changes Jan 11, 2023

View reviewed changes

v0d1ch and others added 8 commits January 11, 2023 09:42

Use type alias for v_initial script datum

e1e856b

Using lambda case style guide

0d4d233

We did'nt fix this acutally

a942c8c

FIX comment

2db7cd1

Don't use InitialState for checkCollectCom

c240341

Add back the traces in the v_head

4f74155

Rename State head fields to be uniform

dd17742

Output a realistic head datum in inspect-script

b056de3

Also refactor couple functions to move them to hydra-plutus. This change is larger than it should be.

abailly-iohk requested review from pgrange, ch1bo, abailly-iohk and ffakenz January 11, 2023 11:18

ch1bo added 2 commits January 11, 2023 15:13

Make import of orphan isntance more obvious

a31cfb2

Hydra.Chain.Direct.Tx was not directly including the required orphan instance, but Hydra.Ledger.Cardano is.

Re-add traces

da1cdd9

ch1bo reviewed Jan 11, 2023

View reviewed changes

ch1bo approved these changes Jan 11, 2023

View reviewed changes

Reduce trace messages length

e780c01

pgrange approved these changes Jan 11, 2023

View reviewed changes

v0d1ch merged commit 21e8855 into master Jan 11, 2023

v0d1ch deleted the ensemble/st-in-output-not-checked branch January 11, 2023 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ST in output not checked #658

ST in output not checked #658

v0d1ch commented Dec 22, 2022 •

edited

Loading

github-actions bot commented Dec 27, 2022 •

edited

Loading

github-actions bot commented Dec 28, 2022 •

edited

Loading

ch1bo left a comment

ch1bo left a comment

ch1bo Jan 11, 2023

ch1bo Jan 11, 2023

ch1bo Jan 11, 2023

ch1bo Jan 11, 2023

ch1bo left a comment •

edited

Loading

v0d1ch commented Jan 11, 2023

v0d1ch commented Jan 11, 2023

		mustBurnAllHeadTokens =
		traceIfFalse "number of inputs do not match number of parties" $

		everyoneHasCommitted =
		traceIfFalse "not everyone committed" $

ST in output not checked #658

ST in output not checked #658

Conversation

v0d1ch commented Dec 22, 2022 • edited Loading

Why

What

github-actions bot commented Dec 27, 2022 • edited Loading

Transactions Costs

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

github-actions bot commented Dec 28, 2022 • edited Loading

Test Results

ch1bo left a comment

Choose a reason for hiding this comment

ch1bo left a comment

Choose a reason for hiding this comment

ch1bo Jan 11, 2023

Choose a reason for hiding this comment

ch1bo Jan 11, 2023

Choose a reason for hiding this comment

ch1bo Jan 11, 2023

Choose a reason for hiding this comment

ch1bo Jan 11, 2023

Choose a reason for hiding this comment

ch1bo left a comment • edited Loading

Choose a reason for hiding this comment

v0d1ch commented Jan 11, 2023

v0d1ch commented Jan 11, 2023

v0d1ch commented Dec 22, 2022 •

edited

Loading

github-actions bot commented Dec 27, 2022 •

edited

Loading

github-actions bot commented Dec 28, 2022 •

edited

Loading

ch1bo left a comment •

edited

Loading