Add arbitrary impl for types used in PSBTs

[shinghim]

No description provided.

[apoelstra]

concept NACK these ones -- we aren't allowed to put arbitrary data into these structures. All the C library guarantees us is that the size is OK to assume. In particular, this will produce invalid x-only public keys which will cause aborts if used with the rest of the library.

[shinghim]

Oh i see. Would it be acceptable to create random SecretKey from the doc example:

/// let secp = Secp256k1::new();
/// let secret_key = SecretKey::new(&mut rand::rng());

and use that to generate the arbitrary types in secp256k1 and leave the ones in ffi alone?

[apoelstra]

@shinghim yeah, secret keys in particular I think you can go ahead and produce using Arbitrary. The upstream C library just uses unsigned char* for these.

For the other FFI types, I think you can do it, but you need to use a similar strategy to the main crate, where you generate byte arrays then use the (unsafe) decoding functions to produce the actual objects.

[shinghim]

Updated to derive XOnlyPublicKey from an arbitrary PublicKey and added an impl for SecretKey. I removed the impl for Signature since I didn't think there would be any use for an arbitrary one. I think it might more make sense to generate an arbitrary SecretKey and use that to create an "arbitrary" Signature

[apoelstra]

In 40f073b:

Sorry to keep iterating on this, but could you:

• Derive XOnlyPublicKey directly by attempting to parse a [u8; 32]
• Derive PublicKey by deriving a Parity and XOnlyPublicKey and using those to construct a PublicKey.

Currently you have the opposite -- to get a XOnlyPublicKey you construct a PublicKey, which consumes a byte to determine parity, which you then just forget.

[shinghim]

No worries, I appreciate the feedback and the review. Updated to create the PublicKey with an XOnlyPublicKey and Parity

[apoelstra]

Can you run cargo +nightly --all --check on this? I see one line that's not formatted.

     fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result<Self> {
         match bool::arbitrary(u)? {
             true => Ok(Parity::Even),
-            false => Ok(Parity::Odd)
+            false => Ok(Parity::Odd),
         }
     }
 }

[apoelstra]

Unsure why CI is not complaining about this..

[shinghim]

Updated after running cargo fmt --all (I think you meant fmt instead of check). The missing comma you mentioned was the only thing it added

[tcharding]

ACK 02ac1f2

[tcharding]

Is there any chance these loops spin forever making debugging fuzzing harder?

[shinghim]

Is there any chance these loops spin forever making debugging fuzzing harder?

I don't think so since we're checking the amount of bytes left each time

[tcharding]

Ah sweet, I misread that check as checking the fill buffer length. Thanks man.

[apoelstra]

Heh, interesting. I considered nitting the check with "eh, who cares if it 0-fills" but actually, since all-zeroes is not a valid x coordinate, the zero-filling would've caused an infinite loop! Good save.

[apoelstra]

ACK 02ac1f2; successfully ran local tests; nice!!