Add `rand_core::InfallibleRng` marker trait by newpavlov · Pull Request #1412 · rust-random/rand

newpavlov · 2024-03-18T16:54:05Z

Implementers of the rand_core::InfallibleRng trait indicate that they will never return errors from the RngCore::try_fill_bytes method and will never panic on unwrapping rand_core::Error while calling other methods.

@ctz
Does it address your concerns mentioned in this comment? If you have other concerns/suggestion, feel free to list them here or in a separate issue.

newpavlov · 2024-03-18T16:57:40Z

This PR got a bunch of unrelated formatting changes in the edited files, which were introduced automatically by my editor. Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

dhardy · 2024-03-18T17:16:43Z

Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

Yes; I was thinking try to resolve most PRs first. But we could also just go ahead...

dhardy · 2024-03-18T19:26:21Z

+/// A marker trait used to indicate that an [`RngCore`] implementation is
+/// supposed to never return errors from the [`RngCore::try_fill_bytes`] method
+/// and never panic on unwrapping [`Error`] while calling other methods.
+///
+/// This trait is usually implemented by PRNGs, as opposed to OS, hardware,
+/// and periodically-reseeded RNGs, which may fail with IO errors.
+pub trait InfallibleRng: RngCore {}


Commenting just for visibility of the significant addition.
It falls into the same trap as std::iter::TrustedLen in that it promises something about another trait.
I think I'm okay with this, but worth thinking about / getting more input on.

I thought about different alternatives and it looks like it's the simplest option. Plus, we already have the CryptoRng trait.
One potential option is to make error an associated type (and CryptoRng may become associated bool constant). This way we could use Infallible by default. But without trait aliases it would be significantly less ergonomic to work with. Here is a draft of how it could look in future: playground.

Adding associated types + consts to RngCore changes the meaning on every existing usage as a generic bound.
For "crypto grade", I prefer the trait-inheritance model. After all, every CryptoRng is also a valid RNG. Also, can't have CRYPTO_STRONG default to true on all impls.
For infallibility, we'd at least need a bound on that error type: Error: std::error::Error or Error: Into<Something> where Something is a summarized RNG error (NotInitialized, EndOfSequence, IOError, MissingHardwareDevice, ???).

every CryptoRng is also a valid RNG.
Yes, and? I don't see why the associated constant would be at odds with it. "Cryptographically strong" is a binary property of RNG, which can be modeled perfectly by an associated bool constant.
Also, can't have CRYPTO_STRONG default to true on all impls.
Yes, it should've been false in my example.
For infallibility, we'd at least need a bound on that error type
Sure. But I think it's an unimportant detail. Without stabilization of at least trait aliases, this approach is, arguably, not practical anyway.

Yes, and?
Only that this case does work well with the trait inheritance model; not every boolean property does.

newpavlov · 2024-03-22T14:29:35Z

I've cleaned up the formatting changes.

dhardy · 2024-03-22T17:15:28Z

Also, if @ctz doesn't appreciate the design of RngCore, he's not the only one. It's a compromise that I don't think anyone likes. There's a tracker of some suggestions here: #1261.

This PR adds something, yes, but is a more fundamental revision a better option?

dhardy · 2024-03-22T17:25:55Z

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

Implication: we should drop OsRng. Since we no longer use this for seeding, I am not aware of any good reason to keep this. (We already removed ReadRng.)

We can also drop try_fill_bytes and all the stuff about converting between fallible and infallible cases.

Am I missing anything important?

newpavlov · 2024-03-22T18:59:57Z

I am open to a more global rework (e.g. it may be worth to remove BlockRng stuff and replace it with helper macros/functions). But I think it's better to do in a separate PR after merging this one, since it's not given that we will be able to finalize it for v0.9 release cycle.

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

In my opinion, it's tantamount to sweeping the problem under the rug.

IO-based RNGs such as OsRng or hardware-based RNGs (not an unusual thing with cryptographic applications) can fail as any other IO. It's a simple truth of life. Imagine someone has misconfigured, or forgot to plug-in a HW RNG. It's better for a cryptographic application/library to return a sensible error, instead of unconditionally panicking outright. Even PRNGs may fail in some cases, e.g. ChaCha-based RNGs may return error on detected looping, which may happen in practice in the presence of seeking.

As for OsRng, it can be a preferred way of generating sensitive information such as cryptographic keys, especially considering that ThreadRng does not implement any protection against exposed state. You may say "then use getrandom directly", but it would be less flexible, e.g. we would not be able to use PRNGs for reproducible testing of such methods.

dhardy · 2024-03-23T08:18:30Z

You are right that such questions are beyond the scope of this PR, however we should at least determine whether we actually want trait InfallibleRng before merging this. Right, new issue then.

newpavlov · 2024-04-01T16:40:21Z

Closing in favor of #1424.

newpavlov · 2024-03-18T16:54:05Z

Implementers of the rand_core::InfallibleRng trait indicate that they will never return errors from the RngCore::try_fill_bytes method and will never panic on unwrapping rand_core::Error while calling other methods.

@ctz
Does it address your concerns mentioned in this comment? If you have other concerns/suggestion, feel free to list them here or in a separate issue.

newpavlov · 2024-03-18T16:57:40Z

This PR got a bunch of unrelated formatting changes in the edited files, which were introduced automatically by my editor. Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

dhardy · 2024-03-18T17:16:43Z

Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

Yes; I was thinking try to resolve most PRs first. But we could also just go ahead...

