This adds serialization for btck_BlockHeader API. Also, updated the CheckHandle to compare the byte content instead of size.
The changes here is done in two commits. First commit adds the SpanWriter class and next one moves the block header serialization to SpanWriter. See commit message for more details.
Follow-up to #33822 .
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34401.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | alexanderwiederin |
| Concept ACK | musaHaruna, theStack, w0xlt |
| Stale ACK | stickies-v |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
No conflicts as of last run.
Possible places where comparison-specific test macros should replace generic comparisons:
BOOST_CHECK_THROW(SpanWriter(small, a, b), std::ios_base::failure); -> Prefer BOOST_CHECK_EXCEPTION(SpanWriter(small, a, b), std::ios_base::failure, HasReason("SpanWriter::write(): exceeded buffer size")) (or match the specific message actually thrown) to avoid a generic exception-only check.2026-04-06 19:58:40
1767@@ -1768,6 +1768,20 @@ BITCOINKERNEL_API int32_t BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_header_get
1768 BITCOINKERNEL_API uint32_t BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_header_get_nonce(
1769 const btck_BlockHeader* header) BITCOINKERNEL_ARG_NONNULL(1);
1770
1771+/**
1772+ * @brief Serializes the btck_BlockHeader through the passed in callback to bytes.
For consistency with other serialization function documentation, would be good to add
This is consensus serialization that is also used for the P2P network.
1775+ * @param[in] writer Non-null, callback to a write bytes function.
1776+ * @param[in] user_data Holds a user-defined opaque structure that will be
1777+ * passed back through the writer callback.
1778+ * @return 0 on success.
1779+ */
1780+BITCOINKERNEL_API int btck_block_header_to_bytes(
Headers are fixed 80 byte structures, so I think we can just return a fixed-length buffer instead of using a writer - see e.g. btck_block_hash_to_bytes.
0diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
1index 525d861827..2fd5a51873 100644
2--- a/src/kernel/bitcoinkernel.cpp
3+++ b/src/kernel/bitcoinkernel.cpp
4@@ -1389,15 +1389,11 @@ uint32_t btck_block_header_get_nonce(const btck_BlockHeader* header)
5 return btck_BlockHeader::get(header).nNonce;
6 }
7
8-int btck_block_header_to_bytes(const btck_BlockHeader* header, btck_WriteBytes writer, void* user_data)
9+void btck_block_header_to_bytes(const btck_BlockHeader* header, unsigned char output[80])
10 {
11- try {
12- WriterStream ws{writer, user_data};
13- ws << btck_BlockHeader::get(header);
14- return 0;
15- } catch (...) {
16- return -1;
17- }
18+ DataStream stream{};
19+ stream << btck_BlockHeader::get(header);
20+ std::memcpy(output, stream.data(), 80);
21 }
22
23 void btck_block_header_destroy(btck_BlockHeader* header)
24diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
25index 53491ea69e..4f6ae41c61 100644
26--- a/src/kernel/bitcoinkernel.h
27+++ b/src/kernel/bitcoinkernel.h
28@@ -1769,18 +1769,13 @@ BITCOINKERNEL_API uint32_t BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_header_ge
29 const btck_BlockHeader* header) BITCOINKERNEL_ARG_NONNULL(1);
30
31 /**
32- * [@brief](/bitcoin-bitcoin/contributor/brief/) Serializes the btck_BlockHeader through the passed in callback to bytes.
33+ * [@brief](/bitcoin-bitcoin/contributor/brief/) Serializes the btck_BlockHeader to bytes.
34 *
35 * [@param](/bitcoin-bitcoin/contributor/param/)[in] header Non-null.
36- * [@param](/bitcoin-bitcoin/contributor/param/)[in] writer Non-null, callback to a write bytes function.
37- * [@param](/bitcoin-bitcoin/contributor/param/)[in] user_data Holds a user-defined opaque structure that will be
38- * passed back through the writer callback.
39- * [@return](/bitcoin-bitcoin/contributor/return/) 0 on success.
40+ * [@param](/bitcoin-bitcoin/contributor/param/)[out] output The serialized block header (80 bytes).
41 */
42-BITCOINKERNEL_API int btck_block_header_to_bytes(
43- const btck_BlockHeader* header,
44- btck_WriteBytes writer,
45- void* user_data) BITCOINKERNEL_ARG_NONNULL(1, 2);
46+BITCOINKERNEL_API void btck_block_header_to_bytes(
47+ const btck_BlockHeader* header, unsigned char output[80]) BITCOINKERNEL_ARG_NONNULL(1, 2);
48
49 /**
50 * Destroy the btck_BlockHeader.
51diff --git a/src/kernel/bitcoinkernel_wrapper.h b/src/kernel/bitcoinkernel_wrapper.h
52index c23ad8ba3b..95afd90480 100644
53--- a/src/kernel/bitcoinkernel_wrapper.h
54+++ b/src/kernel/bitcoinkernel_wrapper.h
55@@ -747,9 +747,11 @@ public:
56 return btck_block_header_get_nonce(impl());
57 }
58
59- std::vector<std::byte> ToBytes() const
60+ std::array<std::byte, 80> ToBytes() const
61 {
62- return write_bytes(impl(), btck_block_header_to_bytes);
63+ std::array<std::byte, 80> header;
64+ btck_block_header_to_bytes(impl(), reinterpret_cast<unsigned char*>(header.data()));
65+ return header;
66 }
67 };
68
69diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
70index 86c2e25057..79c88c94c7 100644
71--- a/src/test/kernel/test_kernel.cpp
72+++ b/src/test/kernel/test_kernel.cpp
73@@ -266,7 +266,7 @@ void run_verify_test(
74
75 template <typename T>
76 concept HasToBytes = requires(T t) {
77- { t.ToBytes() } -> std::convertible_to<std::vector<std::byte>>;
78+ { t.ToBytes() } -> std::convertible_to<std::span<const std::byte>>;
79 };
80
81 template <typename T>
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21541544208/job/62076715283
LLM reason (✨ experimental): Linting failed due to include-usage warnings (lint-includes.py) causing the all_python_linters check to fail.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Thanks for review @stickies-v.
1390@@ -1391,6 +1391,13 @@ uint32_t btck_block_header_get_nonce(const btck_BlockHeader* header)
1391 return btck_BlockHeader::get(header).nNonce;
1392 }
1393
1394+void btck_block_header_to_bytes(const btck_BlockHeader* header, unsigned char output[80])
1395+{
1396+ DataStream stream{};
1397+ stream << btck_BlockHeader::get(header);
1398+ std::memcpy(output, stream.data(), 80);
1399+}
Sorry, my previous suggestion wasn’t entirely sound. This function can throw an exception that crosses the C boundary (e.g. if memory allocation fails), so we should better handle this.
Some options are:
WriterStream approach, which adds some unnecessary overheadSpanWriter to avoid allocating memory. I’m not sure about the guarantees we have that serializing the block header will never throw, though, so this might be in addition to Option 1 (but still has the benefit of avoiding allocation)Possible SpanWriter approach:
0diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
1index 2b129f2952..cdac10a05d 100644
2--- a/src/kernel/bitcoinkernel.cpp
3+++ b/src/kernel/bitcoinkernel.cpp
4@@ -1393,9 +1393,7 @@ uint32_t btck_block_header_get_nonce(const btck_BlockHeader* header)
5
6 void btck_block_header_to_bytes(const btck_BlockHeader* header, unsigned char output[80])
7 {
8- DataStream stream{};
9- stream << btck_BlockHeader::get(header);
10- std::memcpy(output, stream.data(), 80);
11+ SpanWriter{std::span{output, 80}} << btck_BlockHeader::get(header);
12 }
13
14 void btck_block_header_destroy(btck_BlockHeader* header)
15diff --git a/src/streams.h b/src/streams.h
16index 466084e9fa..3358fa994a 100644
17--- a/src/streams.h
18+++ b/src/streams.h
19@@ -77,6 +77,38 @@ private:
20 size_t nPos;
21 };
22
23+/** Minimal stream for writing to an existing span of bytes.
24+ */
25+class SpanWriter
26+{
27+private:
28+ std::span<std::byte> m_data;
29+ size_t m_pos{0};
30+
31+public:
32+ explicit SpanWriter(std::span<unsigned char> data) : m_data{std::as_writable_bytes(data)} {}
33+ explicit SpanWriter(std::span<std::byte> data) : m_data{data} {}
34+ template <typename... Args>
35+ SpanWriter(std::span<unsigned char> data, Args&&... args) : SpanWriter{data}
36+ {
37+ ::SerializeMany(*this, std::forward<Args>(args)...);
38+ }
39+
40+ void write(std::span<const std::byte> src)
41+ {
42+ assert(m_pos + src.size() <= m_data.size());
43+ memcpy(m_data.data() + m_pos, src.data(), src.size());
44+ m_pos += src.size();
45+ }
46+
47+ template <typename T>
48+ SpanWriter& operator<<(const T& obj)
49+ {
50+ ::Serialize(*this, obj);
51+ return *this;
52+ }
53+};
54+
55 /** Minimal stream for reading from an existing byte array by std::span.
56 */
57 class SpanReader
58diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp
59index af75ee987a..29fce0612d 100644
60--- a/src/test/streams_tests.cpp
61+++ b/src/test/streams_tests.cpp
62@@ -207,6 +207,31 @@ BOOST_AUTO_TEST_CASE(streams_vector_writer)
63 vch.clear();
64 }
65
66+BOOST_AUTO_TEST_CASE(streams_span_writer)
67+{
68+ unsigned char a(1);
69+ unsigned char b(2);
70+ unsigned char bytes[] = {3, 4, 5, 6};
71+ std::array<unsigned char, 8> arr{};
72+
73+ // Test operator<<.
74+ SpanWriter writer{arr};
75+ writer << a << b;
76+ std::array<unsigned char, 8> expected1{{1, 2, 0, 0, 0, 0, 0, 0}};
77+ BOOST_CHECK_EQUAL_COLLECTIONS(arr.begin(), arr.end(), expected1.begin(), expected1.end());
78+
79+ // Use variadic constructor and write to subspan.
80+ SpanWriter{std::span{arr}.subspan(2), a, bytes, b};
81+ std::array<unsigned char, 8> expected2{{1, 2, 1, 3, 4, 5, 6, 2}};
82+ BOOST_CHECK_EQUAL_COLLECTIONS(arr.begin(), arr.end(), expected2.begin(), expected2.end());
83+
84+ // Test std::byte span constructor.
85+ std::array<std::byte, 2> byte_arr{};
86+ SpanWriter{std::span{byte_arr}} << a << b;
87+ std::array<std::byte, 2> expected3{{std::byte{1}, std::byte{2}}};
88+ BOOST_CHECK_EQUAL_COLLECTIONS(byte_arr.begin(), byte_arr.end(), expected3.begin(), expected3.end());
89+}
90+
91 BOOST_AUTO_TEST_CASE(streams_vector_reader)
92 {
93 std::vector<unsigned char> vch = {1, 255, 3, 4, 5, 6};
SpanWriter approach as suggested by @stickies-v and also split into two commits. First commit adds the SpanWriter class and next one moves the block header serialization to SpanWriter. See commit message for more details. Also updated the PR description.
Concept ACK.
I recommend we wrap the span serialisation in a try/catch, as you suggested earlier, and return an int like the other to_bytes methods.
The commit messages can be more descriptive and I would also split the second commit into two: one for the method and the other for the HasToBytes change.
Concept ACK
Still familiarizing myself with the Kernel API, but still looked at the code through a newbie lens. In 77c36df, the SpanWriter class provides a minimal serialization stream that writes bytes into a pre-allocated memory buffer represented by a std::span. It maintains an internal write position (m_pos) and appends serialized data sequentially into the span.
One design detail I noticed is the bounds check inside write():
assert(m_pos + src.size() <= m_data.size());
Based on my understanding, the use of assert here instead of throwing an exception means that this condition is a programming invariant rather than a recoverable runtime error. The class assumes that callers are responsible for providing a buffer that is large enough for the serialized data.
So in case of an incorrect input, the caller will be the one to handle the error at runtime? IIUC, this is what this comment below is suggesting if such case of incorrect inputs occurs.
I recommend we wrap the span serialisation in a try/catch, as you suggested earlier, and return an
intlike the otherto_bytesmethods.
🚧 At least one of the CI tasks failed.
Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/22855674050/job/66295237030
LLM reason (✨ experimental): CI failure due to a build error: the cmake/gmake compilation of src/kernel/bitcoinkernel.cpp failed with a non-zero exit status.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
1767@@ -1768,6 +1768,16 @@ BITCOINKERNEL_API int32_t BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_header_get
1768 BITCOINKERNEL_API uint32_t BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_header_get_nonce(
1769 const btck_BlockHeader* header) BITCOINKERNEL_ARG_NONNULL(1);
1770
1771+/**
1772+ * @brief Serializes the btck_BlockHeader through the passed in callback to bytes.
I would suggest @brief Serializes the block header to bytes. - or similar.
there is no callback.
1767@@ -1768,6 +1768,16 @@ BITCOINKERNEL_API int32_t BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_header_get
1768 BITCOINKERNEL_API uint32_t BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_header_get_nonce(
1769 const btck_BlockHeader* header) BITCOINKERNEL_ARG_NONNULL(1);
1770
1771+/**
1772+ * @brief Serializes the btck_BlockHeader through the passed in callback to bytes.
1773+ * This is consensus serialization that is also used for the P2P network.
1774+ *
1775+ * @param[in] header Non-null.
1776+ * @param[out] output The serialized block header (80 bytes).
To align with the others:
0 * [@param](/bitcoin-bitcoin/contributor/param/)[out] output Non-null, 80-byte buffer to write the serialized header into.
1 * [@return](/bitcoin-bitcoin/contributor/return/) 0 on success.
Thanks.
I still think we can improve the commit messages. I use this here for guidance: https://cbea.ms/git-commit/
141+ ::SerializeMany(*this, std::forward<Args>(args)...);
142+ }
143+
144+ void write(std::span<const std::byte> src)
145+ {
146+ assert(m_pos + src.size() <= m_data.size());
Throwing is probably more robust than asserting here (which e.g. wouldn’t be handled in btck_block_header_to_bytes), mirroring SpanReader::read. Also (m_pos + src.size() can overflow, so could address both as:
0diff --git a/src/streams.h b/src/streams.h
1index fcaf835768..d390b712a0 100644
2--- a/src/streams.h
3+++ b/src/streams.h
4@@ -140,7 +140,9 @@ public:
5
6 void write(std::span<const std::byte> src)
7 {
8- assert(m_pos + src.size() <= m_data.size());
9+ if (src.size() > m_data.size() - m_pos) {
10+ throw std::ios_base::failure("SpanWriter::write(): exceeded buffer size");
11+ }
12 memcpy(m_data.data() + m_pos, src.data(), src.size());
13 m_pos += src.size();
14 }
We could then also add a test case:
0diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp
1index ef3b04e2e8..c5ee057d60 100644
2--- a/src/test/streams_tests.cpp
3+++ b/src/test/streams_tests.cpp
4@@ -230,6 +230,10 @@ BOOST_AUTO_TEST_CASE(streams_span_writer)
5 SpanWriter{std::span{byte_arr}} << a << b;
6 std::array<std::byte, 2> expected3{std::byte{1}, std::byte{2}};
7 BOOST_CHECK_EQUAL_COLLECTIONS(byte_arr.begin(), byte_arr.end(), expected3.begin(), expected3.end());
8+
9+ // Writing past the end throws
10+ std::array<unsigned char, 1> small{};
11+ BOOST_CHECK_THROW(SpanWriter(small, a, b), std::ios_base::failure);
12 }
13
14 BOOST_AUTO_TEST_CASE(streams_vector_reader)
124 }
125 };
126
127+/** Minimal stream for writing to an existing span of bytes.
128+ */
129+class SpanWriter
In commit d45a84b250ca57d3da1996a999e89d3a0b342201 message:
… instead of using writerstream, which incurs unnecessary overhead through dynamic memory allocations.
This is a conversation about the best block header serialization approach, and unrelated to the SpanWriter commit. I think just the commit title is fine, here.
745@@ -756,6 +746,13 @@ class BlockHeaderApi
746 {
747 return btck_block_header_get_nonce(impl());
748 }
749+
750+ std::array<std::byte, 80> ToBytes() const
751+ {
752+ std::array<std::byte, 80> header;
753+ btck_block_header_to_bytes(impl(), reinterpret_cast<unsigned char*>(header.data()));
reinterpret_cast
ToBytes() method. f798ece7a72aa025c79ad906b2b7ea7e2fdfa885
1774+ *
1775+ * @param[in] header Non-null.
1776+ * @param[out] output The serialized block header (80 bytes).
1777+ * @return 0 on success.
1778+ */
1779+BITCOINKERNEL_API int btck_block_header_to_bytes(
0BITCOINKERNEL_API int BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_header_to_bytes(
CheckHandle, but I think that’s no longer in this commit.
752+ std::array<std::byte, 80> header;
753+ auto res = btck_block_header_to_bytes(impl(), reinterpret_cast<unsigned char*>(header.data()));
754+ if (res == 0) {
755+ return header;
756+ }
757+ throw std::runtime_error("Failed to serialize block header");
nit: stylistic, but we would typically check for the error condition to throw, and then return. Also I think it’s better to specify the int type here and use brace init to prevent narrowing conversion.
0diff --git a/src/kernel/bitcoinkernel_wrapper.h b/src/kernel/bitcoinkernel_wrapper.h
1index 5a56cbf732..63ca62b0ca 100644
2--- a/src/kernel/bitcoinkernel_wrapper.h
3+++ b/src/kernel/bitcoinkernel_wrapper.h
4@@ -750,11 +750,11 @@ public:
5 std::array<std::byte, 80> ToBytes() const
6 {
7 std::array<std::byte, 80> header;
8- auto res = btck_block_header_to_bytes(impl(), reinterpret_cast<unsigned char*>(header.data()));
9- if (res == 0) {
10- return header;
11+ int res{btck_block_header_to_bytes(impl(), reinterpret_cast<unsigned char*>(header.data()))};
12+ if (res != 0) {
13+ throw std::runtime_error("Failed to serialize block header");
14 }
15- throw std::runtime_error("Failed to serialize block header");
16+ return header;
17 }
18 };
19
142+ }
143+
144+ void write(std::span<const std::byte> src)
145+ {
146+ if (src.size() > m_data.size() - m_pos) {
147+ throw std::ios_base::failure("Spanwriter::write(): exceeded buffer size");
0 throw std::ios_base::failure("SpanWriter::write(): exceeded buffer size");
128+ */
129+class SpanWriter
130+{
131+private:
132+ std::span<std::byte> m_data;
133+ size_t m_pos{0};
Sorry, another approach suggestion. Since this class doesn’t allowing seeking/rewind etc, we don’t actually need the position tracking. We can also improve naming a bit, with dest better reflecting that we’re writing into this span that the generic data.
0diff --git a/src/streams.h b/src/streams.h
1index 78c820e066..b57c443287 100644
2--- a/src/streams.h
3+++ b/src/streams.h
4@@ -126,25 +126,24 @@ public:
5 class SpanWriter
6 {
7 private:
8- std::span<std::byte> m_data;
9- size_t m_pos{0};
10+ std::span<std::byte> m_dest;
11
12 public:
13- explicit SpanWriter(std::span<unsigned char> data) : m_data{std::as_writable_bytes(data)} {}
14- explicit SpanWriter(std::span<std::byte> data) : m_data{data} {}
15+ explicit SpanWriter(std::span<unsigned char> dest) : m_dest{std::as_writable_bytes(dest)} {}
16+ explicit SpanWriter(std::span<std::byte> dest) : m_dest{dest} {}
17 template <typename... Args>
18- SpanWriter(std::span<unsigned char> data, Args&&... args) : SpanWriter{data}
19+ SpanWriter(std::span<unsigned char> dest, Args&&... args) : SpanWriter{dest}
20 {
21 ::SerializeMany(*this, std::forward<Args>(args)...);
22 }
23
24 void write(std::span<const std::byte> src)
25 {
26- if (src.size() > m_data.size() - m_pos) {
27- throw std::ios_base::failure("Spanwriter::write(): exceeded buffer size");
28+ if (src.size() > m_dest.size()) {
29+ throw std::ios_base::failure("SpanWriter::write(): exceeded buffer size");
30 }
31- memcpy(m_data.data() + m_pos, src.data(), src.size());
32- m_pos += src.size();
33+ memcpy(m_dest.data(), src.data(), src.size());
34+ m_dest = m_dest.subspan(src.size());
35 }
36
37 template<typename T>
If you prefer the current approach, we should probably add a m_pos <= m_data.size() at the end of write().
Co-authored-by: stickies-v <stickies-v@protonmail.com>
137+ template <typename... Args>
138+ SpanWriter(std::span<unsigned char> dest, Args&&... args) : SpanWriter{dest}
139+ {
140+ ::SerializeMany(*this, std::forward<Args>(args)...);
141+ }
142+
You expose both std::span<unsigned char> and std::span<std::byte> as valid SpanWriter destinations, but only the unsigned char path currently supports the variadic convenience constructor.
0+ template <typename... Args>
1+ SpanWriter(std::span<std::byte> dest, Args&&... args) : SpanWriter{dest}
2+ {
3+ ::SerializeMany(*this, std::forward<Args>(args)...);
4+ }
Test for this:
0--- a/src/test/streams_tests.cpp
1+++ b/src/test/streams_tests.cpp
2@@ -231,6 +231,11 @@ BOOST_AUTO_TEST_CASE(streams_span_writer)
3 std::array<std::byte, 2> expected3{std::byte{1}, std::byte{2}};
4 BOOST_CHECK_EQUAL_COLLECTIONS(byte_arr.begin(), byte_arr.end(), expected3.begin(), expected3.end());
5
6+ // Use variadic constructor with std::byte span
7+ std::array<std::byte, 2> byte_arr2{};
8+ SpanWriter{std::span{byte_arr2}, a, b};
9+ BOOST_CHECK_EQUAL_COLLECTIONS(byte_arr2.begin(), byte_arr2.end(), expected3.begin(), expected3.end());
10+
11 // Writing past the end throws
12 std::array<unsigned char, 1> small{};
13 BOOST_CHECK_THROW(SpanWriter(small, a, b), std::ios_base::failure);
explicit SpanWriter(std::span<std::byte> dest) : m_dest{dest} {} be removed too ?
Yeah, fair point. Seems wrong to move away from std::bytes so in that case perhaps we should just axe the unsigned char ctors instead?
0diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
1index 5a4bb7d925..b456f41a00 100644
2--- a/src/kernel/bitcoinkernel.cpp
3+++ b/src/kernel/bitcoinkernel.cpp
4@@ -1393,7 +1393,7 @@ uint32_t btck_block_header_get_nonce(const btck_BlockHeader* header)
5 int btck_block_header_to_bytes(const btck_BlockHeader* header, unsigned char output[80])
6 {
7 try {
8- SpanWriter{std::span{output, 80}} << btck_BlockHeader::get(header);
9+ SpanWriter{std::as_writable_bytes(std::span{output, 80})} << btck_BlockHeader::get(header);
10 return 0;
11 } catch (...) {
12 return -1;
13diff --git a/src/streams.h b/src/streams.h
14index b57c443287..12a2cac2c7 100644
15--- a/src/streams.h
16+++ b/src/streams.h
17@@ -129,10 +129,9 @@ private:
18 std::span<std::byte> m_dest;
19
20 public:
21- explicit SpanWriter(std::span<unsigned char> dest) : m_dest{std::as_writable_bytes(dest)} {}
22 explicit SpanWriter(std::span<std::byte> dest) : m_dest{dest} {}
23 template <typename... Args>
24- SpanWriter(std::span<unsigned char> dest, Args&&... args) : SpanWriter{dest}
25+ SpanWriter(std::span<std::byte> dest, Args&&... args) : SpanWriter{dest}
26 {
27 ::SerializeMany(*this, std::forward<Args>(args)...);
28 }
29diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp
30index c5ee057d60..d881ae8d35 100644
31--- a/src/test/streams_tests.cpp
32+++ b/src/test/streams_tests.cpp
33@@ -212,28 +212,20 @@ BOOST_AUTO_TEST_CASE(streams_span_writer)
34 unsigned char a(1);
35 unsigned char b(2);
36 unsigned char bytes[] = {3, 4, 5, 6};
37- std::array<unsigned char, 8> arr{};
38+ std::array<std::byte, 8> arr{};
39
40 // Test operator<<
41 SpanWriter writer{arr};
42 writer << a << b;
43- std::array<unsigned char, 8> expected1{1, 2, 0, 0, 0, 0, 0, 0};
44- BOOST_CHECK_EQUAL_COLLECTIONS(arr.begin(), arr.end(), expected1.begin(), expected1.end());
45+ BOOST_CHECK_EQUAL(HexStr(arr), "0102000000000000");
46
47 // Use variadic constructor and write to subspan.
48 SpanWriter{std::span{arr}.subspan(2), a, bytes, b};
49- std::array<unsigned char, 8> expected2{1, 2, 1, 3, 4, 5, 6, 2};
50- BOOST_CHECK_EQUAL_COLLECTIONS(arr.begin(), arr.end(), expected2.begin(), expected2.end());
51-
52- // Test std::byte span constructor
53- std::array<std::byte, 2> byte_arr{};
54- SpanWriter{std::span{byte_arr}} << a << b;
55- std::array<std::byte, 2> expected3{std::byte{1}, std::byte{2}};
56- BOOST_CHECK_EQUAL_COLLECTIONS(byte_arr.begin(), byte_arr.end(), expected3.begin(), expected3.end());
57+ BOOST_CHECK_EQUAL(HexStr(arr), "0102010304050602");
58
59 // Writing past the end throws
60- std::array<unsigned char, 1> small{};
61- BOOST_CHECK_THROW(SpanWriter(small, a, b), std::ios_base::failure);
62+ std::array<std::byte, 1> small{};
63+ BOOST_CHECK_THROW(SpanWriter(std::span{small}, a, b), std::ios_base::failure);
64 }
65
66 BOOST_AUTO_TEST_CASE(streams_vector_reader)
(using HexStr in the tests to minimize verbosity)
745@@ -756,6 +746,16 @@ class BlockHeaderApi
746 {
747 return btck_block_header_get_nonce(impl());
748 }
749+
750+ std::array<std::byte, 80> ToBytes() const
751+ {
752+ std::array<std::byte, 80> header;
753+ auto res = btck_block_header_to_bytes(impl(), reinterpret_cast<unsigned char*>(header.data()));
nit: As suggested here #34401 (review)
0 int res = btck_block_header_to_bytes(impl(), reinterpret_cast<unsigned char*>(header.data()));
int we should use brace init to prevent narrowing conversion.
Add `btck_block_header_to_bytes` serialization method to serialize a
`btck_BlockHeader` into an 80-byte buffer using `SpanWriter` to ensure
zero-allocation serialization.
Add return type check for `ToBytes()` which should be convertible to
`std::span<const std::byte>`.
231+ std::array<std::byte, 2> expected3{std::byte{1}, std::byte{2}};
232+ BOOST_CHECK_EQUAL_COLLECTIONS(byte_arr.begin(), byte_arr.end(), expected3.begin(), expected3.end());
233+
234+ // Writing past the end throws
235+ std::array<unsigned char, 1> small{};
236+ BOOST_CHECK_THROW(SpanWriter(small, a, b), std::ios_base::failure);
We could also add the following to test the << operator.
0BOOST_CHECK_THROW(SpanWriter(small) << a << b, std::ios_base::failure);
SpanWriter is appropriate here to avoid unnecessary heap allocation and ensure exception safety across the C boundary. The method is consistent with the rest of the API.
Non-blocking: worth clarifying whether explicit SpanWriter(std::span<std::byte> dest) : m_dest{dest} {} should be removed.
ACK https://github.com/bitcoin/bitcoin/pull/34401/changes/b494a9fa11edc37fdef3dde8cb1a0dc761bcebae