fuzz: provide more realistic values to the base58(check) decoders #31917

1. 
   l0rinc commented at 4:23 pm on February 20, 2025: contributor

   This is a follow-up to #30746, expanding coverage by:

   • restricting every input for the base conversions, capping max sizes to 100 instead of 1000 or all available input (suggested by @marcofleon in #30746 (review)) since most actual usage has lengths of e.g. 21, 34, 78.
   • providing more valid values to the decoder (suggested by @maflcko in #30746 (review)) by randomly providing a random input or a valid encoded one; This also enables unifying the roundtrip tests to a single roundtrip per fuzz

   Since I still can’t run the fuzz on my Mac (getting ld: multiple errors: invalid r_symbolnum=1 in ...) I ran it on my Linux nodes which indicated that NEW coverage has decreased rapidly.

2. 
   DrahtBot commented at 4:23 pm on February 20, 2025: contributor

   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/31917.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

3. DrahtBot added the label Tests on Feb 20, 2025
4. in src/test/fuzz/base_encode_decode.cpp:43 in 21b8b8464b outdated

   50 {
   51-    FuzzedDataProvider provider(buffer.data(), buffer.size());
   52-    const std::string random_string{provider.ConsumeRandomLengthString(1000)};
   53-    const int max_ret_len{provider.ConsumeIntegralInRange<int>(-1, 1000)};
   54+    // Encode/Decode roundtrip
   55+    const auto encoded{EncodeBase58Check(buffer)};
   

   ---

   ---

   

   marcofleon commented at 4:30 pm on February 20, 2025:

   The problem actually turned out to be this. The input that times out is about 1MB so passing in the whole buffer causes the timeout.

   I don’t think the change from 1000 to 100 for both random_string and max_ret_len makes a difference.

   ---

   

   brunoerg commented at 5:33 pm on February 20, 2025:

   I think we use this function on the RPC target and we limit the length to 64.

   ---

   

   l0rinc commented at 5:55 pm on February 20, 2025:

   Thanks for the help, I pushed a new version where every input is restricted (used 100 to be slightly different from the RPC one you’ve mentioned) and <1000, even though it seems that wasn’t the actual bottleneck.
5. l0rinc force-pushed on Feb 20, 2025
6. l0rinc force-pushed on Feb 20, 2025
7. fuzz: Always restrict base conversion input lengths

   They seem to cause timeouts:
   > Issue 397734700: bitcoin-core:base58check_encode_decode: Timeout in base58check_encode_decode
   
   Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
   Co-authored-by: marcofleon <marleo23@proton.me>

   23fc349959
8. fuzz: make sure DecodeBase58(Check) is called with valid values more often

   In Base58 the new `decode_input` switches between a completely random input and a valid encoded one, to make sure the decoding passes more often.
   The `max_ret_len` can also exceed the original length now.
   
   Also unified the other encode/decode roundtrips
   
   Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
   Co-authored-by: marcofleon <marleo23@proton.me>

   a63c5c1a8a
9. l0rinc force-pushed on Feb 21, 2025

Contributors
l0rinc DrahtBot marcofleon brunoerg

Labels
Tests