⚠️ Sjors opened an issue: "rpc: getrawtransaction lookup by witness txid"
(https://github.com/bitcoin/bitcoin/issues/34013)
For Stratum v2 custom job declaration to be bandwidth efficient, the pool can request only the transactions that it doesn't know about.

The spec doesn't specify how this is achieved, but one method is to call `getrawtransaction` on each of the transaction ids listed in [DeclareMiningJob](https://stratumprotocol.org/specification/06-Job-Declaration-Protocol?query=DeclareMiningJob#644-declareminingjob-client-server).

Unfortunately this RPC only supports `txid`, not `wtxid`.

I think the easiest
...

🤔 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?