dhardy · 2024-03-18T19:26:21Z

+/// A marker trait used to indicate that an [`RngCore`] implementation is
+/// supposed to never return errors from the [`RngCore::try_fill_bytes`] method
+/// and never panic on unwrapping [`Error`] while calling other methods.
+///
+/// This trait is usually implemented by PRNGs, as opposed to OS, hardware,
+/// and periodically-reseeded RNGs, which may fail with IO errors.
+pub trait InfallibleRng: RngCore {}


Commenting just for visibility of the significant addition.
It falls into the same trap as std::iter::TrustedLen in that it promises something about another trait.
I think I'm okay with this, but worth thinking about / getting more input on.

I thought about different alternatives and it looks like it's the simplest option. Plus, we already have the CryptoRng trait.
One potential option is to make error an associated type (and CryptoRng may become associated bool constant). This way we could use Infallible by default. But without trait aliases it would be significantly less ergonomic to work with. Here is a draft of how it could look in future: playground.

Adding associated types + consts to RngCore changes the meaning on every existing usage as a generic bound.
For "crypto grade", I prefer the trait-inheritance model. After all, every CryptoRng is also a valid RNG. Also, can't have CRYPTO_STRONG default to true on all impls.
For infallibility, we'd at least need a bound on that error type: Error: std::error::Error or Error: Into<Something> where Something is a summarized RNG error (NotInitialized, EndOfSequence, IOError, MissingHardwareDevice, ???).

every CryptoRng is also a valid RNG.
Yes, and? I don't see why the associated constant would be at odds with it. "Cryptographically strong" is a binary property of RNG, which can be modeled perfectly by an associated bool constant.
Also, can't have CRYPTO_STRONG default to true on all impls.
Yes, it should've been false in my example.
For infallibility, we'd at least need a bound on that error type
Sure. But I think it's an unimportant detail. Without stabilization of at least trait aliases, this approach is, arguably, not practical anyway.

Yes, and?
Only that this case does work well with the trait inheritance model; not every boolean property does.

newpavlov · 2024-03-22T14:29:35Z

I've cleaned up the formatting changes.

dhardy · 2024-03-22T17:15:28Z

Also, if @ctz doesn't appreciate the design of RngCore, he's not the only one. It's a compromise that I don't think anyone likes. There's a tracker of some suggestions here: #1261.

This PR adds something, yes, but is a more fundamental revision a better option?

dhardy · 2024-03-22T17:25:55Z

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

Implication: we should drop OsRng. Since we no longer use this for seeding, I am not aware of any good reason to keep this. (We already removed ReadRng.)

We can also drop try_fill_bytes and all the stuff about converting between fallible and infallible cases.

Am I missing anything important?

newpavlov · 2024-03-22T18:59:57Z

I am open to a more global rework (e.g. it may be worth to remove BlockRng stuff and replace it with helper macros/functions). But I think it's better to do in a separate PR after merging this one, since it's not given that we will be able to finalize it for v0.9 release cycle.

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

In my opinion, it's tantamount to sweeping the problem under the rug.

IO-based RNGs such as OsRng or hardware-based RNGs (not an unusual thing with cryptographic applications) can fail as any other IO. It's a simple truth of life. Imagine someone has misconfigured, or forgot to plug-in a HW RNG. It's better for a cryptographic application/library to return a sensible error, instead of unconditionally panicking outright. Even PRNGs may fail in some cases, e.g. ChaCha-based RNGs may return error on detected looping, which may happen in practice in the presence of seeking.

As for OsRng, it can be a preferred way of generating sensitive information such as cryptographic keys, especially considering that ThreadRng does not implement any protection against exposed state. You may say "then use getrandom directly", but it would be less flexible, e.g. we would not be able to use PRNGs for reproducible testing of such methods.

dhardy · 2024-03-23T08:18:30Z

You are right that such questions are beyond the scope of this PR, however we should at least determine whether we actually want trait InfallibleRng before merging this. Right, new issue then.

newpavlov · 2024-04-01T16:40:21Z

Closing in favor of #1424.

newpavlov · 2024-03-18T16:54:05Z

Implementers of the rand_core::InfallibleRng trait indicate that they will never return errors from the RngCore::try_fill_bytes method and will never panic on unwrapping rand_core::Error while calling other methods.

@ctz
Does it address your concerns mentioned in this comment? If you have other concerns/suggestion, feel free to list them here or in a separate issue.

newpavlov · 2024-03-18T16:57:40Z

This PR got a bunch of unrelated formatting changes in the edited files, which were introduced automatically by my editor. Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

dhardy · 2024-03-18T17:16:43Z

Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

Yes; I was thinking try to resolve most PRs first. But we could also just go ahead...

dhardy · 2024-03-18T19:26:21Z

+/// A marker trait used to indicate that an [`RngCore`] implementation is
+/// supposed to never return errors from the [`RngCore::try_fill_bytes`] method
+/// and never panic on unwrapping [`Error`] while calling other methods.
+///
+/// This trait is usually implemented by PRNGs, as opposed to OS, hardware,
+/// and periodically-reseeded RNGs, which may fail with IO errors.
+pub trait InfallibleRng: RngCore {}


Commenting just for visibility of the significant addition.
It falls into the same trap as std::iter::TrustedLen in that it promises something about another trait.
I think I'm okay with this, but worth thinking about / getting more input on.

