💬 romanz commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2173142997)
Thanks!
Will open a new PR to fix.

💬 romanz commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#issuecomment-3015014284)
Many thanks!

📝 romanz opened a pull request: "rest: rename `strURIPart` -> `uri_part`"
(https://github.com/bitcoin/bitcoin/pull/32825)
Following https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2172902737.

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core devel
...

💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3015029219)
Rebased over `master` and fixed `rest_tx_from_block()` argument name case (in addition to https://github.com/bitcoin/bitcoin/pull/32825).

💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2173159314)
True, sole change in latest push.

💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#issuecomment-3015120652)
db63d5bf81 reverts to previous status (do not review). Will apply the commented changes in the next push.

💬 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