💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609482472)
https://github.com/bitcoin/bitcoin/commit/9f8809f2b1206ad34dfc68831c10db76e618de3f

💬 maflcko commented on pull request "bench: run `FindByte` across block-sized buffer":
(https://github.com/bitcoin/bitcoin/pull/34046#discussion_r2609578248)
the file isn't left around. This was fixed already in fa2fbaa4a29f80d3c7d5f0ad6b64035c3156dd12

💬 maflcko commented on pull request "cmake: Add fail pattern to `try_append_cxx_flags`":
(https://github.com/bitcoin/bitcoin/pull/34047#discussion_r2609591510)
i don't understand how this could lead to issues. If someone passes w-no-error during configure, it will also be present during build, and the build should pass, no?

What is the issues that this is trying to fix?

What are the steps to reproduce?

💬 maflcko commented on pull request "refactor: inline constant `f_obfuscate = false` parameter":
(https://github.com/bitcoin/bitcoin/pull/34048#discussion_r2609597196)
i presume this exists to allow indexes to obfuscate, if there is need to? E.g. when storing remote-user-provided data. E.g. an addrindex?

I understand this isn't needed right now, so no strong opinion.

💬 maflcko commented on pull request "doc: guix: Troubleshooting zdiff3 issue and uninstalling.":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2609640100)

running -> run [The sentence starts with an imperative ("Uninstall Guix itself...") and needs a parallel imperative verb; "or running" is a fragment — "or run" makes the coordination grammatical.]

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609654918)
Not sure about the `try`/`finally`. I think it may be better to leave the filesystem as-is in case of failure, so that devs inspecting the directory of a failed test finds it closer to how it looked when the failure happened. Haven't seen this kind of pattern in other functional tests. We don't continue the test regardless, so there's no benefit to checks later in the test.
```diff
--- a/test/functional/interface_rest.py
+++ b/test/functional/interface_rest.py
@@ -500,11 +500,9 @@ class REST
...

💬 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