-
Notifications
You must be signed in to change notification settings - Fork 84
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
Prevents node from starting given persisted network state is inconsistent with configuration #1179
Prevents node from starting given persisted network state is inconsistent with configuration #1179
Conversation
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 TransactionThis is using ada-only outputs for better comparability.
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.
End-To-End Benchmark ResultsThis page is intended to collect the latest end-to-end benchmarks results produced by Hydra's Continuous Integration system from the latest Please take those results with a grain of salt as they are currently produced from very limited cloud VMs and not controlled hardware. Instead of focusing on the absolute results, the emphasis should be on relative results, eg. how the timings for a scenario evolve as the code changes. Generated at 2023-11-27 14:44:10.912697101 UTC 3-nodes ScenarioA rather typical setup, with 3 nodes forming a Hydra head.
Baseline ScenarioThis scenario represents a minimal case and as such is a good baseline against which to assess the overhead introduced by more complex setups. There is a single hydra-node d with a single client submitting single input and single output transactions with a constant UTxO set of 1.
|
bc99ad9
to
9e4fd02
Compare
1644732
to
9aae4a5
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.
Surprisingly large change. I think we can (and should) reduce the number of mentions of acks
and other internal details of the network layer.
One thing I am missing is any concrete instructions to the user. How would they know that they need to delete the <persistence-dir>/acks
file? To solve this, I would still die
with an exception, but implement https://hackage.haskell.org/package/base-4.19.0.0/docs/Control-Exception.html#v:displayException of the thrown exception accordingly to give a human-friendly english instruction to delete acksFile
You're right there's some instructions missing and we should provide better error messages to the user should we crash with an exception. However:
Doing so here would increase even more the surface of this PR so I would like to propose we address this particular issue in a later PR. |
This is needed in order to see in our test the node has died with a misconfiguration, which is not the problem we are trying to solve but which is an important information to have.
Also renaming the spec to better match the scenarion.
This is not strictly needed but allows for fine tuning the time we wait for connection depending on the exact context of the check, and not blindly on the number of nodes.
…sInconsistent occur
… specific function This helps in finding relevant tests and eases future renaming of this function. Co-authored-by: Sebastian Nagel <ch1bo@users.noreply.github.com>
42d4d4c
to
d7095be
Compare
…n saving acks Instead, we can use the interface provided for 'the system under test', here: configureMessagePersistence.
I'm okay in not doing that in this PR.. it's just something which I thought would be nice small step in less confusing error messages. I see your point with different encodings and internationalization (although YAGNI?), but would want to avoid arbitrary centralization of otherwise unreleated exceptions. The case we have here is a good example, I would want to retain information that some file at path "/acks" should be deleted to the component which decides that we do persist into a file at that path. Related to this is the IMO unnecessary addition of That being said, I can approve and might just PR what I think should be myself :) |
Agreed, this is YAGNI and I just raised this point to highlight the current need to avoid putting arbitrary strings into places where they do not belong: Defining and using the (structured) error is one thing, presenting this issue in an understandable way is a different concern.
I agree, this is currently missing from the exception and should be added.
That's where our views differ: This issue comes from a discrepancy between the persisted state and the configuration passed to the node which percolates down to the network layer, hence I think it's legitimate to make it part of those exceptions related to configuration "errors". We could refine the hierarchy more perhaps, but at this stage it's definitely YAGNI :) To be clear, what I have in mind is something like that in the
|
To fix #1174 we choose to
die
when the loaded persisted state for the network acknowledgments is not consistent with the configured parties, eg. when they don't have the same size. This is a bit lame as we don't really check much and it could very well be the case the user restarts a node with a different configuration (eg. different parties) but same number and got tripped by this issue.