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

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2588518399)
nit: could we use brace init consistently in the change (it helps with a few cases, e.g. warns on narrowing conversions)

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2588478938)
What would be the point of an offset equaling the total size? if we don't want to get the value of the last valid offset, at least prohibit 0 sizes:
```suggestion
if (part_offset >= blk_size) {
```

Which should split out the `*part_size == 0` case below as well.

If we added the, a single optional it would also improve readability by separating the partial case neatly, it doesn't pollute the whole method.
We could even `filein.seek` only when the param is specified.

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2588520278)
there's a lot of repetition here, can we separate the data from the algorithm?

And could you please add a fuzz test which exercises the same - if the randomly generated range returns valid bytes for the complete block, the new rpc should also return the same values. If it errs, so should the new RPC.

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2588655840)
I don't like that we have 2 separate methods doing the same thing. if we grouped the new parameters (with default `nullopt`) and used a dedicated return value of
```C++
using ReadRawBlockResult = util::Result<std::vector<std::byte>, ReadRawError>;
```
(needs a slight adjustment to `util::Result`)

we could have a single:
```C++
ReadRawBlockResult ReadRawBlock(const FlatFilePos& pos, std::optional<std::pair<size_t, size_t>> block_part = std::nullopt) const;
```

We would need to adjust a few usag
...

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2588735671)
can we add a test with `std::numeric_limits<size_t>::max()` for both offset and size to make sure we don't have overflow problems?