Skip to content

kernel: add serialization method for btck_BlockHeader API#34401

Open
yuvicc wants to merge 3 commits intobitcoin:masterfrom
yuvicc:2026-11-followup_33822
Open

kernel: add serialization method for btck_BlockHeader API#34401
yuvicc wants to merge 3 commits intobitcoin:masterfrom
yuvicc:2026-11-followup_33822

Conversation

@yuvicc
Copy link
Contributor

@yuvicc yuvicc commented Jan 25, 2026

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 .

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 25, 2026

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34401.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK stickies-v, alexanderwiederin, musaHaruna

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

  • passed in -> passed-in (in "through the passed in callback to bytes.") [compound modifier should be hyphenated for clarity]
  • through the passed in callback to bytes. -> into bytes. (rephrase to "Serializes the btck_BlockHeader into bytes.") [current phrasing references a "callback" that doesn't exist in the signature and "to bytes" is awkward; "into bytes" is clearer]

2026-03-09 13:37:47

@yuvicc yuvicc changed the title kernel: add serialization for btck_BlockHeader API kernel: add serialization method for btck_BlockHeader API Jan 25, 2026
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, definitely useful to have.

@yuvicc yuvicc force-pushed the 2026-11-followup_33822 branch from d14cdea to 98112c4 Compare January 31, 2026 08:14
@DrahtBot
Copy link
Contributor

🚧 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.

Hints

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.

@yuvicc yuvicc force-pushed the 2026-11-followup_33822 branch from 98112c4 to ebc8433 Compare January 31, 2026 14:14
@yuvicc
Copy link
Contributor Author

yuvicc commented Jan 31, 2026

Thanks for review @stickies-v.

  • return fixed length buffer instead of writer comment
  • Addressed nit

Comment on lines +1396 to +1399
DataStream stream{};
stream << btck_BlockHeader::get(header);
std::memcpy(output, stream.data(), 80);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Wrapping this in a try/catch and returning an int status code
  2. Reverting to the WriterStream approach, which adds some unnecessary overhead
  3. Implement a SpanWriter 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:

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@yuvicc
Copy link
Contributor Author

yuvicc commented Feb 21, 2026

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.

@yuvicc yuvicc requested a review from stickies-v February 21, 2026 16:03
@github-project-automation github-project-automation bot moved this to Architecture & Performance in The libbitcoinkernel Project Mar 3, 2026
@sedited sedited moved this from Architecture & Performance to API Development in The libbitcoinkernel Project Mar 3, 2026
Copy link

@alexanderwiederin alexanderwiederin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@alexanderwiederin alexanderwiederin moved this from API Development to Architecture & Performance in The libbitcoinkernel Project Mar 4, 2026
@alexanderwiederin alexanderwiederin moved this from Architecture & Performance to API Development in The libbitcoinkernel Project Mar 4, 2026
Copy link
Contributor

@musaHaruna musaHaruna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 int like the other to_bytes methods.

@yuvicc yuvicc force-pushed the 2026-11-followup_33822 branch from a50cafa to 76dc492 Compare March 9, 2026 13:26
yuvicc and others added 3 commits March 9, 2026 18:59
…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>`
@yuvicc yuvicc force-pushed the 2026-11-followup_33822 branch from 76dc492 to 0d27b03 Compare March 9, 2026 13:37
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 9, 2026

🚧 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.

Hints

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.

@DrahtBot DrahtBot removed the CI failed label Mar 9, 2026
@yuvicc
Copy link
Contributor Author

yuvicc commented Mar 9, 2026

Added try-catch in the serialization part. Also, split the second commit into two and made the commit message more descriptive.

Copy link

@alexanderwiederin alexanderwiederin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest @brief Serializes the block header to bytes. - or similar.

there is no callback.

Comment on lines +1775 to +1776
* @param[in] header Non-null.
* @param[out] output The serialized block header (80 bytes).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To align with the others:

 * @param[out] output   Non-null, 80-byte buffer to write the serialized header into.
 * @return              0 on success.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: API Development

Development

Successfully merging this pull request may close these issues.

5 participants