💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2592108737)
It was part of my suggestion (https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2584393520), inspired by https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582578921. A somewhat common alternative to C++23's `std::unreachable()`.

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2592121707)
Unit test seems sufficient to me.

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2592146736)
Agree it probably makes sense to have an `optional` for the range. It's interesting to explore what could be done with `util::Result`, but I feel that could be done as a follow-up to this PR.

Agree that the ergonomics of `std::expected` are better than `std::variant`, but not that they are terrible. If you are strongly against `std::variant` then maybe we could step back to using out-parameters for now in this PR.

💬 maflcko commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2592196089)
> > * Ideally, low level util functions should not be logging at all, but rather pass up a `Result` (possibly with an error string or error value)
>
> Agreed.

Probably don't want translated strings of those low-level errors here (Result is based on that), so I ported `std::expected` in https://github.com/bitcoin/bitcoin/pull/34006. I haven't tried, but it could be useful here.

💬 sedited commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2592204362)
See the dev notes: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures

⚠️ maflcko opened an issue: "mptest hangs, when run in a loop"
(https://github.com/bitcoin/bitcoin/issues/34014)
Initially ci ran into a segfault in https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19882806964/job/56984144564#step:6:4040:

```
3/152 Test #3: mptest ...............................***Failed 0.05 sec
[ TEST ] test.cpp:117: Call FooInterface methods
[ PASS ] test.cpp:117: Call FooInterface methods (18106 μs)
[ TEST ] test.cpp:209: Call IPC method after client connection is closed
[ PASS ] test.cpp:209: Call IPC method after client connection is closed (744 μs)
[ TEST ] test
...

💬 maflcko commented on issue "mptest hangs, when run in a loop":
(https://github.com/bitcoin/bitcoin/issues/34014#issuecomment-3616337285)
I can also reproduce the i686 segfault from the CI run locally, but I suspect it is similar to issue https://github.com/bitcoin/bitcoin/issues/31772

💬 fanquake commented on issue "mptest hangs, when run in a loop":
(https://github.com/bitcoin/bitcoin/issues/34014#issuecomment-3616352068)
cc @Sjors @ryanofsky

👍 hodlinator approved a pull request: "merkle: migrate `path` arg to reference and drop unused args"
(https://github.com/bitcoin/bitcoin/pull/33805#pullrequestreview-3540270081)
ACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94

Effectively removes dead code.

💬 hodlinator commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2589269947)
meganit: Could declare parameter `const` to decrease cognitive complexity
```suggestion
static void MerkleComputation(const std::vector<uint256>& leaves, const uint32_t leaf_pos, std::vector<uint256>& path)
```

💬 hodlinator commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2592258492)
remark: The expression on the right side is side-effect free, so removing it is safe. (`inner` is a raw C++ array - no overloaded `operator[]`, comparing `uint256` values doesn't mutate them).

👍 sedited approved a pull request: "cmake: Check dependencies after build option interaction"
(https://github.com/bitcoin/bitcoin/pull/33974#pullrequestreview-3543500447)
ACK 56d0a0929d8974a490c57bf84a994ba7c9f19863

Guix build:
```
5bd1a78f590d48e5ceb5815b5ed346d06d738871c9ea26b754c9a851dcb7ee85 guix-build-56d0a0929d89/output/aarch64-linux-gnu/SHA256SUMS.part
cc0525310c1e1c1b1cb69c2256b8089b187830c27da9dd7ba9830884c35a0e95 guix-build-56d0a0929d89/output/aarch64-linux-gnu/bitcoin-56d0a0929d89-aarch64-linux-gnu-debug.tar.gz
bc891ef995772a5f09c48000b0b0e7d6dc142ae184490ef711c800f42c48cdeb guix-build-56d0a0929d89/output/aarch64-linux-gnu/bitcoin-56d0a0929d8
...

💬 sedited commented on pull request "cmake: Check dependencies after build option interaction":
(https://github.com/bitcoin/bitcoin/pull/33974#discussion_r2591737457)
Nit: Extra line.

💬 fanquake commented on pull request "cmake: Check dependencies after build option interaction":
(https://github.com/bitcoin/bitcoin/pull/33974#discussion_r2592292207)
What is this comment meant to be?

💬 hebasto commented on pull request "cmake: Check dependencies after build option interaction":
(https://github.com/bitcoin/bitcoin/pull/33974#discussion_r2592302403)
Some build options, such as `BUILD_FOR_FUZZING`, may alter other user-specified build option values depending on how they are set.

💬 Sjors commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#discussion_r2592303231)
To limit the scope of this PR, I only added the mutex, but called it `template_state_mutex` in anticipation.

💬 fanquake commented on pull request "cmake: Check dependencies after build option interaction":
(https://github.com/bitcoin/bitcoin/pull/33974#discussion_r2592308392)
Can the comment explain that.

💬 fanquake commented on pull request "cmake: Check dependencies after build option interaction":
(https://github.com/bitcoin/bitcoin/pull/33974#discussion_r2592329786)
Also, is this meant to mark that no interaction happens after this point, or is the only place there is any interaction?

💬 fanquake commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3616461192)
Ok. I think @hebasto needs to make a call here then.

💬 Fi3 commented on issue "rpc: getrawtransaction lookup by witness txid":
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616470606)
ACK for dmnd or other pools that will implement sv2 and want to do job declaration and being able to relay the blocks founded by the miners, having fast way to retrieve tx data from the node using a wtxid would be very useful.