-
Notifications
You must be signed in to change notification settings - Fork 20
Make costmdls a map as in the Alonzo spec #1035
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
base: master
Are you sure you want to change the base?
Conversation
| _ = Show-L×C | ||
| _ = Show-List |
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'm puzzled that adding these two instances here is necessary.
Moreover, I don't understand why Show-× isn't enough and I need to replace it with Show-L×C. The meta program fails to produce the Show instance of PParams 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.
Imitating how Show-ProtVer is defined, I implemented a shorter solution in a later commit. I guess my questions stand as concerns of an Agda user. Show-List and Show-× are not defined as instances in standard-library-classes:Class.Show.Instances.
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.
key issue/possible bug
Using lookupᵐ? and mapPartial in collectP2ScriptsWithContext might result in silently skipping required scripts.
We define collectP2ScriptsWithContext with a helper,
toScriptInput : ScriptPurpose → ScriptHash → Maybe (P2Script × List Data × ExUnits × CostModel)
which (in this PR) uses costModel ← lookupᵐ? (PParams.costmdls pp) (language p2s), and collectP2ScriptsWithContext builds a list via:
setToList
$ mapPartial (λ (sp , c) → if isScriptObj c
then (λ {sh} → toScriptInput sp sh)
else nothing)
$ credsNeeded utxo (tx .Tx.body)So, if a cost model is missing for some language, then lookupᵐ? returns nothing, which makes toScriptInput return nothing, which makes mapPartial drop that script from the list, which makes evalP2Scripts run all over a list that is missing a required script. I think this means the transaction could incorrectly pass because a script never ran. This seems like a real risk, unless we ensure that a missing cost model is impossible.
Suggested Fix
I suggest (for now) just adding the following to the list of premises of the UTXOW rule:
∙ languages tx utxo ⊆ dom (PParams.costmdls (PParamsOf Γ))and wherever collectP2ScriptsWithContext is called, treat a nothing in the list a Phase 1 failure.
| toScriptInput | ||
| : ScriptPurpose → ScriptHash | ||
| → Maybe (P2Script × List Data × ExUnits × CostModel) | ||
| toScriptInput sp sh = | ||
| do s ← lookupScriptHash sh tx utxo | ||
| p2s ← toP2Script s | ||
| (rdmr , exunits) ← indexedRdmrs tx sp | ||
| let data' = maybe [_] [] (getDatum tx utxo sp) ++ rdmr ∷ [ valContext (txInfo (language p2s) pp utxo tx) sp ] | ||
| costModel = PParams.costmdls pp | ||
| costModel ← lookupᵐ? (PParams.costmdls pp) (language p2s) | ||
| just (p2s , data' , exunits , costModel) |
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.
In the Alonzo spec, collectTwoPhaseScriptInputs explicitly includes the membership condition language script ↦ cm ∈ costmdls pp. So the input list is defined only when the cost model exists. (Missing cost models are a Phase 1 failure, before script evaluation.) This means "don't silently skip; fail earlier" is actually part of the Alonzo design.
I think the "right" (faithful to Alonzo) way would be to refactor collectP2ScriptsWithContext so that the helper function toScriptInput can use the total map lookup (i.e., let costModel = lookupᵐ (PParams.costmdls pp) (language p2s)), but this is a bit tricky because we would then need to somehow prove, or make it a hypothesis of collectP2ScriptsWithContext, that the language of every script is in the domain of the PParams.costmdls map.
Alternatively, probably much easier (but maybe not as safe), keep the current toScriptInput helper, but don't let a missing cost model just cause a dropped script and then continue as if nothing's wrong. Instead, make collectP2ScriptsWithContext return Maybe List (P2Script x List Data x ...)) and then, wherever collectP2ScriptsWithContext is used, treat a nothing as a Phase 1 failure.
|
It is true that in both the Agda spec and the Alonzo spec, scripts are dropped in the However, Alonzo also requires that all scripts have a cost model. This is done in the UTXOW rule, exactly with the suggested premise, in Figure 13. The Agda implementation did not include this premise yet. Thus the concern is entirely justified!
I don't know what this means. After adding the missing premise, are there other missing bits from Alonzo related to cost models? |
|
It is looking to me like the same trouble would come if |
7f344cf to
6c44502
Compare
|
@williamdemeo, perhaps this is ready for another review. I rebased master and added the suggested premise relating the domain of costmdls and the script languages. |
6c44502 to
0d6497e
Compare
0d6497e to
df6726a
Compare
|
I rebased master, and reverted a small modification to the Dijkstra spec now that we agreed to let Dijkstra alone when porting Alonzo features. |
Addresses part of #723