I thought about different alternatives and it looks like it's the simplest option. Plus, we already have the CryptoRng trait.
One potential option is to make error an associated type (and CryptoRng may become associated bool constant). This way we could use Infallible by default. But without trait aliases it would be significantly less ergonomic to work with. Here is a draft of how it could look in future: playground.

Adding associated types + consts to RngCore changes the meaning on every existing usage as a generic bound.
For "crypto grade", I prefer the trait-inheritance model. After all, every CryptoRng is also a valid RNG. Also, can't have CRYPTO_STRONG default to true on all impls.
For infallibility, we'd at least need a bound on that error type: Error: std::error::Error or Error: Into<Something> where Something is a summarized RNG error (NotInitialized, EndOfSequence, IOError, MissingHardwareDevice, ???).

every CryptoRng is also a valid RNG.
Yes, and? I don't see why the associated constant would be at odds with it. "Cryptographically strong" is a binary property of RNG, which can be modeled perfectly by an associated bool constant.
Also, can't have CRYPTO_STRONG default to true on all impls.
Yes, it should've been false in my example.
For infallibility, we'd at least need a bound on that error type
Sure. But I think it's an unimportant detail. Without stabilization of at least trait aliases, this approach is, arguably, not practical anyway.

Yes, and?
Only that this case does work well with the trait inheritance model; not every boolean property does.

newpavlov · 2024-03-22T14:29:35Z

I've cleaned up the formatting changes.

dhardy · 2024-03-22T17:15:28Z

Also, if @ctz doesn't appreciate the design of RngCore, he's not the only one. It's a compromise that I don't think anyone likes. There's a tracker of some suggestions here: #1261.

This PR adds something, yes, but is a more fundamental revision a better option?

dhardy · 2024-03-22T17:25:55Z

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

Implication: we should drop OsRng. Since we no longer use this for seeding, I am not aware of any good reason to keep this. (We already removed ReadRng.)

We can also drop try_fill_bytes and all the stuff about converting between fallible and infallible cases.

Am I missing anything important?

newpavlov · 2024-03-22T18:59:57Z

I am open to a more global rework (e.g. it may be worth to remove BlockRng stuff and replace it with helper macros/functions). But I think it's better to do in a separate PR after merging this one, since it's not given that we will be able to finalize it for v0.9 release cycle.

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

In my opinion, it's tantamount to sweeping the problem under the rug.

IO-based RNGs such as OsRng or hardware-based RNGs (not an unusual thing with cryptographic applications) can fail as any other IO. It's a simple truth of life. Imagine someone has misconfigured, or forgot to plug-in a HW RNG. It's better for a cryptographic application/library to return a sensible error, instead of unconditionally panicking outright. Even PRNGs may fail in some cases, e.g. ChaCha-based RNGs may return error on detected looping, which may happen in practice in the presence of seeking.

As for OsRng, it can be a preferred way of generating sensitive information such as cryptographic keys, especially considering that ThreadRng does not implement any protection against exposed state. You may say "then use getrandom directly", but it would be less flexible, e.g. we would not be able to use PRNGs for reproducible testing of such methods.

dhardy · 2024-03-23T08:18:30Z

You are right that such questions are beyond the scope of this PR, however we should at least determine whether we actually want trait InfallibleRng before merging this. Right, new issue then.

newpavlov · 2024-04-01T16:40:21Z

Closing in favor of #1424.

newpavlov · 2024-03-18T16:54:05Z

Implementers of the rand_core::InfallibleRng trait indicate that they will never return errors from the RngCore::try_fill_bytes method and will never panic on unwrapping rand_core::Error while calling other methods.

@ctz
Does it address your concerns mentioned in this comment? If you have other concerns/suggestion, feel free to list them here or in a separate issue.

newpavlov · 2024-03-18T16:57:40Z

This PR got a bunch of unrelated formatting changes in the edited files, which were introduced automatically by my editor. Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

dhardy · 2024-03-18T17:16:43Z

Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

Yes; I was thinking try to resolve most PRs first. But we could also just go ahead...

dhardy · 2024-03-18T19:26:21Z

+/// A marker trait used to indicate that an [`RngCore`] implementation is
+/// supposed to never return errors from the [`RngCore::try_fill_bytes`] method
+/// and never panic on unwrapping [`Error`] while calling other methods.
+///
+/// This trait is usually implemented by PRNGs, as opposed to OS, hardware,
+/// and periodically-reseeded RNGs, which may fail with IO errors.
+pub trait InfallibleRng: RngCore {}


Commenting just for visibility of the significant addition.
It falls into the same trap as std::iter::TrustedLen in that it promises something about another trait.
I think I'm okay with this, but worth thinking about / getting more input on.

I thought about different alternatives and it looks like it's the simplest option. Plus, we already have the CryptoRng trait.
One potential option is to make error an associated type (and CryptoRng may become associated bool constant). This way we could use Infallible by default. But without trait aliases it would be significantly less ergonomic to work with. Here is a draft of how it could look in future: playground.

Adding associated types + consts to RngCore changes the meaning on every existing usage as a generic bound.
For "crypto grade", I prefer the trait-inheritance model. After all, every CryptoRng is also a valid RNG. Also, can't have CRYPTO_STRONG default to true on all impls.
For infallibility, we'd at least need a bound on that error type: Error: std::error::Error or Error: Into<Something> where Something is a summarized RNG error (NotInitialized, EndOfSequence, IOError, MissingHardwareDevice, ???).

