💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608555177)
Argh, saw Windows CI failures: https://github.com/bitcoin/bitcoin/actions/runs/20114716290/job/57721194424?pr=33657#step:14:3972

I guess the OS doesn't like moving directories with opened files in them. Maybe best to make these checks conditional on the platform (`if platform.system() != "Windows":`).

🚀 achow101 merged a pull request: "merkle: migrate `path` arg to reference and drop unused args"
(https://github.com/bitcoin/bitcoin/pull/33805)

🤔 w0xlt reviewed a pull request: "kernel, validation: Refactor ProcessNewBlockHeaders to return BlockValidationState"
(https://github.com/bitcoin/bitcoin/pull/33856#pullrequestreview-3565099889)
reACK https://github.com/bitcoin/bitcoin/pull/33856/commits/be379fd52b82805ab99400457ea44f93430f16e2

🤔 ajtowns reviewed a pull request: "log: check fclose() results and report safely in logging.cpp"
(https://github.com/bitcoin/bitcoin/pull/33646#pullrequestreview-3565376667)
Should rebase on top of master rather than including a merge commit.

💬 ajtowns commented on pull request "log: check fclose() results and report safely in logging.cpp":
(https://github.com/bitcoin/bitcoin/pull/33646#discussion_r2608921951)
What's the value in having this outside the `m_cs` guard?

🤔 pablomartin4btc reviewed a pull request: "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency"
(https://github.com/bitcoin/bitcoin/pull/26988#pullrequestreview-3565418908)
tACK 5b05a9959f1633bfee78d9edb180c672b0640ab5

with `bitcoind` < v26:
```
/build/bin/bitcoin-cli -signet -datadir=/tmp/btc -addrinfo
error: -addrinfo requires bitcoind v26.0 or later which supports getaddrmaninfo RPC. Please upgrade your node or use bitcoin-cli from the same version.

```

📝 ajtowns opened a pull request: "rpc: Disallow captures in RPCMethodImpl"
(https://github.com/bitcoin/bitcoin/pull/34049)
When defining `RPCHelpMan` objects, we usually return a lambda, and mostly we define those via `[&](...) { ... }` which explicitly captures any parameters or local variables by reference. If we were to actually use any of those captures (we don't), we would invoke undefined behaviour. So instead, convert all the `[&]` to `[]` to avoid capturing, and as part of `RPCHelpMan` check that the function we provide is convertible to a bare function pointer, so that any attempts to capture anything (even
...

💬 ajtowns commented on pull request "logging: API improvements":
(https://github.com/bitcoin/bitcoin/pull/34038#discussion_r2609006357)
Nice. Left the RPCResult text including both forms of output, as that makes it a little more self-documenting that people can use the deprecatedrpc option if desired. Also left `include` in the client side so that bitcoin-cli still works with old bitcoinds or bitcoinds running deprecatedrpc=logging.

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3640543665)
Very minor touch: moving of a curly nit-fix, rebase (noop); mostly to get CI to run again (had some apparently temporary issue).

(e2e25f4947e65869b014483be70b764a62bd0df7 -> 1a0423cf25bb60f98a29e06c39b7db2998860cd9)

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2609436783)
I kept the change minimal, but since the line is touched, I've adjusted the curly brace placement to be consistent, as suggested.

👍 maflcko approved a pull request: "rpc: Disallow captures in RPCMethodImpl"
(https://github.com/bitcoin/bitcoin/pull/34049#pullrequestreview-3566039894)
lgtm. Seems fine to remove this silent footgun. I left a nit in the second commit, because I think it is fine to relax it a bit.

Also, while doing a global scripted-diff here, might as well do the rename `RPCHelpMan` to `RPCMethod` from https://github.com/bitcoin/bitcoin/pull/19386#discussion_r447743298?

💬 maflcko commented on pull request "rpc: Disallow captures in RPCMethodImpl":
(https://github.com/bitcoin/bitcoin/pull/34049#discussion_r2609441750)
nit in 48c3c41fa604b8634c509bb38d67de9bea2ab59d: I think the first commit makes a lot of sense and should be done. However, the second commit seems overly restrictive. Here, it is easy to re-create the calculation, but will it always be the case? I think it could make sense to either:

* drop the second commit, and let asan/valgrind disallow the dangling references, like they have done in the past
* Clarify the comment around the static_assert which is hit at compile time to say that any capture
...

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609482205)
https://github.com/bitcoin/bitcoin/commit/9f8809f2b1206ad34dfc68831c10db76e618de3f

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609482375)
https://github.com/bitcoin/bitcoin/commit/9f8809f2b1206ad34dfc68831c10db76e618de3f

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609482472)
https://github.com/bitcoin/bitcoin/commit/9f8809f2b1206ad34dfc68831c10db76e618de3f

💬 maflcko commented on pull request "bench: run `FindByte` across block-sized buffer":
(https://github.com/bitcoin/bitcoin/pull/34046#discussion_r2609578248)
the file isn't left around. This was fixed already in fa2fbaa4a29f80d3c7d5f0ad6b64035c3156dd12

💬 maflcko commented on pull request "cmake: Add fail pattern to `try_append_cxx_flags`":
(https://github.com/bitcoin/bitcoin/pull/34047#discussion_r2609591510)
i don't understand how this could lead to issues. If someone passes w-no-error during configure, it will also be present during build, and the build should pass, no?

What is the issues that this is trying to fix?

What are the steps to reproduce?

💬 maflcko commented on pull request "refactor: inline constant `f_obfuscate = false` parameter":
(https://github.com/bitcoin/bitcoin/pull/34048#discussion_r2609597196)
i presume this exists to allow indexes to obfuscate, if there is need to? E.g. when storing remote-user-provided data. E.g. an addrindex?

I understand this isn't needed right now, so no strong opinion.

💬 maflcko commented on pull request "doc: guix: Troubleshooting zdiff3 issue and uninstalling.":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2609640100)

running -> run [The sentence starts with an imperative ("Uninstall Guix itself...") and needs a parallel imperative verb; "or running" is a fragment — "or run" makes the coordination grammatical.]

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609654918)
Not sure about the `try`/`finally`. I think it may be better to leave the filesystem as-is in case of failure, so that devs inspecting the directory of a failed test finds it closer to how it looked when the failure happened. Haven't seen this kind of pattern in other functional tests. We don't continue the test regardless, so there's no benefit to checks later in the test.
```diff
--- a/test/functional/interface_rest.py
+++ b/test/functional/interface_rest.py
@@ -500,11 +500,9 @@ class REST
...