These errors should never happen. However, when they do happen, it is
useful to log the correct error location (function name).
For example, this fixes an incorrect "ConnectBlock()" in
"WriteUndoDataForBlock".
These errors should never happen in normal operation. If they do,
knowing the FlatFilePos may be useful to determine if data corruption
happened. Also, handle the error pos.IsNull() as part of OpenUndoFile,
because it may as well have happened due to data corruption.
This mirrors the LogError behavior from ReadBlockFromDisk.
576828e732bacb61b608cd89f669a2936d3e8d00 ci: test-each-commit merge base optional (Sjors Provoost)
e9bfbb5414ab14ca14d8edcfdf77f28c9ed67c33 ci: forks can opt-out of CI branch push (Cirrus only) (Sjors Provoost)
Pull request description:
Maintainer note: `SKIP_BRANCH_PUSH=true` must be set in Cirrus for `bitcoin-core/gui` before merging this. See `https://cirrus-ci.com/github/bitcoin-core/gui` -> Settings.
---
I find myself making pull requests against my fork (mostly on top of https://github.com/bitcoin/bitcoin/pull/28983, or asking others to do so. Currently only the Github actions are run on forks, because we use self-hosted runners for the Cirrus tasks.
While setting up my own self-hosted runners for my fork, I ran into a number of issues. Some of those were addressed by https://github.com/bitcoin/bitcoin/pull/29441, but remaining issues are:
1. When PRs are opened in the fork, cirrus CI jobs are run twice because PRs and branches reside in the same repository, rather than a main repository and a fork repository, as is the case with bitcoin/bitcoin PRs. Fix this by adding a `SKIP_BRANCH_PUSH` configuration option that allows skipping CI runs not directly associated with a PR. The fix is a generalization of [#20328](https://github.com/bitcoin/bitcoin/pull/20328), which fixed a similar problem for the bitcoin-core/gui mirror repository, and it allows removing a hardcoded reference to that repository.
Github actions jobs will still run twice despite this change, see [#29274 (comment)](https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2188840483). Initially this PR tried to prevent that with b9fdd0dc75, but this had some potentially negative side effects, see [#29274 (comment)](https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457587805), so that commit was dropped for now.
2. When PRs are opened in the fork, the "test-each-commit" github action can fail due to not being able to find a recent merge commit. This problem doesn't happen in the bitcoin/bitcoin repository because branches in this repository used as the base for pull requests always point at merge commits.
This PR replaces https://github.com/bitcoin/bitcoin/pull/29259 using the self hosted workers via Cirrus instead of Github.
You can see this PR in action on this pull request to my fork: https://github.com/Sjors/bitcoin/pull/30
To test it yourself:
1. spin up at least two [self hosted runners](https://github.com/cirruslabs/cirrus-cli/blob/master/PERSISTENT-WORKERS.md). Either use a seperate VM for each, or give them their own user.
3. Install Podman and other CI dependencies (see .cirrus.yml)
4. Give Cirrus access to your fork at https://cirrus-ci.com/settings/github/YOU
5. Get a token from Cirrus and use it to start your worker(s)
6. Optionally set SKIP_BRANCH_PUSH=true ~and NO_ARM=true~ env variables (see .cirrus.yml)
make a pull request to your own fork, with this PR as the base branch
Security wise: when dealing with code from strangers on the internet, review it first before running the CI. There's a Cirrus check-box that requires approval for people without write access to trigger CI.
ACKs for top commit:
maflcko:
ACK 576828e732bacb61b608cd89f669a2936d3e8d00
ryanofsky:
Code review ACK 576828e732bacb61b608cd89f669a2936d3e8d00.
Tree-SHA512: fb6be2f228aa62f45a65ce5c613c979b6f387df396f9601ce4622b27aa317a66f198e7d7a194592b0bb397b32a2f50f8be47065834d74af4ea09407c5c8d306d
d93b79470916b1e6f85c55cc6beb1e41b382196f tests: improve wallet multisig descriptor test and docs (Michael Dietz)
Pull request description:
It is best to store all key origin information
(master key fingerprint and all derivation steps)
in the multisig descriptor. Being explicit with
this information should be beneficial if this approach is used with other wallets/signers (whether hardware or software). There is no harm including all of this with xpubs (if anything it simplifies the test code) and makes this example/docs more complete and safer incase it is referenced by others.
ACKs for top commit:
S3RK:
Code Review ACK d93b79470916b1e6f85c55cc6beb1e41b382196f
achow101:
ACK d93b79470916b1e6f85c55cc6beb1e41b382196f
Tree-SHA512: 0e5c4d13f060489405e6cf50c8a09911f5a0cee71023649235afd80a5e3aae38d52c6e12ad4660205b9357b09f45596941391bdcf6fceccbe07c4e5a1592a482
6ecda04fefad980872c72fba89844393f5581120 random: drop ad-hoc Shuffle in favor of std::shuffle (Pieter Wuille)
da28a26aae3178fb7663efbe20bb650857ace775 bench random: benchmark more functions, and add InsecureRandomContext (Pieter Wuille)
0a9bbc64c157a314e5472ecd98300e30b12d3fdf random bench refactor: move to new bench/random.cpp (Pieter Wuille)
Pull request description:
This adds benchmarks for various operations on `FastRandomContext` and `InsecureRandomContext`, and then removes the ad-hoc `Shuffle` functions, now that it appears that standard library `std::shuffle` has comparable performance. The other reason for keeping `Shuffle`, namely the fact that libstdc++ used self-move (which debug mode panics on) has been fixed as well (see https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1658344049).
ACKs for top commit:
achow101:
ACK 6ecda04fefad980872c72fba89844393f5581120
hodlinator:
ACK 6ecda04fefad980872c72fba89844393f5581120
dergoegge:
Code review ACK 6ecda04fefad980872c72fba89844393f5581120
Tree-SHA512: 2560b7312410581ff2b9bd0716e0f1558d910b5eadb9544785c972384985ac0f11f72d6b2797cfe2e7eb71fa57c30cffd98cc009cb4ee87a18b1524694211417
c9dacd958d7c1e98b08a7083c299d981e4c11193 test: Check that non empty version packet is ignored and no disconnection happens (stratospher)
997cc00b950a7d1b7f2a3971282685f4e81d87d2 test: Check that disconnection happens when AAD isn't filled (stratospher)
b5e6238fdbba5c777a5adfa4477dac51a82f4448 test: Check that disconnection happens when garbage sent/received are different (stratospher)
ad1482d5a20e6b155184a43d0724d2dcd950ce52 test: Check that disconnection happens when wrong garbage terminator is sent (stratospher)
e351576862471fc77b1e798a16833439e23ff0b4 test: Check that disconnection happens when >4095 garbage bytes is sent (stratospher)
e075fd131d668d9d1ba3c8566624481c4a57032d test: Introduce test types and modify v2 handshake function accordingly (stratospher)
7d07daa62311bdb0e2ce23d0b55f711f5088bd28 log: Add V2 handshake timeout (stratospher)
d4a1da8543522a213ac75761131d878eedfd4a5b test: Make global TRANSPORT_VERSION variable an instance variable (stratospher)
c642b08c4e45cb3a625a867ebd66c0ae51bde212 test: Log when the garbage is actually sent to transport layer (stratospher)
86cca2cba230c10324c6aedd12ae9655b83b2856 test: Support disconnect waiting for add_p2p_connection (stratospher)
bf9669af9ccc33dcade09bceb27d6745e9d9a75a test: Rename early key response test and move random_bitflip to util (stratospher)
Pull request description:
Add tests for the following v2 handshake scenarios:
1. Disconnection happens when > `MAX_GARBAGE_LEN` bytes garbage is sent
2. Disconnection happens when incorrect garbage terminator is sent
3. Disconnection happens when garbage bytes are tampered with
4. Disconnection happens when AAD of first encrypted packet after the garbage terminator is not filled
5. bitcoind ignores non-empty version packet and no disconnection happens
All these tests require a modified v2 P2P class (different from `EncryptedP2PState` used in `v2_p2p.py`) to implement our custom handshake behaviour based on different scenarios and have been kept in a single test file (`test/functional/p2p_v2_misbehaving.py`). Shifted the test in `test/functional/p2p_v2_earlykeyresponse.py` which is of the same pattern to this file too.
ACKs for top commit:
achow101:
ACK c9dacd958d7c1e98b08a7083c299d981e4c11193
mzumsande:
ACK c9dacd958d7c1e98b08a7083c299d981e4c11193
theStack:
Code-review ACK c9dacd958d7c1e98b08a7083c299d981e4c11193
Tree-SHA512: 90df81f0c7f4ecf0a47762d290a618ded92cde9f83d3ef3cc70e1b005ecb16125ec39a9d80ce95f99e695d29abd63443240cb5490aa57c5bc8fa2e52149a0672
de71d4dece604907afc4fc26b7788e9c1a4cbecb fuzz: improve utxo_snapshot target (Martin Zumsande)
Pull request description:
Add the possibility of giving more guidance to the creation of the metadata and/or coins, so that the fuzzer gets the chance
to reach more error conditions in ActivateSnapshot and sometimes successfully creates a valid snapshot.
This also changes the asserts for the success case that were outdated (after #29370) and only didn't result in a crash because the fuzzer wasn't able to reach this code before.
ACKs for top commit:
maflcko:
re-ACK de71d4dece604907afc4fc26b7788e9c1a4cbecb 🎆
fjahr:
utACK de71d4dece604907afc4fc26b7788e9c1a4cbecb
TheCharlatan:
ACK de71d4dece604907afc4fc26b7788e9c1a4cbecb
Tree-SHA512: 346974d594164544d8cd3df7d8362c905fd93116215e9f5df308dfdac55bab04d727bfd7fd001cf11318682d11ee329b4b4a43308124c04d64b67840ab8a58a0
fa5b8920be041380fbfa4c7b443918637423d7a0 rpc: Use untranslated error strings in loadtxoutset (MarcoFalke)
fa458657788cc142f14551d86604e3f434d56c0a refactor: Use named arguments to get path arg in loadtxoutset (MarcoFalke)
Pull request description:
Motivation:
* Some are not translated at all, anyway. See https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663631973
* For others translation is not yet needed, because they are not called by the GUI (yet)
* For others translations will never be needed, because they are RPC code. See https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663611194
Also, while touching this:
* Remove the trailing `\n`. See https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663647981
* Add back the path. See https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663666751
* Use named args to get the path.
ACKs for top commit:
fjahr:
re-ACK fa5b8920be041380fbfa4c7b443918637423d7a0
tdb3:
ACK fa5b8920be041380fbfa4c7b443918637423d7a0
ryanofsky:
Code review ACK fa5b8920be041380fbfa4c7b443918637423d7a0
Tree-SHA512: 46504dc5fd55a6274ef885dbe071aa9efb25bca247cd68cd86fb2ff066d70d295e0522e1fe42e63f1fdf7e4c89bd696220edaf06e33b804aba746492eafd852e
9e13ccc50eec9d2efe0f472e6d50dc822df70d84 psbt: Check non witness utxo outpoint early (Ava Chow)
Pull request description:
A common issue that our fuzzers keep finding is that outpoints don't exist in the non witness utxos. Instead of trying to track this down and checking in various individual places, do the check early during deserialization. This also unifies the error message returned for this class of problems.
ACKs for top commit:
maflcko:
lgtm ACK 9e13ccc50eec9d2efe0f472e6d50dc822df70d84
S3RK:
tACK 9e13ccc50eec9d2efe0f472e6d50dc822df70d84
dergoegge:
utACK 9e13ccc50eec9d2efe0f472e6d50dc822df70d84
Tree-SHA512: 81b8055b146c6358052226578ddfec0ae5bd877968c7f4f62dc3d6a684545ea568f37c7f1bd619918441af9e453ba8b26531a2280d218da37fa15480f1b45d0e
606a7ab862470413ced400aa68a94fd37c8ad3d3 kernel: De-globalize signature cache (TheCharlatan)
66d74bfc45ae0f743084475ac3bbfb4355bb6ec2 Expose CSignatureCache class in header (TheCharlatan)
021d38822c0e6a1b9497bcb20401c5c37e1bb84d kernel: De-globalize script execution cache hasher (TheCharlatan)
13a3661aba95b54b822c99ecbb695b14a22536d2 kernel: De-globalize script execution cache (TheCharlatan)
ab14d1d6a4a8ef5fe5013150e6c5ebcb5f5e4ea9 validation: Don't error if maxsigcachesize exceeds uint32::max (TheCharlatan)
Pull request description:
The validation caches are currently setup independently from where the rest of the validation code is initialized. This makes their ownership semantics unclear. There is also no clear enforcement on when and in what order they need to be initialized. The caches are always initialized in the `BasicTestingSetup` although a number of tests don't actually need them.
Solve this by moving the caches from global scope into the `ChainstateManager` class. This simplifies the usage of the kernel library by no longer requiring manual setup of the caches prior to using the `ChainstateManager`. Tests that need to access the caches can instantiate them independently.
---
This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
stickies-v:
re-ACK 606a7ab862470413ced400aa68a94fd37c8ad3d3
glozow:
reACK 606a7ab
ryanofsky:
Code review ACK 606a7ab862470413ced400aa68a94fd37c8ad3d3. Just small formatting, include, and static_assert changes since last review.
Tree-SHA512: e7f3ee41406e3b233832bb67dc3a63c4203b5367e5daeed383df9cb590f227fcc62eae31311029c077d5e81b273a37a88a364db3dee2efe91bb3b9c9ddc8a42e
fa8f53273c7e5965620d31a8c3fe5f223cb76888 refactor: Remove no longer needed clang-15 workaround for std::span (MarcoFalke)
9999dbc1bd171931f02266d7c1a5cfd39f49238e fuzz: Clarify Apple-Clang-16 workaround (MarcoFalke)
fa7462c67ab9b6d45484ce92b44d03f812627d6e build: Bump clang minimum supported version to 16 (MarcoFalke)
Pull request description:
Most supported operating systems ship with clang-16 (or later), so bump the minimum to that and allow new code to drop workarounds for previous clang bugs.
For reference:
* https://packages.debian.org/bookworm/clang-16
* https://packages.ubuntu.com/noble/clang (clang-18)
* CentOS-like 8/9 Stream: All Clang versions from 16 to 17
* FreeBSD 12/13: All Clang versions from 16 to 18
* OpenSuse Tumbleweed ships with https://software.opensuse.org/package/clang (`clang18`); No idea about OpenSuse Leap
On operating systems where the clang version is not shipped by default, the user would have to use GCC, or install clang in a different way. For example:
* https://packages.debian.org/bookworm/g++ (g++-12)
* https://packages.ubuntu.com/jammy/g++ (g++-11)
* https://apt.llvm.org/, or nix, or guix, or compile clang from source, ...
**Ubuntu 22.04 LTS does not ship with clang-16**, so one of the above workarounds is needed there.
macOS 13 is unaffected, and the previous minimum requirement of Xcode15.0 remains, see also b1ba1b178f/.github/workflows/ci.yml (L93). For macOS 11 (Big Sur) and 12 (Monterey) you need to install a more recent version of llvm, this remains unchanged as well, see b1ba1b178f/doc/build-osx.md (L54).
ACKs for top commit:
hebasto:
ACK fa8f53273c7e5965620d31a8c3fe5f223cb76888, I have reviewed the code and it looks OK.
TheCharlatan:
Re-ACK fa8f53273c7e5965620d31a8c3fe5f223cb76888
stickies-v:
ACK fa8f53273c7e5965620d31a8c3fe5f223cb76888
Tree-SHA512: 18b79f88301a63bb5e367d2f52fffccd5fb84409061800158e51051667f6581a4cd71d4859d4cfa6d23e47e92963ab637e5ad87e3170ed23b5bebfbe99e759e2
6af51e819847e737449609daa214e16f9453e85d 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 6af51e819847e737449609daa214e16f9453e85d
TheCharlatan:
ACK 6af51e819847e737449609daa214e16f9453e85d
glozow:
ACK 6af51e819847e737449609daa214e16f9453e85d
willcl-ark:
ACK 6af51e819847e737449609daa214e16f9453e85d
stickies-v:
ACK 6af51e819847e737449609daa214e16f9453e85d
Tree-SHA512: 9884046c70dcad996276931b6d154f0330200332403828f34f7f7b285fc0e770ba7b25056131ab24dcb8a4b18f58d31633aa17fbb09b0eaea8a29e28fca10ec4
46819f5df6de2a860c3ec87d55527b617a3b128f 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 46819f5df6de2a860c3ec87d55527b617a3b128f
ryanofsky:
Code review ACK 46819f5df6de2a860c3ec87d55527b617a3b128f. Nice catch!
furszy:
ACK 46819f5df6de2a860c3ec87d55527b617a3b128f
luke-jr:
utACK 46819f5df6de2a860c3ec87d55527b617a3b128f
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.
Move its ownership to the ChainstateManager class.
Next to simplifying usage of the kernel library by no longer requiring
manual setup of the cache prior to using validation code, it also slims
down the amount of memory allocated by BasicTestingSetup.
Use this opportunity to make SignatureCache RAII styled
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Add the possibility of giving more guidance to the creation of the
metadata and/or coins, so that the fuzzer gets the chance
to reach more error conditions in ActivateSnapshot and sometimes
successfully creates a valid snapshot.
This also changes the asserts for the success case that were outdated,
and only didn't result in a crash because the fuzzer wasn't able
to reach this code before.
Move its ownership to the ChainstateManager class.
Next to simplifying usage of the kernel library by no longer requiring
manual setup of the cache prior to using validation code, it also slims
down the amount of memory allocated by BasicTestingSetup.
f59e9057e2aa596b54cf9e85bab35c3ead137547 depends: switch libevent to CMake (Cory Fields)
Pull request description:
Switches libevent in depends to be built with CMake.
ACKs for top commit:
TheCharlatan:
ACK f59e9057e2aa596b54cf9e85bab35c3ead137547
willcl-ark:
ACK f59e9057e2aa596b54cf9e85bab35c3ead137547
Tree-SHA512: 875bf9bc57653c78775a1f8192a2c964fea8f4490d733ff796d9efb00e786f0ca9a7c1a3fd610cda032273c4f2ae06394585b03567d5f241ab073c83a47cf927
33c48c106cf03ff62994ff5777a775982d90eab9 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 33c48c106cf03ff62994ff5777a775982d90eab9
fjahr:
ACK 33c48c106cf03ff62994ff5777a775982d90eab9
Tree-SHA512: 7a3568d5b7af45efa7bf54bae7bac1f00dc99bc9d47a744d73594f283c952be9500168f680d72f4aee09761da4e878ddca83ba675cdea8ee9e44eeff00ac09da
ce8094246ee95232e9d84f7e37f3c0a43ef587ce random: replace construct/assign with explicit Reseed() (Pieter Wuille)
2ae392d561ecfdf81855e6df6b9ad3d8843cdfa2 random: use LogError for init failure (Pieter Wuille)
97e16f57042cab07e5e73f6bed19feec2006e4f7 tests: make fuzz tests (mostly) deterministic with fixed seed (Pieter Wuille)
2c91330dd68064e402e8eceea3df9474bb7afd48 random: cleanup order, comments, static (Pieter Wuille)
8e31cf9c9b5e9fdd01e8b220c08a3ccde5cf584c net, net_processing: use existing RNG objects more (Pieter Wuille)
d5fcbe966bc501db8bf6a3809633f0b82e6ae547 random: improve precision of MakeExponentiallyDistributed (Pieter Wuille)
cfb0dfe2cf0b46f3ea9e62992ade989860f086c8 random: convert GetExponentialRand into rand_exp_duration (Pieter Wuille)
4eaa239dc3e189369d59144b524cb2808cbef8c3 random: convert GetRand{Micros,Millis} into randrange (Pieter Wuille)
82de1b80d95fc9447e64c098dcadb6b8a2f1f2ee net: use GetRandMicros for cache expiration (Pieter Wuille)
ddc184d999d7e1a87efaf6bcb222186f0dcd87ec random: get rid of GetRand by inlining (Pieter Wuille)
e2d1f84858485650ff743753ffa5c679f210a992 random: make GetRand() support entire range (incl. max) (Pieter Wuille)
810cdf6b4e12a1fdace7998d75b4daf8b67d7028 tests: overhaul deterministic test randomness (Pieter Wuille)
6cfdc5b104caf9952393f9dac2a36539d964077f random: convert XoRoShiRo128PlusPlus into full RNG (Pieter Wuille)
8cc2f45065fc1864f879248d1e1444588e27076b random: move XoRoShiRo128PlusPlus into random module (Pieter Wuille)
8f5ac0d0b608bdf396d8f2d758a792f869c2cd2a xoroshiro128plusplus: drop comment about nonexisting copy() (Pieter Wuille)
8924f5120f66269c04633167def01f82c74ea730 random: modernize XoRoShiRo128PlusPlus a bit (Pieter Wuille)
ddb7d26cfd96c1f626def4755e0e1b5aaac94d3e random: add RandomMixin::randbits with compile-known bits (Pieter Wuille)
21ce9d8658fed0d3e4552e8b02a6902cb31c572e random: Improve RandomMixin::randbits (Pieter Wuille)
9b14d3d2da05f74ffb6a2ac20b7d9efefbe29634 random: refactor: move rand* utilities to RandomMixin (Pieter Wuille)
40dd86fc3b60d7a67a9720a84a685f16e3f05b06 random: use BasicByte concept in randbytes (Pieter Wuille)
27cefc7fd6a6a159779f572f4c3a06170f955ed8 random: add a few noexcepts to FastRandomContext (Pieter Wuille)
b3b382dde202ad508baf553817c5b38fdd2d4a0c random: move rand256() and randbytes() to .h file (Pieter Wuille)
493a2e024e845e623e202e3eefe1cc2010e9b514 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 ce8094246ee95232e9d84f7e37f3c0a43ef587ce
maflcko:
re-ACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce 🐈
hodlinator:
ACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce
dergoegge:
utACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce
Tree-SHA512: 79bc0cbafaf27e95012c1ce2947a8ca6f9a3c78af5f1f16e69354b6fc9b987a28858adf4cd356dc5baf21163e9af8dcc24e70f8d7173be870e8a3ddcdd47c02c
dea7afd5e4c2fda7def2e06cfc44dbd617d7cdc2 lint: remove unneeded trailing line fix (willcl-ark)
4d942547a8155cca4fe2f68afccdb822a174ab1a lint: ignore files ignored by git in mlc (willcl-ark)
Pull request description:
Updating to MLC v0.18.0 includes a new feature which will ignore all files ignored by git: `mlc --gitignore`.
This helps avoid false-positives flagged by this linter in non-project files, such as a developer might expect to have in their working directory (e.g. guix-builds, python venvs, etc.)
Top commit has no ACKs.
Tree-SHA512: 1752448e0c85abd3c73570a17cc69294de2248d7773c6499833ae33806f6c03f3f345261aa7b855a557b45982fbdcb8190e758d087c43b4fb0254fbb39173432
Updating to MLC v0.18.0 includes a new feature which will ignore all
files ignored by git: `--gitignore`.
This helps avoid false-positives flagged by this linter in non-project
files, such as a developer might expect to have in their directory (e.g.
guix-builds, python venvs, etc.)
2342b46c451658a418f8e28e50b2ad0e5abd284d test: Add coverage for getchaintxstats in assumeutxo context (Fabian Jahr)
faf2a6750b2da97a18c48a3acf9c9da2aebe86d0 rpc: Reorder getchaintxstats output (MarcoFalke)
fa2dada0c9ab61266bcca86fcd28ced873976916 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 2342b46c451658a418f8e28e50b2ad0e5abd284d
achow101:
ACK 2342b46c451658a418f8e28e50b2ad0e5abd284d
mzumsande:
ACK 2342b46c451658a418f8e28e50b2ad0e5abd284d
Tree-SHA512: 931cecc40ee5dc0f96be728db7eb297155f8343076cd29c8b8c050c99fd1d568b80f54c9459a34ca7a9489c2474c729796d00eeb1934d6a9f7b4d6a53e3ec430
926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746 [doc] add release note for TRUC (glozow)
19a9b90617419f68d0f1c90ee115b5220be99a16 use version=3 instead of v3 in debug strings (glozow)
881fac8e609be17eb71bd9a54c0284b304e2e2e2 scripted-diff: change names from V3 to TRUC (glozow)
a573dd261748d2a80560f73db08f7dca788c7fcf [doc] replace mentions of v3 with TRUC (glozow)
089b5757dff39a9a06cdb625aaced9beeb72958d rename mempool_accept_v3.py to mempool_truc.py (glozow)
f543852a89d93441645250c40c3980aeb0c3b664 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 926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746
ismaelsadeeq:
Code review ACK 926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746
Tree-SHA512: 16c88add0a29dc6d1236c4d45f34a17b850f6727b231953cbd52eb9f7268d1d802563eadfc8b7928c94ed3d7a615275dd103e57e81439ebf3ba2b12efa1e42af
2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28 test: Remove unnecessary restart in assumeutxo test (Fabian Jahr)
19ce3d407ef546fa50d18b2ffbd67b7417797064 assumeutxo: Check snapshot base block is not marked invalid (Fabian Jahr)
80315c011863d69e7785673283e4c9033fbcd5ac 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 2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28. 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 2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28
Tree-SHA512: 5328dd88c3c7be3f1be97c9eef52ac3666c27188c30a798b3e949f3ffcb83be075127c107e4046f7f39f961a79911ea3d61b61f3c11e451b3e4c541c264eeed4
323ce303086d42292ed9fe7c98e8b459bdf6d1a6 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 323ce303086d42292ed9fe7c98e8b459bdf6d1a6
achow101:
ACK 323ce303086d42292ed9fe7c98e8b459bdf6d1a6
tdb3:
re ACK 323ce303086d42292ed9fe7c98e8b459bdf6d1a6
furszy:
utACK 323ce303086
Tree-SHA512: c2a0aab429646453ad0470956529f1cac8c38778c4c53f82c92c6cbaaaeb69f3d3603c0014ff097844b151e9da7caa2371a4676244caea96527cb540e66825a3
8ec24bdad89e2a72c394060ba5661a91f374b874 test: Added coverage to Block not found error using gettxoutsetinfo (kevkevinpal)
Pull request description:
#### Description
There were no tests that checked for the `Block not found` error called in `ParseHashOrHeight` when using `gettxoutsetinfo`, this change adds coverage to it.
You can see there are no tests that do the following by doing the below
`grep -nri "Block not found.*gettxoutsetinfo" ./test/functional/`
which leads to no results
ACKs for top commit:
achow101:
ACK 8ec24bdad89e2a72c394060ba5661a91f374b874
tdb3:
ACK 8ec24bdad89e2a72c394060ba5661a91f374b874
kristapsk:
ACK 8ec24bdad89e2a72c394060ba5661a91f374b874
brunoerg:
crACK 8ec24bdad89e2a72c394060ba5661a91f374b874
alfonsoromanz:
Re ACK 8ec24bdad89e2a72c394060ba5661a91f374b874
Tree-SHA512: 2c61c681e7304c679cc3d7dd13af1b795780e85716c25c7423d68104e253d01271e048e21bc21be35dbc7ec1a4fde94e439542f3cfd669fe5a16478c5fa982ab
e38eadb2c2d93d2ee3c9accb649b2de144b3732e test: change comments to `self.log.info` for `test_addnode_getaddednodeinfo` (brunoerg)
c838e3b6106adfe3fe3173aaf5b0a7dee023adce test: add coverage for `node` field of `getaddednodeinfo` RPC (brunoerg)
Pull request description:
We currently do not test a successful call to `getaddednodeinfo` filtering by `node`, we only test it with an unknown address and checks whether it fails. This PR adds coverage to it.
ACKs for top commit:
kevkevinpal:
ACK [e38eadb](e38eadb2c2)
achow101:
ACK e38eadb2c2d93d2ee3c9accb649b2de144b3732e
tdb3:
re ACK e38eadb2c2d93d2ee3c9accb649b2de144b3732e
BrandonOdiwuor:
Code Review ACK e38eadb2c2d93d2ee3c9accb649b2de144b3732e
rkrux:
tACK [e38eadb](e38eadb2c2)
Tree-SHA512: e9f768b7aa86e58b0b0ced089ead57040ff9a5204493da1ab99c8bc897b6dcdce7c856855f74c52010fceef19af1e12a39eee9f8f2e7294b42476b6f980fe754
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.
f1478c05458562a9bef5c2ba43959d758e7b4745 mempool: move LoadMempool/DumpMempool to node (Cory Fields)
6d242ff1e9ca02fd8f5cb3ffe82dfb48a52366cc 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 f1478c05458562a9bef5c2ba43959d758e7b4745
glozow:
ACK f1478c0545
stickies-v:
ACK f1478c05458562a9bef5c2ba43959d758e7b4745
Tree-SHA512: 5825da0cf2e67470524eb6ebe397eb90755a368469a25f184df99ab935b3eb6d89eb802b41a6c3661e869bba3bbfa8ba9d95281bc75ebbf790ec5d9d1f79c66f
9ec2c53701a391629b55aeb2804e8060d2c453a4 Revert "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner)
Pull request description:
As suggested in https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200786670, this PR reverts the recently added test #30362 that causes frequent CI failures. A TODO is added in the functional test file to re-add it later when the race condition is fixed.
ACKs for top commit:
mzumsande:
utACK 9ec2c53701a391629b55aeb2804e8060d2c453a4
brunoerg:
utACK 9ec2c53701a391629b55aeb2804e8060d2c453a4
tdb3:
ACK 9ec2c53701a391629b55aeb2804e8060d2c453a4
Tree-SHA512: df211ab194dc47f2ff8192f3827382974db922ed9fa54bc44fac75de4edfb3af43c1340cd5434b15b0b573f7b0ddd4451a0bbbbd7deaf7f4244e4865b9d5977e
