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 .
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34401.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, theStack, alexanderwiederin, w0xlt |
| Concept ACK | musaHaruna |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Possible places where comparison-specific test macros should replace generic comparisons:
BOOST_CHECK_THROW(SpanWriter(std::span{small}, a, b), std::ios_base::failure); -> Prefer BOOST_CHECK_EXCEPTION(..., std::ios_base::failure, HasReason("SpanWriter::write(): exceeded buffer size")) (or the exact thrown message), instead of only checking the exception type.BOOST_CHECK_THROW(SpanWriter(std::span{small}) << a << b, std::ios_base::failure); -> Prefer BOOST_CHECK_EXCEPTION(..., std::ios_base::failure, HasReason(...)) to assert the precise failure reason/message.<sup>2026-04-14 13:50:26</sup>
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.
<details> <summary>git diff on d14cdea5cf</summary>
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index 525d861827..2fd5a51873 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -1389,15 +1389,11 @@ uint32_t btck_block_header_get_nonce(const btck_BlockHeader* header)
return btck_BlockHeader::get(header).nNonce;
}
-int btck_block_header_to_bytes(const btck_BlockHeader* header, btck_WriteBytes writer, void* user_data)
+void btck_block_header_to_bytes(const btck_BlockHeader* header, unsigned char output[80])
{
- try {
- WriterStream ws{writer, user_data};
- ws << btck_BlockHeader::get(header);
- return 0;
- } catch (...) {
- return -1;
- }
+ DataStream stream{};
+ stream << btck_BlockHeader::get(header);
+ std::memcpy(output, stream.data(), 80);
}
void btck_block_header_destroy(btck_BlockHeader* header)
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index 53491ea69e..4f6ae41c61 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -1769,18 +1769,13 @@ BITCOINKERNEL_API uint32_t BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_header_ge
const btck_BlockHeader* header) BITCOINKERNEL_ARG_NONNULL(1);
/**
- * [@brief](/bitcoin-bitcoin/contributor/brief/) Serializes the btck_BlockHeader through the passed in callback to bytes.
+ * [@brief](/bitcoin-bitcoin/contributor/brief/) Serializes the btck_BlockHeader to bytes.
*
* [@param](/bitcoin-bitcoin/contributor/param/)[in] header Non-null.
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] writer Non-null, callback to a write bytes function.
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] user_data Holds a user-defined opaque structure that will be
- * passed back through the writer callback.
- * [@return](/bitcoin-bitcoin/contributor/return/) 0 on success.
+ * [@param](/bitcoin-bitcoin/contributor/param/)[out] output The serialized block header (80 bytes).
*/
-BITCOINKERNEL_API int btck_block_header_to_bytes(
- const btck_BlockHeader* header,
- btck_WriteBytes writer,
- void* user_data) BITCOINKERNEL_ARG_NONNULL(1, 2);
+BITCOINKERNEL_API void btck_block_header_to_bytes(
+ const btck_BlockHeader* header, unsigned char output[80]) BITCOINKERNEL_ARG_NONNULL(1, 2);
/**
* Destroy the btck_BlockHeader.
diff --git a/src/kernel/bitcoinkernel_wrapper.h b/src/kernel/bitcoinkernel_wrapper.h
index c23ad8ba3b..95afd90480 100644
--- a/src/kernel/bitcoinkernel_wrapper.h
+++ b/src/kernel/bitcoinkernel_wrapper.h
@@ -747,9 +747,11 @@ public:
return btck_block_header_get_nonce(impl());
}
- std::vector<std::byte> ToBytes() const
+ std::array<std::byte, 80> ToBytes() const
{
- return write_bytes(impl(), btck_block_header_to_bytes);
+ std::array<std::byte, 80> header;
+ btck_block_header_to_bytes(impl(), reinterpret_cast<unsigned char*>(header.data()));
+ return header;
}
};
diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index 86c2e25057..79c88c94c7 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -266,7 +266,7 @@ void run_verify_test(
template <typename T>
concept HasToBytes = requires(T t) {
- { t.ToBytes() } -> std::convertible_to<std::vector<std::byte>>;
+ { t.ToBytes() } -> std::convertible_to<std::span<const std::byte>>;
};
template <typename T>
</details>
Makes sense, thanks.
Concept ACK, definitely useful to have.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21541544208/job/62076715283</sub>
<sub>LLM reason (✨ experimental): Linting failed due to include-usage warnings (lint-includes.py) causing the all_python_linters check to fail.</sub>
<details><summary>Hints</summary>
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.
</details>
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:
<details> <summary>git diff on ebc8433637</summary>
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index 2b129f2952..cdac10a05d 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -1393,9 +1393,7 @@ uint32_t btck_block_header_get_nonce(const btck_BlockHeader* header)
void btck_block_header_to_bytes(const btck_BlockHeader* header, unsigned char output[80])
{
- DataStream stream{};
- stream << btck_BlockHeader::get(header);
- std::memcpy(output, stream.data(), 80);
+ SpanWriter{std::span{output, 80}} << btck_BlockHeader::get(header);
}
void btck_block_header_destroy(btck_BlockHeader* header)
diff --git a/src/streams.h b/src/streams.h
index 466084e9fa..3358fa994a 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -77,6 +77,38 @@ private:
size_t nPos;
};
+/** Minimal stream for writing to an existing span of bytes.
+ */
+class SpanWriter
+{
+private:
+ std::span<std::byte> m_data;
+ size_t m_pos{0};
+
+public:
+ explicit SpanWriter(std::span<unsigned char> data) : m_data{std::as_writable_bytes(data)} {}
+ explicit SpanWriter(std::span<std::byte> data) : m_data{data} {}
+ template <typename... Args>
+ SpanWriter(std::span<unsigned char> data, Args&&... args) : SpanWriter{data}
+ {
+ ::SerializeMany(*this, std::forward<Args>(args)...);
+ }
+
+ void write(std::span<const std::byte> src)
+ {
+ assert(m_pos + src.size() <= m_data.size());
+ memcpy(m_data.data() + m_pos, src.data(), src.size());
+ m_pos += src.size();
+ }
+
+ template <typename T>
+ SpanWriter& operator<<(const T& obj)
+ {
+ ::Serialize(*this, obj);
+ return *this;
+ }
+};
+
/** Minimal stream for reading from an existing byte array by std::span.
*/
class SpanReader
diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp
index af75ee987a..29fce0612d 100644
--- a/src/test/streams_tests.cpp
+++ b/src/test/streams_tests.cpp
@@ -207,6 +207,31 @@ BOOST_AUTO_TEST_CASE(streams_vector_writer)
vch.clear();
}
+BOOST_AUTO_TEST_CASE(streams_span_writer)
+{
+ unsigned char a(1);
+ unsigned char b(2);
+ unsigned char bytes[] = {3, 4, 5, 6};
+ std::array<unsigned char, 8> arr{};
+
+ // Test operator<<.
+ SpanWriter writer{arr};
+ writer << a << b;
+ std::array<unsigned char, 8> expected1{{1, 2, 0, 0, 0, 0, 0, 0}};
+ BOOST_CHECK_EQUAL_COLLECTIONS(arr.begin(), arr.end(), expected1.begin(), expected1.end());
+
+ // Use variadic constructor and write to subspan.
+ SpanWriter{std::span{arr}.subspan(2), a, bytes, b};
+ std::array<unsigned char, 8> expected2{{1, 2, 1, 3, 4, 5, 6, 2}};
+ BOOST_CHECK_EQUAL_COLLECTIONS(arr.begin(), arr.end(), expected2.begin(), expected2.end());
+
+ // Test std::byte span constructor.
+ std::array<std::byte, 2> byte_arr{};
+ SpanWriter{std::span{byte_arr}} << a << b;
+ std::array<std::byte, 2> expected3{{std::byte{1}, std::byte{2}}};
+ BOOST_CHECK_EQUAL_COLLECTIONS(byte_arr.begin(), byte_arr.end(), expected3.begin(), expected3.end());
+}
+
BOOST_AUTO_TEST_CASE(streams_vector_reader)
{
std::vector<unsigned char> vch = {1, 255, 3, 4, 5, 6};
</details> [@sedited](/bitcoin-bitcoin/contributor/sedited/) do you have any preferences here?
I think as a defensive mechanism, we should still use try-catch here to ensure exceptions never cross the C API boundary, even though header serialization shouldn't fail under normal circumstances.
Added 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.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/22855674050/job/66295237030</sub>
<sub>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.</sub>
<details><summary>Hints</summary>
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.
</details>
Added try-catch in the serialization part. Also, split the second commit into two and made the commit message more descriptive.
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.
Correct will update that. Also, as discussed in WG call, doc naming convention: always use struct names instead of natural names.
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:
* [@param](/bitcoin-bitcoin/contributor/param/)[out] output Non-null, 80-byte buffer to write the serialized header into.
* [@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/
Improved the commits messages and fixed some nits.
Concept ACK
updated commit messages
Concept ACK
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:
<details> <summary>git diff on 80e98e44b7</summary>
diff --git a/src/streams.h b/src/streams.h
index fcaf835768..d390b712a0 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -140,7 +140,9 @@ public:
void write(std::span<const std::byte> src)
{
- assert(m_pos + src.size() <= m_data.size());
+ if (src.size() > m_data.size() - m_pos) {
+ throw std::ios_base::failure("SpanWriter::write(): exceeded buffer size");
+ }
memcpy(m_data.data() + m_pos, src.data(), src.size());
m_pos += src.size();
}
</details>
We could then also add a test case:
<details> <summary>git diff on 80e98e44b7</summary>
diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp
index ef3b04e2e8..c5ee057d60 100644
--- a/src/test/streams_tests.cpp
+++ b/src/test/streams_tests.cpp
@@ -230,6 +230,10 @@ BOOST_AUTO_TEST_CASE(streams_span_writer)
SpanWriter{std::span{byte_arr}} << a << b;
std::array<std::byte, 2> expected3{std::byte{1}, std::byte{2}};
BOOST_CHECK_EQUAL_COLLECTIONS(byte_arr.begin(), byte_arr.end(), expected3.begin(), expected3.end());
+
+ // Writing past the end throws
+ std::array<unsigned char, 1> small{};
+ BOOST_CHECK_THROW(SpanWriter(small, a, b), std::ios_base::failure);
}
BOOST_AUTO_TEST_CASE(streams_vector_reader)
</details>
Done in f615589f2e9dd41f54c95da6c2f728bbf027d852. Thanks
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.
Done.
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()));
Probably shouldn't ignore errors here? Also, double space before reinterpret_cast
Agree, the BlockHeader construction for the C++ wrapper should probably throw for invalid data and I have also added a check in the 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(
BITCOINKERNEL_API int BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_header_to_bytes(
Done in f798ece7a72aa025c79ad906b2b7ea7e2fdfa885. Thanks
b76bcbd12e2202a5cf5ee5bb7c837c1667f12c7a commit message still mentions CheckHandle, but I think that's no longer in this commit.
Done.
Reviewed 80e98e44b735da2e3b80073042b08014199b0e01 - LGTM once comments are addressed.
Addressed some comments from @stickies-v: #34401 (review) #34401 (review) #34401 (review) #34401 (review) #34401 (review)
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.
<details> <summary>git diff on bb92d5fa65</summary>
diff --git a/src/kernel/bitcoinkernel_wrapper.h b/src/kernel/bitcoinkernel_wrapper.h
index 5a56cbf732..63ca62b0ca 100644
--- a/src/kernel/bitcoinkernel_wrapper.h
+++ b/src/kernel/bitcoinkernel_wrapper.h
@@ -750,11 +750,11 @@ public:
std::array<std::byte, 80> ToBytes() const
{
std::array<std::byte, 80> header;
- auto res = btck_block_header_to_bytes(impl(), reinterpret_cast<unsigned char*>(header.data()));
- if (res == 0) {
- return header;
+ int res{btck_block_header_to_bytes(impl(), reinterpret_cast<unsigned char*>(header.data()))};
+ if (res != 0) {
+ throw std::runtime_error("Failed to serialize block header");
}
- throw std::runtime_error("Failed to serialize block header");
+ return header;
}
};
</details>
Done in c7fec6c91f7a79ba5fc1cf8866fbab5b8da43b26
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");
throw std::ios_base::failure("SpanWriter::write(): exceeded buffer size");
done in fdf49462af08dc99a7317d199c7b60b42317f393
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.
<details> <summary>git diff on bb92d5fa65</summary>
diff --git a/src/streams.h b/src/streams.h
index 78c820e066..b57c443287 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -126,25 +126,24 @@ public:
class SpanWriter
{
private:
- std::span<std::byte> m_data;
- size_t m_pos{0};
+ std::span<std::byte> m_dest;
public:
- explicit SpanWriter(std::span<unsigned char> data) : m_data{std::as_writable_bytes(data)} {}
- explicit SpanWriter(std::span<std::byte> data) : m_data{data} {}
+ explicit SpanWriter(std::span<unsigned char> dest) : m_dest{std::as_writable_bytes(dest)} {}
+ explicit SpanWriter(std::span<std::byte> dest) : m_dest{dest} {}
template <typename... Args>
- SpanWriter(std::span<unsigned char> data, Args&&... args) : SpanWriter{data}
+ SpanWriter(std::span<unsigned char> dest, Args&&... args) : SpanWriter{dest}
{
::SerializeMany(*this, std::forward<Args>(args)...);
}
void write(std::span<const std::byte> src)
{
- if (src.size() > m_data.size() - m_pos) {
- throw std::ios_base::failure("Spanwriter::write(): exceeded buffer size");
+ if (src.size() > m_dest.size()) {
+ throw std::ios_base::failure("SpanWriter::write(): exceeded buffer size");
}
- memcpy(m_data.data() + m_pos, src.data(), src.size());
- m_pos += src.size();
+ memcpy(m_dest.data(), src.data(), src.size());
+ m_dest = m_dest.subspan(src.size());
}
template<typename T>
</details>
If you prefer the current approach, we should probably add a m_pos <= m_data.size() at the end of write().
Alright, this approach looks fine to me.
Done. Thanks.
re-ACK bb92d5fa6576e8ff97c2a3afe38622052ec0b540
Implemented some suggestions by @stickies-v: #34401 (review) #34401 (review)
ACK 10aed76c2965cac777cf500c2c015aa24cc7945e
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.
+ template <typename... Args>
+ SpanWriter(std::span<std::byte> dest, Args&&... args) : SpanWriter{dest}
+ {
+ ::SerializeMany(*this, std::forward<Args>(args)...);
+ }
Test for this:
--- a/src/test/streams_tests.cpp
+++ b/src/test/streams_tests.cpp
@@ -231,6 +231,11 @@ BOOST_AUTO_TEST_CASE(streams_span_writer)
std::array<std::byte, 2> expected3{std::byte{1}, std::byte{2}};
BOOST_CHECK_EQUAL_COLLECTIONS(byte_arr.begin(), byte_arr.end(), expected3.begin(), expected3.end());
+ // Use variadic constructor with std::byte span
+ std::array<std::byte, 2> byte_arr2{};
+ SpanWriter{std::span{byte_arr2}, a, b};
+ BOOST_CHECK_EQUAL_COLLECTIONS(byte_arr2.begin(), byte_arr2.end(), expected3.begin(), expected3.end());
+
// Writing past the end throws
std::array<unsigned char, 1> small{};
BOOST_CHECK_THROW(SpanWriter(small, a, b), std::ios_base::failure);
This is intentional, I think additional ctors should be added when they're necessary (i.e. actually used)?
So should this line 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?
<details> <summary>git diff on b494a9fa11</summary>
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index 5a4bb7d925..b456f41a00 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -1393,7 +1393,7 @@ uint32_t btck_block_header_get_nonce(const btck_BlockHeader* header)
int btck_block_header_to_bytes(const btck_BlockHeader* header, unsigned char output[80])
{
try {
- SpanWriter{std::span{output, 80}} << btck_BlockHeader::get(header);
+ SpanWriter{std::as_writable_bytes(std::span{output, 80})} << btck_BlockHeader::get(header);
return 0;
} catch (...) {
return -1;
diff --git a/src/streams.h b/src/streams.h
index b57c443287..12a2cac2c7 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -129,10 +129,9 @@ private:
std::span<std::byte> m_dest;
public:
- explicit SpanWriter(std::span<unsigned char> dest) : m_dest{std::as_writable_bytes(dest)} {}
explicit SpanWriter(std::span<std::byte> dest) : m_dest{dest} {}
template <typename... Args>
- SpanWriter(std::span<unsigned char> dest, Args&&... args) : SpanWriter{dest}
+ SpanWriter(std::span<std::byte> dest, Args&&... args) : SpanWriter{dest}
{
::SerializeMany(*this, std::forward<Args>(args)...);
}
diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp
index c5ee057d60..d881ae8d35 100644
--- a/src/test/streams_tests.cpp
+++ b/src/test/streams_tests.cpp
@@ -212,28 +212,20 @@ BOOST_AUTO_TEST_CASE(streams_span_writer)
unsigned char a(1);
unsigned char b(2);
unsigned char bytes[] = {3, 4, 5, 6};
- std::array<unsigned char, 8> arr{};
+ std::array<std::byte, 8> arr{};
// Test operator<<
SpanWriter writer{arr};
writer << a << b;
- std::array<unsigned char, 8> expected1{1, 2, 0, 0, 0, 0, 0, 0};
- BOOST_CHECK_EQUAL_COLLECTIONS(arr.begin(), arr.end(), expected1.begin(), expected1.end());
+ BOOST_CHECK_EQUAL(HexStr(arr), "0102000000000000");
// Use variadic constructor and write to subspan.
SpanWriter{std::span{arr}.subspan(2), a, bytes, b};
- std::array<unsigned char, 8> expected2{1, 2, 1, 3, 4, 5, 6, 2};
- BOOST_CHECK_EQUAL_COLLECTIONS(arr.begin(), arr.end(), expected2.begin(), expected2.end());
-
- // Test std::byte span constructor
- std::array<std::byte, 2> byte_arr{};
- SpanWriter{std::span{byte_arr}} << a << b;
- std::array<std::byte, 2> expected3{std::byte{1}, std::byte{2}};
- BOOST_CHECK_EQUAL_COLLECTIONS(byte_arr.begin(), byte_arr.end(), expected3.begin(), expected3.end());
+ BOOST_CHECK_EQUAL(HexStr(arr), "0102010304050602");
// Writing past the end throws
- std::array<unsigned char, 1> small{};
- BOOST_CHECK_THROW(SpanWriter(small, a, b), std::ios_base::failure);
+ std::array<std::byte, 1> small{};
+ BOOST_CHECK_THROW(SpanWriter(std::span{small}, a, b), std::ios_base::failure);
}
BOOST_AUTO_TEST_CASE(streams_vector_reader)
</details>
(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)
int res = btck_block_header_to_bytes(impl(), reinterpret_cast<unsigned char*>(header.data()));
I agree, but when specifying int we should use brace init to prevent narrowing conversion.
Done in 42aaca85de7526d212bb6e54cdc1dc8867716ab7
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.
BOOST_CHECK_THROW(SpanWriter(small) << a << b, std::ios_base::failure);
Done. Thanks.
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
Co-authored-by: stickies-v <stickies-v@protonmail.com>
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>`.
re-ACK 577a3e74c82d942cba24c2b3affda9c777f3c75d
Code-review ACK 577a3e74c82d942cba24c2b3affda9c777f3c75d
ACK 577a3e74c82d942cba24c2b3affda9c777f3c75d