👍 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?

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2588506623)
what's the point of an optional `part_size` but a mandatory `part_offset`?
We've introduced two related args here that should likely be groupes, either as
```C++
std::optional<std::pair<size_t, size_t>> block_part
```
or
```C++
struct BlockPart {
size_t offset;
size_t size;
}
```

💬 pablomartin4btc commented on pull request "argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments)":
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3612494268)
> This command works on master but not in this PR: `./build/bin/bitcoin-cli --datadir=/tmp/btc1 --signet getblockhash 1000`
<detail>
<summarys>Thanks for testing it (not sure if that's valid, no test failed, perhaps we need to add one?), anyways just putting the output's diff here.</summarys><br>

`master`:
```
/build_master/bin/bitcoin-cli --datadir=/tmp/btc --signet getblockhash 1000
error code: -8
error message:
Block height out of range
```
this branch:
```
./build/bin/bitcoin-
...

💬 pablomartin4btc commented on pull request "argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments)":
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3612512469)
> Restructuring `ProcessOptionKey` as below seems to solve the problem...

Checking it... Thanks for the suggestion!