RFC: Using `unsigned char (*arg)[size]` as a replacement for `unsigned char *argsize` for fixed length array arguments

[josibake]

As part of #1698 (and earlier versions of the PR), it was pointed out that tools like Rust's bindgen can correctly infer fixed length array arguments when the function signature uses (*arg)[size]. This was discussed, along with possible alternatives, here: https://gnusha.org/secp256k1/2024-11-18.log.

---

I was able to confirm that the only (*arg)[size] works with bindgen here, and decided to see what this would look like for the silent payments module and downstream consumers in josibake@5a10880 and josibake/bitcoin@5835d98. The patches are smaller than I expected and I was happy to see everything Just Worked ™️.

I'm opening this issue for general discussion since this would be a breaking change from the current API. This means we either:

1. Only use this in convention in the silent payments module
2. Use this convention in the silent payments module and new modules going forward
3. Convert the rest of the API to use this convention

I do think having compile time errors for incorrectly sized arguments is quite nice and helps eliminate a whole class of errors, e.g., mixing up argument order and passing a secret key 32 byte array instead of a smallest outpoint 36 byte array, which is why I'm inclined to use this convention in the silent payments module. However, I'd like to hear the maintainers thoughts on having modules with different conventions in their respective API's, or the longer term appetite for converting the whole library to use the new convention.

[real-or-random]

I like this. I help eliminate a whole class of errors, and so this matches the philosophy behind the library. It's uncommon, but we do other uncommon stuff for extra assurance...

I'd love to have this for the entire library, but it will break everything.... I'm a huge fan of consistency, but in this case, I could live with having it in just the silentpayments module.

To understand the entire space of options: How ugly would it be to have this as an optional #ifdef? Could we "just" have two variants of all the declarations in the header? Or would we also need to have ifdefs in the implementation?

Instead of having two variants guarded by an #ifdef we could probably also write some really ugly preprocessor code, but I don't think it's better. At least not in the header because then humans won't be able to parse the declaration anymore...

[josibake]

It's uncommon

I assume you are using uncommon here to mean "the class of errors that this protects against are uncommon errors."
EDIT: uncommon in the sense that using unsigned char (*arg)[size] over unsigned char *argsize is indeed uncommon in C-land, which is an excellent callout.

Anecdotally, having this would have saved me a lot of time and effort when using this module in Bitcoin Core. More specifically, the hardest to chase down bugs were always due to incorrect argument ordering (e.g., using an unsigned char array for the smallest_outpoint, that wasn't actually a 36 byte smallest outpoint), or using what I thought was a valid pointer to a fixed length array, but was in fact just a pointer to.. something.

In general, I think anything we can do to protect against "garbage" arguments being seen as valid arguments at compile time goes a long way towards saving users of the library and helps protect against particularly nasty bugs where the function succeeds, but the resulting output is garbage, effectively burning the funds by making them unfindable by the recipient.

I'm a huge fan of consistency, but in this case, I could live with having it in just the silentpayments module.

Inconsistency with the rest of the library is the biggest drawback, imo. Perhaps a sufficient mitigation is require the new convention for all new modules, and then old modules can be converted one at a time, or converted to use something backwards compatible, like the #ifdef solution you mention below.

How ugly would it be to have this as an optional #ifdef

Big fan of this, as it would allow us to convert existing modules to opt-in, and perhaps eventually deprecate the old method? Using josibake@5a10880 as a concrete example, the majority of changes were in the header, with only a few lines in the implementation needing to be changed. When something in the implementation needed to be changed, it was always due to an argument being passed through from the top-level function to some lower level function and needing either a & or * prepended to the function argument, of which some I think could even be eliminated by being a bit more clever. I'll revisit josibake@5a10880 and try the ifdef approach to see how it looks.

If there is conceptual buy-in on the approach, I'd be happy to sketch out a more general approach for converting the rest of the library. My intuition is in cases where the implementation would need to change, we can come up with some helper functions that encapsulate the ifdef logic and then just wrap lower level function calls where needed.

[theStack]

I was able to confirm that the only (*arg)[size] works with bindgen here, and decided to see what this would look like for the silent payments module and downstream consumers in josibake@5a10880 and josibake/bitcoin@5835d98. The patches are smaller than I expected and I was happy to see everything Just Worked ™️.

To add another language perspective, I can confirm that this notation is picked by Zig as well. Based on a simple example where the silent payments sending function is called, I modified the smallest outpoint array size from 36 and 35 and saw that compilation fails when using your modified branch (josibake@5a10880):

$ zig build
.....
main.zig:97:9: error: expected type '[*c]const [36]u8', found '*[35]u8'
        &outpoint_smallest, null, 0, &seckey_ptrs, 1);
        ^~~~~~~~~~~~~~~~~~

Given that Zig offers direct C interoperability, this is probably not so much of a surprise, but I thought it's nice to verify anyways.