-
Notifications
You must be signed in to change notification settings - Fork 88
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
Align HeadLogic: Not validate tx on NewTx #745
Conversation
a5581bd
to
b73e874
Compare
2630eea
to
93cb1af
Compare
b73e874
to
c7b9bd0
Compare
93cb1af
to
681179a
Compare
c7b9bd0
to
711c98d
Compare
681179a
to
08dc5f6
Compare
08dc5f6
to
1e3e9e3
Compare
711c98d
to
a36a36c
Compare
1e3e9e3
to
62f4885
Compare
62f4885
to
39a9858
Compare
This is a somewhat reasonable timeout (12 seconds on my machine) and the test is not appearing to hang forever. It's not perfect still, as we are presented with a slew of Tick-related via the traceInIOSim and shouldRunInSim.
This should capture the behavior as I read it from the spec: Somewhat robust behavior against out-of-order requesting of NewTx (up to some delay).
This is now more important as we only keep transactions pending for waitDealy * defaultTTL. That is, currently 0.5 seconds.
This means all transactions result in a ReqTx and allows out-of-order ingestion of transactions into the Hydra Head. This also adds TODOs/FIXMEs for now redundant TxInvalid et al
The server outputs TxValid and TxInvalid shall be used instead. Rationale: Before we were checking against the confirmed ledger on NewTx and yield a TxValid if ok, but that is a useless check as it needs to be applicable to the seen ledger to continue in the protocol. Instead we now only yield TxValid if it was applicable against the seen ledger. TxExpired was not giving any error information why it was expired. Similarly, TxInvalid had no real value as it was in response of checking against the confirmed ledger, while the seen ledger might already have advanced and the transaction is actually not applicable. Instead we now return a TxInvalid in this case with the last validationError that we saw.
39a9858
to
cf45fb7
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 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.
|
Test Results292 tests +12 286 ✔️ +12 19m 36s ⏱️ + 3m 49s Results for commit cf45fb7. ± Comparison against base commit 427c97c. This pull request removes 10 and adds 22 tests. Note that renamed tests count towards both.
This pull request removes 1 skipped test and adds 1 skipped test. Note that renamed tests count towards both.
|
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.
LGTM!
When updating the spec we realized we do not need an applicability to the confirmed (last snapshot) UTxO set on
NewTx
. This is reason enough to re-think whether this makes even sense.This PR is now dropping
TxSeen
andTxExpired
in favor ofTxValid
andTxInvalid
. See commit description for rationale.Although it fixes a bug int he
ttl
handling, this PR does not change the behavior. Notably, thettl == 0
cases forReqSn
andAckSn
are not defined. They will just fall out of the queue. We might want to emit aSnapshotExpired
orHeadLikelyStuck
message.