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

Snapshot number and utxo hash need not be in the redeemer on close/contest #677

Conversation

ffakenz
Copy link
Contributor

@ffakenz ffakenz commented Jan 16, 2023

⛰️ Remove snapshot number and utxo hash from Contest and Close redeemers

  • the output datum needs to include them and we can check using that
  • only the signature is enough

⛰️ BTW this improves the coverage for the case where we close with the initialSnapshot

To check before merging:

  • CHANGELOG is up to date
  • Documentation is up to date

@github-actions
Copy link

github-actions bot commented Jan 16, 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-01 16:36:05.646474015 UTC
Max. memory units 14000000
Max. CPU units 10000000000
Max. tx size (kB) 16384

Script summary

Name Hash Size (Bytes)
νInitial bbed691fc7fdce8d95616cf16b01e5ed2abb42f20c0a5de6882bb860 5429
νCommit 08bb32edf316341c4fa7b0e3ed0e77bccb7f1d3b687c10d1b1e48969 2961
νHead e4c9e128a7674f68247a796da72265727c782e54d93e1285b4066c60 9835

Cost of Init Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 4984 12.19 4.86 0.50
2 5187 11.26 4.42 0.50
3 5397 13.20 5.17 0.53
5 5804 18.57 7.27 0.61
10 6829 29.99 11.71 0.78
45 14005 98.23 37.98 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 633 20.90 8.47 0.41

Cost of CollectCom Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 13425 22.01 8.61 0.98
2 13884 42.11 16.67 1.22
3 14235 63.92 25.45 1.48
4 14659 92.72 36.96 1.81

Cost of Close Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 10427 14.87 5.84 0.77
2 10463 13.72 5.25 0.76
3 10761 17.52 7.13 0.82
5 11158 20.61 8.60 0.87
10 11989 27.43 11.92 0.99
30 15292 54.04 24.90 1.44
60 16321 68.76 25.49 1.61

Cost of Contest Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 10426 14.43 5.66 0.77
2 10624 16.08 6.44 0.79
3 10816 17.78 7.23 0.82
5 11086 19.72 8.25 0.86
10 12015 27.78 12.05 0.99
30 15313 54.09 24.91 1.44
36 16271 61.76 28.68 1.57

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 15004 29.47 12.52 1.14
2 15072 42.12 17.79 1.28
3 15181 57.52 24.34 1.46
4 15367 77.15 32.80 1.69

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 14616 10.20 4.44 0.91
2 14652 11.47 5.22 0.93
3 14815 14.05 6.51 0.96
5 14760 16.04 7.86 0.99
10 15135 25.05 12.81 1.11
20 15429 39.96 21.48 1.31
48 16299 81.92 45.86 1.86

@ffakenz ffakenz force-pushed the gap/move_attributes_from_redeemer_to_datum_for_close_and_contest_tx branch from 172a1bc to 2f0b7e2 Compare January 16, 2023 19:04
@github-actions
Copy link

github-actions bot commented Jan 17, 2023

Test Results

282 tests  +13   276 ✔️ +13   16m 1s ⏱️ + 2m 40s
  96 suites +  5       6 💤 ±  0 
    5 files   +  1       0 ±  0 

Results for commit 005ade2. ± Comparison against base commit 179df2f.

♻️ This comment has been updated with latest results.

@ffakenz ffakenz marked this pull request as ready for review January 17, 2023 16:12
@ch1bo ch1bo requested review from ch1bo and v0d1ch January 18, 2023 10:39
@ch1bo ch1bo assigned ffakenz and ch1bo and unassigned ffakenz Jan 19, 2023
Copy link
Contributor

@v0d1ch v0d1ch left a comment

Choose a reason for hiding this comment

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

Looks good to me overall @ffakenz, I just have a concern @ch1bo will be able to resolve which is related to constructing the expected datum.

hydra-plutus/src/Hydra/Contract/Head.hs Outdated Show resolved Hide resolved
hydra-plutus/src/Hydra/Contract/Head.hs Outdated Show resolved Hide resolved
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.

As @v0d1ch mentioned, the on-chain code can and should be simplified before we merge this.

CHANGELOG.md Outdated Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/Contract/Close.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/Contract/Close.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/Contract/Close.hs Outdated Show resolved Hide resolved
hydra-plutus/src/Hydra/Contract/Head.hs Outdated Show resolved Hide resolved
hydra-plutus/src/Hydra/Contract/Head.hs Outdated Show resolved Hide resolved
@v0d1ch v0d1ch force-pushed the gap/move_attributes_from_redeemer_to_datum_for_close_and_contest_tx branch from 9f1d99a to a3174f0 Compare January 20, 2023 10:24
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.

Code duplication when fetching datum from the tx in close and contest. See proposed code change to remove duplication.

Side note: I'm having trouble with naming this function. Maybe you can find a better or, maybe, this is a smell that something's odd here and could be improved but I can't find it.

