silentpayments: NULL requirement for empty key arrays (inconsistency between API docs and ARG_CHECKs) #1930

In the silentpayments API, the following functions take two lists of keys as input (each represented as "pointer to array of pointers" and "size" parameters, i.e. four parameters in total):

• _silentpayments_sender_create_outputs (secret keys: parameters keypairs/n_keypairs, seckeys/n_seckeys)
• _silentpayments_recipient_prevouts_summary_create (public keys: xonly_pubkeys/n_xonly_pubkeys, pubkeys/n_pubkeys)

The public header docs state that the "pointer to array of pointers" parameters each "can be NULL" if no such inputs are provided. However, the ARG_CHECKs currently are more strict and require the pointer parameter to be NULL if the size is zero, i.e. "must be NULL" would be more appropriate.

One obvious way to fix this inconsistency is to just adapt the docs to match the code accordingly, e.g.: https://github.com/theStack/secp256k1/commit/1f0a8bfe9bab4e4a268757a66e0f4ee8c1d601f0

Alternatively, I wonder if dropping this "must be NULL" requirement would also be an option (which AFAICT would be backwards-compatible) , as I think enforcing it doesn't have much value and is in the end just making things slightly more inconvenient for the user, forcing them to use conditional constructs like keys_size == 0 ? NULL : keys_ptrs for the call-sites. This would be a bit of a larger patch as both the ARG_CHECKs within the functions and the tests have to be adapted though, see https://github.com/bitcoin-core/secp256k1/compare/master...theStack:secp256k1:sp-keylists-drop-NULL-requirement.

Noticed while reviewing Bitcoin Core [PR #35301](https://github.com/bitcoin/bitcoin/pull/35301), where the requirement is currently not met, even though the CI is green (background: the C++ standard doesn't guarantee that .data() on an empty std::vector instance returns nullptr, see https://en.cppreference.com/cpp/container/vector/data: "If size() is 0, data() may or may not return a null pointer.").

Alternatively, I wonder if dropping this "must be NULL" requirement would also be an option (which AFAICT would be backwards-compatible) , as I think enforcing it doesn't have much value and is in the end just making things slightly more inconvenient for the user

Concept ACK

If this is inconvenient for Bitcoin Core, let's change it. (And this clearly shows that it will be inconvenient for any C++ caller.)

FTR, this is also the case for rust-secp256k1, in the ffi counterpart of _silentpayments_sender_create_outputs I've the following:

let (ffi_seckeys, n_seckeys) = if ffi_seckeys_ptrs.is_empty() {
  (core::ptr::null::<*const *const u8>() as *const *const u8, 0_usize)
} else {
  (ffi_seckeys_ptrs.as_c_ptr(), ffi_seckeys_ptrs.len())
};