test: add a few more base32/64 calculation corner cases

[l0rinc]

Added tests for base32 and base 64 conversions.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29119 (refactor: Use std::span over Span by maflcko)
• #29071 (refactor: Remove Span operator==, Use std::ranges::equal by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/23660462203}

[fanquake]

https://github.com/bitcoin/bitcoin/pull/29847/checks?check_run_id=23664375509

Run base_encode_decode with args ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/base_encode_decode')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1375269970
INFO: Loaded 1 modules   (578208 inline 8-bit counters): 578208 [0x55e843d75228, 0x55e843e024c8), 
INFO: Loaded 1 PC tables (578208 PCs): 578208 [0x55e843e024c8,0x55e8446d4ec8), 
INFO:     1342 files found in /ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/base_encode_decode
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048564 bytes
util/strencodings.cpp:213:63: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'size_type' (aka 'unsigned long')
    #0 0x55e842a7e2c4 in DecodeBase32(std::basic_string_view<char, std::char_traits<char>>) src/util/strencodings.cpp:213
    #1 0x55e840ea6c48 in base_encode_decode_fuzz_target(Span<unsigned char const>) src/test/fuzz/base_encode_decode.cpp:34:19
    #2 0x55e841344a6d in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
    #3 0x55e841344a6d in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:180:5
    #4 0x55e840c6cad4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1a62ad4) (BuildId: 090547d582a1716a2b3f214f309a94f43fbb1ebd)
    #5 0x55e840c6dd01 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1a63d01) (BuildId: 090547d582a1716a2b3f214f309a94f43fbb1ebd)
    #6 0x55e840c6e2f7 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1a642f7) (BuildId: 090547d582a1716a2b3f214f309a94f43fbb1ebd)
    #7 0x55e840c5b7ef in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1a517ef) (BuildId: 090547d582a1716a2b3f214f309a94f43fbb1ebd)
    #8 0x55e840c85f16 in main (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1a7bf16) (BuildId: 090547d582a1716a2b3f214f309a94f43fbb1ebd)
    #9 0x7f3697ff01c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 232274c0019767b821da1c6ebc2df43e60503035)
    #10 0x7f3697ff028a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 232274c0019767b821da1c6ebc2df43e60503035)
    #11 0x55e840c507d4 in _start (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1a467d4) (BuildId: 090547d582a1716a2b3f214f309a94f43fbb1ebd)

SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow util/strencodings.cpp:213:63 
MS: 0 ; base unit: 0000000000000000000000000000000000000000

https://github.com/bitcoin/bitcoin/actions/runs/8632800923/job/23664375055?pr=29847

 Assertion failed: (ToLower(encoded_string) == ToLower(TrimString(random_encoded_string))), function base_encode_decode_fuzz_target, file base_encode_decode.cpp, line 45.

[maflcko]

Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:

