The header does not say whether the bytes pointer can be null when size is zero. It can: btck_witness_stack_get_item_at passes std::vector::data() straight through, and that may be null for an empty element.
An alternative is to guarantee non-null rather than document that it isn't. In other words, route the vector-backed writer calls through a helper that substitutes a valid, never-read pointer when the buffer is empty.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, sedited |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
ACK 76359bee61edb2062493f0ba4ecd47943c359e0a
Documenting this seems like the correct call, given that developers can, and should probably, already check if the size is zero.
390 | @@ -391,6 +391,10 @@ typedef void (*btck_ValidationInterfaceBlockDisconnected)(void* user_data, btck_ 391 | /** 392 | * Function signature for serializing data. 393 | * 394 | + * The bytes pointer may be null when size is zero. Callers that construct a
Documenting how callers should deal with this seems out of scope, would just describe behaviour. Let's also use more complete Doxygen syntax while we're at it.
<details> <summary>git diff on 76359bee61</summary>
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index ba6daf72fd..0ada5576a9 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -391,11 +391,11 @@ typedef void (*btck_ValidationInterfaceBlockDisconnected)(void* user_data, btck_
/**
* Function signature for serializing data.
*
- * The bytes pointer may be null when size is zero. Callers that construct a
- * language-level slice or array view from the arguments must handle that
- * case, as some languages require a non-null pointer even for an empty view.
- *
- * Returns 0 to indicate success.
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] bytes The data to write. Can be null if size is 0.
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] size The number of bytes in the data.
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] userdata Holds the user-defined opaque structure that was given
+ * with the writer.
+ * [@return](/bitcoin-bitcoin/contributor/return/) 0 on success.
*/
typedef int (*btck_WriteBytes)(const void* bytes, size_t size, void* userdata);
</details>
Taken - thanks!
The commit message in 76359be says that this is relevant only to bindings, but I think it also matters to C users, for example when they give bytes to memcpy. I suggest this more general alternative:
kernel: document that btck_WriteBytes may receive a null pointer
btck_witness_stack_get_item_at passes std::vector::data() to the writer,
which can be null for an empty witness item. Document this in the contract,
together with the other btck_WriteBytes parameters.
Taken - thanks!
Approach ACK, documenting seems like the best choice now.
btck_witness_stack_get_item_at passes std::vector::data() to the writer,
which can be null for an empty witness item. Document this in the
contract, together with the other btck_WriteBytes parameters.
ACK b497a71c6630fe37f25620f9f08f1c52f9d4d558
ACK b497a71c6630fe37f25620f9f08f1c52f9d4d558