NDRS-76: cleanup work

[Fraser999]

This resolves some todos from previous PRs and discussions, the main points being:

• removed dummy storage consumer component
• removed displaydoc
• reduced usage of anyhow
• refactored storage component to return Results

[fizyk20]

Looks like it's because the components module is only pub(crate), so any visibility higher than pub(crate) makes no sense here.

[Fraser999]

Well, the components module itself is actually private, but in lib.rs, we export these two objects:

pub use components::small_network::{Config, Error};

They are really exported via the top level API, as can bee seen if we generate the docs.

[fizyk20]

Yes, but that makes them visible via casperlabs_node::{Config, Error} - they are still not reachable from the outside via casperlabs_node::components::small_network::{Config, Error}. And the first part would work regardless of their visibility in the module itself, as long as they are visible to lib.rs. So the warning is still correct, I think.

[fizyk20]

Same as above.

[marc-casperlabs]

Oh no, I'm too late =).

Important bits are From conversions for errors and the RNG choice. =)

[marc-casperlabs]

small should be small_network here

[Fraser999]

The point of the example was to show that we don't need the full module path. I did a similar thing by abbreviating components to comp in the second group.

[marc-casperlabs]

We should probably say "client certificate" here, to avoid confusion.

[Fraser999]

I always find terminology doesn't translate too well from traditional client/server networking to p2p networking. But I agree here since we've already used the term "server".

[marc-casperlabs]

Maybe add a little more docs to drive home that this means we connected to a different node that we were expected --- the peer ID presented does not match the one we're expecting.

[marc-casperlabs]

There should be a description in the docs as well, do not lets this info only be present in the display string.

[marc-casperlabs]

I am not a fan of these. Can we have an individual error variant for each time where we raise an IO error please?

[Fraser999]

I will make the change, but can you expand on why you dislike nested errors?

[goral09]

Is it possible to handle IO errors right where they were thrown/returned rather than propagating upstream?

[Fraser999]

We pretty much do, where we can. For example, when the async message_sender function returns an io::Error, we try to reconnect to the peer multiple times before giving up and removing the peer from our list of peers.