every CryptoRng is also a valid RNG.
Yes, and? I don't see why the associated constant would be at odds with it. "Cryptographically strong" is a binary property of RNG, which can be modeled perfectly by an associated bool constant.
Also, can't have CRYPTO_STRONG default to true on all impls.
Yes, it should've been false in my example.
For infallibility, we'd at least need a bound on that error type
Sure. But I think it's an unimportant detail. Without stabilization of at least trait aliases, this approach is, arguably, not practical anyway.

Yes, and?
Only that this case does work well with the trait inheritance model; not every boolean property does.

newpavlov · 2024-03-22T14:29:35Z

I've cleaned up the formatting changes.

dhardy · 2024-03-22T17:15:28Z

Also, if @ctz doesn't appreciate the design of RngCore, he's not the only one. It's a compromise that I don't think anyone likes. There's a tracker of some suggestions here: #1261.

This PR adds something, yes, but is a more fundamental revision a better option?

dhardy · 2024-03-22T17:25:55Z

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

Implication: we should drop OsRng. Since we no longer use this for seeding, I am not aware of any good reason to keep this. (We already removed ReadRng.)

We can also drop try_fill_bytes and all the stuff about converting between fallible and infallible cases.

Am I missing anything important?

newpavlov · 2024-03-22T18:59:57Z

I am open to a more global rework (e.g. it may be worth to remove BlockRng stuff and replace it with helper macros/functions). But I think it's better to do in a separate PR after merging this one, since it's not given that we will be able to finalize it for v0.9 release cycle.

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

In my opinion, it's tantamount to sweeping the problem under the rug.

IO-based RNGs such as OsRng or hardware-based RNGs (not an unusual thing with cryptographic applications) can fail as any other IO. It's a simple truth of life. Imagine someone has misconfigured, or forgot to plug-in a HW RNG. It's better for a cryptographic application/library to return a sensible error, instead of unconditionally panicking outright. Even PRNGs may fail in some cases, e.g. ChaCha-based RNGs may return error on detected looping, which may happen in practice in the presence of seeking.

As for OsRng, it can be a preferred way of generating sensitive information such as cryptographic keys, especially considering that ThreadRng does not implement any protection against exposed state. You may say "then use getrandom directly", but it would be less flexible, e.g. we would not be able to use PRNGs for reproducible testing of such methods.

dhardy · 2024-03-23T08:18:30Z

You are right that such questions are beyond the scope of this PR, however we should at least determine whether we actually want trait InfallibleRng before merging this. Right, new issue then.

newpavlov · 2024-04-01T16:40:21Z

Closing in favor of #1424.

newpavlov · 2024-03-18T16:54:05Z

Implementers of the rand_core::InfallibleRng trait indicate that they will never return errors from the RngCore::try_fill_bytes method and will never panic on unwrapping rand_core::Error while calling other methods.

@ctz
Does it address your concerns mentioned in this comment? If you have other concerns/suggestion, feel free to list them here or in a separate issue.

newpavlov · 2024-03-18T16:57:40Z

This PR got a bunch of unrelated formatting changes in the edited files, which were introduced automatically by my editor. Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

dhardy · 2024-03-18T17:16:43Z

Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

Yes; I was thinking try to resolve most PRs first. But we could also just go ahead...

dhardy · 2024-03-18T19:26:21Z

+/// A marker trait used to indicate that an [`RngCore`] implementation is
+/// supposed to never return errors from the [`RngCore::try_fill_bytes`] method
+/// and never panic on unwrapping [`Error`] while calling other methods.
+///
+/// This trait is usually implemented by PRNGs, as opposed to OS, hardware,
+/// and periodically-reseeded RNGs, which may fail with IO errors.
+pub trait InfallibleRng: RngCore {}


Commenting just for visibility of the significant addition.
It falls into the same trap as std::iter::TrustedLen in that it promises something about another trait.
I think I'm okay with this, but worth thinking about / getting more input on.

I thought about different alternatives and it looks like it's the simplest option. Plus, we already have the CryptoRng trait.
One potential option is to make error an associated type (and CryptoRng may become associated bool constant). This way we could use Infallible by default. But without trait aliases it would be significantly less ergonomic to work with. Here is a draft of how it could look in future: playground.

Adding associated types + consts to RngCore changes the meaning on every existing usage as a generic bound.
For "crypto grade", I prefer the trait-inheritance model. After all, every CryptoRng is also a valid RNG. Also, can't have CRYPTO_STRONG default to true on all impls.
For infallibility, we'd at least need a bound on that error type: Error: std::error::Error or Error: Into<Something> where Something is a summarized RNG error (NotInitialized, EndOfSequence, IOError, MissingHardwareDevice, ???).

every CryptoRng is also a valid RNG.
Yes, and? I don't see why the associated constant would be at odds with it. "Cryptographically strong" is a binary property of RNG, which can be modeled perfectly by an associated bool constant.
Also, can't have CRYPTO_STRONG default to true on all impls.
Yes, it should've been false in my example.
For infallibility, we'd at least need a bound on that error type
Sure. But I think it's an unimportant detail. Without stabilization of at least trait aliases, this approach is, arguably, not practical anyway.

Yes, and?
Only that this case does work well with the trait inheritance model; not every boolean property does.

newpavlov · 2024-03-22T14:29:35Z

I've cleaned up the formatting changes.

dhardy · 2024-03-22T17:15:28Z

Also, if @ctz doesn't appreciate the design of RngCore, he's not the only one. It's a compromise that I don't think anyone likes. There's a tracker of some suggestions here: #1261.

