f5647c6c5ae85e9469cfc5df6fcac23752e1695a depends: fix libevent _WIN32_WINNT usage (fanquake)
Pull request description:
Starting with version 13.x, the mingw headers will define the value of
`NTDDI_VERSION`, based on the value of `_WIN32_WINNT`, if that version is <
Windows 10. Given that libevent was undefining our `_WIN32_WINNT`, and
redefining it to a value < Windows 10 (`0x0501`), `NTDDI_VERSION` was also
being defined to that value, leading to functions not being exposed in
the mingw-w64 headers; see here: 9c2668ef77/mingw-w64-headers/include/iphlpapi.h (L36-L41).
Imports a commit from usptream ([a14ff91254f40cf36e0fee199e26fb11260fab49](a14ff91254)).
Fixes#32707.
ACKs for top commit:
willcl-ark:
crACK f5647c6c5ae85e9469cfc5df6fcac23752e1695a
Tree-SHA512: eb429457a4af6191dd27ef3d1087667c5304ff0f49d4c6824883651e3c2dbab5d9784fa1f170402f23cd9238005c5214e0a71a4160562a59dfa35618dc702132
4bb4c865999bfa0b0cb5aa806cf62dbf982fcfe9 test: document HOST for get_previous_releases.py (Sjors Provoost)
609203d5075c5083c0922174e1acfe6bf3279600 test: stop signing previous releases >= v28.2 (Sjors Provoost)
c6dc2c29f8288065096e8ac145c1d8acfd5c8151 test: replace v28.0 with notarized v28.2 (Sjors Provoost)
5bd73d96a3a7914b2345260d3460156c04f3f899 test: fix macOS detection (Sjors Provoost)
Pull request description:
Since https://github.com/bitcoin/bitcoin/pull/31407 macOS guix builds are signed and notarized. This was included in v29 and backported to 28.x.
This PR bumps the v28.0 previous release binary to v28.2 and adjusts the test that uses it. Additionally it no longer manually code signs binaries >= v28.2.
While testing on an M4 mac and redownloading all the binaries, I noticed that `platform == "arm64-apple-darwin"` doesn't actually work. This initially used `args.platform` in #26694, but that was changed to just `platform` in #32219.
So the first commit switches this to use `args.host`. I manually tested on Intel macOS 13.7.6 that code-signing still isn't needed there (when downloading using a script).
Also documented that you can set `HOST`.
ACKs for top commit:
m3dwards:
ACK 4bb4c865999bfa0b0cb5aa806cf62dbf982fcfe9
maflcko:
review ACK 4bb4c865999bfa0b0cb5aa806cf62dbf982fcfe9 🚏
Tree-SHA512: b4803d39a21cb622fd2388a0528b76d2b502956e2505385d3da201143b0afcf6f9d71c8c28937f27b70d2588fb6da677da058bdcd67b90fb53617acc3a727818
61e800e75cffa256ccdbc2ffc7a1739c00880ce0 test: headers sync timeout (stringintech)
Pull request description:
When reviewing PR #32051 and considering which functional tests might need to be adapted/extended accordingly, I noticed there appears to be limited functional test coverage for header sync timeouts and thought it might be helpful to add one.
This test attempts to cover two scenarios:
1. **Normal peer timeout behavior:** When a peer fails to respond to initial getheaders requests within the timeout period, it should be disconnected and the node should attempt to sync headers from the next available peer.
2. **Noban peer behavior:** When a peer with noban privileges times out, it should remain connected while the node still attempts to sync headers from other peers.
ACKs for top commit:
maflcko:
re-ACK 61e800e75cffa256ccdbc2ffc7a1739c00880ce0 🗝
stratospher:
reACK 61e800e7.
Tree-SHA512: b8a867e7986b6f0aa00d81a84b205f2bf8fb2e6047a2e37272e0244229d1f43020e9031467827dabbfe7849a91429f2685e00a25356e2ed477fa1a035fa0b1fd
28416f367a5d720d89214aa8ef94013caa1d1a40 test: fix intermittent failure in rpc_invalidateblock.py (stratospher)
Pull request description:
resolves#32965.
node1 (with 24 blocks) causes node0 (with 6 blocks + 1 extra header) to silently reorg. so move the subtest to a point before the 20 blocks are generated so that node1's state doesn't cause node0 to silently reorg.
ACKs for top commit:
maflcko:
lgtm ACK 28416f367a5d720d89214aa8ef94013caa1d1a40
mzumsande:
Code Review ACK 28416f367a5d720d89214aa8ef94013caa1d1a40
Tree-SHA512: f6cc682b8e5416125f887c094d5e291dd37f0bfc41d7c0de218f3e24fa1ea0cd642f7a1e362f3127f68cde725a67f3054501326b9bd25f0caa9a05de7d0052b0
c18bf0bd9be632e54591493a5309e9ed4020f2e5 refactor: cleanup index logging (Sjors Provoost)
Pull request description:
This PR removes the use of `__func__` from index logging, since we have `-logsourcelocations`.
It also improves readability by putting `GetName()` in a more logical place.
Before
> coinstatsindex: best block of the index not found. Please rebuild the index.
After:
> best block of coinstatsindex not found. Please rebuild the index.
I found myself maintaining this commit as part of https://github.com/Sjors/bitcoin/pull/86, but since that might never land here, it seemed better to split it into its own PR (or get rid of it).
ACKs for top commit:
l0rinc:
Lightweight code review ACK c18bf0bd9be632e54591493a5309e9ed4020f2e5
maflcko:
review ACK c18bf0bd9be632e54591493a5309e9ed4020f2e5 🚣
Tree-SHA512: 755948371e3ff7a5515b63ce48075631ec7868d69c3c1469176d5be0e8b28e1c071e206ae3f7320f87d8c441f815894acfef61621f05795b5ff6b8a5a3031e3b
node1 (with 24 blocks) causes node0 (with 6 blocks) to silently
reorg. so move the subtest to a point before the 20 blocks are
generated so that node1's state doesn't cause node0 to silently
reorg.
44f3bae300dcafbe53f9b07e6cc22a112833e579 depends: Force `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE` (Hennadii Stepanov)
Pull request description:
When using CMake policies 3.14 and below, the `export(PACKAGE)` command by default populates the user package registry, which is stored outside the build tree. Setting the `CMAKE_EXPORT_NO_PACKAGE_REGISTRY` variable disables this side effect.
In CMake 3.15 and later, this behavior is disabled by default, and the variable has no effect.
This PR forces `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE` globally, rather than managing it for each dependency package individually rather. It may be reverted once all CMake-based packages have updated to policies 3.15 or newer.
Fixes https://github.com/bitcoin/bitcoin/issues/32938.
ACKs for top commit:
fanquake:
ACK 44f3bae300dcafbe53f9b07e6cc22a112833e579
Tree-SHA512: 0aac398b7182e80185b064d59f81aece4d8477a609fad9cc3fee317da2aff43b66ef7db1efec0135b4f0feaad23b1db664e33bd035fe659712c5b2a9bf2d6fb6
fad191ff48b15832a90c19d560a7c0525c146be3 ci: Avoid cd into build dir (MarcoFalke)
Pull request description:
Changing into the build dir is confusing and brittle, because the following commands implicitly assume it. So they could break on unrelated changes.
The changes are required for stuff like:
* cmake presets (see https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344031208)
* meta ci tests (like https://github.com/bitcoin/bitcoin/pull/32874)
So remove the `cd` and just make the build dir explicit.
ACKs for top commit:
hebasto:
ACK fad191ff48b15832a90c19d560a7c0525c146be3, I have reviewed the code and it looks OK.
Tree-SHA512: a88a9341445ffe28a0dac3815f235ec8eb0459d10a91a80829fd3184762d3c807d0f68c56243b20c04a6efa5becd8a7fad568f43c2b1e6af1ff8ba07b140ef87
12a6959892cb24b940b3579828f2066651572153 cmake: Drop no longer necessary "cmakeMinimumRequired" object (Hennadii Stepanov)
Pull request description:
The minimum required CMake version is 3.22:6a13a6106e/CMakeLists.txt (L10)
ACKs for top commit:
fanquake:
ACK 12a6959892cb24b940b3579828f2066651572153 - has been unneeded since it was introduced (minimum was already 3.22).
Tree-SHA512: 26f97662bfe52986e19e38dbf4ab8e1e7558bc78c3a65593cbecd1f35887bba7a9f7d8a3d08ccfab8396f41c2334cdad5b0e503999a759cfa158d3bb8d0d14d7
84ef5524d5abaf07cc9970f42e74ee15bd381e3d fix spelling in tor.md docs (stutxo)
Pull request description:
This PR is to fix some spelling mistakes i found of the word occurrences! there are two occurrences of this mistake.
thanks!
ACKs for top commit:
maflcko:
lgtm ACK 84ef5524d5abaf07cc9970f42e74ee15bd381e3d
willcl-ark:
ACK 84ef5524d5abaf07cc9970f42e74ee15bd381e3d
delta1:
ACK 84ef5524d5abaf07cc9970f42e74ee15bd381e3d
Tree-SHA512: 4ba71b772fdc8cf36ada7493d29fb5b312a7a6085099347162eb3495db4de984b0417b7861f2927c617cbd552741356e26688479601bdf7e835c15e097aa28f3
8f766f39df3e312f79f461b2accc2f9c90fa6338 ci: enable -Werror=dev (fanquake)
7b420ca8341a68e196a4f6fbd03104dc27b166ee guix: configure with -Werror=dev (fanquake)
44097ddb19139d07a905d1238fe67e53f8343d54 cmake: enable -Werror=dev in dev-mode preset (fanquake)
Pull request description:
Pass `-Werror=dev` in the CI, Guix and the `dev-mode` preset.
https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-Werror:
> Make developer warnings errors.
> Make warnings that are meant for the author of the CMakeLists.txt files errors. By default this will also turn on deprecated warnings as errors.
Pulled out of #32865.
ACKs for top commit:
Sjors:
re-ACK 8f766f39df3e312f79f461b2accc2f9c90fa6338
hebasto:
ACK 8f766f39df3e312f79f461b2accc2f9c90fa6338, tested on Ubuntu 24.04.
Tree-SHA512: 0fa321b77d2519b5249d90590664c4e5938ac86209b068658647adf97ab55ea4d54c913aae2f622385fe2f41d7c851cd5d7371905fdad38b66cb124371e16ac7
a60f863d3e276534444571282f432b913d3967db scripted-diff: Replace GenTxidVariant with GenTxid (marcofleon)
c8ba1995986323cd9e76097acc1f15eed7c60943 Remove old GenTxid class (marcofleon)
072a198ea4bc9f1e8449cd31e55d397b75ce4ad5 Convert remaining instances of GenTxid to GenTxidVariant (marcofleon)
1b528391c79497ae19f7e481439e350533c7cd1a Convert `txrequest` to GenTxidVariant (marcofleon)
bde4579b0780aa3754af35beffbcfeb31f28045b Convert `txdownloadman_impl` to GenTxidVariant (marcofleon)
c876a892ec0b04851bea0a688d7681b6aaca4cb7 Replace GenTxid with Txid/Wtxid overloads in `txmempool` (marcofleon)
de858ce2bea83c53635dee9a49c8c273a12440dd move-only: make GetInfo a private CTxMemPool member (stickies-v)
eee473d9f3019a0ea4ebbc9c41781813ad574a86 Convert `CompareInvMempoolOrder` to GenTxidVariant (marcofleon)
243553d59071f3e43a42f3809706790495b17ffc refactor: replace get_iter_from_wtxid with GetIter(const Wtxid&) (stickies-v)
fcf92fd640eae60d1f601136a4e1c9de8ccb68b5 refactor: make CTxMemPool::GetIter strongly typed (marcofleon)
11d28f21bb8f0c3094934b3fef45871f73bb216a Implement GenTxid as a variant (marcofleon)
Pull request description:
Part of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
This PR changes the GenTxid class to a variant, which holds both Txids and Wtxids. This provides compile-time type safety and eliminates the manual type check (bool m_is_wtxid). Variables that can be either a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256.
ACKs for top commit:
w0xlt:
ACK a60f863d3e
dergoegge:
Code review ACK a60f863d3e276534444571282f432b913d3967db
maflcko:
review ACK a60f863d3e276534444571282f432b913d3967db 🎽
theStack:
Code-review ACK a60f863d3e276534444571282f432b913d3967db
Tree-SHA512: da9b73b7bdffee2eb9281a409205519ac330d3336094d17681896703fbca8099608782c9c85801e388e4d90af5af8abf1f34931f57bbbe6e9674d802d6066047
94931656b52fddb7c03b29685844d85c008cf6ca cmake: Use newer signature of `qt6_add_lrelease` when available (Hennadii Stepanov)
Pull request description:
See Qt docs here: https://doc.qt.io/qt-6/qtlinguist-cmake-qt-add-lrelease.html.
Fixes https://github.com/bitcoin/bitcoin/issues/32710.
ACKs for top commit:
fanquake:
ACK 94931656b52fddb7c03b29685844d85c008cf6ca
Tree-SHA512: bf0320306967164374499dd0be122473799e830fdff5e070ef13f87af3c14a3b799d90afb423881edd7eea17c13d27af8ced381bbb3cd149353b31b3990dde67
fa894b0f3e13dcc55fd42cec2c56d4aa2115194d log: Properly log warnings with warn loglevel in addrdb (MarcoFalke)
Pull request description:
The logging in addrdb is confusing, because it uses `LogPrintf` (info level) to log warnings.
Fix this by properly using the `warn` level, where needed. Also, drop unused trailing `\n` while touching the lines.
ACKs for top commit:
stickies-v:
ACK fa894b0f3e13dcc55fd42cec2c56d4aa2115194d
dergoegge:
utACK fa894b0f3e13dcc55fd42cec2c56d4aa2115194d
Tree-SHA512: 96d3823623ea8e1698e8cb541ca97cbab7b2a9934b2f894884171045abbca7be796f07965082e997001c97d06d1e0c4d13b29354eb4fe71c3a2ee680eada5516
- don't log function name
- take into account that GetName() always ends with " index"
- replace deprecated LogPrintf with LogInfo
- remove trailing \n
- adjusted log level where needed
fa8862723c14cdd471bbffc7a4897ece2e6d8a7f fuzz: CheckGlobals in init (MarcoFalke)
fa26bfde988b1940e6b0d21accc60fbc4e45f9b1 test: Avoid resetting mocktime in testing setup (MarcoFalke)
fa6b45fa8ec8248544d22ba8429be8f6df19024a Add SetMockTime for time_point types (MarcoFalke)
Pull request description:
(Tracking issue https://github.com/bitcoin/bitcoin/issues/29018)
During fuzzing, `AppInitParameterInteraction` may actually disable a previously set mocktime. This is confusing and can also cause non-determinism.
Fix this issue, by
* fixing the erroneous `-mocktime` parsing in `AppInitParameterInteraction`.
* adding the missing `SetMockTime` calls to the affected fuzz init functions.
* adding a `CheckGlobals` to the fuzz init, to prevent this issue in the future.
This can be tested by
* Cherry-picking the `CheckGlobals`-commit onto current master and observing a fuzz failure in the touched fuzz targets.
* Reverting the touched fuzz fixups and observing a fuzz failure for each target.
ACKs for top commit:
w0xlt:
ACK fa8862723c
dergoegge:
utACK fa8862723c14cdd471bbffc7a4897ece2e6d8a7f
Tree-SHA512: 5a9400f0467c82fa224713af4cc2b525afbefefc7c3f419077110925ad7af6c7fda3dcd2b50f7facf0ee7df2547c6ac20336906d707adcdfd1d652a9d9a735fe
Starting with version 13.x, the mingw headers will define the value of
NTDDI_VERSION, based on the value of _WIN32_WINNT, if that version is <
Windows 10. Given that libevent was undefining our _WIN32_WINNT, and
redefining it to a value < Windows 10 (0x0501), NTDDI_VERSION was also
being defined to that value, leading to functions not being exposed in
the mingw-w64 headers; see here:
9c2668ef77/mingw-w64-headers/include/iphlpapi.h (L36-L41).
Imports a commit from usptream (a14ff91254f40cf36e0fee199e26fb11260fab49).
Fixes#32707.
f43571010e3853e83a21aa4774b1c8da47b5d961 Resolve guix non-determinism with emplace_back instead of push_back (Ava Chow)
Pull request description:
For some reason, building x86_64-w64-mingw32 on x86_64 and aarch64 results in a single instruction difference which can be traced down to prevector.h:174. The ultimate caller of this is the copy constructor for a prevector that ends up being called by std::vector::push_back in walletmodel.cpp:183. By replacing the push_back with an emplace_back, somehow this non-determinism goes away.
Closes#32923
ACKs for top commit:
l0rinc:
code review ACK f43571010e3853e83a21aa4774b1c8da47b5d961
Sjors:
utACK f43571010e3853e83a21aa4774b1c8da47b5d961
maflcko:
lgtm ACK f43571010e3853e83a21aa4774b1c8da47b5d961
Tree-SHA512: 5bf0571f32cb72efc0c533e16d2704cfc3a79bcef2943f0892743572808610fb00ca8ab41223897536f8e5090bf4030735be910942de8116652d02bc3f231e2e
83bb41455715a9e05320ba791987204031626c10 test: less ambiguous error if bitcoind is missing (Sjors Provoost)
Pull request description:
Before this change, when a functional test is run without building the source, the error message suggested that previous release binaries were missing.
When no previous release version is set, make the error message more specifically about bitcoind.
To test, try this before and after:
```sh
git clean -dfx
cmake -B build
build/test/functional/mining_basic.py
cmake --build build
build/test/functional/mining_basic.py
build/test/functional/wallet_backwards_compatibility.py
test/get_previous_releases.py
build/test/functional/wallet_backwards_compatibility.py
```
ACKs for top commit:
achow101:
ACK 83bb41455715a9e05320ba791987204031626c10
janb84:
ACK 83bb41455715a9e05320ba791987204031626c10
w0xlt:
ACK 83bb414557
Tree-SHA512: c6df65019de99d6c214951cf70944c4ddca9b635c5ab60ac2c47e4589478e9c65d5e079c394ace9b470a7eaeea3c9cf68b7246dd413e802c4a1e071913a7fc32
150b5c99ca11885ef15d04139d919d734e2c211a wallet: replace `reload_wallet` with inline functionality (rkrux)
0f86da382d3fdca6fc69ff277794acbc3f1e928d wallet: remove dead code in legacy wallet migration (rkrux)
Pull request description:
A discussion on a previous [PR 32481](https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2145152084) related to legacy wallet dead
code removal made me realize that checking if the legacy
wallet was loaded prior to the start of the migration is not
required ever since legacy wallets can't be loaded in the first
place. I also verified that the `load_on_start` persistent
setting can also not cause the legacy wallets to be loaded, which
further makes the case for removal of the above mentioned checks
during migration.
The current test coverage also shows these lines uncovered.
ACKs for top commit:
achow101:
ACK 150b5c99ca11885ef15d04139d919d734e2c211a
furszy:
ACK 150b5c99ca11885ef15d04139d919d734e2c211a
Tree-SHA512: 9bc7043cac1f4051228557208895e43648de3c7ffae6860c0676d1aa2db3a8ed3a09d1f9defacd96ca50bbb9699ba86652ccb0c5e55cc88be248a1fe727c13d9
d7fca5c171f450621112457ea6f7c99b2e21d354 clusterlin: add big comment explaning the relation between tests (Pieter Wuille)
b64e61d2de65026c46f70cb91e3cb2213c336be0 clusterlin: abstract try-permutations into ExhaustiveLinearize function (Pieter Wuille)
1fa55a64ed18a0a3defbed33f6bd355929f67b48 clusterlin tests: verify that chunks are minimal (Pieter Wuille)
da23ecef29b7e6a1e03de9697a6bdf6fbe944ef7 clusterlin tests: support non-empty ReadTopologicalSubset() (Pieter Wuille)
94f3e17c33e6fe63c4f791de7aacfb912059e394 clusterlin tests: compare with fuzz-provided linearizations (Pieter Wuille)
5f92ebee0d2401d0d7b5c3466d66a4b09e51ccb0 clusterlin tests: compare with fuzz-provided topological sets (Pieter Wuille)
6e37824ac390835d3d9c82d197f58e992ccc584f clusterlin tests: optimize clusterlin_simple_linearize (Pieter Wuille)
98c1c88b6f8d928b3893e2910ce0c400a6fd1928 clusterlin tests: separate testing of SimpleLinearize and Linearize (Pieter Wuille)
10e90f7aef9cff6fa63dc15adbe22e68f76aac58 clusterlin tests: make SimpleCandidateFinder always find connected (Pieter Wuille)
a38c38951e10a61af80e3bca1e1ae04de978d5c0 clusterlin tests: separate testing of Search- and SimpleCandidateFinder (Pieter Wuille)
77a432ee704b4a83d56135ed10cf2adf4b3c18af clusterlin tests: count SimpleCandidateFinder iterations better (Pieter Wuille)
Pull request description:
Part of the cluster mempool project: #30289
The current cluster linearization fuzz tests contain two tests which combine testing of production code with testing of the test code itself:
* `clusterlin_search_finder`: establishes the correctness of `SearchCandidateFinder` by comparing against both `SimpleCandidateFinder` and `ExhaustiveCandidateFinder` (which is even more simple than `SimpleCandidateFinder`). If `SimpleCandidateFinder` works correctly, then this comparison with `ExhaustiveCandidateFinder` is redundant. If it isn't, we ought to find that in a test specific to `SimpleCandidateFinder` rather than as a side-effect of testing `SearchCandidateFinder`. Split this functionality out into a new `clusterlin_simple_finder`.
* `clusterlin_linearize`: establishes the correctness of `Linearize` by comparing against both `SimpleLinearize` and literally every valid linearization for the cluster. Again, if `SimpleLinearize` works correctly, then this comparison with all valid linearizations is redundant, and if it isn't we should find it in a test for `SimpleLinearize`. Do so by splitting off that functionality into `clusterlin_simple_linearize`.
After that, a few general improvements to the affected tests are made (comparing with linearizations and subsets read from the fuzz input, plus a performance improvement).
ACKs for top commit:
marcofleon:
Re ACK d7fca5c171f450621112457ea6f7c99b2e21d354
ismaelsadeeq:
re-ACK d7fca5c171f450621112457ea6f7c99b2e21d354
monlovesmango:
ACK d7fca5c171f450621112457ea6f7c99b2e21d354
Tree-SHA512: 33cb76bd9b9547a5f3ee231fa452e928f064ad03af98e3d9e64246eb972f2b026c13e7367257ccdac1ae57982ee8ef98c907684588ecbb4bc4c82cbec160b3e8
For some reason, building x86_64-w64-mingw32 on x86_64 and aarch64
results in a single instruction difference which can be traced down to
prevector.h:174. The ultimate caller of this is the copy constructor for
a prevector that ends up being called by std::vector::push_back in
walletmodel.cpp:183. By replacing the push_back with an emplace_back,
somehow this non-determinism goes away.
When using CMake policies 3.14 and below, the `export(PACKAGE)` command
by default populates the user package registry, which is stored outside
the build tree. Setting the `CMAKE_EXPORT_NO_PACKAGE_REGISTRY` variable
disables this side effect.
In CMake 3.15 and later, this behavior is disabled by default, and the
variable has no effect.
Changing into the build dir is confusing and brittle.
This can be reviewed using the git option `--word-diff-regex=.`.
Also:
* add missing -j1 to the fallback that prints a verbose build failure
* remove quotes around $GOAL in the fallback
fa2fbaa4a29f80d3c7d5f0ad6b64035c3156dd12 bench: Avoid tmp files in pwd (MarcoFalke)
Pull request description:
It is a bit confusing that one bench run, when aborted, could leave behind temp files in the current working directory. It is similarly confusing to delete those files in the next run of bench.
Fix all issues by using `BasicTestingSetup`, which provides a proper temp folder to use and also cleans up after itself.
Can be tested via:
```
( echo 'my file content' > streams_tmp ) && ls streams_tmp && ./bld-cmake/bin/bench_bitcoin --filter=FindByte && ls streams_tmp
```
Previously the file would be deleted, now it is kept.
ACKs for top commit:
stickies-v:
ACK fa2fbaa4a29f80d3c7d5f0ad6b64035c3156dd12
Tree-SHA512: 33798030f990d1b4c95be4682d8dbfad95e8716d5fc0b99d65937196f2ced1ba649193c2adba4155f4eec9fd06e16be6667f3c3705af1880f47b2ff57a76243b
fa0528479d5e37833fa66395c94d4611aa9270f6 test: Add missing convert_to_json_for_cli (MarcoFalke)
Pull request description:
Currently the tests are failing on current master, if they use the `--usecli` flag. See https://github.com/bitcoin/bitcoin/runs/45676472375, https://cirrus-ci.com/task/5707897310543872.
This can be reproduced locally via:
```
./bld-cmake/test/functional/wallet_reorgsrestore.py --usecli
```
Fix it by adding the missing `hash_or_height=self.convert_to_json_for_cli(tip)` for the value that could either be a string (needs quotes in json), or a number (does not need quotes in json).
ACKs for top commit:
fanquake:
ACK fa0528479d5e37833fa66395c94d4611aa9270f6
Tree-SHA512: 3d6deafca1249b2266cfabcd883edc9daaf985c417035a4b0223da4693f4165f8c9ce91a0e128d626000c10c32fe31f323f4b3f6ea0d0b3a771237a4f1d4cf44
fa4d68cf97b6d77dbb559facbc425376e2c51f62 Turn rpcauth.py test into functional test (MarcoFalke)
Pull request description:
Currently the `rpcauth-test.py` is problematic, because:
* The boilerplate for the test runner is duplicate or inconsistent with the other (functional) tests. Specifically `ConfigParser`.
* The cmake/ci behavior is brittle and can silently fail, as explained in https://github.com/bitcoin/bitcoin/issues/31476.
* Outside of ctest, this single test has to be run manually and separately, which is easy to forget.
* If the test is manually called, it runs single threaded, when it could just run in parallel with the other functional tests.
* It is also the only "unit" test written in Python, but not called by the functional test runner.
Fix all issues by turning it into a functional test.
ACKs for top commit:
l0rinc:
ACK fa4d68cf97b6d77dbb559facbc425376e2c51f62
janb84:
LGTM ACK fa4d68cf97b6d77dbb559facbc425376e2c51f62
w0xlt:
ACK fa4d68cf97
Tree-SHA512: a3b2b03be31c33288dee23c544b33ec43295e796c2047777597ceb86acce9f697478e32d891aa986c1d7d5749d62eded65eeb858e9d7479bda7a400eb1167040
8cc3ac6c2328873e57c31f237d194c5f22b6748a validation: Don't use IsValid() to filter for invalid blocks (Martin Zumsande)
86d98b94e5469e6476dc87f54b8ac25074603f0c test: verify that ancestors of a reconsidered block can become the chain tip (stratospher)
3c39a55e64bedfc384f6a7b9de1410bc8b46a9bb validation: Add ancestors of reconsiderblock to setBlockIndexCandidates (Martin Zumsande)
Pull request description:
When we call `reconsiderblock` for some block, `Chainstate::ResetBlockFailureFlags` puts the descendants of that block into `setBlockIndexCandidates` (if they meet the criteria, i.e. have more work than the tip etc.), but never put any ancestors into the set even though we do clear their failure flags.
I think that this is wrong, because `setBlockIndexCandidates` should always contain all eligible indexes that have at least as much work as the current tip, which can include ancestors of the reconsidered block. This is being checked by `CheckBlockIndex()`, which could fail if it was invoked after `ActivateBestChain` connects a block and releases `cs_main`:
``` diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 7b04bd9a5b..ff0c3c9f58 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3551,6 +3551,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
}
}
// When we reach this point, we switched to a new tip (stored in pindexNewTip).
+ m_chainman.CheckBlockIndex();
if (exited_ibd) {
// If a background chainstate is in use, we may need to rebalance our
```
makes `rpc_invalidateblock.py` fail on master.
Even though we don't currently have a `CheckBlockIndex()` in that place, after `cs_main` is released other threads could invoke it, which is happening in the rare failures of #16444 where an invalid header received from another peer could trigger a `CheckBlockIndex()` call that would fail.
Fix this by adding eligible ancestors to `setBlockIndexCandidates` in `Chainstate::ResetBlockFailureFlags` (also simplifying that function a bit).
Fixes#16444
ACKs for top commit:
achow101:
ACK 8cc3ac6c2328873e57c31f237d194c5f22b6748a
TheCharlatan:
Re-ACK 8cc3ac6c2328873e57c31f237d194c5f22b6748a
stratospher:
reACK 8cc3ac6.
Tree-SHA512: 53f27591916246be4093d64b86a0494e55094abd8c586026b1247e4a36747bc3d6dbe46dc26ee4a22f47b8eb0d9699d13e577dee0e7198145f3c9b11ab2a30b7
1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b wallet, test: best block locator matches scan state follow-ups (rkrux)
Pull request description:
Few follows-ups from #30221: Use `SetLastBlockProcessedInMem` more in `AttachChain`, add not null locator check in `WriteBestBlock`. Add log and few assertions in `wallet_reorgstore` test.
ACKs for top commit:
achow101:
ACK 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b
pablomartin4btc:
cr-ACK 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b
Tree-SHA512: 34edde55beef5714cea2e1131c29b57da2dc32ea091cd81878014de503c128f02c3ab88aee1e456541d7937e033dca5a81b03e9e2888cf781d71b62ad9b5ca5c
922adf66ac7420e21ea171d3586ea84554e5d91b mempool: use `FeeFrac` for calculating regular score (Sebastian Falbesoner)
3322b3a05954a1bac9f49a92e3bb35d2cbd20029 mempool: use `FeeFrac` for calculating ancestor score (Sebastian Falbesoner)
ac9c113bd2a2bb5001391a486d010189368f51f3 mempool: use `FeeFrac` for calculating descendant score (Sebastian Falbesoner)
Pull request description:
Rather than determining fee-rates for the mempool index scores and comparators manually in a rather tedious way (even involving floating-points), use the `FeeFrac` class [1] to simplify and deduplicate the code. Note that though this is intended to be a refactoring PR, there might be subtle differences in behaviour due to floating-point arithmetic involved in the original code (to avoid overflows at the cost of precision loss), but these shouldn't matter.
[1] introduced in PR #29242, commit ce8e22542ed0b4fa5794d3203207146418d59473
ACKs for top commit:
ismaelsadeeq:
Code review ACK 922adf66ac7420e21ea171d3586ea84554e5d91b
glozow:
ACK 922adf66ac7420e21ea171d3586ea84554e5d91b
Tree-SHA512: 6c3a9436f2be668aa8561b40c1b93efa7dc97b4ef354e98233ac3d3286a88804668164a55f2fcce4239fee5830e4e70f520e6285b667b87baa65c7cec09159cf
4c772cbd83e502a1339e8993d192ea6416ecd45c doc: add release notes for new rate limiting logging behavior (Eugene Siegel)
d541409a64c60d127ff912dad9dea949d45dbd8c log: Add rate limiting to LogPrintf, LogInfo, LogWarning, LogError, LogPrintLevel (Eugene Siegel)
a6a35cc0c23d0d529bfeb2f40d83d61f15ca7b40 log: use std::source_location in place of __func__, __FILE__, __LINE__ (Eugene Siegel)
afb9e39ec5552e598a5febaa81820d5509b7c5d2 log: introduce LogRateLimiter, LogLimitStats, Status (Eugene Siegel)
df7972a6cfd919b972bcbba07de85f7797898529 test: Mark ~DebugLogHelper as noexcept(false) (Eugene Siegel)
Pull request description:
This revives the work done by dergoegge in https://github.com/bitcoin/bitcoin/pull/21603. The approach is similar — this PR uses `std::source_location` under the hood now that we can use c++20 features. It also resets the rate limiting statistics via the `CScheduler`. The logging functions have also changed slightly since that PR was opened, so work has been done to preserve the intent of the original rate limiting change. I have tried to give commit attribution where possible.
**Approach:**
Each source code location is given an hourly logging quota of 1MiB of logging per hour. Logging is only dropped from source locations that exceed the quota.
- Only logging to disk is rate limited. Logging to console is not rate limited.
- Printing with the category argument is not rate limited.
- `UpdateTip: new best=[…]` is logged without rate limiting. High log volume is expected for that source location during IBD.
- When logging is restarted a tally of how many bytes were dropped is printed.
- All logs will be prefixed with [*] if there is at least one source location that is currently being suppressed.
I've repurposed the old logging rpc mentioned in #21603 in another branch for testing [here](https://github.com/Crypt-iQ/bitcoin/tree/log_ratelimiting_05192025_rpc). This can be used to log from source locations and test out the new changes in logging behavior. Note that the `setmocktime` RPC needs to be used to set the mock time past the current clock time to reset the logging messages.
Example usage:
```
bitcoin-cli -regtest excessivelog 1 1048500 # log just under 1MiB
bitcoin-cli -regtest excessivelog 1 100 # this should get the total amount logged above 1MiB
# and the rate limiting logic should kick in
bitcoin-cli -regtest excessivelog 2 1048500
bitcoin-cli -regtest excessivelog 2 100 # trigger rate limiting from another location
bitcoin-cli -regtest mockscheduler 3600 # fast-forward the scheduler
bitcoin-cli -regtest excessivelog 1 100 # this should trigger logging to resume and will log the source locations that were reset
```
Example output:
```
2025-07-02T22:03:56Z [warning] Excessive logging detected from rpc/node.cpp:142 (RPCHelpMan excessivelog()): >1048576 bytes logged during the last time window of 3600s. Suppressing logging to disk from this source location until time window resets. Console logging unaffected. Last log entry.
[*] 2025-07-02T22:03:56Z aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
2025-07-02T22:04:58Z (mocktime: 2262-04-11T23:47:15Z) Restarting logging from rpc/node.cpp:142 (RPCHelpMan excessivelog()): 121 bytes were dropped during the last 3600s.
2025-07-02T22:04:58Z (mocktime: 2262-04-11T23:47:15Z) Restarting logging from rpc/node.cpp:139 (RPCHelpMan excessivelog()): 121 bytes were dropped during the last 3600s.
```
ACKs for top commit:
maflcko:
re-ACK 4c772cbd83e502a1339e8993d192ea6416ecd45c 🕚
glozow:
reACK 4c772cb
stickies-v:
re-ACK 4c772cbd83, no changes except release notes update
Tree-SHA512: d07087cd0f2b188100b51c9b8c3da376fa24ec3612a2a284bd83f650bba0ea409f9fa0acd5f3b10f45e664ef4fdf3abc97ed3da08098d2beb599cc83e3fc4504
To mitigate disk-filling attacks caused by unsafe usages of LogPrintf and
friends, we rate-limit them by passing a should_ratelimit bool that
eventually makes its way to LogPrintStr which may call
LogRateLimiter::Consume. The rate limiting is accomplished by
adding a LogRateLimiter member to BCLog::Logger which tracks source
code locations for the given logging window.
Every hour, a source location can log up to 1MiB of data. Source
locations that exceed the limit will have their logs suppressed for the
rest of the window determined by m_limiter.
This change affects the public LogPrintLevel function if called with
a level >= BCLog::Level::Info.
The UpdateTipLog function has been changed to use the private LogPrintLevel_
macro with should_ratelimit set to false. This allows UpdateTipLog to log
during IBD without hitting the rate limit.
Note that on restart, a source location that was rate limited before the
restart will be able to log until it hits the rate limit again.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
The std::source_location conveniently stores the file name, line number,
and function name of a source code location. We switch to using it instead
of the __func__ identifier and the __FILE__ and __LINE__ macros.
BufferedLog is changed to have a std::source_location member, replacing the
source_file, source_line, and logging_function members. As a result,
MemUsage no longer explicitly counts source_file or logging_function as the
std::source_location memory usage is included in the MallocUsage call.
This also changes the behavior of -logsourcelocations as std::source_location
includes the entire function signature. Because of this, the functional test
feature_config_args.py must be changed to no longer include the function
signature as the function signature can differ across platforms.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
LogRateLimiter will be used to keep track of source locations and our
current time-based logging window. It contains an unordered_map and a
m_suppressions_active bool to track source locations. The map is keyed
by std::source_location, so a custom Hash function (SourceLocationHasher)
and custom KeyEqual function (SourceLocationEqual) is provided.
SourceLocationHasher uses CSipHasher(0,0) under the hood to get a
uniform distribution.
A public Reset method is provided so that a scheduler (e.g. the
"b-scheduler" thread) can periodically reset LogRateLimiter's state when
the time window has elapsed.
The LogRateLimiter::Consume method checks if we have enough available
bytes in our rate limiting budget to log an additional string. It
returns a Status enum that denotes the rate limiting status and can
be used by the caller to emit a warning, skip logging, etc.
The Status enum has three states:
- UNSUPPRESSED (logging was successful)
- NEWLY_SUPPRESSED (logging was succcesful, next log will be suppressed)
- STILL_SUPPRESSED (logging was unsuccessful)
LogLimitStats counts the available bytes left for logging per source
location for the current logging window. It does not track actual source
locations; it is used as a value in m_source_locations.
Also exposes a SuppressionsActive() method so the logger can use
that in a later commit to prefix [*] to logs whenenever suppressions
are active.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
We mark ~DebugLogHelper as noexcept(false) to be able to catch the
exception it throws. This lets us use it in test in combination with
BOOST_CHECK_THROW and BOOST_CHECK_NO_THROW to check that certain log
messages are (not) logged.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