hydra-plutus/src/Hydra/Contract/Head.hs Show resolved Hide resolved
hydra-plutus/src/Hydra/Contract/Head.hs Outdated Show resolved Hide resolved
hydra-plutus/src/Hydra/Contract/Head.hs Outdated Show resolved Hide resolved
@pgrange
Copy link
Contributor

pgrange commented Jan 23, 2023

Some tests are failing since what looks like the first commits of the branch.

This build fails since this commit (which has been overwritten since then by some rebase I guess).

This build make me believe this commit introduced the issue.

I'm moving this P.R. back to in progress to investigate and fix.

@ch1bo ch1bo force-pushed the gap/move_attributes_from_redeemer_to_datum_for_close_and_contest_tx branch from 92ba27c to ef28398 Compare January 24, 2023 08:50
@ch1bo ch1bo assigned pgrange and ffakenz and unassigned v0d1ch and ch1bo Jan 24, 2023
@pgrange pgrange force-pushed the gap/move_attributes_from_redeemer_to_datum_for_close_and_contest_tx branch from 5910e4a to 2f235fc Compare January 24, 2023 15:25
@ch1bo ch1bo self-assigned this Jan 25, 2023
@pgrange pgrange force-pushed the gap/move_attributes_from_redeemer_to_datum_for_close_and_contest_tx branch 2 times, most recently from ff00dec to 3f9add4 Compare January 25, 2023 15:41
@ch1bo ch1bo force-pushed the gap/move_attributes_from_redeemer_to_datum_for_close_and_contest_tx branch from fb80c1e to ed4b111 Compare January 26, 2023 09:08
@pgrange pgrange force-pushed the gap/move_attributes_from_redeemer_to_datum_for_close_and_contest_tx branch 2 times, most recently from 7eae215 to ef08385 Compare January 30, 2023 10:31
Extract mutateClosedContestationDeadline to use in both close and
closeInitial mutations
We introduce replaceSomething to ease the mutation of a head state.
This functions are exposed by Mutation.hs as it seems to be the better
place to me right now but maybe we can do better?

Also, we replace some functions which did that and failed when the state
was not an expected one. Now we just return the state unchanged if the
field we want to change does not exist for this state.

It may be a good idea... it also may introduce difficulties in
diagnostic test errors. That is not clear at this point. If ist becomes
a problem, we can sill introduce the errors back easily in the code.
@pgrange pgrange force-pushed the gap/move_attributes_from_redeemer_to_datum_for_close_and_contest_tx branch from fcea1e7 to d511963 Compare January 31, 2023 17:40
@pgrange pgrange mentioned this pull request Feb 1, 2023
2 tasks
@ghost ghost marked this pull request as draft February 1, 2023 11:13
@ch1bo ch1bo marked this pull request as ready for review February 1, 2023 11:15
@pgrange pgrange marked this pull request as draft February 1, 2023 11:16
@pgrange pgrange force-pushed the gap/move_attributes_from_redeemer_to_datum_for_close_and_contest_tx branch from ae00b16 to 8dd1029 Compare February 1, 2023 14:13
pgrange and others added 13 commits February 1, 2023 15:12
The removed test used to check that the validator would fail for a snapshot
with a negative value.

But due to recent changes, it is now OK to have a negative snapshot
number. Although it should fail if we try to use a negative snaphot
number with a non-intiail snapshot.

So we amend the previous test we checked for 0 value with a generated
negative or 0 value.
The 0 case can be captured by ensuring the deadline generator covers
this as well.
There is a problem with the way we test validity bounds mutations. Most
of the arbitrarily generated values will make the validator fail but
there are no reason to not generate valid transaction with what we had.

To fix that, we distinguish between 3 situations:

1. The lower bound is infinite
2. The upper bound is infinite
3. The upper bound is too high given the contestation deadline
No need to call this "healthy" as it cannot be used outside of the healthyCloseInitialTx context.
One could have tried to change the head parameters during a close,
like change the headId or the parties.
@pgrange pgrange force-pushed the gap/move_attributes_from_redeemer_to_datum_for_close_and_contest_tx branch from 51d1c66 to d8250d6 Compare February 1, 2023 15:18
@pgrange pgrange marked this pull request as ready for review February 1, 2023 15:21
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.

Only a very minor thing left and the tests seem to not agree on the maxSupported assets as it fails with 45 utxos on the fanout.

Feel free to merge after fixing this so I approve.

@ch1bo ch1bo mentioned this pull request Feb 1, 2023
2 tasks
@pgrange pgrange merged commit 2c04ccf into master Feb 1, 2023
@pgrange pgrange deleted the gap/move_attributes_from_redeemer_to_datum_for_close_and_contest_tx branch February 1, 2023 16:38
@ch1bo ch1bo unassigned pgrange and ch1bo Feb 2, 2023
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.

5 participants