DifferenceFormatter fuzz target and invariant check
#33252
Adds a fuzz test for the DifferenceFormatter (used in BlockTransactionsRequest, BIP 152). The DifferenceFormatter class implements differential encoding for compact block transactions (BIP 152). This PR ensures that its strictly-monotonic property is maintained. It complements the tests in blocktransactionsrequest_deserialize.
Additionally, there’s an added invariant check after GETBLOCKTXN deserialization in net_processing.cpp.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33252.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/48785229557
LLM reason (✨ experimental): The CI failure is caused by a lint check error related to missing a trailing newline in ‘src/test/fuzz/difference_formatter.cpp’.
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.
@frankomosh Is this LLM generated?
edit: I assumed yes, based on #33038 (comment)
Thanks, but the value of purely LLM generated “vibe” pull requests is limited, because:
Also, specifically here:
src/test/fuzz/CMakeLists.txt isn’t sortedsrc/test/fuzz/difference_formatter.cpp is missing the copyright header (I guess the llm was trained on removing copyright headers)FUZZ_TARGET(difference_formatter) doesn’t add any new code coverage (this point was also already explained to you in your previous attempt at adding a fuzz target)./src/test/fuzz/deserialize.cpp framework.So I’ll close this for now. My recommendation for the future would be to create your first pull request fully by yourself. This will hopefully ensure that you understand the changes yourself and can reply to feedback.
( Generally, I think it is fine to use LLMs, if the author would have written the same code almost exactly the same way without the LLM, and also fully understands it, and also can claim copyright on it. But this is probably a meta discussion. )
4134@@ -4135,6 +4135,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4135 if (msg_type == NetMsgType::GETBLOCKTXN) {
4136 BlockTransactionsRequest req;
4137 vRecv >> req;
4138+ // Verify differential encoding invariant: indexes must be strictly increasing
4139+ // DifferenceFormatter should guarantee this property during deserialization
4140+ for (size_t i = 1; i < req.indexes.size(); ++i) {
4141+ assert(req.indexes[i] > req.indexes[i-1]);
4142+ }
132@@ -133,6 +133,7 @@ add_executable(fuzz
133 validation_load_mempool.cpp
134 vecdeque.cpp
135 versionbits.cpp
136+ difference_formatter.cpp
0@@ -0,0 +1,57 @@
1+#include <blockencodings.h>
2+#include <streams.h>
3+#include <test/fuzz/FuzzedDataProvider.h>
4+#include <test/fuzz/fuzz.h>
5+
6+#include <vector>
7+
8+namespace {
9+struct TestDifferenceFormatterType {
BlockTransactionsRequest?
BlockTransactionsRequest and provide a dummy uint256 in fuzz code (filled into the bytes)
It is less code this way:
0diff --git a/src/test/fuzz/difference_formatter.cpp b/src/test/fuzz/difference_formatter.cpp
1index 49e859ffdc..38b54352c1 100644
2--- a/src/test/fuzz/difference_formatter.cpp
3+++ b/src/test/fuzz/difference_formatter.cpp
4@@ -4,25 +4,13 @@
5
6 #include <blockencodings.h>
7 #include <streams.h>
8-#include <test/fuzz/FuzzedDataProvider.h>
9+#include <random.h>
10 #include <test/fuzz/fuzz.h>
11
12 #include <vector>
13
14 namespace {
15-// Test struct for VectorFormatter<DifferenceFormatter>
16-// Used in BlockTransactionsRequest
17-struct TestDifferenceFormatterType {
18- std::vector<uint16_t> indexes;
19-
20- SERIALIZE_METHODS(TestDifferenceFormatterType, obj) {
21- READWRITE(Using<VectorFormatter<DifferenceFormatter>>(obj.indexes));
22- }
23-};
24-
25 void VerifyDifferenceFormatterInvariants(const std::vector<uint16_t>& indexes) {
26- if (indexes.empty()) return;
27-
28 // Core invariant: strictly monotonic increasing (no duplicates allowed)
29 for (size_t i = 1; i < indexes.size(); ++i) {
30 assert(indexes[i] > indexes[i-1]); // Must be STRICTLY greater
31@@ -32,15 +20,15 @@ void VerifyDifferenceFormatterInvariants(const std::vector<uint16_t>& indexes) {
32
33 FUZZ_TARGET(difference_formatter)
34 {
35- FuzzedDataProvider fdp{buffer.data(), buffer.size()};
36-
37- auto random_bytes = fdp.ConsumeRemainingBytes<uint8_t>();
38- DataStream ss{random_bytes};
39+ const auto block_hash = InsecureRandomContext{{}}.rand256();
40+ DataStream ss{};
41+ ss << block_hash << std::span{buffer};
42
43 // Test deserialization
44 try {
45- TestDifferenceFormatterType test_container;
46+ BlockTransactionsRequest test_container;
47 ss >> test_container;
48+ assert(test_container.blockhash==block_hash);
49
50 // If deserialization succeeded, verify the DifferenceFormatter invariants
51 VerifyDifferenceFormatterInvariants(test_container.indexes);
52@@ -48,5 +36,4 @@ FUZZ_TARGET(difference_formatter)
53 } catch (const std::ios_base::failure&) {
54 // Expected for malformed input
55 }
56-
57 }
Other changes include:
indexes.empty() check
31+ // Arbitrary stream data
32+ DataStream ss{};
33+ auto random_bytes = fdp.ConsumeRemainingBytes<uint8_t>();
34+ for (uint8_t byte : random_bytes) {
35+ ss << byte;
36+ }
DataStream constructor
Thanks, but the value of purely LLM generated “vibe” pull requests is limited, because:
- The “author” does not understand the changes and can not reply to code review feedback.
- There are more than 300 pull requests (most written by real humans) waiting for review.
- Incentive to review purely LLM generated changes is limited, because the feedback likely does not help to train a real human planning to contribute in the future, but is likely only passed on to an LLM api.
Also, specifically here:
- The added P2P assert is not fuzz-only related (it will be run in production as well)
src/test/fuzz/CMakeLists.txtisn’t sortedsrc/test/fuzz/difference_formatter.cppis missing the copyright header (I guess the llm was trained on removing copyright headers)- The fuzz target
FUZZ_TARGET(difference_formatter)doesn’t add any new code coverage (this point was also already explained to you in your previous attempt at adding a fuzz target)- For a trivial serialization roundrip, you can just use the existing
./src/test/fuzz/deserialize.cppframework.- The CI (lint) doesn’t pass.
So I’ll close this for now. My recommendation for the future would be to create your first pull request fully by yourself. This will hopefully ensure that you understand the changes yourself and can reply to feedback.
( Generally, I think it is fine to use LLMs, if the author would have written the same code almost exactly the same way without the LLM, and also fully understands it, and also can claim copyright on it. But this is probably a meta discussion. )
I will work to resolve the issues you raised as soon as I can please
133@@ -133,7 +134,6 @@ add_executable(fuzz
134 validation_load_mempool.cpp
135 vecdeque.cpp
136 versionbits.cpp
137- difference_formatter.cpp
fuzz/difference_formatter.cpp to directly use BlockTransactionsRequest
4134@@ -4135,6 +4135,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4135 if (msg_type == NetMsgType::GETBLOCKTXN) {
4136 BlockTransactionsRequest req;
4137 vRecv >> req;
4138+ // Verify differential encoding invariant: indexes must be strictly increasing
4139+ // DifferenceFormatter should guarantee this property during deserialization
4140+ for (size_t i = 1; i < req.indexes.size(); ++i) {
4141+ assert(req.indexes[i] > req.indexes[i-1]);
0 Assume(req.indexes[i] > req.indexes[i-1]);
Just so the assertion isn’t active in release builds
12+namespace {
13+void VerifyDifferenceFormatterInvariants(const std::vector<uint16_t>& indexes) {
14+ // Core invariant: strictly monotonic increasing (no duplicates allowed)
15+ for (size_t i = 1; i < indexes.size(); ++i) {
16+ assert(indexes[i] > indexes[i-1]); // Must be STRICTLY greater
17+ }
29+ BlockTransactionsRequest test_container;
30+ ss >> test_container;
31+ assert(test_container.blockhash == block_hash);
32+
33+ // If deserialization succeeded, verify the DifferenceFormatter invariants
34+ VerifyDifferenceFormatterInvariants(test_container.indexes);
Add fuzz test to verify that arbitrary input successfully deserialized
by DifferenceFormatter will maintain the sorted-without-duplicates invariant.
7a6b3b4ebfc04f9fa4520e3caaeeae9dfe0d8c5f still mentions assert in the commit message but it’s now using Assume.
Otherwise this looks good to me.
Adds Assume() check in net_processing after deserialization and validate DifferenceFormatter Class invariant.
tACK 65a10fc3c52ea09a4794345bcf607dff908c783a
I modified the fuzz test and checked that the added Assume in net_processing can’t be hit.
4134@@ -4135,6 +4135,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4135 if (msg_type == NetMsgType::GETBLOCKTXN) {
4136 BlockTransactionsRequest req;
4137 vRecv >> req;
4138+ // Verify differential encoding invariant: indexes must be strictly increasing
4139+ // DifferenceFormatter should guarantee this property during deserialization
4140+ for (size_t i = 1; i < req.indexes.size(); ++i) {
4141+ Assume(req.indexes[i] > req.indexes[i-1]);
4142+ }
In 65a10fc3c52ea09a4794345bcf607dff908c783a “p2p: add assertion for BlockTransactionsRequest indexes”
Can checking this be done in deserialization directly, rather than iterating all of the indexes again? Since this is part of compact block relay, we want to avoid doing as much unnecessary work as possible.
Thanks for the review @achow101
I agree it could be done directly in deserialization. The reason it’s in net_processing is that it acts as a boundary for untrusted peer input, so having the invariant explicit here serves as a cheap defense-in-depth check (similar to other message level sanity checks here). The extra loop is only executed after we already know the message is GETBLOCKTXN and after successful deserialization, so it’s a one-time O(n) pass per compact block request. Given the bounded size of these requests, I think the cost is negligible compared to the overall block relay path?