💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3616532437)
Slightly more minified:
```cpp
#include <filesystem>
#include <string>
#include <vector>
#include <cstring> // Unused, but removing this makes it determinstic

namespace fs {

using namespace std::filesystem;

// Dummy class. Using "path = std::filesystem::path" makes it deterministic.
class path : public std::filesystem::path
{
public:
using std::filesystem::path::path;

path(std::filesystem::path path) : std::filesystem::path::path(std::move(path)) {}
};

}

bool
...

💬 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