kernel: add serialization method for btck_BlockHeader API#34401
kernel: add serialization method for btck_BlockHeader API#34401yuvicc wants to merge 3 commits intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34401. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsNo conflicts as of last run. LLM Linter (✨ experimental)Possible typos and grammar issues:
2026-03-09 13:37:47 |
stickies-v
left a comment
There was a problem hiding this comment.
Concept ACK, definitely useful to have.
d14cdea to
98112c4
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
98112c4 to
ebc8433
Compare
|
Thanks for review @stickies-v. |
src/kernel/bitcoinkernel.cpp
Outdated
| DataStream stream{}; | ||
| stream << btck_BlockHeader::get(header); | ||
| std::memcpy(output, stream.data(), 80); | ||
| } |
There was a problem hiding this comment.
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:
- Wrapping this in a try/catch and returning an int status code
- Reverting to the
WriterStreamapproach, which adds some unnecessary overhead - Implement a
SpanWriterto 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:
git diff on ebc8433
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};
@sedited do you have any preferences here?
There was a problem hiding this comment.
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.
ebc8433 to
a50cafa
Compare
|
Added |
There was a problem hiding this comment.
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.
musaHaruna
left a comment
There was a problem hiding this comment.
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.
a50cafa to
76dc492
Compare
…inimal stream class for writing directly to a pre-allocated span of bytes instead of using `writerstream`, which incurs unnecessary overhead through dynamic memory allocations. Co-authored-by: stickies-v <stickies-v@protonmail.com>
… to serialize a `btck_BlockHeader` into an 80-byte buffer using `SpanWriter` to ensure zero-allocation serialization. Also fixes `CheckHandle` to compare exact byte content instead of size comparisons.
…s `ToBytes()` to return a type convertible to `std::span<const std::byte>`
76dc492 to
0d27b03
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
Added try-catch in the serialization part. Also, split the second commit into two and made the commit message more descriptive. |
alexanderwiederin
left a comment
There was a problem hiding this comment.
Thanks.
I still think we can improve the commit messages. I use this here for guidance: https://cbea.ms/git-commit/
| const btck_BlockHeader* header) BITCOINKERNEL_ARG_NONNULL(1); | ||
|
|
||
| /** | ||
| * @brief Serializes the btck_BlockHeader through the passed in callback to bytes. |
There was a problem hiding this comment.
I would suggest @brief Serializes the block header to bytes. - or similar.
there is no callback.
| * @param[in] header Non-null. | ||
| * @param[out] output The serialized block header (80 bytes). |
There was a problem hiding this comment.
To align with the others:
* @param[out] output Non-null, 80-byte buffer to write the serialized header into.
* @return 0 on success.
This adds serialization for
btck_BlockHeaderAPI. Also, updated theCheckHandleto compare the byte content instead of size.The changes here is done in two commits. First commit adds the
SpanWriterclass and next one moves the block header serialization toSpanWriter. See commit message for more details.Follow-up to #33822 .