-
Notifications
You must be signed in to change notification settings - Fork 87
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
Inline snapshot emission #729
Conversation
238677e
to
dbcd50e
Compare
Test Results276 tests - 11 270 ✔️ - 11 13m 35s ⏱️ - 1m 26s Results for commit f6dc69e. ± Comparison against base commit 056da80. This pull request removes 11 tests.
♻️ This comment has been updated with latest results. |
Transactions CostsSizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using
Script summary
Cost of Init Transaction
Cost of Commit TransactionCurrently only one UTxO per commit allowed (this is about to change soon)
Cost of CollectCom Transaction
Cost of Close Transaction
Cost of Contest Transaction
Cost of Abort TransactionSome variation because of random mixture of still initial and already committed outputs.
Cost of FanOut TransactionInvolves spending head output and burning head tokens. Uses ada-only UTxO for better comparability.
|
54e2efc
to
161683c
Compare
There was a problem hiding this 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?
-- 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 |
There was a problem hiding this comment.
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 ⊥)
There was a problem hiding this comment.
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.
if | ||
| not (isLeader parameters party nextSn) -> | ||
ShouldNotSnapshot $ NotLeader nextSn | ||
| -- REVIEW: This is slightly different than in the spec. Also, if we use |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
a895946
to
54004cc
Compare
161683c
to
7fa7d89
Compare
54004cc
to
6d4024f
Compare
7fa7d89
to
925f8c6
Compare
6d4024f
to
237fd61
Compare
925f8c6
to
9a261d4
Compare
9a261d4
to
07930df
Compare
We apply it only after processing ReqTx and AckSn network messages like defined in the spec.
No semantic change, but TODOs indicate where it's not aligned with the Spec
The semantics is still unchanged, but added TODO to point out differences.
07930df
to
f6dc69e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
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. WithCurrently working on this and it's looking good (#733)/= NoSeenSnapshot
was working so I kept it as aREVIEW
item.CHANGELOG updatedDocumentation updated