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

Revert this old work around #746

Merged
merged 2 commits into from
Feb 24, 2023
Merged

Revert this old work around #746

merged 2 commits into from
Feb 24, 2023

Conversation

pgrange
Copy link
Contributor

@pgrange pgrange commented Feb 23, 2023

FIX PermissionDenied issues on os x

When running the tests on os x with the Durable version we see the following errors:

  test/Hydra/API/ServerSpec.hs:61:3:
  1) Hydra.API.Server sends all sendOutput history to all connected clients after a restart
       uncaught exception: IOException of type PermissionDenied
       openFileFromDir: permission denied (Permission denied)

  To rerun use: --match "/Hydra.API.Server/sends all sendOutput history to all connected clients after a restart/"

  test/Hydra/PersistenceSpec.hs:43:5:
  2) Hydra.Persistence.PersistenceIncremental is consistent after multiple append calls in presence of new-lines
       uncaught exception: IOException of type PermissionDenied
       openFileFromDir: permission denied (Permission denied)
       (after 4 tests)

Which is probably due to the fact that we write in the tmp directory, which is a bit special,
and that the Durable version pretends to deal with special directories but, maybe, not so
much in this case.

According to the documentation, withBinaryFileDurable behaves the same way as withBinary on Windows. On other systems it provides the following guarantees:

  • It successfully closes the file in case of an asynchronous exception
  • It reliably saves the file in the correct directory; including edge case situations like a different device being mounted to the current directory, or the current directory being renamed to some other name while the file is being used.
  • It ensures durability by executing an fsync() call before closing the file handle

So on Windows system, this commit does not change a thing.

On other systems, in the context of appending history to the persistent storage of the node we are not impacted by removing the above Durable features:

  • We only append to the end of the file
  • We open/close the file everytime we append an history item to it

In this regard, the worst that could happen is some crash while appending one history item to the file which would result in this history item not written at all or partially written, in which case only the last line of the file would be corrupted.

This is a situation we were already in with the previous version of the code. Hence this P.R..

@pgrange pgrange force-pushed the revert_tmp_workaround branch from 3b5a175 to 5fc54c4 Compare February 23, 2023 12:57
@github-actions
Copy link

github-actions bot commented Feb 23, 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-23 15:41:32.327646805 UTC
Max. memory units 14000000
Max. CPU units 10000000000
Max. tx size (kB) 16384

Script summary

Name Hash Size (Bytes)
νInitial 7bb44e248f92ae5d2c4c744244b469d59a5bfa8a5d8ce9b6b27e5750 5530
νCommit 70c545fc894ada81ad46715bac1a11fa755b3e3a1d94f03254a4d397 2473
νHead c1fa50bb6fb1b7f84f4b103bcfb6f71eaeb4de93ddcab99729c122fb 9928

Cost of Init Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 5047 10.35 4.10 0.49
2 5253 13.92 5.51 0.53
3 5458 16.10 6.36 0.57
5 5873 18.33 7.17 0.61
10 6894 28.07 10.92 0.76
45 14069 97.65 37.74 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 21.20 8.57 0.41

Cost of CollectCom Transaction

Parties UTxO (bytes) Tx size % max Mem % max CPU Min fee ₳
1 56 13136 21.86 8.75 0.97
2 114 13491 36.59 14.79 1.15
3 170 13851 54.05 22.02 1.36
4 228 14194 75.07 30.73 1.60
5 284 14548 97.58 40.14 1.87

Cost of Close Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 10500 14.99 6.52 0.78
2 10700 17.71 7.77 0.82
3 10864 19.36 8.64 0.85
5 11228 22.73 10.58 0.90
10 12133 31.09 15.47 1.05
38 14245 53.62 24.84 1.39

