💬 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

💬 romanz commented on pull request "http: set TCP_NODELAY when creating HTTP server":
(https://github.com/bitcoin/bitcoin/pull/30675#discussion_r1730252361)
Thanks! Fixed in 03d49d0f25.

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

💬 l0rinc commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#discussion_r1730299457)
Not sure, in other cases we've just fixed "re-used": https://github.com/bitcoin/bitcoin/pull/28605/files#diff-794891fcb80950b0c81f330c416efdf5d35789fb56a91a18dcc25266d7cdccfdL55

💬 l0rinc commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#discussion_r1730300095)
Lol, seems like everyone is talking about this: https://github.com/codespell-project/codespell/issues/3521