6af51e8198 Use WITH_LOCK in Warnings::Set (Ava Chow)
Pull request description:
The scope of the lock should be limited to just guarding m_warnings as anything listening on `NotifyAlertChanged` may execute code that requires the lock as well.
Fixes #30400
ACKs for top commit:
maflcko:
lgtm ACK 6af51e8198
TheCharlatan:
ACK 6af51e8198
glozow:
ACK 6af51e8198
willcl-ark:
ACK 6af51e8198
stickies-v:
ACK 6af51e8198
Tree-SHA512: 9884046c70dcad996276931b6d154f0330200332403828f34f7f7b285fc0e770ba7b25056131ab24dcb8a4b18f58d31633aa17fbb09b0eaea8a29e28fca10ec4
46819f5df6 wallet: use LogTrace for walletdb log messages at trace level (Anthony Towns)
Pull request description:
Wallet sqlite logging is enabled by `-debug=walletdb -loglevel=walletdb:trace` however the actual log messages are sent at `BCLog::Level::Info`. Switch to the trace level to make this consistent. This adds `[walletdb:trace]` to the log output, eg:
```
[httpworker.3] [wallet/sqlite.cpp:55] [TraceSqlCallback] [/tmp/bitcoin_func_test_4fsnatpg/node0/regtest/wallets/boring/wallet.dat] SQLite Statement: BEGIN EXCLUSIVE TRANSACTION
```
becomes
```
[httpworker.0] [wallet/sqlite.cpp:55] [TraceSqlCallback] [walletdb:trace] [/tmp/bitcoin_func_test_9lcwth4z/node0/regtest/wallets/boring/wallet.dat] SQLite Statement: BEGIN EXCLUSIVE TRANSACTION
```
ACKs for top commit:
maflcko:
ACK 46819f5df6
ryanofsky:
Code review ACK 46819f5df6. Nice catch!
furszy:
ACK 46819f5df6
luke-jr:
utACK 46819f5df6
Tree-SHA512: 6fc1bc63c2ee686d4ca8f4f558f06c0cd9e7813b5fce1588351f55ef8bedfc23c97ea443e54a6a447008fa79ea022b6d631cb010929932f1db23fa8e255e6482
The scope of the lock should be limited to just guarding m_warnings as
anything listening on `NotifyAlertChanged` may execute code that
requires the lock as well.
33c48c106c validation: Check if mempool exists before asserting in ActivateSnapshot (TheCharlatan)
Pull request description:
The mempool is an optional component of the chainstate manager, so don't assume its presence and instead check if it is there first.
ACKs for top commit:
maflcko:
re-ACK 33c48c106c
fjahr:
ACK 33c48c106c
Tree-SHA512: 7a3568d5b7af45efa7bf54bae7bac1f00dc99bc9d47a744d73594f283c952be9500168f680d72f4aee09761da4e878ddca83ba675cdea8ee9e44eeff00ac09da
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
2342b46c45 test: Add coverage for getchaintxstats in assumeutxo context (Fabian Jahr)
faf2a6750b rpc: Reorder getchaintxstats output (MarcoFalke)
fa2dada0c9 rpc: Avoid getchaintxstats invalid results (MarcoFalke)
Pull request description:
The `getchaintxstats` RPC reply during AU background download may return non-zero, but invalid, values for `window_tx_count` and `txrate`.
For example, `txcount` may be zero for a to-be-downloaded block, but may be non-zero for an ancestor block which is already downloaded. Thus, the values returned may be negative (and cause intermediate integer sanitizer violations).
Also, `txcount` may be accurate for the snapshot base block, or a descendant of it. However it may be zero for an ancestor block that still needs to be downloaded. Thus, the values returned may be positive, but wrong.
Fix all issues by skipping the returned value if either `txcount` is unset (equal to zero).
Also, skip `txcount` in the returned value, if it is unset (equal to zero).
Fixes https://github.com/bitcoin/bitcoin/issues/29328
ACKs for top commit:
fjahr:
re-ACK 2342b46c45
achow101:
ACK 2342b46c45
mzumsande:
ACK 2342b46c45
Tree-SHA512: 931cecc40ee5dc0f96be728db7eb297155f8343076cd29c8b8c050c99fd1d568b80f54c9459a34ca7a9489c2474c729796d00eeb1934d6a9f7b4d6a53e3ec430
926b8e39dc [doc] add release note for TRUC (glozow)
19a9b90617 use version=3 instead of v3 in debug strings (glozow)
881fac8e60 scripted-diff: change names from V3 to TRUC (glozow)
a573dd2617 [doc] replace mentions of v3 with TRUC (glozow)
089b5757df rename mempool_accept_v3.py to mempool_truc.py (glozow)
f543852a89 rename policy/v3_policy.* to policy/truc_policy.* (glozow)
Pull request description:
Adds a release note for TRUC policy which will be live in v28.0.
For clarity, replaces mentions of "v3" with "TRUC" in most places. Suggested in
- https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1629749583
- https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1624500904
I changed error strings from "v3-violation" to "TRUC-violation" but left v3 in the debug strings because I think it might be clearer for somebody who is debugging. Similarly, I left some variables unchanged because I think they're more descriptive this way, e.g. `tx_v3_from_v2_and_v3`. I'm happy to debate places that should or shouldn't be documented differently in this PR, whatever is clearest to everyone.
ACKs for top commit:
instagibbs:
reACK 926b8e39dc
achow101:
ACK 926b8e39dc
ismaelsadeeq:
Code review ACK 926b8e39dc
Tree-SHA512: 16c88add0a29dc6d1236c4d45f34a17b850f6727b231953cbd52eb9f7268d1d802563eadfc8b7928c94ed3d7a615275dd103e57e81439ebf3ba2b12efa1e42af
2f9bde69f4 test: Remove unnecessary restart in assumeutxo test (Fabian Jahr)
19ce3d407e assumeutxo: Check snapshot base block is not marked invalid (Fabian Jahr)
80315c0118 refactor: Move early loadtxoutset checks into ActiveSnapshot (Fabian Jahr)
Pull request description:
This was discovered in a discussion in #29996
If the base block of the snapshot is marked invalid or part of an invalid chain, we currently still load the snapshot and get stuck in a weird state where we have the snapshot chainstate but it will never connect to our valid chain.
While this scenario is highly unlikely to occur on mainnet, it still seems good to prevent this inconsistent state.
The behavior change described above is in the second commit.
The first commit refactors the early checks in the `loadtxoutset` RPC by moving them into `ActivateSnapshot()` in order to have the chance to cover them by unit tests in the future and have a more consistent interface. Previously checks were spread out between `rpc/blockchain.cpp` and `validation.cpp`. In order to be able to return the error message to users of the RPC, the return type of `ActivateSnapshot()` is changed from `bool` to `util::Result`.
The third commit removes an unnecessary restart introduced in #29428.
ACKs for top commit:
mzumsande:
re-ACK 2f9bde6
alfonsoromanz:
Re-ACK 2f9bde69f4. The RPC code looks much cleaner after the refactor. Also, it seems very useful to get the error message in the RPC response rather than having to rely on the logs in some scenarios if you are an RPC user.
achow101:
ACK 2f9bde69f4
Tree-SHA512: 5328dd88c3c7be3f1be97c9eef52ac3666c27188c30a798b3e949f3ffcb83be075127c107e4046f7f39f961a79911ea3d61b61f3c11e451b3e4c541c264eeed4
323ce30308 Moved the repeated -printpriority fetching out of AddToBlock (Lőrinc)
Pull request description:
`AddToBlock` was called repeatedly from `addPackageTxs` where the constant value of `printpriority` is recalculated every time.
<img src="https://github.com/bitcoin/bitcoin/assets/1841944/6fd89647-7b6c-4f44-bd04-98d16cd2a938">
This showed up during profiling of AssembleBlock, fetching it once in the constructor results in a small speed increase for many iterations.
> ./src/bench/bench_bitcoin --filter='AssembleBlock' --min-time=10000
before:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 156,460.15 | 6,391.40 | 0.1% | 11.03 | `AssembleBlock`
after:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 149,289.55 | 6,698.39 | 0.3% | 10.97 | `AssembleBlock`
---
The slight speedup shows up in CI as well:
<img src="https://github.com/bitcoin/bitcoin/assets/1841944/3be779c9-2dce-4a96-ae5f-cab5435bd72f">
ACKs for top commit:
maflcko:
ACK 323ce30308
achow101:
ACK 323ce30308
tdb3:
re ACK 323ce30308
furszy:
utACK 323ce30308
Tree-SHA512: c2a0aab429646453ad0470956529f1cac8c38778c4c53f82c92c6cbaaaeb69f3d3603c0014ff097844b151e9da7caa2371a4676244caea96527cb540e66825a3
Keep mentions of v3 in debug strings to help people who might not know
that TRUC is applied when version=3.
Also keep variable names in tests, as it is less verbose to keep v3 and v2.
f1478c0545 mempool: move LoadMempool/DumpMempool to node (Cory Fields)
6d242ff1e9 kernel: remove mempool_persist.cpp (Cory Fields)
Pull request description:
DumpMempool/LoadMempool are not necessary for the kernel.
Noticed while working on instantiated logging.
I suppose these could have been left in on purpose, but I'm assuming it was probably just an oversight.
ACKs for top commit:
TheCharlatan:
Re-ACK f1478c0545
glozow:
ACK f1478c0545
stickies-v:
ACK f1478c0545
Tree-SHA512: 5825da0cf2e67470524eb6ebe397eb90755a368469a25f184df99ab935b3eb6d89eb802b41a6c3661e869bba3bbfa8ba9d95281bc75ebbf790ec5d9d1f79c66f
There are only a few call sites of these throughout the codebase, so
move the functionality into FastRandomContext, and rewrite all call sites.
This requires the callers to explicit construct FastRandomContext objects,
which do add to the verbosity, but also make potentially apparent locations
where the code can be improved by reusing a FastRandomContext object (see
further commit).
The existing code uses GetRand(nMax), with a default value for nMax, where nMax is the
range of values (not the maximum!) that the output is allowed to take. This will always
miss the last possible value (e.g. GetRand<uint32_t>() will never return 0xffffffff).
Fix this, by moving the functionality largely in RandomMixin, and also adding a
separate RandomMixin::rand function, which returns a value in the entire (non-negative)
range of an integer.
The existing code provides two randomness mechanisms for test purposes:
- g_insecure_rand_ctx (with its wrappers InsecureRand*), which during tests is
initialized using either zeros (SeedRand::ZEROS), or using environment-provided
randomness (SeedRand::SEED).
- g_mock_deterministic_tests, which controls some (but not all) of the normal
randomness output if set, but then makes it extremely predictable (identical
output repeatedly).
Replace this with a single mechanism, which retains the SeedRand modes to control
all randomness. There is a new internal deterministic PRNG inside the random
module, which is used in GetRandBytes() when in test mode, and which is also used
to initialize g_insecure_rand_ctx. This means that during tests, all random numbers
are made deterministic. There is one exception, GetStrongRandBytes(), which even
in test mode still uses the normal PRNG state.
This probably opens the door to removing a lot of the ad-hoc "deterministic" mode
functions littered through the codebase (by simply running relevant tests in
SeedRand::ZEROS mode), but this isn't done yet.
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.
Rather than make all the useful types of randomness be exclusive to
FastRandomContext, move it to a separate RandomMixin class where it can be reused by
other RNGs.
A Curiously Recurring Template Pattern (CRTP) is used for this, to provide the ability
for individual RNG classes to override one or more randomness functions, without
needing the runtime-cost of virtual classes.
Specifically, RNGs are expected to only provide fillrand and rand64, while all others
are derived from those:
- randbits
- randrange
- randbytes
- rand32
- rand256
- randbool
- rand_uniform_delay
- rand_uniform_duration
- min(), max(), operator()(), to comply with C++ URBG concept.
55eea003af test: Make blockencodings_tests deterministic (AngusP)
4c99301220 test: Add ReceiveWithExtraTransactions Compact Block receive test. (AngusP)
4621e7cc8f test: refactor: Rename extra_txn to const empty_extra_txn as it is empty in all test cases (AngusP)
Pull request description:
This test uses the `extra_txn` (`vExtraTxnForCompact`) vector of optional orphan/conflicted/etc. transactions to provide transactions to a PartiallyDownloadedBlock that are not otherwise present in the mempool, and check that they are used.
This also covers a former nullptr deref bug that was fixed in #29752 (bf031a517c) where the `extra_txn` vec/circular-buffer was null-initialized and not yet filled when dereferenced in `PartiallyDownloadedBlock::InitData`.
ACKs for top commit:
marcofleon:
Code review ACK 55eea003af. I ran the `blockencodings` unit test and no issues with the new test case.
dergoegge:
Code review ACK 55eea003af
glozow:
ACK 55eea003af
Tree-SHA512: d7909c212bb069e1f6184b26390a5000dcc5f2b18e49b86cceccb9f1ec4f874dd43bc9bc92abd4207c71dd78112ba58400042c230c42e93afe55ba51b943262c
e009bf681c Don't use iterator addresses in IteratorComparator (dergoegge)
Pull request description:
See #29018.
Stability for `txorphan` is now >90%. `mini_miner` needs further investigation, stability still low (although slightly improved by this PR) at ~62%.
ACKs for top commit:
marcofleon:
Tested ACK e009bf681c. Using afl++, stability for `txorphan` went from 82% to ~94% and for `mini_miner` it went from 84% to 97%. I ran them both using the corpora in qa-assets.
glozow:
utACK e009bf681c
Tree-SHA512: 6d0a20fd7ceedca8e702d8adde5fca500d8b0187147aee8d43b4e9eb5176dcacf60180f42a7158f037d18dbb27e479b6c069a0f3c912226505cbff5aa073a415
4d81b4de33 fuzz: FuzzedSock::Recv() don't lose bytes from MSG_PEEK read (Vasil Dimov)
b51d75ea97 fuzz: simplify FuzzedSock::m_peek_data (Vasil Dimov)
Pull request description:
Problem:
If `FuzzedSock::Recv(N, MSG_PEEK)` is called then `N` bytes would be
retrieved from the fuzz provider, saved in `m_peek_data` and returned
to the caller (ok).
If after this `FuzzedSock::Recv(M, 0)` is called where `M < N`
then the first `M` bytes from `m_peek_data` would be returned
to the caller (ok), but the remaining `N - M` bytes in `m_peek_data`
would be discarded/lost (not ok). They must be returned by a subsequent
`Recv()`.
To resolve this, only remove the head `N` bytes from `m_peek_data`.
---
This is a followup to https://github.com/bitcoin/bitcoin/pull/30211, more specifically:
https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633199919https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633216366
ACKs for top commit:
marcofleon:
ACK 4d81b4de33. Tested this with the I2P fuzz target and there's no loss in coverage. I think overall this is an improvement in the robustness of `Recv` in `FuzzedSock`.
dergoegge:
Code review ACK 4d81b4de33
brunoerg:
utACK 4d81b4de33
Tree-SHA512: 73b5cb396784652447874998850e45899e8cba49dcd2cc96b2d1f63be78e48201ab88a76cf1c3cb880abac57af07f2c65d673a1021ee1a577d0496c3a4b0c5dd
AddToBlock was called repeatedly from `addPackageTxs` where the constant value of `printpriority` is recalculated every time.
Since its behavior was changed in 400b151, I've named the variable accordingly.
This showed up during profiling of AssembleBlock, fetching it once in the constructor results in a measurable speed increase for many iterations.
> ./src/bench/bench_bitcoin --filter='AssembleBlock' --min-time=1000
before:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 155,558.97 | 6,428.43 | 0.1% | 1.10 | `AssembleBlock`
after:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 148,083.68 | 6,752.94 | 0.1% | 1.10 | `AssembleBlock`
Co-authored-by: furszy <mfurszy@protonmail.com>
a74b0f93ef Have testBlockValidity hold cs_main instead of caller (Sjors Provoost)
f6dc6db44d refactor: use CHECK_NONFATAL to avoid single-use symbol (Sjors Provoost)
5fb2b70489 Drop unneeded lock from createNewBlock (Sjors Provoost)
75ce7637ad refactor: testBlockValidity make out argument last (Sjors Provoost)
83a9bef0e2 Add missing include for mining interface (Sjors Provoost)
Pull request description:
Followups from #30200
Fixes:
- `std::unique_ptr` needs `#include <memory>` (noticed while working on #30332, which has fewer includes than its parent PR that I originally tested with)
- Drop lock from createNewBlock that was spuriously added
- Have testBlockValidity hold cs_main instead of caller (also fixes a race condition in test-only code)
Refactor:
- Use CHECK_NONFATAL to avoid single-use symbol (refactor)
- move output argument `state` to the end of `testBlockValidity`, see https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1647987176
ACKs for top commit:
AngusP:
Code Review ACK a74b0f93ef
itornaza:
Tested ACK a74b0f93ef
ryanofsky:
Code review ACK a74b0f93ef. Just new error string is added since last review, and a commit message was updated
Tree-SHA512: 805e133bb59303fcee107d6f02b3e2761396c290efb731a85e6a29ae56b4b1b9cd28ada9629e979704dcfd98cf35034e7e6b618e29923049eb1eca2f65630e41
73f0a6cbd0 doc: detail -rpccookieperms option (willcl-ark)
d2afa2690c test: add rpccookieperms test (willcl-ark)
f467aede78 init: add option for rpccookie permissions (willcl-ark)
7df03f1a92 util: add perm string helper functions (willcl-ark)
Pull request description:
This PR picks up #26088 by aureleoules which adds a bitcoind launch option `-rpccookieperms` to set the file permissions of the cookie generated by bitcoin core.
Example usage to make the generated cookie group-readable: `./src/bitcoind -rpccookieperms=group`.
Accepted values for `-rpccookieperms` are `[owner|group|all]`. We let `fs::perms` handle platform-specific permissions changes.
ACKs for top commit:
achow101:
ACK 73f0a6cbd0
ryanofsky:
Code review ACK 73f0a6cbd0. Main change since last review is no longer throwing a skip exception in the rpc test on windows, so other checks can run after it, and overall test result is passing, not skipped. Also were clarifying renames and documentation improvements.
tdb3:
cr ACK 73f0a6cbd0
Tree-SHA512: e800d59a44aca10e1c58ca69bf3fdde9f6ccf5eab4b7b962645af6d6bc0cfa3a357701e409c8c60d8d7744fcd33a91e77ada11790aa88cd7811ef60fab86ab11