• Time spent on review
• Accidental introduction of bugs
• (Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer time or introduce bugs.

For more information about refactoring changes and stylistic cleanup, see

• https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring
• https://github.com/bitcoin/bitcoin/blob/master/.github/PULL_REQUEST_TEMPLATE.md
• Code style PRs after v0.18 branch split #15465
• refactor: Continue moving application data from CNode to Peer #26621 (comment)

Generally, if the style is not mentioned nor enforced by the developer notes, we leave it up to the original author to pick whatever fits them best personally and then leave it that way until the line is touched for other reasons.

Leave a comment below, if you have any questions.

[l0rinc]

Thanks for the review @fanquake and @maflcko - though it was still in draft form because of the unsigned problems.

I didn't mean the change as a purely stylistic refactoring.

In the encodings the original str.reserve doesn't enforce the final size, but after the change it's obvious that it's an exact calculation, not just an upper bound, i.e. while (str.size() % 4) str += '='; vs str.append(size - str.size(), '=');.

In the second change the point was to unify the suffix removal with other trim algos (e.g. TrimStringView). This would unify the 32 and 64 bit algos and obviate the inconsistency between the 32 and 64 base conversions for paddings (i.e. roundtrip tests aren't working without padding).

If you think the changes are controversial, I can commit the test and the proposed changes separately.

[l0rinc]

@maflcko, you were right, the changes didn't add enough value, I've reverted them.
I've kept the tests, since I think they're valuable and unified the style between base 32 and 64 in the source. If you think that's still too much, I'll revert it.

[maflcko]

Please make sure to re-read the whole comment I shared and make sure to read all pages it links to. The burden to clearly motivate a change (and the review effort it requires) is on the pull request author (in this case here it would be you).

Open Source does not work by doing random unmotivated changes, opening a pull request, seeing it fail CI and tests, then adjust it in a ping-pong fashion to make CI green and go back and forth with reviewers until they have essentially written the whole changeset themselves.

At a minimum a (refactoring) pull request should have:

• A clear motivation
• Be correct and easy to review
• Have passing tests and CI

If any of the above do not apply, a refactoring pull request should not be opened in the first place.

[l0rinc]

@maflcko, which of the listed motivation/review/passing CI doesn't apply here?

[maflcko]

All of it.

• "unified ... code" is not a motivation, but a style choice
• The CI didn't pass on several tries
• Review doesn't seem worth it, unless there is a motivation

[laanwj]

Thanks for your first contribution!

As it is hard to review PRs that change things all over the place, this is unlikely to be merged like this. I'd suggest keeping the code change focused. For example, make it add test cases only, and remove the changes (refactoring and otherwise) to strecodings.cpp and other non-test code.

[l0rinc]

Hey @laanwj, thanks for your first review.
Please see the commits separately, don't worry, they're not all over the place.

[maflcko]

@paplorinc If you are not being receptive to reviewers' feedback, it is unlikely that anyone will review the pull request and without review it will not be merged.

[l0rinc]

@maflcko, I've responded to every single comment within minutes. I just don't appreciate the patronizing style I keep seeing in this repo.

[laanwj]

Take it how you wish, i was just trying to be helpful, not patronizing.

[maflcko]

Please understand that this software project requires review of all code changes. It is not a typical software project that auto-merges any change as soon as the CI is green.

When writing a patch as a pull request, please consider how reviewers are supposed to review it and how easy it will be for them to follow the changes and their motivation.

[fanquake]

I don't yet understand how we expect this code to become more and more maintainable, if we don't regularly leave the campground cleaner than we've found it.

The issue here is that even if you've found the campground, and while maybe it looks like it could use a little maintenance, if you zoom out for a birds-eye view, you'll actually notice that everyone else is busy somewhere next door, trying to fight the forest fires. Even if the code here is (maybe arbitrarily) "better" or "faster" by some metric, the impact to the whole project is ~0. In fact, it could even be argued that it's a net negative, given the burden of changing and reviewing the code, probably doesn't outweigh distracting engineers working on more important things and/or actually breaking something that already works (and will likely continue to work for many many years) + all the time spent here.

I understand that as a new contributor, you're looking for things to do, and picking random utility functions/code, and trying to generically improve it may seem easiest / appealing, and potentially something that other open source projects will happily take, however that isn't how things work here.

I'd suggest looking through https://github.com/bitcoin/bitcoin/issues (you can filter for "good first issues"), and either picking a bug that needs fixing, and working on fixing it, or, looking through, https://github.com/bitcoin/bitcoin/pulls, and finding something that interests you, and leaving some meaningful review comments. These will be much more productive avenues for you to become a productive contributor to this project. You should also familiarise yourself with https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md and the https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md.

[l0rinc]

Thanks for the detailed answer @fanquake.

everyone else is busy [...] trying to fight the forest fires

So how do we prevent future forest fires while extinguishing current ones?

I have looked through good first issue since the beginning, there's barely any, all of which were started by others, often years ago.
That's why I started working on what I can contribute instead.

[maflcko]

Another great way to contribute would be to spend time on in-depth review of open pull requests and compare your review with the review done by others. Obviously this will be time-consuming, starting out, but if you care about the project, over time it will teach you relevant skills (for this project and for yourself to apply elsewhere).

On some pull requests, there remain small follow-ups that were found during review, so you'll naturally find stuff to work on if you start out by reviewing pull requests done by others. Another benefit of this is, that if you end up creating a follow-up, the code will be fresh in the minds of some developers, who may be willing to spend time on review.

[DrahtBot]

🐙 This pull request conflicts with the target branch and needs rebase.

[l0rinc]

@maflcko, would it make sense for me to rebase this or should I just close it at this stage?

[maflcko]

Adding tests seems fine, but maybe open a new pull request, because the discussion here was about a different change to non-test code.

[l0rinc]

moved to a clean slate in #30746