-
Notifications
You must be signed in to change notification settings - Fork 304
Introduce Arbitrary crate and add PublicKey arbitrary impl #826
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
Conversation
e025ffe
to
e68d976
Compare
src/key.rs
Outdated
#[cfg(feature = "global-context")] | ||
impl<'a> Arbitrary<'a> for PublicKey { | ||
fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result<Self> { | ||
Ok(PublicKey::from_secret_key(&SECP256K1, &SecretKey::arbitrary(u)?)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In e68d976:
We can be much faster (at least 100x) than this by choosing 32 random bytes, putting 2 or 3 in front of it, and then attempting to deserialize the resulting 33-byte array as a public key. If we fail, we should retry. On average this will take 2 tries.
If the intention is to use this logic in fuzzers and such then we should definitely do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But concept ACK bringing arbitrary
into this crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can be much faster (at least 100x) than this by choosing 32 random bytes, putting 2 or 3 in front of it, and then attempting to deserialize the resulting 33-byte array as a public key. If we fail, we should retry. On average this will take 2 tries.
Nice! Updated with that logic
Nice. fa8af6e looks great. I'm a liiiitle tempted to try to save on entropy by using a PRNG for retries. But I think that's the wrong approach. Aside from being slow or biased (or both :)) it's not very kind to fuzzers to try to "stretch" their entropy with a PRNG. Since then the fuzzer wastes effort trying to probe the insides of the PRNG instead of the actual code being tested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa8af6e; successfully ran local tests; nice!
…add PublicKey arbitrary impl fa8af6e1273503b6ebb71969a5260dc705ebe305 Introduce Arbitrary crate and add PublicKey impl (Shing Him Ng) Pull request description: I was looking into writing some `Arbitrary` impls in the `bitcoin` crate, and one of the types contained a `PublicKey`. Any interest in adding the arbitrary feature here? ACKs for top commit: apoelstra: ACK fa8af6e1273503b6ebb71969a5260dc705ebe305; successfully ran local tests; nice! Tree-SHA512: cee541d209309e0629e6674523972a67cfc22026975349967fc8eb51b68eda134536956f29b31cfa3e89b04aff47e2544907a52947ff5e9eb53e477361776072
…add PublicKey arbitrary impl fa8af6e1273503b6ebb71969a5260dc705ebe305 Introduce Arbitrary crate and add PublicKey impl (Shing Him Ng) Pull request description: I was looking into writing some `Arbitrary` impls in the `bitcoin` crate, and one of the types contained a `PublicKey`. Any interest in adding the arbitrary feature here? ACKs for top commit: apoelstra: ACK fa8af6e1273503b6ebb71969a5260dc705ebe305; successfully ran local tests; nice! Tree-SHA512: cee541d209309e0629e6674523972a67cfc22026975349967fc8eb51b68eda134536956f29b31cfa3e89b04aff47e2544907a52947ff5e9eb53e477361776072
I was looking into writing some
Arbitrary
impls in thebitcoin
crate, and one of the types contained aPublicKey
. Any interest in adding the arbitrary feature here?