💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718241896)
This was quite simple before, now it might be faster by a few nanos, but it's less readable.
Given that `HexStr` was already tested properly, could we maybe go the other way and convert `detsig` to hex string and compare against the hard coded value, i.e.
```suggestion
BOOST_CHECK_EQUAL(HexStr(detsig), "304402205dbbddda71772d95ce91cd2d14b592cfbc1dd0aabd6a394b6c2d377bbe59d31d022014ddda21494a4e221f0824f0b8b924c43fa43c0ad57dccdaa11f81a6bd4582f6");
```

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718252997)
btw, this is in the preprocessing phase of the benchmark, the performance doesn't matter here at all - only inside the `.run(` part, so I'd go with the cleanest code here, too.

💬 sipa commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2291069514)
@vasild Yes, but that gratuitously reveals to its peers that it is the transaction originator too (not really an issue in your private broadcast setting, but in nearly all others, this would be undesirable for that reason).

💬 twofaktor commented on issue "[FEATURE REQUEST] Enable new Tor PoW feature for automatic creation of Bitcoin Core onion hidden service":
(https://github.com/bitcoin/bitcoin/issues/28499#issuecomment-2291079391)
> @twofaktor if you'd like me to open a PR with this doc update, then let me know and I'd be happy to do that.

Hi, thanks for your dedication, IMO, if it is possible to add this secure protection feature in any of the variety of configuration cases (manually, without using the port control method), I think it should be added to the docs. If at some point the possibility of using it also using the port control arrives, add it when the time comes

💬 tdb3 commented on pull request "wallet: fix blank legacy detection":
(https://github.com/bitcoin/bitcoin/pull/30621#issuecomment-2291090303)
> > Saw some `may be used uninitialized` warnings when building, e.g.
>
> What compiler? I don't see those warnings.

gcc/g++ (Debian 12.2.0-14) 12.2.0

Looks like it's on my end. Did a fresh clone and build and didn't see the same warnings.

👍 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.