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

issue josibake openend this issue on July 22, 2025
  1. josibake commented at 11:09 am on July 22, 2025: member

    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 https://github.com/josibake/secp256k1/commit/5a1088066b2ce5e2e77b4e4bc190575d1171c374 and https://github.com/josibake/bitcoin/commit/5835d987477fc8eae391e4d1cc5033d921925ea1. 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.

  2. real-or-random commented at 8:16 am on July 23, 2025: contributor

    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…

  3. real-or-random added the label assurance on Jul 23, 2025
  4. josibake commented at 11:33 am on July 23, 2025: member

    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 https://github.com/josibake/secp256k1/commit/5a1088066b2ce5e2e77b4e4bc190575d1171c374 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 https://github.com/josibake/secp256k1/commit/5a1088066b2ce5e2e77b4e4bc190575d1171c374 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.

  5. theStack commented at 12:59 pm on July 23, 2025: contributor

    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):

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

    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.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-08-30 14:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me