Improve error handling in approval voting block import

[koute]

We already print out a warning here, so we might as well also print out the actual error message instead of just ignoring it.

(Motivation: I was investigating this issue which triggers this warning. I'd like to have the WASM stack trace printed out in the error message when something panics; we can't do that if the errors are ignored and never printed out the first place.)

[wigy-opensource-developer]

Nice.

Just noticed I was not asked to review.

[drahnr]

Canceled should be annotated with #[source]

[koute]

Ah, I missed that.

That said, in this case in practice since Canceled is a ZST and doesn't actually carry any information it shouldn't make any practical difference.

I'm happy to make a followup PR and add this in though.

[drahnr]

Just a nit :)

[drahnr]

nit: source will be printed twice now in the backtrace, once as part of the backtrace and as part of the error message itself iirc

[koute]

Do you know how you'd have to print it out to actually get it printed out twice?

Because when printed out through a normal Display trait:

    #[test]
    fn test_foobar() {
        #[derive(thiserror::Error, Debug)]
        #[error("source")]
        struct Source;
        let err = RuntimeApiError::Execution { runtime_api_name: "Test", source: Arc::new(Source) };
        panic!("{}", err);
    }

it's printed out like this:

thread 'import::tests::test_foobar' panicked at 'The runtime API 'Test' cannot be executed: source', node/core/approval-voting/src/import.rs:1351:9

And we usually print out errors through Display (or Debug).

[drahnr]

There are two ways to get the full error chain:

while let Some(err) = err.source() {
eprintln!("caused by: {}");
}

or using a predfined formatter, i.e. eyre::Report https://docs.rs/eyre/0.6.8/eyre/struct.Report.html

eprintln!("{:?}", Report::from(err));

which also shows the backtrace if captured :)

[koute]

There are two ways to get the full error chain:

while let Some(err) = err.source() {
eprintln!("caused by: {}");
}

or using a predfined formatter, i.e. eyre::Report https://docs.rs/eyre/0.6.8/eyre/struct.Report.html

eprintln!("{:?}", Report::from(err));

which also shows the backtrace if captured :)

Okay, I see! I was asking because I've never seen this in our codebase, and a quick grep confirms that neither the .source() nor Report::from are called in both substrate or polkadot even a single time (if I'm grepping this right, which I might not).

So in theory you're right, but yeah, it practice it doesn't seem to matter; at this point this train has already left the station a long time ago, and it'd take a huge effort to rework the whole codebase to use any other way of printing out errors. We can't even guarantee that we use Display instead of Debug everywhere; enforcing an alternative backtrace-y way of printing errors would be a nightmare without some sort of an automatic clippy lint to actually make sure the errors are not printed any other way.

[drahnr]

We do, every error that bubbles up to the main fn will be printed using eyre (wrapped in color_eyre) - but that's only fatal errors make it that far up.

So in theory you're right, but yeah, it practice it doesn't seem to matter; at this point this train has already left the station a long time ago, and it'd take a huge effort to rework the whole codebase to use any other way of printing out errors. We can't even guarantee that we use Display instead of Debug everywhere; enforcing an alternative backtrace-y way of printing errors would be a nightmare without some sort of an automatic clippy lint to actually make sure the errors are not printed any other way.

Using a helper fn and using that consistently should suffice, I think there are only a few dozend of those and it's only relevant when being the end of the line print for this error with no further actions being done upon. I am mostly talking about polkadot / subsystems here.