This refactor does not change any behavior, except for the integer
sanitizer warning.
Can be tested via:
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./bld-cmake/bin/bitcoin-cli -stdinrpcpass uptime
Move the `cachedCoinsUsage` subtract in `AddCoin()` to after the `possible_overwrite` check.
Previously a throw before assignment decremented the counter without changing the entry, which corrupted accounting and later underflowed.
In `Flush()`, reset `cachedCoinsUsage` to `0` only when `BatchWrite()` succeeds and `cacheCoins` is actually cleared. In production `BatchWrite()` returns `true`, so this mostly affects tests. On failure, leave the counter unchanged to keep it in sync with the cache.
The existing `Flush()` workaround in fuzzing was also removed now that the source of the problem was fixed, so the fuzzer no longer needs `coins_view_cache.Flush()` to realign `cachedCoinsUsage` after an exception.
Replace the prior `expected_code_path` tracking with direct assertions. The role of the variable was to verify that code execution follows only expected paths, either successful addition, or if it's an exception, the message is verified and checked that overwrite was disallowed.
With these changes the counter stays consistent across success and exception paths, so we can finally remove the `UBSan` suppressions for `CCoinsViewCache` that were masking the issue.
Included a unit test as well, attempting to add a different coin to the same outpoint without allowing overwrites and make sure it throws.
We use `SelfTest()` to validates accounting, and check that the cache remains usable.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: w0xlt <woltx@protonmail.com>
516f0689b5 refactor: re-enable UBSan implicit-sign-change in serialize.h (Lőrinc)
5827e93507 refactor: use consistent size type for serialization template parameters (Lőrinc)
Pull request description:
Inspired by https://github.com/bitcoin/bitcoin/pull/32154, the main goal of this PR is to reenable sanitizer checks for `serialize.h` since it's modified in a few other PRs.
ACKs for top commit:
maflcko:
review ACK 516f0689b5🎈
stickies-v:
ACK 516f0689b5, nice cleanup!
Tree-SHA512: 63da9bf1988a5b68e3c053b0ed786b8735f8f75b05763511522d1601b728b55798006e063137447615c266582622642d3226318fa83e488bd363f1756f8811e8
Made every signed/unsigned conversion in the serialization helpers explicit so the UBSan `implicit-sign-change` check passes and the `serialize.h` suppression can be dropped.
For consistency, a few other simple changes were also applied to the serialization helpers:
* remove redundant `inline` on function templates;
* unify formatting to make the differences between similar methods obvious.
rend() creates a pointer with offset -1. This is UB, according to the
C++ standard: https://eel.is/c++draft/expr.add#4:
When an expression J that has integral type is added to [...] an
expression P of pointer type, the result has the type of P.
... if P points to a (possibly-hypothetical) array element i of an
array object x with n elements [...] the expressions P + J and J + P
(where J has the value j) point to the (possibly-hypothetical) array
element i+j of x if 0≤i+j≤n [...]
Otherwise, the behavior is undefined.
Also, it is unclear why the functions exist at all, when stdlib utils
such as std::reverse_iterator{it} or std::views::reverse can be used out
of the box.
So remove them, along with the ubsan suppressions, that are no longer
used.
fa6a007b8e fuzz: Avoid integer sanitizer warnings in policy_estimator target (MarcoFalke)
Pull request description:
It seems odd to write a fuzz target to trigger integer sanitizer warnings in `CBlockPolicyEstimator::processBlockTx` and then suppress them. If the scenario can happen in reality, the code should be properly fixed to handle the cases. If not, it seems better to fix the fuzz target to not trigger meaningless traces.
Do that here by keeping track of the current height and limiting mempool entries to at most this entry height.
ACKs for top commit:
brunoerg:
ACK fa6a007b8e
dergoegge:
utACK fa6a007b8e
Tree-SHA512: 2092017dc309fb095fe5d43cfb76efb691795f303d567ee919be2b5cac19a944293636229903dc4d1e8b9fe5daf9dc3058544321eff1735f91f804c3baa36cd0
ce8094246e random: replace construct/assign with explicit Reseed() (Pieter Wuille)
2ae392d561 random: use LogError for init failure (Pieter Wuille)
97e16f5704 tests: make fuzz tests (mostly) deterministic with fixed seed (Pieter Wuille)
2c91330dd6 random: cleanup order, comments, static (Pieter Wuille)
8e31cf9c9b net, net_processing: use existing RNG objects more (Pieter Wuille)
d5fcbe966b random: improve precision of MakeExponentiallyDistributed (Pieter Wuille)
cfb0dfe2cf random: convert GetExponentialRand into rand_exp_duration (Pieter Wuille)
4eaa239dc3 random: convert GetRand{Micros,Millis} into randrange (Pieter Wuille)
82de1b80d9 net: use GetRandMicros for cache expiration (Pieter Wuille)
ddc184d999 random: get rid of GetRand by inlining (Pieter Wuille)
e2d1f84858 random: make GetRand() support entire range (incl. max) (Pieter Wuille)
810cdf6b4e tests: overhaul deterministic test randomness (Pieter Wuille)
6cfdc5b104 random: convert XoRoShiRo128PlusPlus into full RNG (Pieter Wuille)
8cc2f45065 random: move XoRoShiRo128PlusPlus into random module (Pieter Wuille)
8f5ac0d0b6 xoroshiro128plusplus: drop comment about nonexisting copy() (Pieter Wuille)
8924f5120f random: modernize XoRoShiRo128PlusPlus a bit (Pieter Wuille)
ddb7d26cfd random: add RandomMixin::randbits with compile-known bits (Pieter Wuille)
21ce9d8658 random: Improve RandomMixin::randbits (Pieter Wuille)
9b14d3d2da random: refactor: move rand* utilities to RandomMixin (Pieter Wuille)
40dd86fc3b random: use BasicByte concept in randbytes (Pieter Wuille)
27cefc7fd6 random: add a few noexcepts to FastRandomContext (Pieter Wuille)
b3b382dde2 random: move rand256() and randbytes() to .h file (Pieter Wuille)
493a2e024e random: write rand256() in function of fillrand() (Pieter Wuille)
Pull request description:
This PR contains a number of vaguely-related improvements to the random module.
The specific changes and more detailed rationale is in the commit messages, but the highlights are:
* `XoRoShiRo128PlusPlus` (previously a test-only RNG) moves to random.h and becomes `InsecureRandomContext`, which is even faster than `FastRandomContext` but non-cryptographic. It also gets all helper randomness functions (`randrange`, `fillrand`, ...), making it a lot more succinct to use.
* During tests, **all** randomness is made deterministic (except for `GetStrongRandBytes`) but non-repeating (like `GetRand()` used to be when `g_mock_deterministic_tests` was used), either fixed, or from a random seed (overridden by env var).
* Several infrequently used top-level functions (`GetRandMillis`, `GetRandMicros`, `GetExponentialRand`) are converted into member functions of `FastRandomContext` (and `InsecureRandomContext`).
* `GetRand<T>()` (without argument) can now return the maximum value of the type (previously e.g. `GetRand<uint32_t>()` would never return 0xffffffff).
ACKs for top commit:
achow101:
ACK ce8094246e
maflcko:
re-ACK ce8094246e🐈
hodlinator:
ACK ce8094246e
dergoegge:
utACK ce8094246e
Tree-SHA512: 79bc0cbafaf27e95012c1ce2947a8ca6f9a3c78af5f1f16e69354b6fc9b987a28858adf4cd356dc5baf21163e9af8dcc24e70f8d7173be870e8a3ddcdd47c02c
Convert XoRoShiRo128PlusPlus into a full RandomMixin-based RNG class,
providing all utility functionality that FastRandomContext has. In doing so,
it is renamed to InsecureRandomContext, highlighting its non-cryptographic
nature.
To do this, a fillrand fallback is added to RandomMixin (where it is used by
InsecureRandomContext), but FastRandomContext still uses its own fillrand.
In many cases, it is known at compile time how many bits are requested from
randbits. Provide a variant of randbits that accepts this number as a template,
to make sure the compiler can make use of this knowledge. This is used immediately
in rand32() and randbool(), and a few further call sites.
The previous randbits code would, when requesting more randomness than available
in its random bits buffer, discard the remaining entropy and generate new.
Benchmarks show that it's usually better to first consume the existing randomness
and only then generate new ones. This adds some complexity to randbits, but it
doesn't weigh up against the reduced need to generate more randomness.
This adds a bitset module that implements a BitSet<N> class, a variant
of std::bitset with a few additional features that cannot be implemented
in a wrapper without performance loss (specifically, finding first and
last bit set, or iterating over all set bits).
Add ubsan suppressions for integer overflows in the getchaintxstats RPC.
getchainstatstx line "int nTxDiff = pindex->nChainTx - past_block.nChainTx" can
trigger ubsan integer overflows when assumeutxo snapshots are loaded, from
subtracting unsigned values and assigning the result to a signed int.
The overflow behavior probably exists in current code but is hard to trigger
because it would require calling getchainstatstx at the right time with
specific parameters as background blocks are being downloaded. But the overflow
behavior becomes easier to trigger in the upcoming commit removing fake
nChainTx values, so a suppression needs to be added before then for CI to pass.
getchainstatstx should probably be improved separately in another PR to not
need this suppression, and handle edge cases and missing nChainTx values more
carefully.
This is a refactor as long as no signed integer overflow appears. In
normal operation and absent bugs, signed integer overflow should never
happen in the touched code paths.
The main benefit of this refactor is to drop the file-wide ubsan
suppression unsigned-integer-overflow:txmempool.cpp.
For now, this only changes the internal private representation and the
publicly returned type remains uint64_t.
* The GCC suppression was fixed in gcc-11, which is available on all LTS
releases of Linux distros.
* The feerate suppression was likely fixed and does not trigger anymore.
If it was to trigger again, the underlying bug should be fixed instead
of suppressing it.
* The bench suppression does not trigger anymore.
Also, add comments to tsan suppressions on how to reproduce.
Xoroshiro128++ is a fast non-cryptographic random generator.
Reference implementation is available at https://prng.di.unimi.it/
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
fad84a2595 refactor: Fixup uint64_t-cast style in touched line (MarcoFalke)
fa041878de Fix implicit-integer-sign-change in bloom (MarcoFalke)
Pull request description:
Signed values don't really make sense when using `std::vector::operator[]`.
Fix that and remove the suppression.
ACKs for top commit:
PastaPastaPasta:
utACK fad84a2595
theStack:
Code-review ACK fad84a2595
Tree-SHA512: 7139dd9aa098c41e4af1b6e63dd80e71a92b0a98062d1676b01fe550ffa8e21a5f84a578afa7a536d70dad1b8a5017625e3a9e2dda6f864b452ec77b130ddf2a
fa2406a50a zmq: Fix implicit-integer-sign-change (MarcoFalke)
Pull request description:
uint256::begin() returns unsigned data, so there is no reason to make it signed.
Fix that and remove the sanitizer suppression.
ACKs for top commit:
hebasto:
ACK fa2406a50a
PastaPastaPasta:
utACK fa2406a50a, I have reviewed the code and think it makes sense
Tree-SHA512: 150ebcf3fdc3e0f60b6fd8e5fe638737b01e8a0863296bd545fb5ed17d33ab23b2ff94204996aa7b4617650b7383bd86ed2d2bf46746b410feae449de179a2bd
faa630aa15 test: Fix sanitizer suppresions in streams_tests (MarcoFalke)
Pull request description:
Two changes (that also make sense on their own) to remove the file-wide sanitizer suppression:
* `FindByte` no longer takes a `char`, but an `uint8_t`, after commit 196b459920.
* The `key` vector of unsigned chars can be removed and inlined as initializer-list. This avoids a bunch of verbose code like `clear()` and `push_back` of `char`s.
ACKs for top commit:
PastaPastaPasta:
utACK faa630aa15, I have reviewed the changes and agree it makes sense to merge
Tree-SHA512: 747b9d4676fad6d07f3955668639c93333625e69199ff4c499f01167de3875990d93db85e775a7f5b1b684575dceaec8aa000b4db15525fc47b699bac1c85e3d
MarcoFalke