💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2308510085)
I'm not sure what's with the `Instruction clone failed` CI failures, but the change looks good to me

ACK 92e599d57c507f35d889c3a804d2a7020dcc6b1d

💬 1440000bytes commented on issue "DNS seed "seed.bitcoinstats.com" doesn't support filtering while the comments says it does":
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2308513735)
> Only bumping this because there's a good chance this seed will serve zero addresses by release time.

[Pristine node count](https://21.ninja/dns-seeds/pristine-count/) is already zero.


🤔 jonatack reviewed a pull request: "doc: fix 3 simple CI codespell warnings"
(https://github.com/bitcoin/bitcoin/pull/30700#pullrequestreview-2258924418)
Missing the following one with current codespell 2.3.0

```
src/qt/forms/optionsdialog.ui:344: incomin ==> incoming
```

Perhaps allow "highTS" but not "hights", as the latter could be an misspelling of "heights"

```diff
--- a/test/lint/spelling.ignore-words.txt
+++ b/test/lint/spelling.ignore-words.txt
hashIn
+highTS
inflight
```

💬 jonatack commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#discussion_r1730112466)
maybe add "re-use" to the ignore list

https://www.oxfordreference.com/display/10.1093/acref/9780195135084.001.0001/acref-9780195135084-e-1902

```diff
--- a/test/lint/spelling.ignore-words.txt
+++ b/test/lint/spelling.ignore-words.txt
outIn
+re-use
requestor
```

💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2308521018)
> Thought about a potential alternative that updates/enhances `scanblocks` instead of adding a new RPC (with the advantage of not needing to provide block hashes). Adding `getdescriptoractivity` seems like a better solution, since it avoids breaking RPC compatibility. Carrying block hashes from `scanblocks` to `getdescriptoractivity` seems like a reasonably price to pay for compatibility.

I thought about this initially and I think it's probably worth doing at some point via an additional `wit
...

💬 maaku commented on pull request "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test":
(https://github.com/bitcoin/bitcoin/pull/30681#issuecomment-2308521346)
Hate to be that guy, but this form of the time-warp mitigation closes off the possibility of using something like forward blocks for scaling bitcoin on-chain in the future. That was my voiced objection to the "create consensus clean-up" on the mailing list. Of course it only applies to testnet4, but in practice this ends up throwing up a hurdle to later deployment of something like forward blocks, as it can no longer be tested on the primary testnet. Has there been consideration given to limitin
...

💬 l0rinc commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#discussion_r1730121698)
We have a few cases of both, but I don't recall seeing it spelled this way anywhere else. My vote goes for fixing them instead.

💬 l0rinc commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2308524353)
> src/qt/forms/optionsdialog.ui:344: incomin ==> incoming

I've seen this one before, it's not a typo and we can't really whitelist it either: `incomin&g` (I think it means that `g` is a shortcut key)

🤔 ismaelsadeeq reviewed a pull request: "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings"
(https://github.com/bitcoin/bitcoin/pull/30697#pullrequestreview-2258946638)
> Today, the race is in the wallets flow, but tomorrow it could easily affect another settings option.

I agree with this point. All clients that update settings will be victim to the same bug.

> Given this context, I've implemented a different solution that makes the underlying settings update operation atomic: [furszy@de05ba1](https://github.com/furszy/bitcoin-core/commit/de05ba1144bd6332d1d903d07e49463268412dc0). This commit also shortens the test code.

This seems like a great idea an
...

💬 jonatack commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#discussion_r1730128226)
Yes, I mentioned it because it doesn't need fixing as it's not incorrect, and the linter raising on it will just annoy contributors needlessly.

💬 jonatack commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2308532905)
> I've seen this one before, it's not a typo and we can't really whitelist it either: `incomin&g` (I think it means that `g` is a shortcut key)

Probably preferable to avoid needlessly raising. It will need to handled in any case when the CI is updated to a current version of codespell.

```diff
--- a/test/lint/spelling.ignore-words.txt
+++ b/test/lint/spelling.ignore-words.txt
+incomin
inflight
```

💬 jonatack commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#discussion_r1730131890)
We already do the same for words in the ignore list like invokable, requestor, unparseable, useable, and warmup. (It looks like "warmup" can be removed from the ignore list now.)

💬 jonatack commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1730137103)
I think the new test file needs to be added to the runner.

```diff
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -396,6 +396,7 @@ BASE_SCRIPTS = [
'feature_config_args.py',
'feature_presegwit_node_upgrade.py',
'feature_settings.py',
+ 'rpc_getdescriptoractivity.py',
'rpc_getdescriptorinfo.py',
'rpc_mempool_info.py',
'rpc_help.py',
```

💬 theStack commented on pull request "http: set TCP_NODELAY when creating HTTP server":
(https://github.com/bitcoin/bitcoin/pull/30675#discussion_r1730163427)
nit: imho slightly preferred, for not having to repeat the type
```suggestion
if (setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (sockopt_arg_type)&one, sizeof(one)) == SOCKET_ERROR) {
```

👍 theStack approved a pull request: "http: set TCP_NODELAY when creating HTTP server"
(https://github.com/bitcoin/bitcoin/pull/30675#pullrequestreview-2259013554)
Tested ACK c1bec8269712059705d5f416257ad5201ec3d4a8

With the benchmark instructions from the opening post (using Apache's `ab` tool, found in the `apache2-utils` package on Debian Linux) adapted to fetch a recent block, this leads to a >10x speed-up on my machine: mean `Time per request` is 46.617ms on master vs. 3.717ms on the PR.

👍 theStack approved a pull request: "test: XORed blocks test follow up"
(https://github.com/bitcoin/bitcoin/pull/30669#pullrequestreview-2259021660)
ACK ab9d2451c47fd30abecaea3f8d8a6e33541911d7

💬 theStack commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1730169778)
an 's' got lost on the move up
```suggestion
# from InitBlocksdirXorKey::xor_key.size()
```

💬 tdb3 commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1730172019)
Thanks. Fixed.

👍 theStack approved a pull request: "test: XORed blocks test follow up"
(https://github.com/bitcoin/bitcoin/pull/30669#pullrequestreview-2259025660)
re-ACK 1c403af92be13cbdfa6ab1906f8cc34fabc21e63

👍 tdb3 approved a pull request: "test: Avoid intermittent block download timeout in p2p_ibd_stalling"
(https://github.com/bitcoin/bitcoin/pull/30705#pullrequestreview-2259034843)
CR ACK fa5b58ea01fac1adb6336b8b6b5217193295c695