🤔 hodlinator reviewed a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3543967284)
> Could you provide a patch for me which would go through every single transaction in mainchain history and query it both ways (full block + new RPC) and assert that they're equal? I expect it to be heavy, I can run it on my benchmark servers to make sure it works for every real scenario we have.

What value would that provide? The new API allows fetching arbitrary byte-ranges from within blocks. The current unit tests checking the bytes match seem sufficient for validating the logic (testing wi
...

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