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.
<!--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/33252.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/48785229557</sub>
<sub>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'.</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>
@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. )
Re-opening, based on request by @dergoegge
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 | + }
in 358ad3f7df44e2715b69e80fc973e4f55517a67c: This is not around "validation", the commit title should start with "p2p" and the pull request title as well. (This code is run in production, not only during fuzz tests)
Ok. I'll change this
Stiil needs to update the PR title.
@brunoerg changed. Please let me know if you’d still like to see any wording tweaks?
132 | @@ -133,6 +133,7 @@ add_executable(fuzz 133 | validation_load_mempool.cpp 134 | vecdeque.cpp 135 | versionbits.cpp 136 | + difference_formatter.cpp
nit: pls sort
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 {
nit: should have a comment to be based on BlockTransactionsRequest?
an alternative would be to directly use BlockTransactionsRequest and provide a dummy uint256 in fuzz code (filled into the bytes)
thought it better to use this format because it is more focused toward testing the differential encoding logic. perhaps a comment(as you suggested) would do?
It is less code this way:
diff --git a/src/test/fuzz/difference_formatter.cpp b/src/test/fuzz/difference_formatter.cpp
index 49e859ffdc..38b54352c1 100644
--- a/src/test/fuzz/difference_formatter.cpp
+++ b/src/test/fuzz/difference_formatter.cpp
@@ -4,25 +4,13 @@
#include <blockencodings.h>
#include <streams.h>
-#include <test/fuzz/FuzzedDataProvider.h>
+#include <random.h>
#include <test/fuzz/fuzz.h>
#include <vector>
namespace {
-// Test struct for VectorFormatter<DifferenceFormatter>
-// Used in BlockTransactionsRequest
-struct TestDifferenceFormatterType {
- std::vector<uint16_t> indexes;
-
- SERIALIZE_METHODS(TestDifferenceFormatterType, obj) {
- READWRITE(Using<VectorFormatter<DifferenceFormatter>>(obj.indexes));
- }
-};
-
void VerifyDifferenceFormatterInvariants(const std::vector<uint16_t>& indexes) {
- if (indexes.empty()) return;
-
// Core invariant: strictly monotonic increasing (no duplicates allowed)
for (size_t i = 1; i < indexes.size(); ++i) {
assert(indexes[i] > indexes[i-1]); // Must be STRICTLY greater
@@ -32,15 +20,15 @@ void VerifyDifferenceFormatterInvariants(const std::vector<uint16_t>& indexes) {
FUZZ_TARGET(difference_formatter)
{
- FuzzedDataProvider fdp{buffer.data(), buffer.size()};
-
- auto random_bytes = fdp.ConsumeRemainingBytes<uint8_t>();
- DataStream ss{random_bytes};
+ const auto block_hash = InsecureRandomContext{{}}.rand256();
+ DataStream ss{};
+ ss << block_hash << std::span{buffer};
// Test deserialization
try {
- TestDifferenceFormatterType test_container;
+ BlockTransactionsRequest test_container;
ss >> test_container;
+ assert(test_container.blockhash==block_hash);
// If deserialization succeeded, verify the DifferenceFormatter invariants
VerifyDifferenceFormatterInvariants(test_container.indexes);
@@ -48,5 +36,4 @@ FUZZ_TARGET(difference_formatter)
} catch (const std::ios_base::failure&) {
// Expected for malformed input
}
-
}
Other changes include:
indexes.empty() check
I think this is more cleaner and simpler to follow. thanks
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 | + }
nit: There is no need to serialize single bytes. You can just pass the vector into the DataStream constructor
(left my previous comment as in-line review comments)
@maflcko sorry for not being able to reply soon, that was because I was away from my computer. And no, it is not LLM generated code, I have been working on it for a while now.
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
dropped the serialization roundrip as @maflcko suggests, they can be done elsewhere (./src/test/fuzz/deserialize.cpp).
133 | @@ -133,7 +134,6 @@ add_executable(fuzz 134 | validation_load_mempool.cpp 135 | vecdeque.cpp 136 | versionbits.cpp 137 | - difference_formatter.cpp
this is the wrong commit. You'll have to apply the patch to the correct commit.
okey. Will sort it out
For commit b5c3a72, updated 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]);
Assume(req.indexes[i] > req.indexes[i-1]);
Just so the assertion isn't active in release builds
yes. thanks
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 | + }
The two comments are saying the same thing, I'd suggest to just keep one
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);
There's only one invariant we're checking, perhaps we don't need the function?
Had only kept it for readability, but yes we can definately do without it
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.
Code review ACK 65a10fc3c52ea09a4794345bcf607dff908c783a
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?
Since this is using Assume, the loop shouldn't exist in the release binaries (i.e the compiler optimises it out), so the overhead doesn't exist.
rfm
ACK 65a10fc3c52ea09a4794345bcf607dff908c783a