[tests] Add fuzz testing for BlockTransactions and BlockTransactionsRequest #10409

The BlockTransactions deserialization code is reachable with tainted data via ProcessMessage(…, "BLOCKTXN", vRecv [tainted], …).

The same thing applies to BlockTransactionsRequest which is reachable via "GETBLOCKTXN".

utACK d36fe73294b57821a793cd270bb0b4365fcdba00

Maybe add CBlockHeaderAndShortTxIDs while you're at it?

@TheBlueMatt Good point! Adding also CBlockHeaderAndShortTxIDs was my original intention, but when doing so I encountered a long chain of linking errors which I was unable to fully resolve.

More specifically I didn't manage to create a test_test_bitcoin_fuzzy_LDADD variable (in src/Makefile.test.include) which would allow test_bitcoin_fuzzy to be built cleanly with this patch applied:

diff --git a/src/test/test_bitcoin_fuzzy.cpp b/src/test/test_bitcoin_fuzzy.cpp
index 7b52ac9..173351f 100644
--- a/src/test/test_bitcoin_fuzzy.cpp
+++ b/src/test/test_bitcoin_fuzzy.cpp
@@ -48,6 +48,7 @@ enum TEST_ID {
     CTXOUTCOMPRESSOR_DESERIALIZE,
     BLOCKTRANSACTIONS_DESERIALIZE,
     BLOCKTRANSACTIONSREQUEST_DESERIALIZE,
+    CBLOCKHEADERANDSHORTTXIDS_DESERIALIZE,
     TEST_ID_END
 };

@@ -273,6 +274,16 @@ int main(int argc, char **argv)

             break;
         }
+        case CBLOCKHEADERANDSHORTTXIDS_DESERIALIZE:
+        {
+            try
+            {
+                CBlockHeaderAndShortTxIDs bhastids;
+                ds >> bhastids;
+            } catch (const std::ios_base::failure& e) {return 0;}
+
+            break;
+        }
         default:
             return 0;
     }

Help with solving this would be appreciated!

@TheBlueMatt I solved the linking issue. Fuzzing of CBlockHeaderAndShortTxIDs has been added as part of this PR! :-)

Reverted the CBlockHeaderAndShortTxIDs deserialization commit. Couldn't create a proper test_test_bitcoin_fuzzy_LDADD in src/Makefile.test.include that would allow test_test_bitcoin_fuzzy to compile cleanly under all configuration options.

Finally managed to make the version including fuzzing of CBlockHeaderAndShortTxIDs to compile cleanly under all configuration options.

This PR should be ready for review now :-)

Anyone willing to review? :-)

Friendly ping @TheBlueMatt or @sipa - is this one getting ready for merge? :-)

@laanwj Thanks for merging the libFuzzer support (#10440) today!

This is a friendly ping about my only currently outstanding fuzzing PR.

It adds fuzzing of the remaining deserialization code that is reachable with tainted data via ProcessMessage(…, "…", vRecv [tainted], …) but not currently covered by test_bitcoin_fuzzy. More specifically it adds fuzzing of the deserialization code for BlockTransactions, BlockTransactionsRequest and CBlockHeaderAndShortTxIDs.

Do you think it might be getting ready for merge?

When this PR is merged I'll look into having command-line argument determine the type of fuzzer as suggested in #11045.

Looks good, though does the significant extra linking cause issues? See comment at #11045 (comment) indicating that at least some fuzzers may get much slower just cause of the extra binary size.

Seems like I'm unable to create a correct Makefile.test.include for this. Anyone willing to help me resolve that issue? :-)

If not I'll probably remove the CBlockHeaderAndShortTxIDs part of this PR to avoid the linking dependency order nightmare :-)

Changed PR to only cover BlockTransactions and BlockTransactionsRequest. Hence no extra linking.

This one should be ready for merge :-)

utACK fd3a2f3130ebd1d1001c5dff80c1ff026654b00d

utACK fd3a2f3