👍 tdb3 approved a pull request: "wallet: fix blank legacy detection"
(https://github.com/bitcoin/bitcoin/pull/30621#pullrequestreview-2240178506)
ACK 6ed424f2db609f9f39ec1d1da2077c7616f3a0c2

🤔 hodlinator reviewed a pull request: "refactor: Implement missing error checking for ArgsManager flags"
(https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2240110993)
Concept ACK 1e37bcf9fc11562baaedea24685c31f60ef2de31

The current state of things with no/ad-hoc validation of command line args since #22766 in Nov 2021 is not a good place to be. There can be a tendency to neglect stringent argument parsing on larger projects as long as the happy path of correctly specified args still works.

Other more embryonic and radical initiatives exist to make args more strictly typed at compile time (#22978, and @ajtowns https://github.com/ajtowns/bitcoin/pull/8).
...

💬 hodlinator commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1718228472)
The example for `ALLOW_INT | ALLOW_BOOL` in 6865a198f5db30bd494b3a2540f47ee728963908 is a bit worrying.

```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 2f2931cef1..e34bb35b7c 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3622,8 +3622,8 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
int prev_version = walletInstance->GetVersion();
if (gArgs.GetBoolArg("-upgradewallet", fFirstRun))
{
-
...

🤔 tdb3 reviewed a pull request: "addrman: change internal id counting to int64_t"
(https://github.com/bitcoin/bitcoin/pull/30568#pullrequestreview-2240216484)
Concept ACK.
Briefly reviewed, and will return to review in more detail. Love the approach to introduce `nid_type`. Makes things safer moving forward.

💬 glozow commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718299849)
This fails for me locally, because somehow I have 0000000000000000 as my xor key... but it fails consistently. Unsure if that's supposed to be possible?

👍 TheCharlatan approved a pull request: "refactor: Remove Span operator==, Use std::ranges::equal"
(https://github.com/bitcoin/bitcoin/pull/29071#pullrequestreview-2240220684)
ACK fad0cf6f2619df8df435a2da6da49eeb5510a10f

The last commit seems a bit random, why change that function and not any of the other occurrences of `std::equal`?

💬 ismaelsadeeq commented on issue "wallet: setting changes are subject to race conditions":
(https://github.com/bitcoin/bitcoin/issues/30620#issuecomment-2291135117)
@wydengyre
Thanks for reporting the issue.

> It's likely possible to demonstrate this with a quick BASH or Python script that starts `bitcoind` and creates two wallets simultaneously.

I tried to recreate this in the functional test by spawning threads and calling `createwallet` simultaneously with `load_on_startup=True`.

<details>
<summary>See the diff</summary>

```diff
diff --git a/test/functional/wallet_startup.py b/test/functional/wallet_startup.py
index 6feb00af8e1..2fed8
...

📝 hodlinator opened a pull request: "test: Shut down framework cleanly on RPC connection failure"
(https://github.com/bitcoin/bitcoin/pull/30660)
Without this PR, `BitcoinTestFramework.start_nodes()`, upon failing to establish RPC connections and calling `stop_nodes()`, would raise additional exceptions which prevented a clean shutdown.

Related: #30390

💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2291136157)
Encountered a case of #30390 here:
https://github.com/bitcoin/bitcoin/actions/runs/10380115902/job/28739417666?pr=30377

Output with only the test (no fix commit):
```
$ test/functional/feature_framework_rpc_failure.py
2024-08-15T07:30:20.427000Z TestFramework (INFO): PRNG seed is: 635212259632049987
2024-08-15T07:30:20.427000Z TestFramework (INFO): Initializing test directory /run/user/1001/bitcoin_func_test_ujsvbdnm
2024-08-15T07:30:20.428000Z TestFramework (INFO): Forcing us to timeou
...

👍 TheCharlatan approved a pull request: "util: Use consteval checked format string in FatalErrorf"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2240226231)
ACK fa1f64d15e75137a6d4b469e7de6e1be0fda762a

💬 TheCharlatan commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1718305178)
Nit: Add include for `util/string.h`

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1718333530)
Sure, done

👍 Sjors approved a pull request: "kernel: pre-28.x chainparams and headerssync update"
(https://github.com/bitcoin/bitcoin/pull/30658#pullrequestreview-2240249200)
ACK cd913de6488d25057e3bf7dcad1508e9d689f87f

If you have to touch, can you make `contrib/devtools/headerssync-params.py` executable?

💬 Sjors commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1718320300)
985d1ec267f44b7897855fd2312da76b3fcd165b: this is already 13G for me.

💬 maflcko commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2291183910)
Can you explain this a bit more? I think optimizing the test framework to cleanly shut down on a failure is a bit premature, and may not be needed. However, if a failing test may leave behind a dangling bitcoind process, this seems like something to fix, because it will otherwise lead to test warnings and failures down the road.

💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#issuecomment-2291189197)
> The last commit seems a bit random, why change that function and not any of the other occurrences of `std::equal`?

Good point! I'll address this, if I have to re-touch.

💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718344848)
Maybe a leftover previous `bitcoind` was used? Can you try `make clean`, just to double check?

🤔 ryanofsky reviewed a pull request: "refactor: Replace ParseHex with consteval ArrayFromHex"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2239084398)
Code review 734ac5a9002493013c0f8afe763f751ac99f89c8

After experimenting with simplifying this. I don't think I like this approach anymore. I think adding the `ConstevalHexLiteral` class is basically going out of the way to make the code less efficient and more confusing, because instead of having a clear separation between runtime and compile time functions, we are duplicating code at runtime and compile time to provide a hybrid function that checks hex strings at compile time and then store
...

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717501165)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717382220

On second thought, while this warning message is more accurate, I don't think it is clear, because it is implying that return value will use stack space, when we should not expect that to be the case normally. Would suggest:

* It may be preferable to call VecFromHex instead of this function to save stack space, or to declare the returned constexpr to avoid using stack space, if returned array is large and being used t
...

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717819227)
In commit "refactor: Add consteval ArrayFromHex and VecFromHex" (b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795)

I can't see a benefit to defining a string_view constructor here and depending on string_view when this class is only supposed to accept null terminated string literals. This class could be simplified by dropping this constructor, dropping the string_view member and just declaring a const char* member.