This PR adds something, yes, but is a more fundamental revision a better option?

dhardy · 2024-03-22T17:25:55Z

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

Implication: we should drop OsRng. Since we no longer use this for seeding, I am not aware of any good reason to keep this. (We already removed ReadRng.)

We can also drop try_fill_bytes and all the stuff about converting between fallible and infallible cases.

Am I missing anything important?

newpavlov · 2024-03-22T18:59:57Z

I am open to a more global rework (e.g. it may be worth to remove BlockRng stuff and replace it with helper macros/functions). But I think it's better to do in a separate PR after merging this one, since it's not given that we will be able to finalize it for v0.9 release cycle.

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

In my opinion, it's tantamount to sweeping the problem under the rug.

IO-based RNGs such as OsRng or hardware-based RNGs (not an unusual thing with cryptographic applications) can fail as any other IO. It's a simple truth of life. Imagine someone has misconfigured, or forgot to plug-in a HW RNG. It's better for a cryptographic application/library to return a sensible error, instead of unconditionally panicking outright. Even PRNGs may fail in some cases, e.g. ChaCha-based RNGs may return error on detected looping, which may happen in practice in the presence of seeking.

As for OsRng, it can be a preferred way of generating sensitive information such as cryptographic keys, especially considering that ThreadRng does not implement any protection against exposed state. You may say "then use getrandom directly", but it would be less flexible, e.g. we would not be able to use PRNGs for reproducible testing of such methods.

dhardy · 2024-03-23T08:18:30Z

You are right that such questions are beyond the scope of this PR, however we should at least determine whether we actually want trait InfallibleRng before merging this. Right, new issue then.

newpavlov · 2024-04-01T16:40:21Z

Closing in favor of #1424.

newpavlov · 2024-03-18T16:54:05Z

Implementers of the rand_core::InfallibleRng trait indicate that they will never return errors from the RngCore::try_fill_bytes method and will never panic on unwrapping rand_core::Error while calling other methods.

@ctz
Does it address your concerns mentioned in this comment? If you have other concerns/suggestion, feel free to list them here or in a separate issue.

newpavlov · 2024-03-18T16:57:40Z

This PR got a bunch of unrelated formatting changes in the edited files, which were introduced automatically by my editor. Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

dhardy · 2024-03-18T17:16:43Z

Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

Yes; I was thinking try to resolve most PRs first. But we could also just go ahead...

dhardy · 2024-03-18T19:26:21Z

+/// A marker trait used to indicate that an [`RngCore`] implementation is
+/// supposed to never return errors from the [`RngCore::try_fill_bytes`] method
+/// and never panic on unwrapping [`Error`] while calling other methods.
+///
+/// This trait is usually implemented by PRNGs, as opposed to OS, hardware,
+/// and periodically-reseeded RNGs, which may fail with IO errors.
+pub trait InfallibleRng: RngCore {}


Commenting just for visibility of the significant addition.
It falls into the same trap as std::iter::TrustedLen in that it promises something about another trait.
I think I'm okay with this, but worth thinking about / getting more input on.

I thought about different alternatives and it looks like it's the simplest option. Plus, we already have the CryptoRng trait.
One potential option is to make error an associated type (and CryptoRng may become associated bool constant). This way we could use Infallible by default. But without trait aliases it would be significantly less ergonomic to work with. Here is a draft of how it could look in future: playground.

Adding associated types + consts to RngCore changes the meaning on every existing usage as a generic bound.
For "crypto grade", I prefer the trait-inheritance model. After all, every CryptoRng is also a valid RNG. Also, can't have CRYPTO_STRONG default to true on all impls.
For infallibility, we'd at least need a bound on that error type: Error: std::error::Error or Error: Into<Something> where Something is a summarized RNG error (NotInitialized, EndOfSequence, IOError, MissingHardwareDevice, ???).

every CryptoRng is also a valid RNG.
Yes, and? I don't see why the associated constant would be at odds with it. "Cryptographically strong" is a binary property of RNG, which can be modeled perfectly by an associated bool constant.
Also, can't have CRYPTO_STRONG default to true on all impls.
Yes, it should've been false in my example.
For infallibility, we'd at least need a bound on that error type
Sure. But I think it's an unimportant detail. Without stabilization of at least trait aliases, this approach is, arguably, not practical anyway.

Yes, and?
Only that this case does work well with the trait inheritance model; not every boolean property does.

newpavlov · 2024-03-22T14:29:35Z

I've cleaned up the formatting changes.

dhardy · 2024-03-22T17:15:28Z

Also, if @ctz doesn't appreciate the design of RngCore, he's not the only one. It's a compromise that I don't think anyone likes. There's a tracker of some suggestions here: #1261.

This PR adds something, yes, but is a more fundamental revision a better option?

dhardy · 2024-03-22T17:25:55Z

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

Implication: we should drop OsRng. Since we no longer use this for seeding, I am not aware of any good reason to keep this. (We already removed ReadRng.)

We can also drop try_fill_bytes and all the stuff about converting between fallible and infallible cases.

Am I missing anything important?

newpavlov · 2024-03-22T18:59:57Z

I am open to a more global rework (e.g. it may be worth to remove BlockRng stuff and replace it with helper macros/functions). But I think it's better to do in a separate PR after merging this one, since it's not given that we will be able to finalize it for v0.9 release cycle.

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

In my opinion, it's tantamount to sweeping the problem under the rug.

