Skip to content
This repository has been archived by the owner on Feb 9, 2021. It is now read-only.

Add ToCBOR and FromCBOR instances for UTxOValidationError #598

Merged
merged 1 commit into from
Aug 2, 2019

Conversation

mrBliss
Copy link
Contributor

@mrBliss mrBliss commented Aug 2, 2019

These will be used by ouroboros-network to transmit validation errors over the local TxSubmission protocol.

These will be used by ouroboros-network to transmit validation errors over the
local TxSubmission protocol.
@mrBliss mrBliss requested review from ruhatch and intricate August 2, 2019 07:44
@mrBliss mrBliss requested a review from dnadales as a code owner August 2, 2019 07:44
@mrBliss
Copy link
Contributor Author

mrBliss commented Aug 2, 2019

See input-output-hk/cardano-byron-proxy@c0288c0#diff-699e579d3647145c4dd3a5c36aa8dfe7R125

This still needs some roundtrip property tests.

mrBliss referenced this pull request in IntersectMBO/ouroboros-network Aug 2, 2019
In #864 (8d8d45a) I changed the
`LocalTxSubmission` protocol failure from `String` to `ApplyTxErr blk`, but
without providing a de/encoder for `ApplyTxErr blk`, which breaks
`cardano-node` and `cardano-byron-proxy`. This commit rectifies this by adding
a de/encoder to `RunDemo`.

TODO: wait until IntersectMBO/cardano-ledger#598 is
merged.
@dcoutts
Copy link
Contributor

dcoutts commented Aug 2, 2019

It's not completely clear to me that we should include the fully structured error into the codec, or if we do we will need to do it in a way that is compatible and doesn't cause clients to break when the exact details of the validation error types change.

@KtorZ What is your view on how we report back the validation errors for submitting transactions?

Perhaps we should take a dual approach and return both a rendered string, and a structured term, use the CBOR-in-CBOR encoding trick to wrap the structured term in such a way that it's easy to ignore if the encoding is not understood or changes.

Thoughts?

@KtorZ
Copy link

KtorZ commented Aug 2, 2019

@dcoutts From the perspective of a client, a validation error would be seen as a fatal error for which you'd probably want the client to fail hard... We aren't supposed to submit malformed transactions, or invalid signatures. If that happens, that's the sign of a deeper problem in the wallet software and, I am not sure there's any "nice" way to handle validation errors. So, in a way, a string would probably be sufficient for that purpose.

Having said that, we had the case with the http-bridge which would simply report validation errors as HTTP 500 with an error message, forcing us to "grep" on the error message in order to identify the kind of error we were facing and provide a better error message and hints depending on the failure type.

Hence, a structured error type would allow for a nicer handling on the client's side, especially if there's some tag or error code to pattern-match on.

@dcoutts
Copy link
Contributor

dcoutts commented Aug 2, 2019

I was about to say that you would never expect to submit a wrong tx, but in fact it's inevitable due to concurrency that you can have txs where, by the time you submit it, some input has already been spent. This can happen if people are using the same account from multiple wallet clients.

But yes there's also the debugging aspect, both for you and also for 3rd party wallets who are still debugging their code and hitting limits like tx size, getting fees wrong etc, it's useful to have the detailed info on what they did wrong.

Ok, so how about we aim for a CBOR-in-CBOR version of the structured error, plus a text rendering. The latter is useful for 3rd party wallets debugging their stuff, and the former is useful once you have programtically recognise certain expected failures.

@KtorZ
Copy link

KtorZ commented Aug 2, 2019

@dcoutts said: but in fact it's inevitable due to concurrency that you can have txs

Hmmm. There's a difference between malformed transactions and, forbidden ones. From what I see in this in this PR, the validation errors are really about the transaction structure and "validity" (vs legitimacy). Unless I misinterpret constructors like TxValidationInvalidWitness (do we consider a transaction with valid signatures but, on already spent outputs to have InvalidWitness ?)

I think it'd be worth making the separation between, what a client can reasonably expects to get right (i.e. the format, fees, number of witnesses etc ...) and the things that could go wrong because of the very nature of the protocol (i.e. attempt to double-spend...).

But yes there's also the debugging aspect, both for you and also for 3rd party wallets who are still debugging their code and hitting limits like tx size, getting fees wrong etc, it's useful to have the detailed info on what they did wrong.

Indeed. That's probably not relevant for systems running in production but very helpful during development to understand what's wrong.

@ruhatch
Copy link
Contributor

ruhatch commented Aug 2, 2019

@dcoutts @KtorZ should we hold off on merging this or is this good enough for now? We could create a ticket to revisit it?

Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be needed anyway for the solution I'm proposing.

Lets make a ticket for the encoding change I'm suggesting, and adding the string representation.

Do we have any human readable rendering for UTxOValidationUTxOError, or is it just Show for now? We should probably make a low priority ticket for that.

@mrBliss
Copy link
Contributor Author

mrBliss commented Aug 2, 2019

bors r+

iohk-bors bot added a commit that referenced this pull request Aug 2, 2019
598: Add ToCBOR and FromCBOR instances for UTxOValidationError r=mrBliss a=mrBliss

These will be used by ouroboros-network to transmit validation errors over the local TxSubmission protocol.

Co-authored-by: Thomas Winant <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 2, 2019

@iohk-bors iohk-bors bot merged commit e68b38c into master Aug 2, 2019
@iohk-bors iohk-bors bot deleted the mrBliss/cbor-utxovalidationerror branch August 2, 2019 11:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants