Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validator can assume head output is the first one #700

Merged

Conversation

v0d1ch
Copy link
Contributor

@v0d1ch v0d1ch commented Feb 3, 2023

Why

Since we can assume the v_head output is always the first one in the list of outputs there is a possibility to simplify some
validator code

To check before merging:

  • CHANGELOG is up to date
  • Up to date with master

@v0d1ch v0d1ch marked this pull request as draft February 3, 2023 10:09
@v0d1ch v0d1ch force-pushed the ensemble/validator-can-assume-head-output-is-the-first-one branch from 5b398c8 to cc7f8f4 Compare February 3, 2023 13:13
@v0d1ch v0d1ch marked this pull request as ready for review February 3, 2023 13:57
@v0d1ch v0d1ch requested a review from ch1bo February 3, 2023 15:35
@github-actions
Copy link

github-actions bot commented Feb 3, 2023

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-02-06 12:13:07.971611523 UTC
Max. memory units 14000000
Max. CPU units 10000000000
Max. tx size (kB) 16384

Script summary

Name Hash Size (Bytes)
νInitial 6e4ce2bd32260424babefe087c2a5ee0a414745fe0281d5608ae9bf8 5530
νCommit 03f2e29fd352f8c2961ad2f61d87a48f88bf01ea6c079b46a3184aee 2547
νHead 3dcd919e2a1b9a7d25fe32d7815c322ca4b4c0881b88926beff43b8f 9169

Cost of Init Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 4984 10.43 4.13 0.48
2 5189 11.72 4.61 0.51
3 5398 16.46 6.51 0.57
5 5803 20.58 8.10 0.63
10 6827 27.97 10.88 0.76
45 14005 97.56 37.70 1.83

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 630 21.18 8.56 0.41

Cost of CollectCom Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 12522 23.03 9.19 0.95
2 12696 36.07 14.52 1.11
3 13294 58.77 23.84 1.38
4 13472 76.61 31.25 1.59
5 13753 98.90 40.52 1.85

Cost of Close Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 9765 12.45 4.99 0.72
2 9963 14.40 5.89 0.75
3 10162 15.80 6.56 0.77
5 10429 17.74 7.57 0.81
10 11320 25.37 11.21 0.93
30 14655 52.46 24.38 1.39
67 16345 73.22 27.16 1.66

Cost of Contest Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 9752 12.27 4.91 0.71
2 9926 13.59 5.56 0.74
3 10123 15.50 6.44 0.77
5 10455 18.14 7.73 0.81
10 11280 24.81 10.98 0.93
30 14614 51.91 24.16 1.39
40 16302 65.41 30.73 1.62

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 14056 24.50 10.09 1.04
2 14405 42.00 17.75 1.25
3 14519 57.55 24.37 1.43
4 14625 74.88 31.80 1.63

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 14081 10.68 4.62 0.89
2 14183 12.59 5.65 0.92
3 14152 14.23 6.59 0.94
5 14159 16.40 8.00 0.96
10 14334 24.02 12.40 1.06
20 14694 39.26 21.20 1.27
59 16165 99.15 55.74 2.06

@github-actions
Copy link

github-actions bot commented Feb 3, 2023

Test Results

271 tests  ±0   265 ✔️ ±0   16m 20s ⏱️ + 4m 23s
  92 suites ±0       6 💤 ±0 
    4 files   ±0       0 ±0 

Results for commit d22a236. ± Comparison against base commit 6eaaf72.

♻️ This comment has been updated with latest results.

@v0d1ch v0d1ch assigned v0d1ch and ghost Feb 3, 2023
Copy link
Collaborator

@ch1bo ch1bo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that we need to add artificial change outputs to our off-chain code in some tests is smelly and must be changed IMO.

hydra-node/src/Hydra/Chain/Direct/State.hs Outdated Show resolved Hide resolved
hydra-plutus/src/Hydra/Contract/Commit.hs Outdated Show resolved Hide resolved
hydra-plutus/src/Hydra/Contract/Head.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@pgrange pgrange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review tries to apply the perfection game protocol

I'm expecting the author of the P.R. to just not include my ideas if they disagree with them without any form of justification. Hence I, in advance, approve the P.R. Please ask for clarifications if my comments are unclear.

I will try to extensively use the suggest change feature of GitHub as I believe this is the most efficient way to upload data from my brain to the authors' brains. That being said, the suggestions I will write will probably not compile or pass the test so do not just accept them but consider them for what they are: the description of an improvement to make the P.R. perfect to me.

What I like about this P.R.:

  • I can see a small list of clear and small commits which makes the review smoother as I can just review one commit at a time and understand how you walked through the change

For me to find it perfect you would have to:

  • amend the commit Use foldr when traversing inputs to explain why it's more interesting to do so
  • amend the commits with failing tests to make them pass. For instance, now, looking at the commit history, I can't figure out how a commit named Add a note related to change outputs in collectCom can make the test pass when they were not passing in the previous commit
    *amend the commit Add a note related to change outputs in collectCom to make two separate commits, one only adding the note and another one adding the production code and explaining why we add this code.
  • see other improvement proposals below

@@ -205,28 +206,17 @@ checkCollectCom ctx@ScriptContext{scriptContextTxInfo = txInfo} (contestationPer
-- Collect fuel and commits from resolved inputs. Any output containing a PT
-- is treated as a commit, "our" output is the head output and all remaining
-- will be accumulated as 'fuel'.
traverseInputs (fuel, commits, nCommits) = \case
[] ->
traverseInputs TxInInfo{txInInfoResolved} (fuel, commits, nCommits)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function does not traverse all the inputs anymore, I would, either, rename the function to make that clear but I'm not sure:

Suggested change
traverseInputs TxInInfo{txInInfoResolved} (fuel, commits, nCommits)
traverseInput TxInInfo{txInInfoResolved} (fuel, commits, nCommits)

Or I would keep the function's name and keep the call site as it was by introducing some auxiliary function to traverseInputs:

Suggested change
traverseInputs TxInInfo{txInInfoResolved} (fuel, commits, nCommits)
traverseInputs someList =
foldr auxiliary someList
where
auxiliary TxInInfo{txInInfoResolved} (fuel, commits, nCommits) =

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extractAndCountCommits it is! :)

hydra-node/src/Hydra/Chain/Direct/Util.hs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@v0d1ch v0d1ch force-pushed the ensemble/validator-can-assume-head-output-is-the-first-one branch from a3db761 to c1fa047 Compare February 6, 2023 11:46
@v0d1ch v0d1ch requested a review from ch1bo February 6, 2023 12:01
@v0d1ch v0d1ch force-pushed the ensemble/validator-can-assume-head-output-is-the-first-one branch from d391fa2 to d22a236 Compare February 6, 2023 12:03
traverseInputs
(negate (txInfoAdaFee txInfo), [], 0)
(collectedCommits, nTotalCommits) =
foldr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice refactor!

Copy link
Collaborator

@ch1bo ch1bo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and small. Notice that we removed some 1.2kB of script code in this PR!

@v0d1ch v0d1ch merged commit 8db4dae into master Feb 6, 2023
@v0d1ch v0d1ch deleted the ensemble/validator-can-assume-head-output-is-the-first-one branch February 6, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants