💬 maflcko commented on pull request "rest: rename `strURIPart` -> `uri_part`":
(https://github.com/bitcoin/bitcoin/pull/32825#issuecomment-3015151022)
could be a scripted-diff? See e.g. 8888beea8d477b1d4a2dfd2a0bb5f686de62f3ff

💬 purpleKarrot commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#issuecomment-3015173238)
NACK

I am with @fanquake here. The complexity increases exponentially with the number of build options.
Since the proposed build option does not actually affect build artifacts (like optional python bindings for some library would do), this does not really require a user facing setting. The approach in https://github.com/bitcoin/bitcoin/pull/31233 is much more elegant.

💬 musaHaruna commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#issuecomment-3015180485)
> Concept ACK [be75fa4](https://github.com/bitcoin/bitcoin/commit/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3)
>
> Might be good to add release notes for this PR as it is adding to the user-facing RPC interface, similarly to: https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/doc/release-notes-27826.md

Added in [ede20b4](https://github.com/bitcoin/bitcoin/commit/ede20b438cf62b8292a3cae95893d34c3faa5b0c)

💬 musaHaruna commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2173209518)
Fixed in [b5d472](https://github.com/bitcoin/bitcoin/pull/32800/commits/b5d472e1c5436a9398c533d7ff07767d13399a5b). Thanks!

💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#issuecomment-3015183434)
2b8b4e0673 Adds unit test for negative size in the full constructor. Assumes `num_bytes >= 0` in `GetFee` and modifies fuzz test to avoid negative inputs to `GetFee` (to not make the Assume crash).

💬 musaHaruna commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2173209928)
Fixed as suggested in [70316b5](https://github.com/bitcoin/bitcoin/pull/32800/commits/70316b5b4a6d9a6676525618de3445b4968479be). Thanks

✅ hebasto closed a pull request: "cmake: Introduce `WITH_PYTHON` build option"
(https://github.com/bitcoin/bitcoin/pull/31669)

💬 romanz commented on pull request "rest: rename `strURIPart` -> `uri_part`":
(https://github.com/bitcoin/bitcoin/pull/32825#issuecomment-3015200308)
> could be a scripted-diff?

Thanks, TIL :)
Fixed in https://github.com/bitcoin/bitcoin/commit/cc2ec2c174c899fd71e3a0205e38077df0e7424f.

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