-
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
Stateless observation hydra node #1196
Conversation
fb1969d
to
1bda91b
Compare
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-12-08 21:04:41.934947077 UTC Baseline Scenario
Baseline Scenario
|
b448dca
to
e2c4e99
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.
There are still some things to clean up. All in all, the change looks better than expected. I have left several suggestions, but plan to address them also myself.
There is also quite some refactoring potential and i will try to remove the Hydra.Chain.Direct.State
module completely from the "production code". That is, only use it to generate transactions in tx-cost
and the hydra-node
test suite.
hydra-node/src/Hydra/HeadLogic.hs
Outdated
notifyClient = ClientEffect $ ServerOutput.Committed headId pt utxo | ||
|
||
postCollectCom = | ||
OnChainEffect | ||
{ postChainTx = CollectComTx $ fold newCommitted | ||
{ postChainTx = CollectComTx{headId, headParameters = parameters} |
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.
Odd that we had constructed the tx with expected commits before, but now we seemingly infer them from the spent UTxO. But on the other hand parameters were added here instead of inferring them from the spendable UTxO.
Also, the AbortTx
does have the committed UTxO.. this is very inconsistent.
Must: be consistent in one or the other approach
@@ -189,12 +197,12 @@ finalizeTx :: | |||
MonadThrow m => | |||
TinyWallet m -> | |||
ChainContext -> | |||
ChainStateType Tx -> | |||
UTxO.UTxO -> | |||
UTxO.UTxO -> |
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: document which UTxO is which in haddock (it's an impossible to understand signature otherwise)
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.
Do we need to separate it even?
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 think we want to separate them, so to favor documentation... but you are correct; we can combine them all and have the function to take a single argument.
wdyt?
b9438f8
to
715013d
Compare
c5f3319
to
2399328
Compare
2316d04
to
d99092c
Compare
af7bbc4
to
232a5ad
Compare
251d97b
to
1fd440c
Compare
5f9faa1
to
4713778
Compare
8788952
to
b12a5a3
Compare
002957b
to
7299a5e
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.
I am happy with the changes now!
Ideally we would update the CHANGELOG and bump versions of packages accordingly.
Let's merge this!
Test Results377 tests +5 372 ✔️ +5 21m 2s ⏱️ -49s Results for commit ead4dab. ± Comparison against base commit 06feee9. This pull request removes 9 and adds 14 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
bf83efd
to
ce00931
Compare
This is mocking out the usage sites of the chain state on the transaction creation site.
As we removed those guards from the chain layer, it is important that the logic layer is doing this filtering.
We were missing to check the headId from the observation with ours within the local state.
…reetings message'
In order to avoid breaking changes in the Greetings output messages introduce a new projection. This projection only gives you a HeadId if the Head is in Initializing state.
This is needed to ignore OnInitTx because it was not using the right on-chain keys.
We were only using observeRawInitTx and all checking of whether "its the right head" needs to be in the logic layer now. This commit does not yet migrate the test for this (which was in TxSpec)
This was done by the chain layer before, but needs to be done now by the logic layer. This is the only usage of on-chain identities in the off-chain Head protocol, but it still is required to retain semantics from before. This made initializing the Environment a bit hacky, as it is very specific to the Cardano configuration.
Originally only needed for the observation side (OnInitTx), the BehaviorSpec actually nudges us into also specifying the 'participants' on the creation side (InitTx). This feels right, despite it requiring quite a lot of changes in our test suites, because we can remove the other cardano keys from the direct chain handle! At some point this will enable us to open a head on-demand with any other participants.
This avoids a breaking change on the API, but keeps additional information about parties and contestationPeriod. This is helpful as all three things need to match.
We cannot use "derived" on chain identifiers in the Model tests, because we are actually validating the transactions and need to sign them correctly. Hence, the participation tokens need to be match the verification keys used.
We must reset the headId when the head is open or aborted, but keep it on all other server outputs.
This was correct originally, but got re-added incorrectly a few commits back
Avoid some diff to master and fix comments
While the DirectChainSpec tests would not need it, the EndToEnd scenarios have a fulll hydra-node running where the chain context and wallet still do load these files.
This was the last step to make all e2e tests green (two heads on the same network turned out to be flaky).
6b5f8c4
to
ead4dab
Compare
TODO:
fromPlutusCurrencySymbol
partialGreetings
observe
functions with multiple Head UTxOHydra.Chain.Direct.State
module from production code (use it only to generate transactions and rename it)