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

Improve diagnostics from SerializationErrors #758

Closed
Tracked by #2310
yaahc opened this issue Jul 24, 2020 · 1 comment
Closed
Tracked by #2310

Improve diagnostics from SerializationErrors #758

yaahc opened this issue Jul 24, 2020 · 1 comment
Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup E-help-wanted Call for participation: Help is requested to fix this issue. good first issue

Comments

@yaahc
Copy link
Contributor

yaahc commented Jul 24, 2020

Right now our serialization error uses some fairly generic error variants that don't include enough context. For example, recently I ran into a bug in tests for zebra-state where the error was particularly unhelpful


running 1 test
The application panicked (crashed).
Message:  called `Result::unwrap()` on an `Err` value: Io(Custom { kind: UnexpectedEof, error: "failed to fill whole buffer" })
Location: /home/jlusby/git/zfnd/zebra/zebra-test/src/transcript.rs:41

So I tweaked the SerializationError type to use a more useful error message and include extra context about what it was trying to deserialize:

running 1 test
The application panicked (crashed).
Message:  called `Result::unwrap()` on an `Err` value: 
   0: could not serialize zebra_chain::block::hash::BlockHeaderHash
   1: failed to fill whole buffer

By making the following changes:

#[derive(Error, Debug)]
pub enum SerializationError {
    /// An underlying IO error.
    #[error("could not deserialize {1}")]
    Io(#[source] io::Error, &'static str),
    /// The data to be deserialized was malformed.
    // XXX refine errors
    #[error("parse error: {0}")]
    Parse(&'static str),
    /// An error caused when validating a zatoshi `Amount`
    #[error("input couldn't be parsed as a zatoshi `Amount`")]
    Amount {
        /// The source error indicating how the num failed to validate
        #[from]
        source: crate::types::amount::Error,
    },
}

followed by a ton of this throughout the code

                .map_err(|source| SerializationError::Io(source, std::any::type_name::<Self>()))?

However, these changes were very invasive and I ran into some issues with impl Decoder in zebra-network where we're using SerializationError as our error kind and fixing it appropriately was more of a yak shave than I wanted to get into.

Long term we should probably refine these error types. We should probably rename SerializationError to DeserializationError as it is only the error return type for ZcashDeserialize. We should look into giving more context for why a serialization error was thrown, the solution that adds a static str to put the type name in has the advantage of requiring that the context is provided via compiler errors, but its not ideal ergonomically, if we can find a way to require instrumention on all of our deserialize impls it might be better to use SpanTrace to capture the context about what is being deserialized.

For codec we should introduce our own error kind, us implicitly constructing a DeserializeError::Io from errors originating in the tokio codec upstream code misrepresents the meaning of the Io error, which likely has nothing to do with deserialization.

@yaahc yaahc added C-cleanup Category: This is a cleanup E-easy E-help-wanted Call for participation: Help is requested to fix this issue. good first issue A-rust Area: Updates to Rust code labels Jul 31, 2020
@teor2345 teor2345 added this to the Block Validation milestone Sep 29, 2020
@mpguerra mpguerra removed this from the Block Validation milestone Jan 5, 2021
@mpguerra mpguerra removed the E-easy label Mar 23, 2021
@conradoplg
Copy link
Collaborator

Closing since serialization errors aren't very common in node usage. This would be more important for the crate itself, but we can reopen this if it becomes relevant.

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 E-help-wanted Call for participation: Help is requested to fix this issue. good first issue
Projects
None yet
Development

No branches or pull requests

4 participants