👍 l0rinc approved a pull request: "rest: rename `strURIPart` -> `uri_part`"
(https://github.com/bitcoin/bitcoin/pull/32825#pullrequestreview-2968521623)
utACK cc2ec2c174c899fd71e3a0205e38077df0e7424f

nit: it might be slightly simpler in my opinion to inline the function and only do it for a single file instead of all tracked ones:
```bash
-BEGIN VERIFY SCRIPT-
sed -i 's/\<strURIPart\>/uri_part/g' src/rest.cpp
-END VERIFY SCRIPT-
```

nit2: as I've seen, the script is usually at the very end of the commit message

💬 romanz commented on pull request "rest: rename `strURIPart` -> `uri_part`":
(https://github.com/bitcoin/bitcoin/pull/32825#issuecomment-3015208951)
Thanks @l0rinc - fixed the nits in https://github.com/bitcoin/bitcoin/commit/856f4235b1ae56540e1d2279c27405d44a5c7b34.

💬 purpleKarrot commented on pull request "cmake: Improve robustness and usability":
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2173220922)
If the test requires environment variables to be set, setting them in the github workflow is not a good approach. How are developers expected to reproduce that locally?

Better use the [`ENVIRONMENT`](https://cmake.org/cmake/help/latest/prop_test/ENVIRONMENT.html) property:

```cmake
set_property(TEST util_test_runner PROPERTY ENVIRONMENT
BITCOINUTIL=$<TARGET_FILE:bitcoin-util>
BITCOINTX=$<TARGET_FILE:bitcoin-tx>
)
```

💬 purpleKarrot commented on pull request "cmake: Improve robustness and usability":
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2173215537)
Maybe those two custom commands should also be turned into tests and disabled when python is not available?

🤔 polespinasa reviewed a pull request: "refactor: CFeeRate encapsulates FeeFrac internally"
(https://github.com/bitcoin/bitcoin/pull/32750#pullrequestreview-2968519019)
c4af90f4a80832cbaa925d33dbf4ba0f0eab6374 adds tests for negative size in full constructor.
0aaeba9fe79df5ed51b3802056f93c0e7857a9fe uses `Assume(num_bytes >= 0)` instead of returning -1 if size is negative.

💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2173219182)
Not sure if there's a cleaner way to handle this. Without it negative values can be provided to `GetFee()` hitting the `Assume(num_bytes >= 0)` statement and failing the code.

💬 l0rinc commented on pull request "rest: rename `strURIPart` -> `uri_part`":
(https://github.com/bitcoin/bitcoin/pull/32825#issuecomment-3015210185)
reACK 856f4235b1ae56540e1d2279c27405d44a5c7b34

💬 pinheadmz commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3015228080)
> drop HTTPRPCTimer altogether

Interesting, I can try that approach... for that matter, could we rip out `QtRPCTimerInterface` and the base class `RPCTimerInterface` as well? I don't think any of it is used outside of `walletpassphrase` and any future RPCs we add can just use the node/wallet scheduler as well...

💬 HowHsu commented on pull request "validation: fetch block inputs on parallel threads 10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2173389714)
> Is there anything prohibiting us from doing something like this to minimize synchronization and lock contention during the fetch phase? I understand some synchronization would still be needed during the merge, but this could help reduce global locks and unnecessary synchronization throughout the process.

As far as I know, the advantage of coroutines over threads is faster context switching, since it doesn’t go through the operating system kernel. This advantage only becomes apparent unde
...

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where Base64 encoding is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3015984834)
I generalised this change to any string parameter which might contain the `=` char in it. I also added the functional test for most of the RPCs I included in the new table. For any new RPC method which might need to be parsed this way with -named enabled, one can add the new entry of that RPC in the `vRPCStringParams` table.