IO-based RNGs such as OsRng or hardware-based RNGs (not an unusual thing with cryptographic applications) can fail as any other IO. It's a simple truth of life. Imagine someone has misconfigured, or forgot to plug-in a HW RNG. It's better for a cryptographic application/library to return a sensible error, instead of unconditionally panicking outright. Even PRNGs may fail in some cases, e.g. ChaCha-based RNGs may return error on detected looping, which may happen in practice in the presence of seeking.

As for OsRng, it can be a preferred way of generating sensitive information such as cryptographic keys, especially considering that ThreadRng does not implement any protection against exposed state. You may say "then use getrandom directly", but it would be less flexible, e.g. we would not be able to use PRNGs for reproducible testing of such methods.

dhardy · 2024-03-23T08:18:30Z

You are right that such questions are beyond the scope of this PR, however we should at least determine whether we actually want trait InfallibleRng before merging this. Right, new issue then.

newpavlov · 2024-04-01T16:40:21Z

Closing in favor of #1424.

newpavlov · 2024-03-18T16:54:05Z

Implementers of the rand_core::InfallibleRng trait indicate that they will never return errors from the RngCore::try_fill_bytes method and will never panic on unwrapping rand_core::Error while calling other methods.

@ctz
Does it address your concerns mentioned in this comment? If you have other concerns/suggestion, feel free to list them here or in a separate issue.

newpavlov · 2024-03-18T16:57:40Z

This PR got a bunch of unrelated formatting changes in the edited files, which were introduced automatically by my editor. Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

dhardy · 2024-03-18T17:16:43Z

Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

Yes; I was thinking try to resolve most PRs first. But we could also just go ahead...

dhardy · 2024-03-18T19:26:21Z

+/// A marker trait used to indicate that an [`RngCore`] implementation is
+/// supposed to never return errors from the [`RngCore::try_fill_bytes`] method
+/// and never panic on unwrapping [`Error`] while calling other methods.
+///
+/// This trait is usually implemented by PRNGs, as opposed to OS, hardware,
+/// and periodically-reseeded RNGs, which may fail with IO errors.
+pub trait InfallibleRng: RngCore {}


Commenting just for visibility of the significant addition.
It falls into the same trap as std::iter::TrustedLen in that it promises something about another trait.
I think I'm okay with this, but worth thinking about / getting more input on.

I thought about different alternatives and it looks like it's the simplest option. Plus, we already have the CryptoRng trait.
One potential option is to make error an associated type (and CryptoRng may become associated bool constant). This way we could use Infallible by default. But without trait aliases it would be significantly less ergonomic to work with. Here is a draft of how it could look in future: playground.

Adding associated types + consts to RngCore changes the meaning on every existing usage as a generic bound.
For "crypto grade", I prefer the trait-inheritance model. After all, every CryptoRng is also a valid RNG. Also, can't have CRYPTO_STRONG default to true on all impls.
For infallibility, we'd at least need a bound on that error type: Error: std::error::Error or Error: Into<Something> where Something is a summarized RNG error (NotInitialized, EndOfSequence, IOError, MissingHardwareDevice, ???).

every CryptoRng is also a valid RNG.
Yes, and? I don't see why the associated constant would be at odds with it. "Cryptographically strong" is a binary property of RNG, which can be modeled perfectly by an associated bool constant.
Also, can't have CRYPTO_STRONG default to true on all impls.
Yes, it should've been false in my example.
For infallibility, we'd at least need a bound on that error type
Sure. But I think it's an unimportant detail. Without stabilization of at least trait aliases, this approach is, arguably, not practical anyway.

Yes, and?
Only that this case does work well with the trait inheritance model; not every boolean property does.

newpavlov · 2024-03-22T14:29:35Z

I've cleaned up the formatting changes.

dhardy · 2024-03-22T17:15:28Z

Also, if @ctz doesn't appreciate the design of RngCore, he's not the only one. It's a compromise that I don't think anyone likes. There's a tracker of some suggestions here: #1261.

This PR adds something, yes, but is a more fundamental revision a better option?

dhardy · 2024-03-22T17:25:55Z

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

Implication: we should drop OsRng. Since we no longer use this for seeding, I am not aware of any good reason to keep this. (We already removed ReadRng.)

We can also drop try_fill_bytes and all the stuff about converting between fallible and infallible cases.

Am I missing anything important?

newpavlov · 2024-03-22T18:59:57Z

I am open to a more global rework (e.g. it may be worth to remove BlockRng stuff and replace it with helper macros/functions). But I think it's better to do in a separate PR after merging this one, since it's not given that we will be able to finalize it for v0.9 release cycle.

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

In my opinion, it's tantamount to sweeping the problem under the rug.

IO-based RNGs such as OsRng or hardware-based RNGs (not an unusual thing with cryptographic applications) can fail as any other IO. It's a simple truth of life. Imagine someone has misconfigured, or forgot to plug-in a HW RNG. It's better for a cryptographic application/library to return a sensible error, instead of unconditionally panicking outright. Even PRNGs may fail in some cases, e.g. ChaCha-based RNGs may return error on detected looping, which may happen in practice in the presence of seeking.

As for OsRng, it can be a preferred way of generating sensitive information such as cryptographic keys, especially considering that ThreadRng does not implement any protection against exposed state. You may say "then use getrandom directly", but it would be less flexible, e.g. we would not be able to use PRNGs for reproducible testing of such methods.

dhardy · 2024-03-23T08:18:30Z

You are right that such questions are beyond the scope of this PR, however we should at least determine whether we actually want trait InfallibleRng before merging this. Right, new issue then.

