- Move the decision whether to translate an error message to where it is
defined. This simplifies call sites: no more `InitError(Untranslated(...))`.
- Make all functions in `util/error.h` consistently return a
`bilingual_str`. We've decided to use this as error message type so
let's roll with it.
This has no functional changes: no messages are changed, no new
translation messages are defined.
b00266fe0c refactor: replace pointers by references within tx_verify.{h,cpp} (Sebastian Falbesoner)
Pull request description:
This PR gets rid of another unnecessary use of raw pointers, similar to PR #19053 (see also issue #19062 where useful commands for finding potential candidates are listed) but in the tx verification module.
For the functions `CalculateSequenceLocks()` and `SequenceLocks()`, the `prevHeights` vector parameter type is changed to be passed as a reference. Note that there were no checks for null pointers -- if one would pass `nullptr` to one of the functions, the following line would immediately lead to a crash:
dcacea096e/src/consensus/tx_verify.cpp (L32)
ACKs for top commit:
Empact:
Code Review ACK b00266fe0c
Tree-SHA512: 0eb71591467905434082029128bdca4df94988c372af40dca325654f6c002c72a00c73776cb5e72d6de2b2f218649211a5dbf19300a2e01f1841d6034e0f01e0
34645c4dd0 Test txinwitness is accessible on coinbase vin (Rod Vagg)
3e4421070a Expose txinwitness for coinbase in JSON form (Rod Vagg)
Pull request description:
## Rationale
The CLI can provide you with everything about transactions and blocks that you need to reconstruct the block structure and raw block itself **except** for the witness commitment nonce which is stored in the `scriptWitness` of the coinbase and is not printed. You could manually parse the raw `"hex"` fields for transactions if you really wanted to, but this seems to defeat the point of having a JSONification of the raw block/transaction data.
Without the nonce you can't:
1. calculate and validate the witness commitment yourself, you can generate the witness tx merkle root but you don't have the nonce to combine it with
2. reconstruct the raw block form because you don't have `scriptWitness` stack associated with the coinbase (although you know how big it will be and can guess the common case of `[0x000...000]`)
I'm building some archiving tooling for block data and being able to do a validated two-way conversion is very helpful.
## What
This PR simply makes the `txinwitness` field not dependent on whether we are working with the coinbase or not. So you get it for the coinbase as well as the rest.
## Examples
Common case of a `[0x000...000]` nonce: 00000000000000000000140a7289f3aada855dfd23b0bb13bb5502b0ca60cdd7
```json
"vin": [
{
"coinbase": "0368890904c1fe8d5e2f706f6f6c696e2e636f6d2ffabe6d6d5565843a681160cf7b08b1b74ac90a719e6d6ab28c16d336b924f0dc2fcabdc6010000000000000051bf2ad74af345dbe642154b2658931612a70d195e007add0100ffffffff",
"txinwitness": [
"0000000000000000000000000000000000000000000000000000000000000000"
],
"sequence": 4294967295
}
],
...
```
Novel nonce value: 000000000000000000008c31945b2012258366cc600a3e9a3ee0598e8f797731
```json
"vin": [
{
"coinbase": "031862082cfabe6d6d80c099b5e21f4c186d54eb292e17026932e52b1b807fa1380574c5adc1c843450200000000000000",
"txinwitness": [
"5b5032506f6f6c5d5b5032506f6f6c5d5b5032506f6f6c5d5b5032506f6f6c5d"
],
"sequence": 4294967295
}
],
...
```
## Alternatives
This field could be renamed for the coinbase, `"witnessnonce"` perhaps. It could also be omitted when null/zero (`0x000...000`).
## Tests
This didn't break any tests and I couldn't find an obvious way to include a test for this. If this is desired I'd apreicate some pointers.
ACKs for top commit:
MarcoFalke:
ACK 34645c4dd0
Tree-SHA512: b192facc1dfd210a5ec3f0d5d1ac6d0cae81eb35be15eaa71f60009a538dd6a79ab396f218434e7e998563f7f0df2c396cc925cb91619f6841c5a67806148c85
34e641a564 test: Remove unnecessary disconnect_nodes call in rpc_psbt.py (Danny Lee)
e6e7abd51a test: remove redundant two-way disconnect_nodes calls (Danny Lee)
a9bd1f9adf test: warn if nodes not connected before disconnect_nodes (Danny Lee)
Pull request description:
There's no harm in calling `disconnect_nodes` for nodes that weren't connected (in this case it's a no-op). However, detecting this case and logging a warning can help ensure that tests are behaving as expected.
In addition, since `disconnect_nodes` works bidirectionally, I removed all instances of this pattern:
```
disconnect_nodes(self.nodes[0], 1)
disconnect_nodes(self.nodes[1], 0)
```
ACKs for top commit:
MarcoFalke:
review ACK 34e641a564👔
amitiuttarwar:
ACK 34e641a564. Thanks for this test improvement!
Tree-SHA512: 344855ceb46c012d43c13d7c09f44d32dcb7645706d10ae1e4645d9edca54c6c6c13fee26b79480755cdfcdf39b4b5770b36bb03ce71ba002d5be8a27fe008af
cb38b069b0 util: Don't reference errno when pthread fails. (MIZUTA Takeshi)
Pull request description:
Pthread library does not set errno.
Pthread library's errno is returned by return value.
ACKs for top commit:
practicalswift:
ACK cb38b069b0 -- patch looks correct
MarcoFalke:
review ACK cb38b069b0
hebasto:
ACK cb38b069b0, only squashed commits since the [previous](https://github.com/bitcoin/bitcoin/pull/19194#pullrequestreview-425831739) review.
Tree-SHA512: e6c950e30726e5031db97a7b84c8a9215da5ad3e5d233bcc349f812ad15957ddfe378e26d18339b9e0a5dcac2f50b47a687b87a6a6beaf6139df84f31531321e
fa2c2b50d8 doc: Extract net permissions doc (MarcoFalke)
Pull request description:
Moving the documentation of each flag form the already over-large init.cpp into the net permissions module should clean up the code a bit. Moreover, making the documentation available is also required for an (currently imaginary) `setnetpermissions` RPC.
ACKs for top commit:
Sjors:
re-utACK fa2c2b50d8
Tree-SHA512: c0a75facc9768913c28d2ffcdfaad8d60f7604d5584ee546adaf77d270563558d361aeaf354e49e349aca7e2e80814b27ffc24247e7b4f045c63cbdc079b449f
cc5c0d2299 refactor: Fix formatting of timedata.cpp (Hennadii Stepanov)
c2410ceb84 refactor: Replace RecursiveMutex with Mutex in timedata.cpp (Hennadii Stepanov)
Pull request description:
Only `GetTimeOffset()` and `AddTimeData()` functions lock this mutex. They do not call itself recursively, and do not call each other either directly or indirectly. Therefore, the `g_timeoffset_mutex` could be a non-recursive mutex.
Related to #19180.
ACKs for top commit:
MarcoFalke:
ACK cc5c0d2299 , checked the second commit with --word-diff-regex=. --ignore-all-space -U0 🦉
vasild:
ACK cc5c0d22 verified that recursion is not happening
Tree-SHA512: 38f6df689374d4a1a0e9aedb3ed5e885d8285c4da6b75f9bc84ae036936a159ef8276462db33b4f4dd5c71c6312fa9b45380f7a5726959665bc71dc39031be88
78c8f4fe11 refactor: Replace RecursiveMutex with Mutex in netbase.cpp (Hennadii Stepanov)
Pull request description:
The functions that could lock this mutex, i.e., `{S,G}etProxy()`, `{S,G}etNameProxy()`, `HaveNameProxy()`, `IsProxy()`, do not call itself recursively, and do not call each other either directly or indirectly. Therefore, the `g_proxyinfo_mutex` could be a non-recursive mutex.
Related to #19180.
ACKs for top commit:
MarcoFalke:
ACK 78c8f4fe11 , reviewed with the -W git option 👮
vasild:
ACK 78c8f4fe verified that recursion does not happen
Tree-SHA512: fc077fb371f38af5d05f1383c6bebf9926167c257892936fefd2d4fe6f679ca40124d25099e09f645d8ec266df222f96c5d0f9fd39eddcad15cbde0b427bc205
1a9ef1d398 refactor: Replace RecursiveMutex with Mutex in Shutdown() (Hennadii Stepanov)
Pull request description:
Step by step, going to replace all of the `RecursiveMutex` instances with the `Mutex` ones throughout the code base :)
Not sure if it is possible in all cases though...
This one is a low-hanging fruit.
ACKs for top commit:
MarcoFalke:
ACK 1a9ef1d398 Shutdown is not recursive, so the same thread can never lock twice (UB)
vasild:
ACK 1a9ef1d3 verified manually that `Shutdown()` is not called from places that could be called from inside `Shutdown()`.
Tree-SHA512: 362a507b1a6f97dc351f708224aedbfe4bee03c4398f394d78ee31c24d76a7012ffff0e6766866cd5fd9a8e0d8840f05a2741111fe583aa20d45f0af3df0dcfa
Pthread library does not set errno.
Pthread library's errno is returned by return value.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
fac6b9b938 test: Avoid overwriting the NodeContext member of the testing setup (MarcoFalke)
fa16e7816b build: Add -Wshadow-field (MarcoFalke)
Pull request description:
Adding this warning will eliminate unexpected test failures and hard to review code. Moreover, there shouldn't be a use case in Bitcoin Core that relies on fields to be shadowed.
ACKs for top commit:
fanquake:
ACK fac6b9b938 - Warnings compiling fa16e7816b are below. No warnings with fac6b9b938. The `-Wshadow-field` diagnostic has been available in Clang since 5.0.0. It's not available for GCC.
practicalswift:
ACK fac6b9b938 -- patch looks correct
hebasto:
ACK fac6b9b938, tested on Linux Mint 19.3 (x86_64):
Tree-SHA512: 824874ca10877efea7463cf934a2953147f3f99c486f04679426c14ff968975d8652cbba5729bfb7cb2c86c637ade5d1e5b873d611c06bad013a7cad8427e2bf
c514a4f59a doc: release note for `db` log category removal (Jon Atack)
4c0c89307d log: remove deprecated `db` log category (Jon Atack)
Pull request description:
The `db` log category was renamed to `walletdb` (like `coindb`) in #17410 and its upcoming removal announced in the 0.20 release notes.
```
- The `-debug=db` logging category has been renamed to
`-debug=walletdb` to distinguish it from `coindb`. The `-debug=db`
option has been deprecated and will be removed in the next major
release. (#17410)
```
This PR removes the warning and reverts to the usual behavior for an unrecognised log category.
```
$ bitcoin-cli logging '["db"]'
error code: -8
error message:
unknown logging category db
```
```
$ ./src/bitcoind -debug=db
Warning: Unsupported logging category -debug=db.
2020-06-07T15:30:45Z Bitcoin Core version v0.20.99.0-4c0c89307d (debug build)
2020-06-07T15:30:45Z Warning: Unsupported logging category -debug=db.
2020-06-07T15:30:45Z Assuming ancestors of block 0000000000000000000f2adce67e49b0b6bdeb9de8b7c3d7e93b21e7fc1e819d have valid signatures.
2020-06-07T15:30:45Z Setting nMinimumChainWork=00000000000000000000000000000000000000000e1ab5ec9348e9f4b8eb8154
2020-06-07T15:30:45Z Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
2020-06-07T15:30:45Z Using RdSeed as additional entropy source
```
ACKs for top commit:
MarcoFalke:
ACK c514a4f59a🔄
Tree-SHA512: fd62fd7ae0dc65446ba4401d75b4047e055396a33f7f1b176e79a7753250aec2a474ae604163d3f7e68710443c0ed2f45e44435d15f35612d794807e2142d5a3
fa4cd1fdae ci: Switch to bitcoincore.org download (MarcoFalke)
Pull request description:
bitcoin.org is down and not in our control, so it seems odd to rely on it for our ci infrastructure
ACKs for top commit:
troygiorshev:
ACK fa4cd1f
Tree-SHA512: f9f0e9c69a52b8b1906ceae195e8bcc189799fb39be921b26e3a37d1f8f3999831f86c96c3546848c0d01429c36cfb2d7c5f314655ac5282d3e8e4cdd838960e
501e6ab4e7 doc: Add documentation for 'checklevel' argument in 'verifychain' RPC call (Calvin Kim)
Pull request description:
Rationale: When ```bitcoin-cli help verifychain``` is called, the user doesn't get any documentation about the ```checklevel``` argument, leading to issues like #18995.
This PR addresses that issue and adds documentation for what each level does, and that each level includes the checks of the previous levels.
ACKs for top commit:
jonatack:
ACK 501e6ab4e7 `git diff 292ed3c 501e6ab` shows only change since last review is the verifychain RPCHelpMan edit; rebuild and retested manually anyway
MarcoFalke:
ACK 501e6ab4e7🚝
Tree-SHA512: 09239f79c25b5c3022b8eb1f76198ba681305d7e8775038e46becffe5f6a14c572e0c5d06b0723fe9d4a015ec42c9f7ca7b80a2a93df0b1b66f5a84a80eeeeb1
fa9604c46f doc: noban precludes maxuploadtarget disconnects (MarcoFalke)
fa3999fe35 net: Reformat excessively long if condition into multiple lines (MarcoFalke)
Pull request description:
Whitelisting has been replaced by permission flags, so properly document this. See also #10131
ACKs for top commit:
hebasto:
ACK fa9604c46f, I have reviewed the code and it looks OK, I agree it can be merged.
ariard:
ACK fa9604c
Tree-SHA512: 5aee917ab9817719f01ec155487542118e17fa3d145ae7e4bc0e872b2cec39cde9e7fbdee2ae77e9a52700dd8bcc366de4224152e08e709d44d08e0d2f19c613
5d77549d8b doc: Add mypy to test dependencies (Hennadii Stepanov)
7dda912e1c test: Do not swallow flake8 exit code (Hennadii Stepanov)
Pull request description:
After #18210 the `flake8` exit code in `test/lint/lint-python.sh` just not used that makes the linter broken.
This PR:
- combines exit codes of `flake8` and `mypy` into the `test/lint/lint-python.sh` exit code
- documents `mypy` as the test dependency
ACKs for top commit:
MarcoFalke:
Approach ACK 5d77549d8b, fine with me
practicalswift:
ACK 5d77549d8b
Tree-SHA512: e948ba04dc4d73393967ebf3c6a26c40d428d33766382a0310fc64746cb7972e027bd62e7ea76898b742a656cf7d0fcda2fdd61560a21bfd7be249cea27f3d41
4a7253ab6c Remove g_rpc_chain global (Russell Yanofsky)
e783197bf0 refactor: replace RegisterWalletRPCCommands with GetWalletRPCCommands (Russell Yanofsky)
Pull request description:
Replace with RPC request reference to new WalletContext struct similar to the existing NodeContext struct and reference.
This PR is a followup to #18740 removing the g_rpc_node global.
Some later PRs will follow this up and move more wallet globals to the WalletContext struct.
ACKs for top commit:
MarcoFalke:
ACK 4a7253ab6c🎋
ariard:
Code Review ACK 4a7253a, feel free to ignore comment it's super nit.
Tree-SHA512: 5bb5561c89f81811ca5232a58bf450e230d4218e62471c03227d142395fd36131672e99cb88329b33b9680a235db01e8b9d1c1e2a18288349e57205528deabab
0012471391 build: turn on --enable-c++17 by --enable-fuzz (Vasil Dimov)
Pull request description:
Fuzzing code uses C++17 specific code (e.g. std::optional), so it is not
possible to compile with --enable-fuzz and without --enable-c++17.
Thus, turn on --enable-c++17 whenever --enable-fuzz is used.
ACKs for top commit:
hebasto:
ACK 0012471391, tested on Linux Mint 19.3 (x86_64); verified that it fails to compile with `--enable-fuzz` and without `--enable-c++17` on master.
Tree-SHA512: 290531ea8d79de3b9251ea4ad21e793478b18150cc0124eea1e50c3a4ed92bab89c3e70ed0aa526906f8723ea952cdba4268f1560ae4be9bd25b9e4f9b97436c
Fuzzing code uses C++17 specific code (e.g. std::optional), so it is not
possible to compile with --enable-fuzz and without --enable-c++17.
Thus, turn on --enable-c++17 whenever --enable-fuzz is used.
fa7e002d52 ci: tsan with wallet (MarcoFalke)
Pull request description:
ACKs for top commit:
practicalswift:
ACK fa7e002d52 -- patch looks correct and Travis is happy
hebasto:
ACK fa7e002d52, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 1138459bbef72f402f32dae1e28d96f174901d4248d959b538b973747c8b06f221ecd81a386a1f915c0a2faaefb9fb8f11e3be39e6c5e2468bf9ae43d9f97757
f46b678acf qt: lock cs_main, m_cached_tip_mutex in that order (Vasil Dimov)
Pull request description:
Always lock the mutexes `cs_main` and `m_cached_tip_mutex` in
the same order: `cs_main`, `m_cached_tip_mutex`. Otherwise we may end up
in a deadlock.
`ClientModel::m_cached_tip_blocks` is protected by
`ClientModel::m_cached_tip_mutex`. There are two access paths that
lock the two mutexes in opposite order:
```
validation.cpp:2868 CChainState::ActivateBestChain(): lock cs_main
validation.cpp:2916 CChainState::ActivateBestChain(): call uiInterface.NotifyBlockTip()
ui_interface.cpp:52 CClientUIInterface::NotifyBlockTip(): go deep in boost
...
qt/clientmodel.cpp:255 BlockTipChanged(): lock m_cached_tip_mutex
```
and
```
qt/clientmodel.cpp:119 ClientModel::getBestBlockHash(): lock m_cached_tip_mutex
qt/clientmodel.cpp:121 ClientModel::getBestBlockHash(): call m_node.getBestBlockHash()
interfaces/node.cpp:200 NodeImpl::getBestBlockHash(): lock cs_main
```
From `debug.log`:
```
POTENTIAL DEADLOCK DETECTED
Previous lock order was:
m_cs_chainstate validation.cpp:2851
(1) cs_main validation.cpp:2868
::mempool.cs validation.cpp:2868
(2) clientmodel->m_cached_tip_mutex qt/clientmodel.cpp:255
Current lock order is:
(2) m_cached_tip_mutex qt/clientmodel.cpp:119
(1) ::cs_main interfaces/node.cpp:200
```
The possible deadlock was introduced in #17993
ACKs for top commit:
jonasschnelli:
Tested ACK f46b678acf
Tree-SHA512: 904f24b39bdc97c4d0ecb897a6980d8d479814535eb167e23105238800ea2f1f85273e3370cf894db58bc597f94c4f2e81fb68d0ff3362d468c16af5ce8f5d78
c4b574899a gui: Add Close All Wallets action (João Barbosa)
f30960adc0 gui: Add closeAllWallets to WalletController (João Barbosa)
Pull request description:
This PR adds the action to close all wallets.
<img width="405" alt="Screenshot 2020-06-01 at 01 06 12" src="https://user-images.githubusercontent.com/3534524/83365986-25a8b980-a3a4-11ea-9613-24dcd8eaa55c.png">
ACKs for top commit:
jonasschnelli:
Tested ACK c4b574899a
Tree-SHA512: 049ad77ac79949fb55f6bde47b583fbf946f4bfaf3d56d768e85f813d814cff0fe326b700f7b5e383cda4af7b5666e13043a6aaeee3798a69fc94385d88ce809
89f9fef1f7 refactor: Specify boost/thread/thread.hpp explicitly (Hennadii Stepanov)
fad8c890f5 txdb: Remove unused boost/thread (MarcoFalke)
faa958bc28 txindex: Remove unused boost/thread (MarcoFalke)
Pull request description:
There are predefined interruption points for `boost::thread`: https://www.boost.org/doc/libs/1_71_0/doc/html/thread/thread_management.html#interruption_points
However, non-boost threads such as `std::thread` or the `main()` thread can obviously not be interrupted. So remove all unused boost/thread from methods that are never executed in a `boost::thread`.
Most of them were accompanied by a `ShutdownRequested` anyway. So even if the current thread was a `boost::thread`, the interruption point would be redundant. (We only interrupt threads during shutdown)
ACKs for top commit:
fanquake:
ACK 89f9fef1f7
hebasto:
ACK 89f9fef1f7, tested on Linux Mint 19.3 (x86_64), verified shutdown in different scenarios.
Tree-SHA512: 17221dadedf2d107e5bda9e4f371cc4f8ffce6ad27cae41aa2b8f1150d8f1adf23d396585ca4a2dd25b1dc6f0d5c81fecd950d8557966ccb45a6d4a85a331d90
8b3136bd30 refactor: replace CNode pointers by references within net_processing.{h,cpp} (Sebastian Falbesoner)
Pull request description:
This PR is inspired by a [recent code review comment](https://github.com/bitcoin/bitcoin/pull/19010#discussion_r426954791) on a PR that introduced new functions to the net_processing module. The point of the discussion was basically that whenever we pass something not by value (in the concrete example it was about `CNode*` and `CConnman*`) we should either use
* a pointer (```CType*```) with null pointer check or
* a reference (```CType&```)
To keep things simple, this PR for a first approach
* only tackles `CNode*` pointers
* only within the net_processing module, i.e. no changes that would need adaption in other modules
* keeps the names of the variables as they are
I'm aware that PRs like this are kind of a PITA to review, but I think the code quality would increase if we get rid of pointers without nullptr check -- bloating up the code by adding all the missing checks would be the worse alternative, in my opinion.
Possible follow-up PRs, in case this is received well:
* replace CNode pointers by references for net module
* replace CConnman pointers by references for net_processing module
* ...
ACKs for top commit:
MarcoFalke:
ACK 8b3136bd30🔻
practicalswift:
ACK 8b3136bd30
Tree-SHA512: 15b6a569ecdcb39341002b9f4e09b38ed4df077e3a3a50dfb1b72d98bdc9f9769c7c504f106456aa7748af8591af7bb836b72d46086df715ab116e4ac3224b3b
7daffc6a90 [test] CScriptNum Decode Check as Unit Tests (Gillian Chu)
Pull request description:
The CScriptNum test (#14816) is a roundtrip test of the test framework. Thus, it would be better suited as a unit test. This is now possible with the introduction of the unit test module for the functional tests. See #18576.
This PR:
1. Refactors the CScriptNum tests into 2 unit tests, one in script.py and one in blocktools.py.
2. Extends the script.py CScriptNum test to trial larger numbers.
ACKs for top commit:
laanwj:
ACK 7daffc6a90
Tree-SHA512: 17a04a4bfff1b1817bfc167824c679455d9e06e6e0164c00a7e44f8aa5041c5f5080adcc1452fd80ba1a6d8409f976c982bc481d686c434edf97a5893a32a436
fa1433ac1b rpc: Remove special case for unknown service flags (MarcoFalke)
Pull request description:
The special case to return a bit as an integer is clumsy and undocumented. Probably also irrelevant because there shouldn't currently be a non-misbehaving client that connects to Bitcoin Core and advertises an unknown service flag.
Thus, simply remove the code.
ACKs for top commit:
laanwj:
ACK fa1433ac1b
Tree-SHA512: 942de6a577a9ee076ce12c92be121617640d53ee8c3424064c45a30a7ff789555d3722a4203670768faf81da2a40adfed3ec5cdeb5da06f04be81ddb53b9db7e
ac94141af0 validation: delay flushing undo files in syncing node case (Karl-Johan Alm)
Pull request description:
Fixes#17890. Replaces #17892.
Data files (`{blk|rev}<number>.dat`) pre-allocate space as they are written, and then trims down to the final size once they move on to the next sequence ("finalized flush"). The code currently assumes (incorrectly) that blk and rev files finish at the same time, but because blk files are written as blocks come in, and rev files are written in block height order, rev files end up being written to for awhile after moving on to the next block file, resulting in pre-allocation and waste of up to 1 MB of space per rev file.
The exact point at which rev file writing finishes is the highest height block found inside the corresponding block file, which is already available in the CBlockFileInfo vector. This PR moves finalized flushing of undo files to to directly after the undo data for the previous block file has been written.
There is a branch with annotation that demonstrates how this is handling flushing here: https://github.com/kallewoof/bitcoin/tree/200124-rev-files-annotated
ACKs for top commit:
vasild:
ACK ac94141af (no changes in the code since ed34e00da).
fjahr:
Code review re-ACK ac94141af0
jonatack:
Code review ACK ac94141af0
Tree-SHA512: 1d4e3b3d1d99bd7ebe7a2f632b1231146dd4f9f993c54db3a4090d9c086d95d2e4c327fd936066392b3afc6277b8f3a908d5c5993d4c8e49f72b92a417716dd2
eea8114657 build: Enable unreachable-code-loop-increment (Jonathan Schoeller)
d15db4b1fc refactor: Fix unreachable code in init arg checks (Jonathan Schoeller)
Pull request description:
Closes: #19017
In #19015 it's been suggested that we add some new compiler warnings to our build. Some of these, such as `-Wunreachable-code-loop-increment`, generate warnings. We'll likely want to fix these up if we're going to turn these warnings on.
```shell
init.cpp:969:5: warning: loop will run at most once (loop increment never executed) [-Wunreachable-code-loop-increment]
for (const auto& arg : gArgs.GetUnsuitableSectionOnlyArgs()) {
^~~
1 warning generated.
```
aa8d76806c/src/init.cpp (L968-L972)
To fix this, collect all errors, and output them in a single error message after the loop completes. This resolves the unreachable code warning, and avoids popup hell that could result from outputting a seperate message for each error or warning one by one.
ACKs for top commit:
laanwj:
Code review ACK eea8114657
hebasto:
re-ACK eea8114657, only suggested changes applied since the [previous](https://github.com/bitcoin/bitcoin/pull/19131#pullrequestreview-421772387) review.
Tree-SHA512: 2aa3ceb7fab581b6ba2580900668388d8eba1c3001c8ff9c11c1f4a9a10fbc37f30e590249862676858446e3f4950140a252953ba1643ba3bfd772f8eae20583
