🚀 fanquake merged a pull request: "net: fix use-after-free with v2->v1 reconnection logic"
(https://github.com/bitcoin/bitcoin/pull/33956)

💬 maflcko commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3616239972)
> I don't see that as a concern for this PR, since the current `Result` class could just be renamed to `Expected` in #25665 to avoid that if desired. So #34005 and #34006 both seem like good solutions to me, and I'd be happy with either.

Either is fine by me as well. It just seems more churn to (1) introduce the Expected alias based on Result, (2) move it a different file, (3) likely rework it to remove the implicit bilingual_string error stuff (because it is not needed and confusing). It see
...

💬 maflcko commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#discussion_r2592174675)
if this alias is added, one should also be added for `std::unexpected`.

✅ maflcko closed an issue: "ASAN use-after-free in `m_reconnections`"
(https://github.com/bitcoin/bitcoin/issues/33615)

⚠️ 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?