💬 gmaxwell commented on pull request "test: disable secp256 tests by default":
(https://github.com/bitcoin/bitcoin/pull/32782#issuecomment-3016185525)
As an author of that library I am uncomfortable with any usage unguarded by correctness tests. The software is sensitive to compiler errors (which can be triggered by all sorts of random environmental nonsense) and ought to be tested with the exact compiler environment used. A single predictable bit error in signing is enough to leak private keys and can happen in ways that still result in valid signatures. (e.g. miscompilation that results in the first byte/bit of the nonce being set to any s
...

💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3016379665)
Low-latency HTTP request benchmark improvement - compared with `main` branch with https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2169518557:
```
$ ab -k -c 1 -n 100000 http://localhost:8332/rest/txfromblock/000000000000000000017bfd05b5fa367a424c4a565a4baf7950d9e8605df8ec-5000.bin

Document Path: /rest/txfromblock/000000000000000000017bfd05b5fa367a424c4a565a4baf7950d9e8605df8ec-5000.bin
Document Length: 234 bytes

Concurrency Level: 1
Time taken for tests:
...

👋 l0rinc's pull request is ready for review: "clang-format: modernize and realign clang-format configuration"
(https://github.com/bitcoin/bitcoin/pull/32813)

💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3016428956)
The latency of 404 response has improved too:
```
$ ab -k -c 1 -n 100000 http://localhost:8332/rest/
...
Time taken for tests: 2.876 seconds
Complete requests: 100000
Failed requests: 0
Non-2xx responses: 100000
Keep-Alive requests: 100000
Total transferred: 9500000 bytes
HTML transferred: 0 bytes
Requests per second: 34774.44 [#/sec] (mean)
Time per request: 0.029 [ms] (mean)
Transfer rate: 3226.14 [Kbytes/sec] received
```

`ma
...

🤔 romanz reviewed a pull request: "Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061#pullrequestreview-2969148112)
Tested ACK e531a7cd2c

💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2173661408)
Doing a differential perf flame graph on b3bb4031 vs 1ba50d81 (master vs rebased `darosior:2503_nonstd_tx_sigops`) indicates the slowdown is indeed caused by both `AccessCoin` and the extra `CScript::GetSigOpCount` calls:
<img width="1000" alt="image" src="https://github.com/user-attachments/assets/24ef8b91-bd9c-4fc0-be1a-1c7564d2a418" />

<details>
<summary>Details</summary>

```bash
COMMITS="b3bb4031ab32a1306c610f8683b25b8459b21dbc 1ba50d81b251c705aae872a2a9b37778ffa50c62"; \
CC=gcc; C
...

💬 HowHsu commented on pull request "checkqueue: implement a new scriptcheck worker pool with atomic variables":
(https://github.com/bitcoin/bitcoin/pull/32791#issuecomment-3016695705)
> hmmm...Ok, I realized that this may only be suitable for when all the preout Coin are in CCoinsViewCache, otherwise the `Add()` is not fast due to I/O there.

With #31132 , this one makes sense again, need to test them together in all-cache-miss case

💬 HowHsu commented on pull request "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1":
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-3016699198)
I post a new scriptcheck pool implementation #32791 with atomic variables rather than mutex to reduce thread sync contention. That achieves very good performance improvement. But the idea is based on that `the adding period is very short compared to the script verification period`, so it has to be with PR #31132. @sipa and others, welcome to take a look.

📝 HowHsu reopened a pull request: "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1"
(https://github.com/bitcoin/bitcoin/pull/32692)
A fixed MAX_SCRIPTCHECK_THREADS value is not flexible for users to leverage their cpu resources, and a value large than nCores - 1 doesn't make sense since it only adds some context switch overhead. Set it to nCores - 1. Assumption: A user who sets the number of script verification workers is aware of how this affects the system performance, otherwise he/she leaves it as default (which is 0)

💬 HowHsu commented on pull request "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1":
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-3016707658)
> Maybe this is something best solved by runtime benchmarking? (Or perhaps just at startup, since early blocks may not give us the parallelization we need for it)

I think we can do it so, binary search the best worker thread number with bencharking on some standard test data.