In other places, (e.g. if we can't start a TCP listener inside the new() constructor), we propagate the error upwards to the caller (the reactor's c'tor) since there's nothing we can do about that afaik. That error is propagated all the way up: this is a hard failure and causes the app to fail on startup.

[goral09]

Sure, if we can't recover/handle in the new() that makes sense. Still Io(io::Error) is hard to correctly handle as it may have many representations/variants. Should we "unwrap" the error cause in the new() constructor and signal a more narrow version to the caller?

[marc-casperlabs]

To be clear, I advocate on making errors more meaningful than just "an IO error has occured". In general, this is only fine if there's just a single point where such an error can occur and even then I tend to give context in the variant. Naming it Io is redundant, since all that info is already in the type.

Imagine you have a function copying a file from a to b. There are two places where you could have an IO error: Reading a file, parsing it, making some changes and writing out the result file. Let's assume there's a permission issue, you'll get a non-descript "permission denied" message once your program crashes, without even the context of the filename.

Here's an example for what the error type could be:

Read(io::Error),
Parse(serde::de::Error),
Write(io::Error),

If there was just one error named Io, I'd have zero chance of figuring out what went wrong.

(I'm aware that the stdlib does precisely what I am advocating against =))

anyhow supports shortcutting this process using .context, for when you do not write this many errors.

A good example in the existing codebase is in the TLS module, just imagine how cryptic errors would be if there was just a single ErrorStack (which is openssl's error) variant: https://github.com/CasperLabs/casperlabs-node/blob/8724ef9960d4d95077f8b1117c1b42ad8b7ca5f9/src/tls.rs#L375

Trust me, the OpenSSL error messages aren't exactly easily understood without context. =)

I'll admit, I haven't checked too deeply of what kinds of errors we need, but if it is no longer named Io but had a meaningful name instead, it should be apparent.

[marc-casperlabs]

Rereading this, I want to point out that I am not against nested errors, to the contrary! Nested vs non-nesting is entirely orthogonal to this, I believe.

I.e. without nesting the example error would be

Read,
Parse,
Write,

which I am not advocating.

[marc-casperlabs]

Same as IO, they should be individual.

[Fraser999]

Will do, but with same question as above.

[marc-casperlabs]

Did we fail to capture others here?

If you do not want anyhow present here, an alternative would be Other(Box<std::error::Error>).

[Fraser999]

The tls module still uses anyhow extensively. I didn't remove anyhow from there since I'm still not sure if you plan to move this module to its own crate. I also feel there's less likelihood of other devs copying the error handling style from that module compared to the higher-profile small_network.

I believe that's the only place left where we use anyhow outside of application code.

[marc-casperlabs]

What's the rationale behind not using OsRng by the way? It's the same as reading from /dev/urandom or using one of the blessed interfaces for getting random numbers. People with hardware RNGs will get theirs used that way!

We can "verify" it:

https://github.com/rust-random/rand/blob/63d53c09c197542e6ec6271e528aea1bc287c99d/rand_core/src/os.rs#L67

    fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> {
        getrandom(dest)?;
        Ok(())
    }

In the getrandom crate:

OS	interface
Linux, Android	getrandom system call if available, otherwise /dev/urandom after successfully polling /dev/random

unsafe fn getrandom(
    buf: *mut libc::c_void,
    buflen: libc::size_t,
    flags: libc::c_uint,
) -> libc::ssize_t {
    libc::syscall(libc::SYS_getrandom, buf, buflen, flags) as libc::ssize_t
}

From the getrandom(2) man page:

The getrandom() system call fills the buffer pointed to by buf with up to buflen random bytes. These bytes can be used to seed user-space random number generators or for cryptographic purposes.

Now here it says "seeding" but urandom also delivers an infinite amount of random bits. What's the reason not to use it?

[Fraser999]

In no particular order, my reasons for choosing ChaCha20Rng over OsRng are:

• its operations can't fail, which makes it simpler to use
• it should be faster
• it can be seeded (potentially critical in order to be able to reproduce test failures)
• we already use it in the EE codebase
• using an OS RNG to seed a CSPRNG is fairly standard practice afaik. I guess we can even reseed the ChaCha20Rng periodically if we think that level of security is required.

Note that if we were to go for the OsRng, there'd be no point to holding an instance in the reactor and passing a mut ref to components, since it's pretty much zero cost for any component to just construct an OsRng on the fly.

[marc-casperlabs]

its operations can't fail, which makes it simpler to use

I don't think the RNG interfaces exposes failures either, right?

it should be faster

I believe that's not relevant in this case, OsRng should be by far fast enough.

it can be seeded (potentially critical in order to be able to reproduce test failures)
using an OS RNG to seed a CSPRNG is fairly standard practice afaik. I guess we can even reseed the ChaCha20Rng periodically if we think that level of security is required.

Note that if we were to go for the OsRng, there'd be no point to holding an instance in the reactor and passing a mut ref to components, since it's pretty much zero cost for any component to just construct an OsRng on the fly.

This I don't agree with at all. We pass in an Rng implementing type into each function and pass that all the way down. We only create an RNG at the very top of the call stack, on program instantiation. During normal operation, that is an OsRng instance, during testing, it's a PRNG with a fixed seed. But that only happens once!

[Fraser999]

I don't think the RNG interfaces exposes failures either, right?

Well, since getrandom() can fail, we shouldn't use the ones which don't return Results. All the methods which don't return a result can potentially panic as they're based on fill_bytes. I believe the chances of panic are slim, especially if the first usage succeeded, but still... why risk it?

I believe that's not relevant in this case, OsRng should be by far fast enough.

I don't see how we can possibly know yet what will be fast enough! We don't know all the use cases yet, do we?

This I don't agree with at all. We pass in an Rng implementing type into each function and pass that all the way down. We only create an RNG at the very top of the call stack, on program instantiation. During normal operation, that is an OsRng instance, during testing, it's a PRNG with a fixed seed. But that only happens once!

For all other rand RNGs (afaik) that makes sense, but not for OsRng. If you have a look at its implementation you'll see it's an empty struct. There's just no point in holding onto a single instance anywhere.