💬 achow101 commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#issuecomment-3639270227)
ACK 0ac969cddfdba52f7947e9b140ef36e2b19c2c41

💬 ajtowns commented on pull request "doc: improvements to doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/pull/33986#discussion_r2608498884)
Wrong bracket for bip 384

🚀 achow101 merged a pull request: "[IBD] coins: reduce lookups in dbcache layer propagation"
(https://github.com/bitcoin/bitcoin/pull/33602)

💬 achow101 commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3639323671)
ACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94

🤔 hodlinator reviewed a pull request: "refactor: Add util::Result failure types and ability to merge result values"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-2343976961)
re: https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3617394520

Excuse me, I somehow glossed over the fact that you had 3 template parameters in this PR, not 2. :facepalm: I missed that you allowed for a `FailureType` apart from `MessageType`, the former defaulting to `void` but messages being default-on. I see a greater justification for the distinct `Result` type. However, it seems the either/or aspect remains, if I'm not mistaken.

Inspired by your reply, I nerd-sniped myself i
...

💬 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.