💬 hodlinator commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2593959586)
in e78cfb399e5e59805557992c32d34080bfca6eee "refactor: Add util::Result failure values":

Isn't it closer to:
```suggestion
//! tuple<optional<SuccessType>, unique_ptr<tuple<FailureType, MessagesType>>>
```

💬 hodlinator commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2594047330)
Would anything be gained from making the template parameter an r-value? (Earlier commits do explicit `std::move()`s, justifying the function's name, but that justification appears to have dissipated).
```suggestion
static void Move(DstResult& dst, SrcResult&& src)
```

💬 hodlinator commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2599090229)
nit Q in 3c535e299efbf445ccd33c633ed455399d9785cd "wallet: fix clang-tidy warning performance-no-automatic-move":
I got the impression our expectation is that all commits should pass CI. So I would expect this change to come before or in the same commit that would cause CI failure. Is that only valid for the HEAD commit when it comes to commits that resolve clang-tidy and similar checks?

💬 hodlinator commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2608521278)
An example from my larger experimentation showed that `Expected` can be used to implement a variant of the chainstate refactor from this PR (0ad3a45633433377b44c9ae89b52703e0c750fdd), see self-contained commit https://github.com/hodlinator/bitcoin/commit/b40a36cbbab59440d84dba6d1cc16bce17d4869c.

💬 hodlinator commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2594016419)
remark: Took a few minutes to realize that `has_value()` doesn't exist because of the `void` template specialization of `SuccessHolder` which doesn't have that. Makes sense.

💬 hodlinator commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2608405722)
I'm guessing the reason for the existence of a separate `SuccessHolder` type is in order to specialize away a minimal subset of functionality in `SuccessHolder<void, ...>`. If that's part of the reason, it could be admitted in the comment block for the main `SuccessHolder` template?

🚀 achow101 merged a pull request: "precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442)

💬 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