https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817851827 introduced an unsequenced operations with side-effects - which is undefined behavior, i.e. the right hand side can be evaluated before the left hand side, which happens to mutate it.
Tried:
```
clang++ --analyze -std=c++20 -I./src -I./src/test -I./src/test/fuzz src/test/fuzz/base_encode_decode.cpp src/psbt.cpp
```
but it didn't warn about UB.
Grepped for similar ones, but could find any other one in the codebase:
> grep -rnE --include='*.cpp' --include='*.h' '\b(\w+)\(([^)]*\b(\w+)\b[^)]*)\)\s*==\s*\3\.' .
```
./src/test/arith_uint256_tests.cpp:373: BOOST_CHECK(R1L.GetHex() == R1L.ToString());
./src/test/arith_uint256_tests.cpp:374: BOOST_CHECK(R2L.GetHex() == R2L.ToString());
./src/test/arith_uint256_tests.cpp:375: BOOST_CHECK(OneL.GetHex() == OneL.ToString());
./src/test/arith_uint256_tests.cpp:376: BOOST_CHECK(MaxL.GetHex() == MaxL.ToString());
./src/test/fuzz/cluster_linearize.cpp:565: assert(depgraph.FeeRate(best_anc.transactions) == best_anc.feerate);
./src/test/fuzz/cluster_linearize.cpp:646: assert(depgraph.FeeRate(found.transactions) == found.feerate);
./src/test/fuzz/cluster_linearize.cpp:765: assert(depgraph.FeeRate(chunk_info.transactions) == chunk_info.feerate);
./src/test/fuzz/base_encode_decode.cpp:95: assert(DecodeBase64PSBT(psbt, random_string, error) == error.empty());
./src/test/fuzz/key.cpp:102: assert(pubkey.data() == pubkey.begin());
./src/test/skiplist_tests.cpp:42: BOOST_CHECK(vIndex[from].GetAncestor(0) == vIndex.data());
./src/script/signingprovider.cpp:535: ComputeTapbranchHash(node.sub[1]->hash, node.sub[1]->hash) == node.hash) {
./src/pubkey.h:78: return vch.size() > 0 && GetLen(vch[0]) == vch.size();
./src/cluster_linearize.h:881: Assume(elem.inc.feerate.IsEmpty() == elem.pot_feerate.IsEmpty());
```
Hodlinator deduced the UB on Windows in https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2751723855
Github-Pull: #32141
Rebased-From: b1de59e896
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
In Base58 fuzz the two roundtrips are merged now, 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 and is being validated more thoroughly.
Github-Pull: #31917
Rebased-From: d5537c18a9
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Co-authored-by: marcofleon <marleo23@proton.me>
They seem to cause timeouts:
> Issue 397734700: bitcoin-core:base58check_encode_decode: Timeout in base58check_encode_decode
The `encoded_string.empty()` check was corrected here to `decoded.empty()` to make sure the `(0, decoded.size() - 1)` range is always valid.
Github-Pull: #31917
Rebased-From: bad1433ef2
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Co-authored-by: marcofleon <marleo23@proton.me>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Different values are used for max_ret_len throughout the codebase (e.g., 21, 34, 78).
Theoretically, negative and zero values are also permitted. Let's stress-test those as well.
Co-authored-by: brunoerg <brunoely.gc@gmail.com>
This commit introduces symmetric encode-decode roundtrips for all bases.
Minor refactors were also included:
• Split each base into a separate fuzz target.
• Added symmetric encode-decode roundtrip tests for all bases.
• Removed trim testing for encoded_string, as Base58 does not use whitespace padding.
• Made comparisons stricter by removing unnecessary lowercase conversions for bases that have mixed-case alphabets.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
There are no changes to behavior. Changes in this commit are all additions, and
are easiest to review using "git diff -U0 --word-diff-regex=." options.
Motivation for this change is to keep util functions with really generic names
like "Split" and "Join" out of the global namespace so it is easier to see
where these functions are defined, and so they don't interfere with function
overloading, especially since the util library is a dependency of the kernel
library and intended to be used with external code.
2022917223 Add secp256k1_selftest call (Pieter Wuille)
3bfca788b0 Remove explicit enabling of default modules (Pieter Wuille)
4462cb0498 Adapt to libsecp256k1 API changes (Pieter Wuille)
9d47e7b71b Squashed 'src/secp256k1/' changes from 44c2452fd3..21ffe4b22a (Pieter Wuille)
Pull request description:
Now that libsecp256k1 has a release (https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-December/021271.html), update the subtree to match it.
The changes themselves are not very impactful for Bitcoin Core, but include:
* It's no longer needed to specify whether contexts are for signing or verification or both (all contexts support everything), so make use of that in this PR.
* Verification operations can use the static context now, removing the need for some infrastructure in pubkey.cpp to make sure a context exists.
* Most modules are now enabled by default, so we can drop explicit enabling for them.
* CI improvements (in particular, MSVC and more recent MacOS)
* Introduction of an internal int128 type, which has no effect for GCC/Clang builds, but enables 128-bit multiplication in MSVC, giving a ~20% speedup there (but still slower than GCC/Clang).
* Release process changes (process documentation, changelog, ...).
ACKs for top commit:
Sjors:
ACK 2022917223, but 4462cb0498 could use more eyes on it.
achow101:
ACK 2022917223
jonasnick:
utACK 2022917223
Tree-SHA512: 8a9fe28852abe74abd6f96fef16a94d5a427b1d99bff4caab1699014d24698aab9b966a5364a46ed1001c07a7c1d825154ed4e6557c7decce952b77330a8616b
* Use SECP256K1_CONTEXT_NONE when creating signing context, as
SECP256K1_CONTEXT_SIGN is deprecated and unnecessary.
* Use secp256k1_static_context where applicable.
Base32/base64 are mechanisms for encoding binary data. That they'd
decode to a string is just bizarre. The fact that they'd do that
based on the type of input arguments even more so.
