a56c96507a9e943bbcd7e126bc827de9495f0ebd ci: use clang-16 in tidy task (fanquake)
Pull request description:
Follow up to https://github.com/bitcoin/bitcoin/pull/27311#issuecomment-1481020371, as IWYU now has a [clang_16 branch](https://github.com/include-what-you-use/include-what-you-use/tree/clang_16).
This also removes some workarounds for (now fixed) clang-tidy issues, and simplifies the IWYU install steps.
ACKs for top commit:
MarcoFalke:
lgtm ACK a56c96507a9e943bbcd7e126bc827de9495f0ebd
josibake:
ACK a56c96507a
hebasto:
ACK a56c96507a9e943bbcd7e126bc827de9495f0ebd
Tree-SHA512: 5bbec6cc196c3305302895c77986f3695fc6f4024363ee57503654d54e0ebf108719a7a1d7908817f84115dcaa13377493eb764b00bdf574f1290c73251426fa
73f4eb511cf80cf52b78627347727ca02774b731 Check that the Timestamp String is valid (John Moffett)
Pull request description:
Follow-up to https://github.com/bitcoin/bitcoin/pull/27233
The current `FormatISO8601DateTime` function will return an empty string if it encounters an error when converting the `int64_t` seconds-since-epoch to a formatted date time. In the unlikely case that happens, here `strStamped.pop_back()` would be undefined behavior.
ACKs for top commit:
MarcoFalke:
lgtm ACK 73f4eb511cf80cf52b78627347727ca02774b731
stickies-v:
ACK 73f4eb511cf80cf52b78627347727ca02774b731
Tree-SHA512: 089ed639c193deb98870a8385039b31b4baed821ea907937bfc6f65a5b0981bbf8284b2afec81b2d0a922e2340716b48cf55349640eb6b8c311ef7af25abc361
00e9b97f37e0bdf4c647236838c10b68b7ad5be3 refactor: Move fs.* to util/fs.* (TheCharlatan)
106b46d9d25b5228ef009fbbe6f9a7ae35090d15 Add missing fs.h includes (TheCharlatan)
b202b3dd6393b415fa68e18dc49c9431dc6b58b2 Add missing cstddef include in assumptions.h (TheCharlatan)
18fb36367a28819bd5ab402344802796a1248979 refactor: Extract util/fs_helpers from util/system (Ben Woosley)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". This commit was originally authored by empact and is taken from its parent PR #25152.
#### Context
There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel library (https://github.com/bitcoin/bitcoin/pull/25290, https://github.com/bitcoin/bitcoin/pull/25487, https://github.com/bitcoin/bitcoin/pull/25527, https://github.com/bitcoin/bitcoin/pull/25862, https://github.com/bitcoin/bitcoin/pull/26177, and https://github.com/bitcoin/bitcoin/pull/27125). The `ArgsManager` is defined in `system.h`. A similar pull request extracting functionality from `system.h` has been merged in https://github.com/bitcoin/bitcoin/pull/27238.
#### Changes
Next to providing better code organization, this PR removes some reliance of the tree of libbitcoinkernel header includes on `system.h` (and thus the `ArgsManager` definition) by moving filesystem related functions out of the `system.*` files.
There is already a pair of `fs.h` / `fs.cpp` in the top-level `src/` directory. They were not combined with the files introduced here, to keep the patch cleaner and more importantly because they are often included without the utility functions. The new files are therefore named `fs_helpers` and the existing `fs` files are moved into the util directory.
Further commits splitting more functionality out of `system.h` are still in #25152 and will be submitted in separate PRs once this PR has been processed.
ACKs for top commit:
hebasto:
ACK 00e9b97f37e0bdf4c647236838c10b68b7ad5be3
Tree-SHA512: 31422f148d14ba3c843b99b1550a6fd77c77f350905ca324f93d4f97b652246bc58fa9696c64d1201979cf88733e40be02d262739bb7d417cf22bf506fdb7666
3fa4c54ac54b2d738e0c43b57b5c232ee02fe3b3 [net processing] Pass TxRelay to FindTxForGetData instead of Peer (dergoegge)
c85ee76a3647a42d3a59c28444c5ca52e7db7c5e [net processin] Don't take cs_main in FindTxForGetData (dergoegge)
Pull request description:
Addresses left over feedback from #26140.
* https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1153498543
* https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1153499627
`mapRelay` is only accessed from the message processing thread and does not need to be kept in sync with anything validation specific, it is therfore perfectly fine to have it guarded by `g_msgproc_mutex`.
ACKs for top commit:
jnewbery:
utACK 3fa4c54ac54b2d738e0c43b57b5c232ee02fe3b3
hebasto:
ACK 3fa4c54ac54b2d738e0c43b57b5c232ee02fe3b3, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 3ef84bfe4abfa8d991a7e65d9184221294d80e0df0bbb47f0270ab6ca1593266c98abf83c610f9f86b4d16c7a4b62bcf83f8856c68d3c2e10894bff6ed3e88cd
e414edd8fc88b55a91f446dcc4d55cc4cd6d32e7 qt: Update translation source file (Hennadii Stepanov)
b7800950913c47bc1a939bd7f99c6b0629aea530 qt: Adjust plural forms for translations (Hennadii Stepanov)
6ae8a24009cb006476b3be612a34e4deda30433a GUI: Send: Make feerates translatable (Luke Dashjr)
bd42f5e1cdb0cbd829c8fe1700d036b681f3930d Bugfix: GUI: Send/PSBT: Correct virtual size unit and make translatable (Luke Dashjr)
1b0407f5f1f5902f67b58c7ada9c4b0c45457244 Bugfix: GUI: transactiondesc: Translate outlier "own address" and "watch-only" (Luke Dashjr)
170f3126f220f9ffe3db471e0024a23dede3f90b GUI: Use translated external signer errors for messagebox text (Luke Dashjr)
96989599d6c7264022c188babdcd9cacac9ab69f GUI: Make messages for copying unsigned PSBTs translatable (Luke Dashjr)
08b8b287d3762dd6d4be09fad0b71c4cacfe6658 Bugfix: GUI: Debug info: Use correct "kB" case for small mempool sizes, and make translation-friendly (Luke Dashjr)
dacc322be10c6709338d2f0d7f5c4366609ad1a0 GUI: PSBTOperationsDialog: Support translating window title (Luke Dashjr)
5a4fe5527045ad3d7f1c834c3c6ea1d531c87879 GUI: Intro: Support translating caption of data directory chooser (Luke Dashjr)
3868ba3a27d594dd2968548fff3db457cbeb0080 GUI: Support translating peer network names (Luke Dashjr)
f1f981119884389f1f355bc11847a73812a2836e GUI: Support translating address type dropdown entries (Luke Dashjr)
Pull request description:
This PR updates the `src/qt/locale/bitcoin_en.xlf` translation source file according to [Release schedule for 25.0](https://github.com/bitcoin/bitcoin/issues/26549).
Some translation-related fixes have been picked from https://github.com/bitcoin-core/gui/pull/599 and https://github.com/bitcoin-core/gui/pull/716.
Note for reviewers: it is expected to get a zero diff after running `make -C src translate` locally.
ACKs for top commit:
jarolrod:
ACK e414edd8fc88b55a91f446dcc4d55cc4cd6d32e7
Tree-SHA512: 5b0c70db1e2f5871067e84f43ebea4ee4f0027fc5f2be49bbcb1d04e162ae76607b2b038e9d0622bcb5b3658d0ede8c10c4421ddaa3343f0e0be54315ca7a4f5
1869310f3cfa4ab26b5090d8a4002eefdc84870e refactor: remove unused param from legacy pubkey (Bushstar)
Pull request description:
Unused param present in legacy pubkey manager interface. This param will not be used and should be removed to prevent unintended usage.
ACKs for top commit:
Sjors:
ACK 1869310f3cfa4ab26b5090d8a4002eefdc84870e
furszy:
ACK 1869310f3cfa4ab26b5090d8a4002eefdc84870e
Tree-SHA512: 0fb41fc8f481f859262f2e8e9a93c990c1b4637e74fd9191ccc0b3c523d0e7d94217a3074bb357276e1941a10d29326f850f9b27eccc1eca57cf6b549353400c
Taking cs_main is no longer necessary since we moved
`m_recently_announced_invs` to `Peer` and `mapRelay` is actually only
accessed from the message processing thread.
faf8dc496e761a15956f8226d727f4bbab8dff82 fuzz: Remove legacy int parse fuzz tests (MarcoFalke)
Pull request description:
The fuzz tests checked that the result of the new function was equal to the legacy function. (Side note: The checks were incomplete, as evident by the follow-up fix in commit b5c9bb5cb9f4a8db57b33ef7399310c7d6de5822).
Given that they haven't found any issues in years (beside missing the above issue, that they couldn't catch), it seems time to remove them.
They may come in handy in the rare case that someone would want to modify `LocaleIndependentAtoi()` or `Parse*Int*()`, however that seems unlikely. Also, appropriate checks can be added then.
ACKs for top commit:
fanquake:
ACK faf8dc496e761a15956f8226d727f4bbab8dff82
dergoegge:
ACK faf8dc496e761a15956f8226d727f4bbab8dff82
Tree-SHA512: 4ec88b9fa8ba49a923b0604016f0f471b3c9b9e0ba6c5c3dc4e20503c6994789921e7221d9ec467a2a37a73f21a70ba51ba3370ed5ad311dee989e218290b29a
cd0c8eeb0940790b6ba83786d1c9e362d4dc4829 [net] Pass nRecvFloodSize to CNode (dergoegge)
860402ef2ed728ef096dda4e65e77d566782209f [net] Remove trivial GetConnectionType() getter (dergoegge)
b5a85b365a4abd98176b0935015dbb502cc3e6f6 [net] Delete CNetMessage copy constructor/assignment op (dergoegge)
Pull request description:
Follow-up PR for #27257
* Deletes the copy constructor/assignment operator of `CNetMessage`
* Removes trivial getter for the connection type
* Avoids passing `nRecvFloodSize` to CNode methods by passing it to `CNode` on creation
ACKs for top commit:
jnewbery:
utACK cd0c8eeb0940790b6ba83786d1c9e362d4dc4829
theStack:
ACK cd0c8eeb0940790b6ba83786d1c9e362d4dc4829
Tree-SHA512: 673a758668617f69fba77e61f0eaa1538da27a4849c82c98742436692baa2d7f001129af3e7a66b160e599d12109dac08137a146f10ff9b9ebdc5c2237311d41
9a1d73fdffa4f35e33bc187ac9b3afbba28b1950 Fix segfault when shutdown during wallet open (John Moffett)
Pull request description:
Fixes#689
## Summary
If you open a wallet and send a shutdown signal during that process, you'll get a segfault when the wallet finishes opening. That's because the `WalletController` object gets deleted manually in bitcoin.cpp during shutdown, but copies of the pointer (and pointers to child objects) are dangling in various places and are accessed in queued events after the deletion.
## Details
The issue in #689 is caused by the following sequence of events:
1. Wallet open modal dialog is shown and worker thread does the actual work.
2. Every 200ms, the main event loop checks to see if a shutdown has been requested, but only if a modal is not being shown.
3. Request a shutdown while the modal window is shown.
4. The wallet open process completes, the modal window is dismissed, and various `finish` signals are sent.
5. During handling of one of the `finish` signals, `qApp->processEvents()` is [called](e9262ea32a/src/qt/sendcoinsdialog.cpp (L603)), which causes the main event loop to detect the shutdown (now that the modal window has been dismissed). The `WalletController` and all the `WalletModel`s are [deleted](65de8eeeca/src/qt/bitcoin.cpp (L394-L401)).
6. Control returns to the `finish` method, which eventually tries to send a [signal](e9262ea32a/src/qt/sendcoinsdialog.cpp (L167)) from a wallet model, but it's been deleted already (and the signal is sent from a now-[dangling](d8bdee0fc8/src/qt/walletview.cpp (L65)) pointer).
The simplest fix for that is to change the `qApp->processEvents()` into a `QueuedConnection` call. (The `qApp->processEvents() was a [workaround](https://github.com/bitcoin/bitcoin/pull/593#issuecomment-3050699) to get the GUI to scroll to the last item in a list that just got added, and this is just a safer way of doing that).
However, once that segfault is fixed, another segfault occurs due to some queued wallet events happening after the wallet controller object is deleted here:
65de8eeeca/src/qt/bitcoin.cpp (L394-L401)
Since `m_wallet_controller` is a copy of that pointer in `bitcoingui.cpp`, it's now dangling and `if(null)` checks won't work correctly. For instance, this line:
65de8eeeca/src/qt/bitcoingui.cpp (L413)
sets up a `QueuedConnection` to `setCurrentWallet`, but by the time control reaches that method (one event cycle after shutdown deleted `m_wallet_controller` in `bitcoin.cpp`), the underlying objects have been destroyed (but the pointers are still dangling).
Ideally, we'd use a `QPointer` or `std::shared_ptr / std::weak_ptr`s for these, but the changes would be more involved.
This is a minimal fix for the issues. Just set `m_wallet_controller` to `nullptr` in `bitcoingui.cpp`, check its value in a couple places, and avoid a use of `qApp->processEvents`.
ACKs for top commit:
hebasto:
ACK 9a1d73fdffa4f35e33bc187ac9b3afbba28b1950, I have reviewed the code and it looks OK.
furszy:
ACK 9a1d73fdff
Tree-SHA512: a1b94676eb2fcb7606e68fab443b1565b4122aab93c35382b561842a049f4b43fecc459535370d67a64d6ebc4bcec0ebcda981fff633ebd41bdba6f7093ea540
8c47d599b87d6b2d43e7d37ce0aaf4f541535bb9 doc: improve -debuglogfile help to be a bit clearer (jonatack)
20d89d6802b67e2b8c864bedf23673d008e3faab bench: document expected results in logging benchmarks (jonatack)
d8deba8c36a42481b1c1e73009d7c9cc2cb25f70 bench: add LogPrintfCategory and LogPrintLevel benchmarks (Jon Atack)
102b2033493f0d61e9763d094cb8a0017f7e3a10 bench: order the logging benchmark code by output (Jon Atack)
4b3fdbf6fe20dc70c8dbdaacce161bc4e76b9c84 bench: update logging benchmark naming for clarity (Jon Atack)
4684aa8733e831d7858d43bec4271d5d026ba183 bench: allow logging benchmarks to be order-independent (Larry Ruane)
Pull request description:
Update our logging benchmarks for evaluating ongoing work like #25203 and refactoring proposals like #26619 and #26697.
- make the logging benchmarks order-independent (Larry Ruane)
- add missing benchmarks for the `LogPrintLevel` and `LogPrintfCategory` macros that our logging is migrating to; at some later point it should be feasible to drop some of the previous logging benchmarks
- update the logging benchmark naming to be clear which benchmark corresponds to which log macro, and update the ordering to be the same as the output
- add clarifying documentation to the logging benchmarks
- improve the `-debuglogfile` config option help to be clearer; can be tested by running `./src/bitcoind -help | grep -A4 '\-debuglogfile'`
Reviewers can run the logging benchmarks with:
```bash
./src/bench/bench_bitcoin -filter='LogP*.*'
```
ACKs for top commit:
LarryRuane:
ACK 8c47d599b87d6b2d43e7d37ce0aaf4f541535bb9
martinus:
code review & tested ACK 8c47d599b87d6b2d43e7d37ce0aaf4f541535bb9, here are my benchmark results:
achow101:
ACK 8c47d599b87d6b2d43e7d37ce0aaf4f541535bb9
Tree-SHA512: 705f8720c9ceaf14a1945039c7578a0c17a12215cbc44908099af4ac444561c3f95d833c5a91b325cdd4470737d8a01e2da64db2d542dd7c9a3747fbfdbf213e
The current `FormatISO8601DateTime` function will
return an empty string if it encounters an error
when converting the `int64_t` seconds since epoch
to a formatted date time. In the unlikely case that happens,
`strStamped.pop_back()` would be undefined behavior.
