Substrate: Proper error types

[Demi-Marie]

The consensus engines (at least AuRa and BABE; haven’t checked GRANDPA) currently use String as the error type, which is not awesome. This should be fixed.

I will probably implement this after #2689 and session key verification. If someone wants to do this, I will be happy to mentor them.

[expenses]

I'm looking into this. I think ideally you'd have each ProvideInherentData have an associated Error type that they parse the encoded error into. The problem is that InherentDataProviders holds a Vec<Box<ProvideInherentData<Error=Something>>> so you end up using types anyway. You could use a dyn trait like Error=dyn std::error::Error possibly but that seems messy. What do you think @DemiMarie-parity?

[Demi-Marie]

@expenses I think that a dyn trait is a good idea here. Errors are slow-path code, so the overhead in runtime performance should be more than compensated by the decrease in code size, to the extent that either matters.

[burdges]

I suppose substrate might actually be complex enough to benefit form the failure crate, although libbacktrace sounds like C which folks dislike, and maybe the wasm boundary makes this problematic. And simpler solutions likely suffice too.

[rphmeier]

Failure looks promising, but after error-chain we decided to stay away from all of the ecosystem error-handling stuff until it's very clearly stable

[bkchr]

@expenses I think that a dyn trait is a good idea here. Errors are slow-path code, so the overhead in runtime performance should be more than compensated by the decrease in code size, to the extent that either matters.

Replacing a String with a dynamic traits makes no sense at all. If you want proper errors, define an enum and this only make sense for the consensus crates. The inherent data providers are that generic, that you can not forsee all the combinations of errors that could occur.

[expenses]

The problem here is really how you fit different error types into the Vec<Box<dyn ProvideInherentData>> of InherentDataProviders. You could have something like

enum InherentErrors {
    AuraError1,
    AuraError2,
    BABEError1,
    ...
}

but this of course would create problems for custom modules. I wish that you have traits with concrete assocated types that you didn't have to specify when the trait is dyn, which would enable something like

trait Whatever {
    type Something: Display;
}

struct A;

impl Whatever for A {
    type Something = &'static str;
}

struct B;

impl Whatever for B {
    type Something = String;
}

struct C {
    inner: Vec<Box<dyn Whatever>>
}

but at present you get

error[E0191]: the value of the associated type `Something` (from the trait `Whatever`) must be specified
  --> src/main.rs:20:20
   |
4  |     type Something: Display;
   |     ------------------------ `Something` defined here
...
20 |     inner: Vec<Box<dyn Whatever>>
   |                    ^^^^^^^^^^^^ associated type `Something` must be specified

I'm open to any better ideas though!

[bkchr]

The problem here is really how you fit different error types into the Vec<Box<dyn ProvideInherentData>> of InherentDataProviders. You could have something like

enum InherentErrors {
    AuraError1,
    AuraError2,
    BABEError1,
    ...
}

This issue is not about the inherent data providers, it is about the consesus crates. While creating the inherent extrinsics, you could see any error of a module that wants to create an inherent extrinsics. So, Aura can also see an error from module whatever.

but this of course would create problems for custom modules. I wish that you have traits with concrete assocated types that you didn't have to specify when the trait is dyn, which would enable something like

trait Whatever {
    type Something: Display;
}

struct A;

impl Whatever for A {
    type Something = &'static str;
}

struct B;

impl Whatever for B {
    type Something = String;
}

struct C {
    inner: Vec<Box<dyn Whatever>>
}

but at present you get

error[E0191]: the value of the associated type `Something` (from the trait `Whatever`) must be specified
  --> src/main.rs:20:20
   |
4  |     type Something: Display;
   |     ------------------------ `Something` defined here
...
20 |     inner: Vec<Box<dyn Whatever>>
   |                    ^^^^^^^^^^^^ associated type `Something` must be specified

I'm open to any better ideas though!

I know that this does not work and to repeat myself again, this issue is about the consesus crates. It makes no sense to change the error type for the inherents stuff. The only change I could see, would be to return a Cow<'static, str>, but even that is just some minor minor optimization.

[bkchr]

Using just a string is also one less allocation than having a box of a string.

[expenses]

I've thought about this a bit more. While I don't like using Strings as an error type for InherentDataProviders, using a trait that doesn't do anything except require Display isn't much of an improvement, especially if you aren't going to downcast it. Until something like failure (which unfortunately hasn't seen any improvements since March) becomes more stabilised, it's probably the best option.

I'll keep the PR open for a bit to see what others think though.

[bkchr]

failure will probably just die, there is already something "new": https://github.com/withoutboats/fehler

But as @rphmeier already has written, we decided to switch to not use these error crates.

Another point is that downcasting would not work as well, as we can not know about all possible error types in Aura. Anyone can write an inherent data provider with his own error type, how should Aura know of that?