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.

Sign up for GitHub

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

Jump to bottom

Inline snapshot emission #729

Merged
merged 3 commits into from
Feb 22, 2023
Merged

Inline snapshot emission #729

merged 3 commits into from
Feb 22, 2023

Conversation

ch1bo
Copy link
Collaborator

@ch1bo ch1bo commented Feb 17, 2023 •
edited
Loading

This is slightly more inline with the spec and was identified a (minor) gap in #452.

It seems the definition from the spec is in principle okay, as all tests pass if we do emitSnapshot only in the cases which are also outlined in the spec.

However, I was unable to use snapshot numbers (like in the spec) without a model test not passing. It lacks observability and I didn't feel like tackling this now. With /= NoSeenSnapshot was working so I kept it as a REVIEW item. Currently working on this and it's looking good (#733)

  • CHANGELOG updated
  • Documentation updated
  • Added and/or updated haddocks
  • No new TODOs introduced or explained herafter

@ch1bo ch1bo mentioned this pull request Feb 17, 2023
Closed
20 tasks
@github-actions
Copy link

github-actions bot commented Feb 17, 2023 •
edited
Loading

Test Results

276 tests   - 11   270 ✔️  - 11   13m 35s ⏱️ - 1m 26s
  94 suites  -   4       6 💤 ±  0 
    4 files    -   1       0 ❌ ±  0 

Results for commit f6dc69e. ± Comparison against base commit 056da80.

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.

@github-actions
Copy link

github-actions bot commented Feb 17, 2023 •
edited
Loading

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-22 08:57:30.744253234 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 5052 10.81 4.28 0.49
2 5253 11.31 4.43 0.50
3 5458 13.67 5.36 0.54
5 5869 19.50 7.65 0.62
10 6894 28.10 10.93 0.76
45 14069 99.68 38.58 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 630 21.04 8.49 0.41

Cost of CollectCom Transaction

Parties UTxO (bytes) Tx size % max Mem % max CPU Min fee ₳
1 57 13137 22.07 8.83 0.97
2 114 13490 36.85 14.89 1.15
3 170 13850 53.72 21.88 1.35
4 228 14205 74.96 30.68 1.60
5 284 14547 97.87 40.25 1.87

Cost of Close Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 10568 16.72 7.33 0.80
2 10695 17.67 7.76 0.82
3 10698 17.40 7.61 0.82
5 11195 21.89 10.08 0.89
10 12053 29.69 14.43 1.02
49 15328 65.00 30.18 1.57

Cost of Contest Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 10572 21.01 8.79 0.85
2 10736 22.81 9.71 0.88
3 10934 25.32 11.07 0.92
5 11239 28.43 12.54 0.96
10 12117 38.12 17.81 1.12
35 16283 82.11 40.66 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 15089 28.55 12.07 1.13
2 15228 42.02 17.76 1.29
3 15581 63.29 27.06 1.54
4 15449 75.24 31.94 1.67

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 15008 10.39 4.23 0.93
5 1 57 15049 11.91 5.11 0.95
5 5 285 15188 17.97 8.62 1.03
5 10 571 15366 25.64 13.04 1.13
5 20 1140 15733 40.90 21.85 1.33
5 30 1708 16088 56.17 30.66 1.53
5 38 2163 16375 68.93 37.94 1.70

@ch1bo ch1bo self-assigned this Feb 20, 2023
@ch1bo ch1bo changed the base branch from master to ch1bo/head-logic-refactor February 20, 2023 19:17
@ch1bo ch1bo marked this pull request as ready for review February 20, 2023 19:19
@ch1bo ch1bo mentioned this pull request Feb 20, 2023
Merged
2 tasks
v0d1ch
v0d1ch requested changes Feb 21, 2023
View reviewed changes
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.

My main concern is the review comment and if this is safe to merge as is?

hydra-node/src/Hydra/HeadLogic.hs
-- TODO: Verify the request is signed by (?) / comes from the leader
-- (Can we prove a message comes from a given peer, without signature?)

-- Spec: wait U̅ ◦ T ̸= ⊥ combined with Û ← Ū̅ ◦ T
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add some spec glossary somewhere? I don't know what these symbols mean (a part from ⊥)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd say the spec is the glossary (section 6.3 right now). If these comments are not helpful I can remove them.

hydra-node/src/Hydra/HeadLogic.hs
if
| not (isLeader parameters party nextSn) ->
ShouldNotSnapshot $ NotLeader nextSn
| -- REVIEW: This is slightly different than in the spec. Also, if we use
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a session to figure this out before merging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I already did (will be in a follow-up PR)

@ch1bo ch1bo force-pushed the ch1bo/head-logic-refactor branch 4 times, most recently from a895946 to 54004cc Compare February 21, 2023 19:46
Base automatically changed from ch1bo/head-logic-refactor to master February 22, 2023 08:42
@ch1bo
Not call emitSnapshot from Hydra.Node but inline
c9baa78 
We apply it only after processing ReqTx and AckSn network
messages like defined in the spec.
@ch1bo
Refactor onOpenNetworkAckSn further
918ded0 
No semantic change, but TODOs indicate where it's not aligned with the Spec
@ch1bo
Refactor onOpenNetworkReqSn to use more spec-like structure
f6dc69e 
The semantics is still unchanged, but added TODO to point out differences.
v0d1ch
v0d1ch approved these changes Feb 22, 2023
View reviewed changes
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.

done!

@ch1bo ch1bo merged commit b1a82cd into master Feb 22, 2023
@ch1bo ch1bo deleted the ch1bo/inline-snapshotting branch February 22, 2023 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@v0d1ch v0d1ch v0d1ch approved these changes

Assignees

@ch1bo ch1bo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ch1bo @v0d1ch