Skip to content
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

Dedupe Verification error types (Block, Chain) #1186

Closed
dconnolly opened this issue Oct 20, 2020 · 4 comments
Closed

Dedupe Verification error types (Block, Chain) #1186

dconnolly opened this issue Oct 20, 2020 · 4 comments
Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup C-design Category: Software design work

Comments

@dconnolly
Copy link
Contributor

dconnolly commented Oct 20, 2020

Hmm, it looks like the error type duplication is pre-existing to this PR: there's VerifyBlockError and VerifyTransactionError that both shouldn't exist. We could either fix it now, or in a later PR. The downside of fixing it later is that implementing other consensus rules requires having the right error types, so all later changes would still block on the later PR.

Comes out of #1173

  1. VerifyBlockError -> BlockError
  2. VerifyChainError -> (new) ChainError
  3. Distinguish timeouts by wrapping Elapsed in different errors, depending on where it comes from
  4. Fix the downcast from BoxError to redjubjub::Error at zebra-consensus/src/error.rs:76
    • Remove the InternalDowncastError type
    • Remove as many BoxErrors as possible
@dconnolly dconnolly added C-design Category: Software design work A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup labels Oct 20, 2020
@dconnolly dconnolly added this to the Transaction Validation milestone Oct 20, 2020
@dconnolly dconnolly self-assigned this Oct 20, 2020
@dconnolly dconnolly changed the title Dedupe Verification error types (Block and Transaction) Dedupe Verification error types (Block, Chain) Oct 29, 2020
@teor2345
Copy link
Contributor

Moved into the first alpha because it's causing a panic, see #1357 for details.

teor2345 added a commit to teor2345/zebra that referenced this issue Nov 24, 2020
Instead, format the original error as a string, to provide better
diagnostics.

Temporary fix for ZcashFoundation#1357, the permanent fix ticket is ZcashFoundation#1186.
teor2345 added a commit that referenced this issue Nov 24, 2020
Instead, format the original error as a string, to provide better
diagnostics.

Temporary fix for #1357, the permanent fix ticket is #1186.
@mpguerra mpguerra removed this from the First Alpha Release milestone Nov 30, 2020
@mpguerra mpguerra added this to the Transaction Validation milestone Dec 17, 2020
@mpguerra mpguerra removed this from the Transaction Validation milestone Jan 5, 2021
@conradoplg
Copy link
Collaborator

A related issue is discussed here. Basically the script verifier can timeout, which returns a use tower::timeout::error::Elapsed error. This is converted to a TransactionError (since that's the error type of the transaction verifier service), and since there's no matching enum item, it ends up being converted to InternalDowncastError.

I'm not sure what's the best approach for this.

  • Create a Elapsed error in TransactionError? (Would solve this specific case, but there may be others)
  • Try to remove the BoxError -> TransactionError conversion by using TransactionError instead of BoxError where applicable (e.g. AsyncChecks)?
  • Or just leave as is?

@teor2345
Copy link
Contributor

teor2345 commented Sep 2, 2021

  • Create a Elapsed error in TransactionError? (Would solve this specific case, but there may be others)

I think we might want a different error variant for each different kind of timeout, so we know what has timed out.

  • Try to remove the BoxError -> TransactionError conversion by using TransactionError instead of BoxError where applicable (e.g. AsyncChecks)?

I'd like to replace BoxError with enum error types as much as possible.

But we'll still need it in some places, because some types and traits require BoxError (like ServiceExt::oneshot, and maybe tower::Buffer?).

@teor2345
Copy link
Contributor

teor2345 commented Jun 2, 2022

This isn't particularly important.

@teor2345 teor2345 closed this as not planned Won't fix, can't repro, duplicate, stale Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup C-design Category: Software design work
Projects
None yet
Development

No branches or pull requests

4 participants