Improve translation errors #2848
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…LogicErrorInput
The 'BasicFailure' was initially introduced as an extra sanity check to make
sure that inputs referenced in the transaction were also present in the
provided UTxO.
Now however, this is done regardless in the `txInfoIn` function which is used
to construct the 'PV1.TxInInfo' for both Alonzo & Babbage transactions:
```hs
case Map.lookup txin mp of
Nothing -> Left TranslationLogicErrorInput
Just txout -> case transTxOutAddr txout of
```
Thus, the `TranslationLogicErrorInput` branch from `TranslationError` makes
the `UnknownInputs` branch redundant, and consequently, `BasicFailure`
redundant completely. To be 100% equivalent, `TranslationLogicErrorInput`
should be modified to also contain the missing inputs. I didn't do to that
extend here because `TranslationError` seems to be use as a kind of _enum_,
with only 0-ary constructors.
…xOutDatum. Avoid the impossible state in TxInfo where both DataHash and inline datum are supplied in TxOut
JaredCorduan
reviewed
Jun 7, 2022
This is necessary because `basicValidation` has been removed in #2825
Previously it was simply checking if the txin was not present in an empty UTxO. Which was more of a test for `txInfo`, rather than `evaluateTransactionExecutionUnits`
lehins
force-pushed
the
lehins/improve-translation-errors
branch
from
a07dbea
to
49d71b0
Compare
Jimbo4350
reviewed
Jun 7, 2022
| @@ -150,25 +122,19 @@ evaluateTransactionExecutionUnits :: | |||
| SystemStart -> | |||
| -- | The array of cost models, indexed by the supported languages. | |||
| Array Language CostModel -> | |||
| -- | If the transaction meets basic validation, we return a map from | |||
| dec n = Invalid n | ||
|
|
||
| data TranslationError crypto | ||
| = ByronTxOutInContext !(TxOutSource crypto) |
JaredCorduan
reviewed
Jun 7, 2022
|
|
||
| -- | Datum can be described by a either a data hash or binary data, but not | ||
| -- both. It can also be neither one of them. | ||
| data Datum era |
JaredCorduan
approved these changes
Jun 7, 2022
There was a problem hiding this comment.
Thank you @lehins for improving these errors, this is extremely helpful. The changes all look good to me (though it was a bit tricky makings sure we didn't change the Alonzo semantics). We review the subtle parts of this PR in a call, which I went back and confirmed to myself.
lehins
force-pushed
the
lehins/improve-translation-errors
branch
from
c53b085
to
451cb52
Compare
JaredCorduan
reviewed
Jun 8, 2022
| failBecause (CollectErrors alonzoFailures) | ||
| | otherwise -> pure () | ||
| where | ||
| -- BadTranslation was introduced in Babbage, thus we need to filter those failures out. |
lehins
force-pushed
the
lehins/improve-translation-errors
branch
from
451cb52
to
9276e2d
Compare
|
@lehins the "failed" CI job is yet another instance of the tests actually passing after a re-run, but not reporting properly. With your 👍 I'd like to merge this great PR :) |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While looking at #2825 and #2836 I realized that TranslationError was a new type in Babbage and thus we can in fact improve it before the release, so the node can use it to produce better errors to the users. I started at the point of the #2825 PR, since it seemed like a sensible cleanup.
While working on improving the type I found a few problems:
TxOutSource(previouslyOutputSource). This will be enough to point at the offender.TranslationLogicErrorDoubleDatumwas an error that could never happen in practice, but it was impossible to avoid without some changes to the way inline datums are being extracted from TxOut in a general way. Thus is the addition ofgetTxOutDatumand removal of less powerfulHasField "datum"instances.Two more problems found, that
haven'thave been resolvedyet:basicValidation, which is redundant for Babbage, however for Alonzo txInfo check does not verify presence of inputs in the utxo, it simply filters them out. Will now produceTranslationLogicMissingInput"attempt calculate ExUnits with invalid tx"is totally wrong.