newpavlov · 2024-04-01T16:40:21Z

Closing in favor of #1424.

newpavlov · 2024-03-18T16:54:05Z

Implementers of the rand_core::InfallibleRng trait indicate that they will never return errors from the RngCore::try_fill_bytes method and will never panic on unwrapping rand_core::Error while calling other methods.

@ctz
Does it address your concerns mentioned in this comment? If you have other concerns/suggestion, feel free to list them here or in a separate issue.

newpavlov · 2024-03-18T16:57:40Z

This PR got a bunch of unrelated formatting changes in the edited files, which were introduced automatically by my editor. Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

dhardy · 2024-03-18T17:16:43Z

Later we probably need to fix formatting for the whole project and add rustfmt check to CI.

Yes; I was thinking try to resolve most PRs first. But we could also just go ahead...

dhardy · 2024-03-18T19:26:21Z

+/// A marker trait used to indicate that an [`RngCore`] implementation is
+/// supposed to never return errors from the [`RngCore::try_fill_bytes`] method
+/// and never panic on unwrapping [`Error`] while calling other methods.
+///
+/// This trait is usually implemented by PRNGs, as opposed to OS, hardware,
+/// and periodically-reseeded RNGs, which may fail with IO errors.
+pub trait InfallibleRng: RngCore {}


Commenting just for visibility of the significant addition.
It falls into the same trap as std::iter::TrustedLen in that it promises something about another trait.
I think I'm okay with this, but worth thinking about / getting more input on.

I thought about different alternatives and it looks like it's the simplest option. Plus, we already have the CryptoRng trait.
One potential option is to make error an associated type (and CryptoRng may become associated bool constant). This way we could use Infallible by default. But without trait aliases it would be significantly less ergonomic to work with. Here is a draft of how it could look in future: playground.

Adding associated types + consts to RngCore changes the meaning on every existing usage as a generic bound.
For "crypto grade", I prefer the trait-inheritance model. After all, every CryptoRng is also a valid RNG. Also, can't have CRYPTO_STRONG default to true on all impls.
For infallibility, we'd at least need a bound on that error type: Error: std::error::Error or Error: Into<Something> where Something is a summarized RNG error (NotInitialized, EndOfSequence, IOError, MissingHardwareDevice, ???).

every CryptoRng is also a valid RNG.
Yes, and? I don't see why the associated constant would be at odds with it. "Cryptographically strong" is a binary property of RNG, which can be modeled perfectly by an associated bool constant.
Also, can't have CRYPTO_STRONG default to true on all impls.
Yes, it should've been false in my example.
For infallibility, we'd at least need a bound on that error type
Sure. But I think it's an unimportant detail. Without stabilization of at least trait aliases, this approach is, arguably, not practical anyway.

Yes, and?
Only that this case does work well with the trait inheritance model; not every boolean property does.

newpavlov · 2024-03-22T14:29:35Z

I've cleaned up the formatting changes.

dhardy · 2024-03-22T17:15:28Z

Also, if @ctz doesn't appreciate the design of RngCore, he's not the only one. It's a compromise that I don't think anyone likes. There's a tracker of some suggestions here: #1261.

This PR adds something, yes, but is a more fundamental revision a better option?

dhardy · 2024-03-22T17:25:55Z

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

Implication: we should drop OsRng. Since we no longer use this for seeding, I am not aware of any good reason to keep this. (We already removed ReadRng.)

We can also drop try_fill_bytes and all the stuff about converting between fallible and infallible cases.

Am I missing anything important?

newpavlov · 2024-03-22T18:59:57Z

I am open to a more global rework (e.g. it may be worth to remove BlockRng stuff and replace it with helper macros/functions). But I think it's better to do in a separate PR after merging this one, since it's not given that we will be able to finalize it for v0.9 release cycle.

There is a much simpler alternative to this PR: require that all RngCore impls are infallible (or at least document possible panics).

In my opinion, it's tantamount to sweeping the problem under the rug.

IO-based RNGs such as OsRng or hardware-based RNGs (not an unusual thing with cryptographic applications) can fail as any other IO. It's a simple truth of life. Imagine someone has misconfigured, or forgot to plug-in a HW RNG. It's better for a cryptographic application/library to return a sensible error, instead of unconditionally panicking outright. Even PRNGs may fail in some cases, e.g. ChaCha-based RNGs may return error on detected looping, which may happen in practice in the presence of seeking.

As for OsRng, it can be a preferred way of generating sensitive information such as cryptographic keys, especially considering that ThreadRng does not implement any protection against exposed state. You may say "then use getrandom directly", but it would be less flexible, e.g. we would not be able to use PRNGs for reproducible testing of such methods.

dhardy · 2024-03-23T08:18:30Z

You are right that such questions are beyond the scope of this PR, however we should at least determine whether we actually want trait InfallibleRng before merging this. Right, new issue then.

newpavlov · 2024-04-01T16:40:21Z

Closing in favor of #1424.

newpavlov requested a review from dhardy March 18, 2024 16:54

dhardy reviewed Mar 18, 2024
View reviewed changes

Add rand_core::InfallibleRng marker trait
e567a95

newpavlov force-pushed the infallible_rng branch from f2d07f3 to e567a95Compare March 22, 2024 14:29

dhardy mentioned this pull request Mar 23, 2024
Make RngCore infallible? #1418
Closed

newpavlov closed this Apr 1, 2024

