💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609627988)
nit: In my opinion it would be better to use `REQUIRE` which stops the test in case of failure, so that we don't trigger less clear failures on the following lines (internal assert in `Expected::value()` - might be an exception after #34032, assert inside `Expected::error()`).

```diff
--- a/src/test/blockmanager_tests.cpp
+++ b/src/test/blockmanager_tests.cpp
@@ -152,7 +152,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_part, TestChain100Setup)

const auto expect_part{[&](siz
...

💬 Fi3 commented on issue "consider adding a new `interface RawTxFeed` on Mining IPC":
(https://github.com/bitcoin/bitcoin/issues/34030#issuecomment-3640907952)
> reply to [@Fi3](https://github.com/Fi3):
>
> first, let me explain the rationale behind this feature request
>
> once `DeclareMiningJob` arrives, I wanted JDS to be able to validate it as fast as possible
>
> assuming the best case scenario (where all `wtxid`s have already been seen and no extra `ProvideMissingTransactions` round-trip is needed), having all txs readily available on JDS memory would allow the validation of `DeclareMiningJob` to happen ASAP
>
> if we're worried about unbound
...

💬 ajtowns commented on pull request "rpc: Disallow captures in RPCMethodImpl":
(https://github.com/bitcoin/bitcoin/pull/34049#discussion_r2609754446)
> 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.

💬 Ataraxia009 commented on pull request "rpc: Disallow captures in RPCMethodImpl":
(https://github.com/bitcoin/bitcoin/pull/34049#discussion_r2609757315)
I dont think the second commit is necessary either should be fine with just removal of the captures

💬 Ataraxia009 commented on pull request "refactor: inline constant `f_obfuscate = false` parameter":
(https://github.com/bitcoin/bitcoin/pull/34048#issuecomment-3640958735)
Concept NAck, could be needed in the future, no point in hiding it

🤔 hodlinator reviewed a pull request: "util: Add some more Unexpected and Expected methods"
(https://github.com/bitcoin/bitcoin/pull/34032#pullrequestreview-3566334104)
re-ACK fa0a1a6a4986c1a977bbcd5f48834bedeac5dffb

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2609718958)
nit: Would be nice to add `swap()` as well. I already needed that here: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2608521278 / https://github.com/hodlinator/bitcoin/commit/b40a36cbbab59440d84dba6d1cc16bce17d4869c

(I forgot to include the test in that commit).
```C++
BOOST_AUTO_TEST_CASE(expected_swap)
{
Expected<const char*, int> a{Unexpected{55}};
Expected<const char*, int> b{"21"};
a.swap(b);
BOOST_CHECK_EQUAL(a.value(), "21");
BOOST_CHECK_EQUAL(b
...

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2609758576)
nit: Should this be a require since otherwise we would start triggering internal asserts or exceptions further down on failure?
```suggestion
BOOST_REQUIRE(e.has_value());
```

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2609690504)
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).

I prefer the current way of implementing `operator bool` in terms of `has_value()` though, think it's clearer.

Agree on making the other operators `noexcept`.

💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2609770958)
nit: Should be `noexcept` according to https://en.cppreference.com/w/cpp/utility/expected/unexpected.html#error

👍 hodlinator approved a pull request: "net: make m_nodes_mutex non-recursive"
(https://github.com/bitcoin/bitcoin/pull/32394#pullrequestreview-3566493787)
re-ACK a6ccd6fdbf838d70ada83b6079112c648c490e1c

Rebase to fix conflict with #33956

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