💬 maflcko commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2609835030)
This just brings back the problem we are trying to solve: Solve exposing monostate externally?

`+ Expected<void, std::string> e{std::monostate{}};
`

💬 maflcko commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2609836034)
thx, done

💬 maflcko commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2609838881)
> Agree on making the other operators `noexcept`.

thx, done

> Agree on change to use the default ctor, `variant` will initialize with the first type (https://en.cppreference.com/w/cpp/utility/variant/variant.html, 1).

shouldn't hurt to be explicit

💬 maflcko commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2609842427)
thx, done

💬 maflcko commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2609909433)
thx, done

💬 maflcko commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2609913103)
Yes, but that seems fine? Not sure what problem this would be solving. Leaving as-is for now.

💬 maflcko commented on pull request "rpc: Disallow captures in RPCMethodImpl":
(https://github.com/bitcoin/bitcoin/pull/34049#discussion_r2609954008)
> > Here, it is easy to re-create the calculation, but will it always be the case?
>
> I think so -- the `RpcMethodFnType` functions don't take any params, so if you're doing a calculation (like a deprecatedrpc test) you can just do the same thing inside the `RPCMethodImpl`, and if you're providing something hardcoded, you can pass a boolean or enum via a template (eg `template <bool wants_enum> static RPCHelpMan bumpfee_filter`) and make your logic conditional on that.

Oh, I meant "succin
...

🤔 janb84 reviewed a pull request: "test: Log IP of download server in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/34045#pullrequestreview-3566682091)
ACK cdaf25f9c3e56b4e80b90a471bdc0a175160534c

Thanks for incorporating my suggestion.

💬 maflcko commented on pull request "test: Detect truncated download in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/34040#discussion_r2610000459)
Actually, `progress_bytes < total_size:` could be considered stricter, because if the server starts replying with the wrong payload size, or appends data, it seems like this could be useful to know via a checksum error.

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2610003251)
(Others seem to disagree with me here though: https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2609913103).

💬 l0rinc commented on pull request "refactor: inline constant `f_obfuscate = false` parameter":
(https://github.com/bitcoin/bitcoin/pull/34048#issuecomment-3641276577)
if it's needed in the future, it's trivial to introduce it.

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2610060424)
I asked the two to be unified, both are fine here

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2610071673)
I don't really like this Windows distinction - can we rather find a way that does work on all platforms? Haven't checked, is it only triggered when the file is missing, or can a truncated file also trigger the error?

👍 hodlinator approved a pull request: "util: Add some more Unexpected and Expected methods"
(https://github.com/bitcoin/bitcoin/pull/34032#pullrequestreview-3566738644)
re-ACK fa874c9f050500dc521ef8d9c81b043687f0dcc7

Thanks for grabbing `swap`!

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2610026288)
How come this method overload is not calling itself recursively?

Tested adding `BOOST_CHECK_EQUAL((Expected<void, int>{Unexpected{1}}.error()), 1);` to the tests to confirm it doesn't but don't understand why.

Is `this` in some kind of `&`-mode, making it call the method above?

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2610039385)
nit: Might be nice to have these assert before failing due to an exception being thrown inside of `noexcept`.

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2610108777)
Yeah, finding a solution that works on all platforms would be even better. Don't think it's a case of the file missing, but rather that Windows forbids renaming parent directories when a process holds a file handle to a file.

Maybe we could rename files inside of the directory matching `blk*.dat` to `blk*.dat.bkp`? Haven't tested yet.

👍 rkrux approved a pull request: "test: Log IP of download server in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/34045#pullrequestreview-3566844824)
tACK cdaf25f9c3e56b4e80b90a471bdc0a175160534c

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2610120563)
I think it's slightly better as a general principle, to make test failures more intelligible. But it's very unlikely that anyone would even locally tweak the code being tested here so that it fails.

💬 stickies-v commented on pull request "logging: API improvements":
(https://github.com/bitcoin/bitcoin/pull/34038#discussion_r2610132975)
> Also left `include` in the client side so that bitcoin-cli still works with old bitcoinds or bitcoinds

I'm not sure that's feasible without also adding a `-deprecatedrpc` option to `cli` (unless that already exists), which I don't think is worth it? Afaik we don't support disjoint `bitcoind` and `bitcoin-cli` versions, and I think `bitcoin-cli` is more for manual usage anyway at which point just typing "debug" instead of "include" is trivial? The diff I added passes all tests and also adds
...