💬 rkrux commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2592395736)
It's documented that `LogPrintf` is deprecated, prefer to avoid its usage?

https://github.com/bitcoin/bitcoin/blob/0c9ab0f8f8c85719ff3aa4aefe3198cd2f8d63d1/src/logging.h#L372-L373

💬 hebasto commented on issue "ci: failure in Windows native job":
(https://github.com/bitcoin/bitcoin/issues/34012#issuecomment-3616545810)
> I haven't looked here, but generally CI does not work well with re-running tasks, when the code is merged with current master, but the ci config is stale.
>
> As the error is zmq related, it could be due to [49c6728](https://github.com/bitcoin/bitcoin/commit/49c672853503e561cd1e7fed19a32f21ad345370), but again, I haven't checked this closely.

I concur with both assessments above.

The `.github/workflows/ci.yml` for the failed run looks outdated: https://github.com/bitcoin/bitcoin/actions/run
...

💬 plebhash commented on issue "rpc: getrawtransaction lookup by witness txid":
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616550030)
currently SRI Job Declarator Server monitors the mempool via RPC and we have considered switching to ZMQ on this issue https://github.com/stratum-mining/sv2-apps/issues/26, which is about to be tackled soon

but I like @Sjors idea of leveraging IPC

👍 marcofleon approved a pull request: "[30.x] Backports & 30.1rc1"
(https://github.com/bitcoin/bitcoin/pull/33997#pullrequestreview-3544399527)
ACK d59ebac7187dceb60f880ee3d2c5fab8368da176

✅ hebasto closed a pull request: "qa: Remove no longer needed `feature_dirsymlinks.py`"
(https://github.com/bitcoin/bitcoin/pull/33924)

💬 sedited commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2592425740)
This is on purpose from my part. I think it is confusing and not helpful to expose. If you look at where it is used, you'll see that all it does is invalidate a header that does not extend the pre-synced best work chain. The only case it is still used in our code is when processing unrequested blocks, but since we're processing headers here, I don't think that is relevant. I also don't think that this should be documented either. The current `min_pow_checked` handling in our validation code is c
...

💬 Sjors commented on issue "rpc: getrawtransaction lookup by witness txid":
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3616579875)
ZMQ will give you all new mempool transactions, whereas a new `getRawTransactions()` IPC method would only give you the ones you request. As does the RPC change suggested here.

That's probably enough if you keep a local cache of transactions you've seen before and only request unknown ones from `DeclareMiningJob`.

💬 fanquake commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2592464223)
Yes. It should be removed soon: #29641.

💬 l0rinc commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3616641991)
> It seems easier to just add Expected in its intended form in one go

Glad you have that alternative ready, closing in favor of https://github.com/bitcoin/bitcoin/pull/34006

✅ l0rinc closed a pull request: "util: generalize `util::Result` to support custom errors"
(https://github.com/bitcoin/bitcoin/pull/34005)

🤔 hodlinator reviewed a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3544455114)
Concept ACK fa1eee4f46fd089d6e19b124addee7c7679585f4

I prefer introducing `util::Expected` as a distinct type from `util::Result`, with a clear upgrade path to `std::expected` once we have C++23. (Maybe `util::Result` can later be implemented as a special case of `util::Expected`).

Recently suggested `std::variant` in another PR instead of using out-parameters (https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2584393520) but would strongly prefer `util::Expected`, had it existed.

💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592454740)
This example shows that replacing `std::optional<ErrorType>` with `util::Expected<void, ErrorType>` is not totally risk-free as the old condition would still compile but be inverted. `err.value()` on the next line would have failed in this case though since it would return `void` which is an invalid argument type to the `JSONRPCError`-ctor.

It's tempting to add constraints to `value()` like so:
```C++
constexpr const T& value() const LIFETIMEBOUND
requires(!std::is_same_v<V, void>)

...

💬 hebasto commented on pull request "cmake: Check dependencies after build option interaction":
(https://github.com/bitcoin/bitcoin/pull/33974#issuecomment-3616646688)
@sedited @fanquake

Thank you for the review! Your feedback has been addressed.

💬 hebasto commented on pull request "cmake: Check dependencies after build option interaction":
(https://github.com/bitcoin/bitcoin/pull/33974#discussion_r2592486190)
An explanatory comment has been added.

💬 hebasto commented on pull request "cmake: Check dependencies after build option interaction":
(https://github.com/bitcoin/bitcoin/pull/33974#discussion_r2592486916)
Thanks! Fixed.

💬 frankomosh commented on pull request "test: add unit test coverage for the empty leaves path in MerkleComputation":
(https://github.com/bitcoin/bitcoin/pull/33858#issuecomment-3616679044)
> rfm?

I think it is

💬 hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2592510783)
> nit: Looking at the output, I think this is already done correctly?

I don't think so. The generated diffs still [suggest](https://github.com/bitcoin/bitcoin/actions/runs/19938614368/job/57170256836) the C headers:
```diff
+#include <errno.h>
```

🤔 l0rinc reviewed a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3544513831)
> > 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 (test
...

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2592501977)
I'm mostly against introducing an extra method that basically does the same thing

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2592495614)
This is unreachable code, all code paths before return, both in the try and in the catch. The compiler would already warn if any of the code paths wouldn't return. We don't add "assert(false)" after a throw or return, since those are all exiting,
This isn't the same as non-exhaustive switches (which can change and leave cases uncovered, though a linter could be configured to warn instead), but this is just confusing dead code.