Cost of Contest Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 10563 21.01 8.79 0.85
2 10731 22.73 9.67 0.88
3 10943 25.32 11.07 0.92
5 11299 29.41 13.27 0.98
10 12121 38.15 17.75 1.12
35 16300 81.87 40.65 1.82

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 14876 24.49 10.09 1.08
2 15441 47.02 20.12 1.35
3 15587 63.29 27.06 1.54
4 16078 90.32 38.99 1.87

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 15009 10.09 4.11 0.92
5 1 57 15045 12.16 5.22 0.95
5 5 285 15188 17.97 8.62 1.03
5 10 568 15366 25.59 13.02 1.13
5 20 1137 15725 40.61 21.73 1.33
5 30 1709 16089 55.87 30.54 1.53
5 38 2164 16377 68.33 37.70 1.69

@github-actions
Copy link

github-actions bot commented Feb 23, 2023

Test Results

276 tests   - 11   270 ✔️  - 11   14m 28s ⏱️ - 2m 26s
  94 suites  -   4       6 💤 ±  0 
    4 files    -   1       0 ±  0 

Results for commit 4f96981. ± Comparison against base commit c71d925.

This pull request removes 11 tests.
Hydra.TUI.Options ‑ parses --cardano-signing-key option
Hydra.TUI.Options ‑ parses --connect option
Hydra.TUI.Options ‑ parses --network-id option
Hydra.TUI.Options ‑ parses --node-socket option
Hydra.TUI/end-to-end smoke tests ‑ display feedback long enough
Hydra.TUI/end-to-end smoke tests ‑ doesn't allow multiple initializations
Hydra.TUI/end-to-end smoke tests ‑ starts & renders
Hydra.TUI/end-to-end smoke tests ‑ supports the full Head life cycle
Hydra.TUI/end-to-end smoke tests ‑ supports the init & abort Head life cycle
Hydra.TUI/text rendering errors ‑ should show not enough fuel message and suggestion
…

♻️ This comment has been updated with latest results.

It seems that this old work-around is not needed anymore to run on Darwin
systems.
When running the tests on os x with the Durable version we see the following errors:

```
  test/Hydra/API/ServerSpec.hs:61:3:
  1) Hydra.API.Server sends all sendOutput history to all connected clients after a restart
       uncaught exception: IOException of type PermissionDenied
       openFileFromDir: permission denied (Permission denied)

  To rerun use: --match "/Hydra.API.Server/sends all sendOutput history to all connected clients after a restart/"

  test/Hydra/PersistenceSpec.hs:43:5:
  2) Hydra.Persistence.PersistenceIncremental is consistent after multiple append calls in presence of new-lines
       uncaught exception: IOException of type PermissionDenied
       openFileFromDir: permission denied (Permission denied)
       (after 4 tests)
```

Which is probably due to the fact that we write in the tmp directory, which is a bit special,
and that the _Durable_ version pretends to deal with special directories but, maybe, not so
much in this case.

According to the documentation, `withBinaryFileDurable` behaves the same way as `withBinary` on Windows. On other systems it provides the following guarantees:

> * It successfully closes the file in case of an asynchronous exception
> * It reliably saves the file in the correct directory; including edge case situations like a different device being mounted to the current directory, or the current directory being renamed to some other name while the file is being used.
> * It ensures durability by executing an fsync() call before closing the file handle

So on Windows system, this commit does not change a thing.

On other systems, in the context of appending history to the persistent storage of the node we are not impacted by removing the above _Durable_ features:

* We only append to the end of the file
* We open/close the file everytime we append an history item to it

In this regard, the worst that could happen is some crash while appending one history item to the file which would result in this history item not written at all or partially written, in which case only the last line of the file would be corrupted.

This is a situation we were already in with the previous version of the code. Hence this commit.
@pgrange pgrange force-pushed the revert_tmp_workaround branch from 41756b1 to 4f96981 Compare February 23, 2023 13:59
@pgrange pgrange marked this pull request as ready for review February 23, 2023 16:34
@pgrange pgrange requested review from ch1bo and a user February 23, 2023 16:34
Copy link
Member

@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.

I'm happy that we don't need more changes than these to make it work on macos

@ch1bo ch1bo merged commit 59a5abf into master Feb 24, 2023
@ch1bo ch1bo deleted the revert_tmp_workaround branch February 24, 2023 08:44
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.

2 participants