newpavlov deleted the infallible_rng branch April 1, 2024 16:40

newpavlov requested a review from dhardy March 18, 2024 16:54

dhardy reviewed Mar 18, 2024
View reviewed changes

Add rand_core::InfallibleRng marker trait
e567a95

newpavlov force-pushed the infallible_rng branch from f2d07f3 to e567a95Compare March 22, 2024 14:29

dhardy mentioned this pull request Mar 23, 2024
Make RngCore infallible? #1418
Closed

newpavlov closed this Apr 1, 2024

newpavlov deleted the infallible_rng branch April 1, 2024 16:40

newpavlov requested a review from dhardy March 18, 2024 16:54

dhardy reviewed Mar 18, 2024
View reviewed changes

Add rand_core::InfallibleRng marker trait
e567a95

newpavlov force-pushed the infallible_rng branch from f2d07f3 to e567a95Compare March 22, 2024 14:29

dhardy mentioned this pull request Mar 23, 2024
Make RngCore infallible? #1418
Closed

newpavlov closed this Apr 1, 2024

newpavlov deleted the infallible_rng branch April 1, 2024 16:40

newpavlov requested a review from dhardy March 18, 2024 16:54

dhardy reviewed Mar 18, 2024
View reviewed changes

Add rand_core::InfallibleRng marker trait
e567a95

newpavlov force-pushed the infallible_rng branch from f2d07f3 to e567a95Compare March 22, 2024 14:29

dhardy mentioned this pull request Mar 23, 2024
Make RngCore infallible? #1418
Closed

newpavlov closed this Apr 1, 2024

newpavlov deleted the infallible_rng branch April 1, 2024 16:40

newpavlov requested a review from dhardy March 18, 2024 16:54

dhardy reviewed Mar 18, 2024
View reviewed changes

Add rand_core::InfallibleRng marker trait
e567a95

newpavlov force-pushed the infallible_rng branch from f2d07f3 to e567a95Compare March 22, 2024 14:29

dhardy mentioned this pull request Mar 23, 2024
Make RngCore infallible? #1418
Closed

newpavlov closed this Apr 1, 2024

newpavlov deleted the infallible_rng branch April 1, 2024 16:40

newpavlov requested a review from dhardy March 18, 2024 16:54

dhardy reviewed Mar 18, 2024
View reviewed changes

Add rand_core::InfallibleRng marker trait
e567a95

newpavlov force-pushed the infallible_rng branch from f2d07f3 to e567a95Compare March 22, 2024 14:29

dhardy mentioned this pull request Mar 23, 2024
Make RngCore infallible? #1418
Closed

newpavlov closed this Apr 1, 2024

newpavlov deleted the infallible_rng branch April 1, 2024 16:40

newpavlov requested a review from dhardy March 18, 2024 16:54

dhardy reviewed Mar 18, 2024
View reviewed changes

Add rand_core::InfallibleRng marker trait
e567a95

newpavlov force-pushed the infallible_rng branch from f2d07f3 to e567a95Compare March 22, 2024 14:29

dhardy mentioned this pull request Mar 23, 2024
Make RngCore infallible? #1418
Closed

newpavlov closed this Apr 1, 2024

newpavlov deleted the infallible_rng branch April 1, 2024 16:40

newpavlov requested a review from dhardy March 18, 2024 16:54

dhardy reviewed Mar 18, 2024
View reviewed changes

Add rand_core::InfallibleRng marker trait
e567a95

newpavlov force-pushed the infallible_rng branch from f2d07f3 to e567a95Compare March 22, 2024 14:29

dhardy mentioned this pull request Mar 23, 2024
Make RngCore infallible? #1418
Closed

newpavlov closed this Apr 1, 2024

newpavlov deleted the infallible_rng branch April 1, 2024 16:40

Uh oh!

Uh oh!

Conversation

newpavlov commented Mar 18, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

newpavlov commented Mar 18, 2024

Uh oh!

dhardy commented Mar 18, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newpavlovMar 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhardyMar 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newpavlov commented Mar 22, 2024

Uh oh!

dhardy commented Mar 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhardy commented Mar 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

newpavlov commented Mar 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhardy commented Mar 23, 2024

Uh oh!

newpavlov commented Apr 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

newpavlov commented Mar 18, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

newpavlov commented Mar 18, 2024

Uh oh!

dhardy commented Mar 18, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newpavlovMar 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhardyMar 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newpavlov commented Mar 22, 2024

Uh oh!

dhardy commented Mar 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhardy commented Mar 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

newpavlov commented Mar 18, 2024•
edited
Loading

newpavlovMar 22, 2024•
edited
Loading

dhardyMar 22, 2024•
edited
Loading

dhardy commented Mar 22, 2024•
edited
Loading

dhardy commented Mar 22, 2024•
edited
Loading

newpavlov commented Mar 22, 2024•
edited
Loading

newpavlov commented Mar 18, 2024•
edited
Loading

newpavlovMar 22, 2024•
edited
Loading

dhardyMar 22, 2024•
edited
Loading

dhardy commented Mar 22, 2024•
edited
Loading

dhardy commented Mar 22, 2024•
edited
Loading

newpavlov commented Mar 22, 2024•
edited
Loading

newpavlov commented Mar 18, 2024•
edited
Loading

newpavlovMar 22, 2024•
edited
Loading

dhardyMar 22, 2024•
edited
Loading

dhardy commented Mar 22, 2024•
edited
Loading

dhardy commented Mar 22, 2024•
edited
Loading