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

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2592500035)
If so, I'd randomize the unit tests to avoid our bias (such as not having max values)

💬 l0rinc commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2592525042)
We don't usually do that with primitives, but I agree that sometimes it helps documenting the behavior better

💬 l0rinc commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2592525157)
Yes, we should do that in a follow-up, thanks

💬 l0rinc commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3616715057)
rfm?

💬 alexanderwiederin commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#issuecomment-3616771992)
ACK https://github.com/bitcoin/bitcoin/pull/33822/commits/30367c1b3e7e94ed59d0a3ee816da14a3ee0424f

💬 ryanofsky commented on issue "mptest hangs, when run in a loop":
(https://github.com/bitcoin/bitcoin/issues/34014#issuecomment-3616776123)
Thanks for the report. I also saw failures in the "thread busy" test on i686 here https://github.com/bitcoin-core/libmultiprocess/pull/218#issuecomment-3366301777. The problem here seems easier to reproduce though, so maybe solving this issue with the "thread busy" the test will help with the other issue too.

Curious how long approximately it takes for the hang to happen?

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592580254)
> 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.

Yes, the inversions is one of the reasons to do this change. I agree there is a risk, but is should be pretty obvious for reviewers to look out for. Also, hopefully the cases are rare where the value type and the error type are close enough to make `value()` and `error()` swappable at the call-site.

> constr
...

💬 maflcko commented on issue "mptest hangs, when run in a loop":
(https://github.com/bitcoin/bitcoin/issues/34014#issuecomment-3616809162)
> Curious how long approximately it takes for the hang to happen?

Maybe 5-15 minutes on my system. Just make sure to saturate all CPUs on the system. (You can run the functional tests with -j 99, or compile without cccache in a loop.)

💬 fanquake commented on pull request "depends: update freetype and document remaining `bitcoin-qt` runtime libs":
(https://github.com/bitcoin/bitcoin/pull/33952#issuecomment-3616849382)
Guix Build (aarch64):
```bash
760ca6bb241b02b496785a0123cea4e9e49e03e27e2280824237bd4125ffb1d0 guix-build-41e657aacfa6/output/aarch64-linux-gnu/SHA256SUMS.part
432c63a2879ef00793c9378f64d5130752258c0d845b565c4fa7c708399be01a guix-build-41e657aacfa6/output/aarch64-linux-gnu/bitcoin-41e657aacfa6-aarch64-linux-gnu-debug.tar.gz
15c65f8720598ff04dba8d15b0b49e5ab9064f6798159b331681806a62f28426 guix-build-41e657aacfa6/output/aarch64-linux-gnu/bitcoin-41e657aacfa6-aarch64-linux-gnu.tar.gz
d53c1e
...

💬 maflcko commented on issue "mptest hangs, when run in a loop":
(https://github.com/bitcoin/bitcoin/issues/34014#issuecomment-3616849854)
> I also saw failures in the "thread busy" test on i686 here

I tried reproducing those in `podman run -it --rm --platform linux/i386 alpine:3.23` (with the steps from above), and also in `podman run -it --rm --platform linux/i386 debian:unstable` (using `FILE_ENV="./ci/test/00_setup_env_native_nowallet.sh"` or `FILE_ENV="./ci/test/00_setup_env_native_previous_releases.sh"`), but they wouldn't segfault, only hang.

Only the modified `FILE_ENV="./ci/test/00_setup_env_i686_no_ipc.sh"` would reprod
...

💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592629697)
Requires `#include <cassert>`.