💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608396257)
If we don't have a default, the compiler will warn since we have the `-Wswitch` set - in which case I don't see why the `assert(false)` is needed.
Alternatively, if we add a `default: assert(false)`, we don't need to specify all values and the compiler won't warn, the failure will be on runtime.
Anyway, off topic, I'm fine either way.

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3639185341)
ACK 44758e8387e66ec49d190b59f8a8fefab84b1e69

Since last push a `HTTP_NOT_FOUND` error code was changed to `HTTP_INTERNAL_SERVER_ERROR`, a test `BOOST_REQUIRE` was changed to a more permissive `BOOST_CHECK`, and in functional tests an assert was adjusted and a missing-block-data case was added for the status of the response.

💬 achow101 commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3639185350)
ACK 6eb5ba569141b722a5e27ef36e04994886769c44

📝 l0rinc opened a pull request: "refactor: inline constant `f_obfuscate = false` parameter"
(https://github.com/bitcoin/bitcoin/pull/34048)
Split out of https://github.com/bitcoin/bitcoin/pull/33324 since it makes sense on its own.

Currently the parameter is only called with a single constant parameter, it doesn't make sense to needlessly obfuscate the constructor (pun intended).

💬 stickies-v commented on pull request "logging: API improvements":
(https://github.com/bitcoin/bitcoin/pull/34038#discussion_r2608431613)
I think this should work?

<details>
<summary>git diff on 77f272c094</summary>

```diff
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index 0693b60557..25c313c552 100644
--- a/src/rpc/client.cpp
+++ b/src/rpc/client.cpp
@@ -325,7 +325,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "psbtbumpfee", 1, "replaceable"},
{ "psbtbumpfee", 1, "outputs"},
{ "psbtbumpfee", 1, "original_change_index"},
- { "logging", 0, "include" },
+ { "logging", 0, "d
...

🤔 sipa reviewed a pull request: "precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442#pullrequestreview-3564766146)
ACK 6eb5ba569141b722a5e27ef36e04994886769c44

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2608446338)
Not sure whether `getblock` JSON RPC performance measurement will be precise enough (due to RPC & serialization overhead)...

Maybe it would be easier to inline `GetRawBlockChecked` (similar to how it's done in `rest.cpp`)?

```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 40761d5cab..e179284fcf 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -678,21 +678,6 @@ static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex& blockin
...

💬 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":`).