💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3611774702)
@maflcko suggested just declaring the util functions. This has reduced test.cpp further, with the same non-determinism (https://github.com/fanquake/bitcoin/commit/c12ef2a5312cb66cd01a2f1230fe4e976759f4e1).

💬 sedited commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2588768798)
Looking at the additional branches introduced here by these swiftsync conditionals, I wonder if a better approach would be having a dedicated swiftsync coins view, that takes care of swiftsync specific operations without adding extra logic to the actual validation code. Not sure what the best way would be to do this, but I guess it could look similarly to the async coins view introduced in #31132. Then again, the current caching structure is already complicated enough and moving the complexity o
...

💬 hebasto commented on pull request "doc: Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#issuecomment-3611961731)
> Previously one had to read the Makefile (and various *.mk configuration
> files) to see how to correctly override CC and CXX when building native
> depends packages.

I also wanted to highlight the distinction between environment variables and Makefile variables. There should be no difference in behaviour between the two when supplied by the user, provided they are recognised by the build system. But that is not currently the case. See: https://github.com/bitcoin/bitcoin/pull/29963.

👍 pablomartin4btc approved a pull request: "init: point out -stopatheight may be imprecise"
(https://github.com/bitcoin/bitcoin/pull/33993#pullrequestreview-3539838956)
ACK ff06e2468a5d3eeebeffe781904c34c9d1b44385

💬 rustaceanrob commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2588961718)
To suppress the UB sanitizer here I added a `__attribute__(no_sanitize ... ))` which cause a CI failure on windows. Is there 1. a wrapping addition/subtraction API I am not aware of 2. a way to suppress UB across all targets

💬 Sjors commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2588964479)
Ok, done and I rebased #33966 on that change.

💬 hebasto commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#discussion_r2589020763)
> But I don't understand why the `#include <src/...>` lines were working previously?

This code is responsible for adding the required include path:https://github.com/bitcoin/bitcoin/blob/75baff98fcf987735437196a4db1919e390c4bd2/src/ipc/libmultiprocess/cmake/TargetCapnpSources.cmake#L96-L103

💬 Sjors commented on pull request "refactor: disentangle miner startup defaults from runtime options":
(https://github.com/bitcoin/bitcoin/pull/33966#issuecomment-3612267713)
I moved `MiningArgs` and static `DEFAULT_PRINT_MODIFIED_FEE` from `node/mining.h` to `node/mining_args.h`.

> You mean you want the mining interface options separate from the `TransactionError` `TxBroadcast` enum types?

Yes, I think it's easier for Mining IPC client developers to understand how things work that way.

👍 sedited approved a pull request: "merkle: migrate `path` arg to reference and drop unused args"
(https://github.com/bitcoin/bitcoin/pull/33805#pullrequestreview-3540024475)
ACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94

💬 sedited commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2589073735)
Do you think there would be value in also passing in nullptr:
```diff
diff --git a/src/test/fuzz/merkle.cpp b/src/test/fuzz/merkle.cpp
index 4bb91faf0b..5b91a97b75 100644
--- a/src/test/fuzz/merkle.cpp
+++ b/src/test/fuzz/merkle.cpp
@@ -55 +55,2 @@ FUZZ_TARGET(merkle)
- const uint256 merkle_root = ComputeMerkleRoot(tx_hashes, &mutated);
+ bool* pmutated = fuzzed_data_provider.PickValueInArray({&mutated, static_cast<bool*>(nullptr)});
+ const uint256 merkle_root = ComputeMerkleR
...

👍 sedited approved a pull request: "test: add unit test coverage for the empty leaves path in MerkleComputation"
(https://github.com/bitcoin/bitcoin/pull/33858#pullrequestreview-3540078670)
ACK ffcae82a68104c1992964b26c592b62cbca391bf

rfm?

💬 hebasto commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#issuecomment-3612366549)
Going to merge this. Leaving the following for follow-ups:

> The last path in that list pointing at the build root should not be there I think, and the bad `target_capnp_sources` calls fixed here were relying on it to work. It would be nice to remove if we can find the source.

🚀 hebasto merged a pull request: "cmake: Move IPC tests to `ipc/test`"
(https://github.com/bitcoin/bitcoin/pull/33774)

💬 maflcko commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2589210000)
You
may need to use a suppressions file, see `test/sanitizer_suppressions`. They may be
used as follows:
```bash
export LSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/lsan"
export TSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/tsan:halt_on_error=1:second_deadlock_stack=1"
export UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1"
```

📝 l0rinc opened a pull request: "util: generalize `util::Result` to support custom errors"
(https://github.com/bitcoin/bitcoin/pull/34005)
### Context

While reviewing https://github.com/bitcoin/bitcoin/pull/33657 it became clear we don’t have a good value-or-error wrapper, similar in spirit to `std::expected<T, E>` in C++23.

### Problem

The `util::Result` helper currently stores a `std::variant<bilingual_str, T>` and is effectively only usable for high-level code that needs to propagate user-facing error strings.
Low-level code that wants typed error codes instead of strings still has to expose raw `std::variant` (or roll
...

💬 fanquake commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3612442557)
Have you looked at #25665?

🤔 l0rinc requested changes to a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3536872444)
Thanks for taking care of this and thanks for splitting the change into smaller chunks.
The change is quite streight-forward now, it helped with experimenting with it locally.

Concept ACK

I left a few suggestions:
* `std::variant` is not meant to be used as an error monad, it results in very awkward code (e.g. compared to the usage of `std::optional` or `std::expected`). But it seems we already have such a try sum type that we only have to adjust slightly for it to simplify the change a lot -
...

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2586568153)
what role does this serve, don't all paths already return? Seems like dead code to me.

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2586569022)
```suggestion
std::vector<std::byte> data(blk_size); // Zeroing of memory is intentional here
```

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2588463650)
While `std::variant` is a lot better that output parameters (as long as it doesn't incur a slowdown, since this is on the critical path for IBD), it *is* a bit awkwards for this situation.

C++23 provides a better alternative (https://en.cppreference.com/w/cpp/utility/expected.html), but that's not available for us yet.
But we have our own alternative:
https://github.com/bitcoin/bitcoin/blob/9a29b2d331eed5b4cbd6922f63e397b68ff12447/src/util/result.h#L19-L90

Which would make usage so simple tha
...