-
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
ST in output not checked #658
Conversation
a0fbacc
to
76949e0
Compare
Transactions CostsSizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using
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 Results268 tests - 2 262 ✔️ - 2 12m 21s ⏱️ - 3m 26s Results for commit e780c01. ± Comparison against base commit 5ed00df. This pull request removes 4 and adds 2 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
c2f5832
to
f650d86
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 think we are missing the datum check in the commit validator. Other than that, some minor stuff in the mutations and I don't see where this is now enabling 8 parties to collect/abort a head?
ef6add8
to
cb38604
Compare
ada2638
to
8957d10
Compare
2c519f8
to
fa01315
Compare
62023b6
to
6ff7ab4
Compare
We use the healthyXXX prefix for our starting values to identify them as good values to mutate "away" 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.
We are still missing a lot of error traces in the scripts.
This is required before I can approve.
I already addressed some of the inconsistencies and oversights. In general, the PR really shows that it's open too long already. Messy.
Also refactor couple functions to move them to hydra-plutus. This change is larger than it should be.
Hydra.Chain.Direct.Tx was not directly including the required orphan instance, but Hydra.Ledger.Cardano is.
@@ -194,7 +190,7 @@ commitTx scriptRegistry networkId party utxo (initialInput, out, vkh) = | |||
initialScriptRef = | |||
fst (initialReference scriptRegistry) | |||
initialDatum = | |||
mkScriptDatum $ Initial.datum () | |||
mkScriptDatum $ Initial.datum (headIdToCurrencySymbol headId) |
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.
👍
mustBurnAllHeadTokens = | ||
traceIfFalse "number of inputs do not match number of parties" $ |
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.
:(
everyoneHasCommitted = | ||
traceIfFalse "not everyone committed" $ |
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.
:(
@@ -372,10 +350,10 @@ checkHeadOutputDatum ctx d = | |||
NoOutputDatum -> | |||
traceError "missing datum" | |||
OutputDatumHash actualHash -> | |||
traceIfFalse "output datum hash mismatch" $ |
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 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.
Re-added some last traces in da1cdd9.. now it's good to go :)
Nice but now our tx sizes grew bigger :) Shall I increase the limit or revert? |
We reduced the tx traces and hitting the button now! |
fix #596
Why
We want to make sure that the ST token is present in the tx output of the head validator and that it has correct
CurrencySymbol
that uniquely identifies a Hydra Head instance. We can't tell if some arbitrary NFT with the validTokenName
is indeed our ST token we expect so to tackle this security hole we are enforcing the check for the actualCurrencySymbol
we expect.What
CurrencySymbol
to the datum and check the tx output for its existence in the head validators check forcollectCom
,close
andcontest
.ViaAbort
caseNote: This PR removes some of the tests that did exist previously. TxSpec ones are outdated, highy specialized and we have a coverage for the same properties already. It also removes the mutation tests tied to
MutateHeadScriptInput
since we are not using the head script output to check the datum contents.To check before merging: