💬 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?
(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 -
...
(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.
(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
```
(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
...
(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)
(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.
(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.
(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
...
(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?
(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;
}
```
(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-
...
(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!
(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
...
(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?
(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)
(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
...
(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
...
(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
...
(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.
(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.