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

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

Looking at bb61eca55a563d7486df3c356d163c4af10a36c8, I think this is compatible with #25665. This PR is a minimal tweak to `util::Result` that can let it work as a substitute for `std::expected` before `std::expected` is available to use. #25665 could be easily rebased on top of this. In the long run, though, I think code that doesn't need `util::Result` error reporting functionality should just use `std::expected` directly.

It might be good if this added an al
...

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3612546191)
glancing through the PR now

how bad would it be to merge all the way through "clusterlin: replace cluster linearization with SFL implementation (feature)", maybe some of the commits that drops unused things (ala "clusterlin: remove unused MergeLinearizations (cleanup)") and defer the other optimisations for next PR?

📝 maflcko opened a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006)

💬 maflcko commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3612561978)
I wonder if this is the right approach. `util::Result` is mostly meant for high level code (dealing with translations), such as init, the wallet, or the gui. For low-level stuff, it could make sense to use `std::expected` directly?

I am not sure how fast we'll be getting C++23. C++20 was enabled two years ago in https://github.com/bitcoin/bitcoin/pull/28349, but there are still questions around which C++20 features are supported. (e.g. jthread is only in libc++ 20, std::format is only in g++1
...

💬 l0rinc commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3612570208)
> It might be good if this added an alias like

Good idea, added and used it in the new test cases.

> Have you looked at https://github.com/bitcoin/bitcoin/pull/25665?

I agree with @ryanofsky that this is likely compatible with the PR and as far as I understand it addresses a small chunk of what it's also trying to achieve.

> So I think just porting a minimal std::expected seems easier?
So I did that in https://github.com/bitcoin/bitcoin/pull/34006

I'm fine with both, let me know
...

💬 maflcko commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3612589376)
> Looking at [bb61eca](https://github.com/bitcoin/bitcoin/commit/bb61eca55a563d7486df3c356d163c4af10a36c8), I think this is compatible with #25665. This PR is a minimal tweak to `util::Result` that can let it work as a substitute for `std::expected` before `std::expected` is available to use. #25665 could be easily rebased on top of this. In the long run, though, I think code that doesn't need `util::Result` error reporting functionality should just use `std::expected` directly.

Not sure they
...

💬 ryanofsky commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3612602449)
> So I think just porting a minimal `std::expected` seems easier?
>
> So I did that in #34006

Nice, that's more complicated than this PR, but not too bad. I don't have any particular preference between these two PR's. Either seem fine.

💬 ajtowns commented on issue "SENDTEMPLATE Tracking Issue":
(https://github.com/bitcoin/bitcoin/issues/33691#issuecomment-3612640676)
It looks like reusing compact block messages doesn't make sense -- including two blocks' worth of data seems pretty useful, but that could result in an ~8MB `blocktxns` data if your mempool was empty and the top of the mempool was full of inscriptions, which would in turn [exceed the 4MB maximum message size we have](https://github.com/bitcoin/bitcoin/pull/33191#pullrequestreview-3383627846). We could expand the maximum message size, but large messages can be a DoS vector, so splitting the templ
...

💬 maflcko commented on pull request "util: generalize `util::Result` to support custom errors":
(https://github.com/bitcoin/bitcoin/pull/34005#issuecomment-3612682436)
> > So I think just porting a minimal `std::expected` seems easier?
> > So I did that in #34006
>
> Nice, that's more complicated than this PR, but not too bad. I don't have any particular preference between these two PR's. Either seem fine.

Yeah, it is a bit more code, but, the inner guts of `util::Expected` are mostly copy-pasted from the current `util::Result` in master, so it shouldn't be conceptually complicated.

💬 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-3612742963)
> > Restructuring `ProcessOptionKey` as below seems to solve the problem...
>
> Checking it... Thanks for the suggestion!

Ok, all tests pass...

First checked only the affected ones by my changes:

```
./build/test/functional/feature_config_args.py
./build/test/functional/tool_wallet.py
./build/test/functional/wallet_multiwallet.py
./build/test/functional/wallet_transactiontime_rescan.py

./build/bin/test_bitcoin --log_level=all --run_test=argsman_tests
```

Then I ran them al
...

💬 sedited commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3612743633)
Concept ACK

💬 Sjors commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3612759553)
Concept ACK

`FetchBlock()` is a good usage example.