637f437a1642ce845d7f750b45b71320a82d83b5 doc: remove PR Review Club frequency (Gabriele Bocchi)
Pull request description:
The PR Review Club is mentioned as weekly in the CONTRIBUTING.md file, but it is held monthly as per the official [Bitcoin Core PR Review Club website](https://bitcoincore.reviews/). This PR updates the documentation to just remove the frequency.
ACKs for top commit:
fanquake:
ACK 637f437a1642ce845d7f750b45b71320a82d83b5
Tree-SHA512: 27bf8a0e32edd8bedb5301ceb3c744ff4629403292a7ad00b633921f36278443ae297cd53708a533b1d6e6eab863b831e11247b95277b94cce28e3d5ddb7d9b9
fa7857ccda5d82739bba16eeb7244433978d28f3 build: Enable -Wbidi-chars=any (MarcoFalke)
Pull request description:
I don't see a use-case for UTF-8 bidirectional control characters in this codebase. So disable them for now.
Ref: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wbidi-chars_003d
ACKs for top commit:
fanquake:
ACK fa7857ccda5d82739bba16eeb7244433978d28f3
Tree-SHA512: 29cf78a2bd0fd94f919f4cd1d1038009a574b4d011146c69bf94d3c06951606200b7d1f827ac6f2fb4540e8f45118ba72b3ccf6c20ef8048e819974371d8f67a
bcd82b13f4649e57d7d106856aab7b2a6296d728 Remove pkgconfig from toolchain file (TheCharlatan)
319a4e82614283afb3dbc5d38ff3b9d17fb911b3 depends: drop sqlite pkgconfig file (fanquake)
a8fe1fd38bf496356dc4f28963d4edfa75fe04a5 depends: better cleanup after fontconfig (fanquake)
17e79c92607e2e32b48ffd388828184c5d1a65df depends: fully remove libtool archives from Qt build (fanquake)
8ca85651c8350a6edb069eae0f88cf03c6eae0d5 guix: move pkg-config to Linux builds (fanquake)
e3e648cf410d30185927d031c81a85d5fa890b8c depends: drop pkg-config option from Qt build (fanquake)
0d185bd99f9e40913f678af0fc224add2e1d2f14 doc: update depends doc to prefer .cmake outputs (fanquake)
Pull request description:
After #31181, `pkg-config` is no-longer needed for macOS or Windows Guix builds. It's still needed for Linux, as it's used by a Qt subdependency (fontconfig to find freetype). However we should also no-longer need it for Qt itself, when building using depends.
ACKs for top commit:
TheCharlatan:
ACK bcd82b13f4649e57d7d106856aab7b2a6296d728
Tree-SHA512: 89ae68281030d43fcb6c5c96429cd038a21f13a8ca19ea828ada47e8f9f0aa7407854a67c9003652817e47ab9565573b7028342e3e11bb1cca1d823c483081cd
faaaf59f71ede057b2c1d369ef8db973c2f2dbc2 test: Make g_rng_temp_path rand, not dependent on SeedRandomForTest (MarcoFalke)
fa80b08fef0eaa600339caa678fdf80a8aec3ce3 test: Revert to random path element (MarcoFalke)
Pull request description:
The randomness in the path element is required to allow a single fuzz test to run in parallel. Previous releases used a uint256 random value, but 10 random bytes should be sufficient as well, while avoiding a `MAX_PATH` violation on Windows.
The issue was introduced by myself, by suggesting to use the current time in https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1835351305.
ACKs for top commit:
kevkevinpal:
reACK faaaf59f71
hodlinator:
ACK faaaf59f71ede057b2c1d369ef8db973c2f2dbc2
tdb3:
re ACK faaaf59f71ede057b2c1d369ef8db973c2f2dbc2
dergoegge:
ACK faaaf59f71ede057b2c1d369ef8db973c2f2dbc2
Tree-SHA512: f12256c8b353618291030f71bf36eab97a25ffeaa28e36a5f2c6718dfc1fbbc8548c71475edec53d59026f2a779a05778db83f0530dd3e1d1faf6e4fc0ee7d70
111465d72dd35e42361fc2a089036f652417ed37 test: Remove unused attempts parameter from wait_until (Fabian Jahr)
5468a23eb9a3fd2b0c08dbca69fe3df58af42530 test: Add check_interval parameter to wait_until (Fabian Jahr)
16c87d91fd4d7709fa9d8824d5b641ef71821931 test: Introduce ensure_for helper (Fabian Jahr)
Pull request description:
A repeating pattern in the functional tests is that the test sleeps for a while to ensure that a certain condition is still true after some amount of time has elapsed. Most recently a new case of this was added in #30807. This PR here introduces an `ensure` helper to streamline this functionality.
Some approach considerations:
- It is possible to construct this by reusing `wait_until` and wrapping it in `try` internally. However, the logger output of the failing wait would still be printed which seems irritating. So I opted for simplified but similar internals to `wait_until`.
- This implementation starts for a failure in the condition right away which has the nice side-effect that it might give feedback on a failure earlier than is currently the case. However, in some cases, it may be expected that the condition may still be false at the beginning and then turns true until time has run out, something that would work when the test sleeps without checking in a loop. I decided against this design (and even against adding it as an option) because such a test design seems like it would be racy either way.
- I have also been going back and forth on naming. To me `ensure` works well but I am also not a native speaker, happy consider a different name if others don't think it's clear enough.
ACKs for top commit:
maflcko:
re-ACK 111465d72dd35e42361fc2a089036f652417ed37 🍋
achow101:
ACK 111465d72dd35e42361fc2a089036f652417ed37
tdb3:
code review re ACK 111465d72dd35e42361fc2a089036f652417ed37
furszy:
utACK 111465d72dd35e42361fc2a089036f652417ed37
Tree-SHA512: ce01a4f3531995375a6fbf01b27d51daa9d4c3d7cd10381be6e86ec5925d2965861000f7cb4796b8d40aabe3b64c4c27e2811270e4e3c9916689575b8ba4a2aa
b2d536100282bd901d3e0be7f7f4a6966e0ef817 build: Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC (Hennadii Stepanov)
Pull request description:
This PR suggests a temporary workaround for a compiler bug [introduced](https://github.com/bitcoin/bitcoin/issues/31303) in Visual Studio 2022 version 17.12.
This workaround is required to fix the CI until the upstream compiler bug is resolved.
ACKs for top commit:
maflcko:
lgtm ACK b2d536100282bd901d3e0be7f7f4a6966e0ef817
TheCharlatan:
ACK b2d536100282bd901d3e0be7f7f4a6966e0ef817
brunoerg:
ACK b2d536100282bd901d3e0be7f7f4a6966e0ef817
Tree-SHA512: 8f8b772253f6f61d9e24c6ae8692511c7c1229c7d20a45fe680ad6a3909c59b58b504589f06d3135d9708a61684be4aba6a426f7f5f60020551f52b9090d3030
This ensures the options are applied consistently from contexts where
they might not pass through the args manager, such as in some tests, or
when used through the kernel library.
This is similar to the patch applied in 09ef322acc0a88a9e119f74923399598984c68f6.
This ensures that if a client no longer needs a block template,
the node can clear its memory as soon as possible.
A block template may hold on to transactions that are no longer
in the mempool, so this can be significant.
Requested by clang-tidy:
src/wallet/salvage.cpp:119:18: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
119 | warnings.push_back(Untranslated("Salvage: Database salvage found errors, all data may not be recoverable."));
| ^~~~~~~~~~
| emplace_back(
This is required for a future commit. Can be reviewed via the git
options --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Also move util::detail::Hex to a proper namespace instead of an inline
namespace so it doesn't conflict with the new util::detail namespace, and
won't create other problems for callers trying to use the inline namespaces.
Also fix a misleading comment in util_string_tests.cpp.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d refactor: Avoid std::string format strings (MarcoFalke)
Pull request description:
This changes some unchecked `std::string` format strings to use string literals, which are `consteval` checked at compile-time.
Split out, because it is used in several pull requests.
ACKs for top commit:
l0rinc:
ACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d
tdb3:
code review and light test ACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d
rkrux:
tACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d
ryanofsky:
Code review ACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d
Tree-SHA512: 7cc70a49b07dadc386336687b463043e79e94a46d18db0184c9813218536e87e954a1afeb8739d5d8706e7b2f355d3f7984048c7de2725851b463985f1c5369f
0bd53d913c1c2ffd2d0779f01bc51c81537b6992 test: add test for getchaintips behavior with invalid chains (Martin Zumsande)
ccd98ea4c88fc1aa959e41e0686d8dff00a44209 test: cleanup rpc_getchaintips.py (Martin Zumsande)
f5149ddb9b7de3559943d7fda0f440e59413dfb5 validation: mark blocks building on an invalid block as BLOCK_FAILED_CHILD (Martin Zumsande)
783cb7337f72a3c7b2e74efd677a8ff0c375fe10 validation: call RecalculateBestHeader in InvalidChainFound (Martin Zumsande)
9275e9689a426964f5eaee65e356754a0548d926 rpc: call RecalculateBestHeader as part of reconsiderblock (Martin Zumsande)
a51e91783aac0beefcb604be159eb1cb96a39051 validation: add RecalculateBestHeader() function (Martin Zumsande)
Pull request description:
`m_best_header` (the most-work header not known to be on an invalid chain) can be wrong in the context of invalidation / reconsideration of blocks. This can happen naturally (a valid header is received and stored in our block tree db; when the full block arrives, it is found to be invalid) or triggered by the user with the `invalidateblock` / `reconsiderblock` rpc.
We don't currently use `m_best_header` for any critical things (see OP of #16974 for a list that still seems up-to-date), so it being wrong affects mostly rpcs.
This PR proposes to recalculate it if necessary by looping over the block index and finding the best header. It also suggest to mark headers between an invalidatetd block and the previous `m_best_header` as invalid, so they won't be considered in the recalculation.
It adds tests to `rpc_invalidateblock.py` and `rpc_getchaintips.py` that fail on master.
One alternative to this suggested in the past would be to introduce a continuous tracking of header tips (#12138).
While this might be more performant, it is also more complicated, and situations where we need this data are only be remotely triggerable by paying the cost of creating a valid PoW header for an invalid block.
Therefore I think it isn't necessary to optimise for performance here, plus the solution in this PR doesn't perform any extra steps in the normal node operation where no invalidated blocks are encountered.
Fixes #26245
ACKs for top commit:
fjahr:
reACK 0bd53d913c1c2ffd2d0779f01bc51c81537b6992
achow101:
ACK 0bd53d913c1c2ffd2d0779f01bc51c81537b6992
TheCharlatan:
Re-ACK 0bd53d913c1c2ffd2d0779f01bc51c81537b6992
Tree-SHA512: 23c2fc42d7c7bb4f9b4ba4949646b3d0031dd29ed15484e436afd66cd821ed48e0f16a1d02f45477b5d0d73a006f6e81a56b82d9721e0dee2e924219f528b445
8610bcef9d030013f9e36cffe0c58dd2cfe85d66 ci: skip Github CI on branch pushes for forks (Sjors Provoost)
Pull request description:
When a contributor maintains a fork of the repo, any pull request they make to their own fork, or to the main repository, will trigger two CI runs one for the branch push and one for the pull request.
After this PR when `SKIP_BRANCH_PUSH` is set, pushes made to git branches inside fork repositories will no longer trigger CI runs, unless the git branches are associated with PRs in the fork repository, or the main repository.
The same behaviour was added for Cirrus in e9bfbb5414ab14ca14d8edcfdf77f28c9ed67c33.
Note to maintainers: `SKIP_BRANCH_PUSH=true` needs to be set for the GUI repo to maintain existing behaviour.
ACKs for top commit:
m3dwards:
ACK 8610bcef9d030013f9e36cffe0c58dd2cfe85d66
achow101:
ACK 8610bcef9d030013f9e36cffe0c58dd2cfe85d66
vasild:
ACK 8610bcef9d030013f9e36cffe0c58dd2cfe85d66
Tree-SHA512: 4055153f03f0cb60a97ce26157ab9db40a4609dee9f060ed7b06aa8841df5bd8e1a42ff2ac0f20bd69e221e8e67bff062a9a361a291124070a03dd51c609e845
42066f45ff5d48e78a317eda63c035809bd657c6 Refactor SipHash_32b benchmark to improve accuracy and avoid optimization issues (Lőrinc)
Pull request description:
This PR stems from the discussions in https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1649187336
The previous benchmark for `SipHash` was slightly less accurate in representing real-world usage and allowed for potential compiler optimizations that could invalidate the benchmark.
This change aims to ensure the benchmark produces more realistic results.
By modifying the initial values and only incrementing the bytes of `val`, the benchmark should reflects a more typical usage patterns - and prevent the compiler from optimizing away the calculations.
-------
On my M1 processor the benchmark's speed changed significantly (but the CI seems to produce the same result as before):
> cmake -B build -DCMAKE_BUILD_TYPE=Release -DBUILD_BENCH=ON && cmake --build build -j10 &&
./build/src/bench/bench_bitcoin --filter=SipHash_32b --min-time=1000
Before:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 35.15 | 28,445,856.66 | 0.2% | 1.10 | `SipHash_32b`
After (note that only the benchmark changed):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 22.05 | 45,350,886.64 | 0.3% | 1.10 | `SipHash_32b`
ACKs for top commit:
maflcko:
review ACK 42066f45ff5d48e78a317eda63c035809bd657c6
achow101:
ACK 42066f45ff5d48e78a317eda63c035809bd657c6
hodlinator:
ACK 42066f45ff5d48e78a317eda63c035809bd657c6
Tree-SHA512: 6bbe9d725d4c3396642e55ce48c31baa5339e56838d6d5fb377fb1069daa9292375e7020ceff7da0d78befffc1e984f717b5232217fe911989613480adaa937e
192dac1d3370edd579db235d69c034726f37c8da [refactor] Cleanup BlockAssembler mempool usage (TheCharlatan)
Pull request description:
The `addPackageTxs` method of the `BlockAssembler` currently has access to two mempool variables, as an argument and as a member. Clean this up and clarify that they both are the same mempool instance by removing the argument and instead only using the member variable in the method.
This was noticed in this PR review: https://github.com/bitcoin/bitcoin/pull/25223#discussion_r898164322.
ACKs for top commit:
achow101:
ACK 192dac1d3370edd579db235d69c034726f37c8da
danielabrozzoni:
re-ACK 192dac1
stickies-v:
ACK 192dac1d3370edd579db235d69c034726f37c8da
Tree-SHA512: a5ae7d6d771fbb5b54f23624b4d3429acf07bbe38179a462a078c825d60c89a725ad4e13fe7067eebea7dfec63c56c8f39b5077b0d949d594f500affcc1272d1
After port collisions are no longer tolerated but lead to
a startup failure in v28.0, local setups of multiple nodes,
each with a different -port value would not be possible anymore
due to collision of the onion default port - even if the nodes
were using tor or not interested in receiving onion inbound connections.
Fix this by deriving the onion listening port to be -port + 1.
(idea by vasild / laanwj)
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Pass literal format strings instead of std::string so formats can be
checked at compile time.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: stickies-v <stickies-v@protonmail.com>
ac286e0d1bd4c8dbe6d21cbe8c0a131ad8ba1854 doc: Fix grammatical errors in multisig-tutorial.md (secp512k2)
Pull request description:
This pull request fixes grammatical errors in the `multisig-tutorial.md` document.
ACKs for top commit:
Abdulkbk:
ACK ac286e0d1bd4c8dbe6d21cbe8c0a131ad8ba1854
Tree-SHA512: 684fe16e802431109957b9cde441353edeb16ffffde4282310c1a6f104adffc53347d00a2cf3a5969a78803f3177d6056ca37d3b7e8be52c2ec43ec0b9fcf4d9
fe39acf88ff552bfc4a502c99774375b91824bb1 tinyformat: Add compile-time checking for literal format strings (Ryan Ofsky)
184f34f2d0fa2e56ad594966b2b99ff4cf840d95 util: Support dynamic width & precision in ConstevalFormatString (Ryan Ofsky)
Pull request description:
Add compile-time checking for literal format strings passed to `strprintf` and `tfm::format` to make sure the right number of format arguments are passed.
There is still no compile-time checking if non-literal `std::string` or `bilingual_str` format strings are passed, but this is improved in other PRs:
- [#31061](https://github.com/bitcoin/bitcoin/pull/31061) implements compile-time checking for translated strings
- [#31072](https://github.com/bitcoin/bitcoin/pull/31072) increases compile-time checking by using literal strings as format strings, instead of `std::string` and `bilingual_str`
- [#31149](https://github.com/bitcoin/bitcoin/pull/31149) may drop the `std::string` overload for `strprintf` to [require](https://github.com/bitcoin/bitcoin/pull/31149#issuecomment-2444579999) compile-time checking
ACKs for top commit:
maflcko:
re-ACK fe39acf88ff552bfc4a502c99774375b91824bb1 🕐
l0rinc:
ACK fe39acf88ff552bfc4a502c99774375b91824bb1
hodlinator:
re-ACK fe39acf88ff552bfc4a502c99774375b91824bb1
Tree-SHA512: f1ddef0c96b9468c5ffe31b42dc19f1922583dd14f2e180b618d992c98614c5cc7db9f9cd917ef503f833bbc7dbec78e4489d0035416dce6840837e1d66d87cb
e80e4c6ff91e27d7d40f099a2d7942c29085234c validation: Remove RECENT_CONSENSUS_CHANGE validation result (TheCharlatan)
Pull request description:
The *_RECENT_CONSENSUS_CHANGE variants in the validation result enumerations were always unused. They seem to have been kept around speculatively for a soft fork after segwit, however they were never used for taproot either. This points at them not having a clear purpose. Based on the original pull requests' comments their usage was never entirely clear:
https://github.com/bitcoin/bitcoin/pull/11639#issuecomment-370234133https://github.com/bitcoin/bitcoin/pull/15141#discussion_r271039747
Since they are part of the validation interface and need to be exposed by the kernel library keeping them around may also be confusing to future users of the library.
ACKs for top commit:
sipa:
ACK e80e4c6ff91e27d7d40f099a2d7942c29085234c
naumenkogs:
ACK e80e4c6ff9
dergoegge:
ACK e80e4c6ff91e27d7d40f099a2d7942c29085234c
ajtowns:
ACK e80e4c6ff91e27d7d40f099a2d7942c29085234c
Tree-SHA512: 0af17c4435bb1b5a4f43600da30545cbbe95a7d642419cabdefabfb82b9335d92262c1c48be7ca2f2a024078ae9447161228b6f951d2f508a51159a31947fb54
With --skip-missing-binaries, instead of stopping the execution of
gen-manpages.py when a binary is not found, continue generating
manpages for the available binaries and skip the missing ones.
fa66e0887ca1a1445d8b18ba1fadb12b2d911048 bench: add support for custom data directory (furszy)
ad9c2cceda9cd893c0f754e49f7fca6e417ee95f test, bench: specialize working directory name (furszy)
Pull request description:
Expands the benchmark framework with the existing `-testdatadir` arg,
enabling the ability to change the benchmark data directory.
This is useful for running benchmarks on different storage devices, and
not just under the OS `/tmp/` directory.
A good use case is #28574, where we are benchmarking the wallet
migration process on an HDD.
ACKs for top commit:
maflcko:
re-ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
achow101:
ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
tdb3:
re ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
hodlinator:
re-ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
pablomartin4btc:
re-ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
Tree-SHA512: 4e87206c07e26fe193c07074ae9eb0cc9c70a58aeea8cf27d18fb5425d77e4b00dbe0e6d6a75c17b427744e9066458b9a84e5ef7b0420f02a4fccb9c5ef4dacc
The mempool:replaced tracepoint now reports either a txid or a
package hash (previously it always was a txid). To let users know
if a txid or package hash is passed, a boolean argument is added
the the tracepoint.
In the functional test, a ctypes.Structure class for MempoolReplaced
is introduced as Python warns the following when not explcitly
casting it to a ctype:
Type: 'bool' not recognized. Please define the data with ctypes manually.
