💬 sipa commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-3627526721)
Concept/Approach ACK. I think it's fine to break compatibility with unmaintained node versions, given that there is a good error message for this case.

💬 sedited commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2599118974)
Do you mean "this function will not check for a minimum proof of work threshold"? But as I said [here](https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2596217297), I think the potential for misuse at this point is low. I'd gladly add a text if you want to suggest one though.

💬 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-3627591458)
-<ins>_**Updates**_</ins>:
- Added @w0xlt's [fix](https://github.com/bitcoin/bitcoin/pull/33540#pullrequestreview-3490620415) on long (double dash --) options that [worked](https://github.com/bitcoin/bitcoin/pull/33540#pullrequestreview-3490254189) on `master`, adding a new test for it (the test would pass in `master` but fail without @w0xlt's changes).
- Refactored both `ArgsManager::ProcessOptionKey` (new function added in this PR) and `ArgsManager::ParseParameters` in separated commits maki
...

👍 ryanofsky approved a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3552578391)
Code review ACK faa23738fc2576e412edb04a4004fab537a3098e, only adding `value_or` implementation and `CheckedReturnTypes` clang-tidy commit since last review.

I think this is ok to merge as-is but I left some suggestions about `value_or`, and I think the `[[maybe_unused]] auto _{...}` uses in the second commit are pretty ugly and it would be nice if they could be changed to use `(void)` instead.

It could also be nice make the other suggested improvements if there is not a hurry:

- Avoidi
...

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2599049418)
In commit "Add util::Expected (std::expected)" (fa114be27b17ed32c1d9a7106f313a0df8755fa2)

Not very important, but since value_or methods are template methods it would make sense for them to return `T` instead of `ValueType` to avoid returning std::monostate.

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2599097104)
re: https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592820086

> You are right, but I can't see this going wrong in practise, so I'll leave this as-is for now and postpone to a follow-up. (The solution would probably be to specialize, but that requires writing a lot more code)

I think hodlinator's solution adding requires clauses might avoid the need to specialize https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592454740

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2599070287)
In commit "Add util::Expected (std::expected)" (fa114be27b17ed32c1d9a7106f313a0df8755fa2)

Not very important, but it's not consistent for `std::move(result).value_or(...)` to move from the result when `std::move(result).value()` does not move from it.

The upstream `std::expected` seems to move in both cases by defining a `value() &&` overload. (If you added that overload this line could also be simplified changing `std::move(value())` to `value()`)

Would suggest making `value()` and `va
...

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2599018832)
re: https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2597786362

Very much agree with having this code use `(void)` not `[[maybe_unused]] auto _{}`. I don't understand why clang-tidy would encourage the latter.

🤔 furszy reviewed a pull request: "Added rescan option for import descriptors"
(https://github.com/bitcoin/bitcoin/pull/31668#pullrequestreview-3552684128)
Instead of using `timestamp=never`, which is not really descriptive. What about using `new`?

💬 furszy commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2599106467)
In cda5cb3ec27ac9bf15b947eff0c5cc87cc835c48:

Instead of returning -1, could change the return value to optional, and here return `std::nullopt`.

💬 furszy commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2599095627)
In cda5cb3ec27ac9bf15b947eff0c5cc87cc835c48:

This change should be in a different commit.

💬 furszy commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2599145016)
In https://github.com/bitcoin/bitcoin/commit/cda5cb3ec27ac9bf15b947eff0c5cc87cc835c48:

Need to unload the wallets after using them; otherwise they stay loaded for other tests in this file as well.
Also, you can do this in fewer lines of code with a loop. As far as I can see, the only per-round variants are the timestamp, the balance, and the tx list.

💬 maflcko commented on pull request "refactor: C++20 operators":
(https://github.com/bitcoin/bitcoin/pull/33771#issuecomment-3627658898)
review ACK 48840bfc2d7beeac0ddf56a3c26b243156ec8936 🌖

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 48840bfc2d7b
...

💬 stickies-v commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3627705445)
ACK faa23738fc2576e412edb04a4004fab537a3098e

Current code seems fine to merge as-is, but I would prefer addressing more of the outstanding comments (from myself and other reviewers) first.

👍 stickies-v approved a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3552257782)
ACK faa23738fc2576e412edb04a4004fab537a3098e

Current code seems fine to merge as-is, but I would prefer addressing more of the outstanding comments (from myself and other reviewers) first.

💬 stickies-v commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2598773353)
nit: can use `std::in_place_index` shorthand

```suggestion
constexpr Expected() : m_data{std::in_place_index<0>, ValueType{}} {}
constexpr Expected(ValueType v) : m_data{std::in_place_index<0>, std::move(v)} {}
template <class Err>
constexpr Expected(Unexpected<Err> u) : m_data{std::in_place_index<1>, std::move(u.err)}
```

💬 stickies-v commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2599122933)
It's a larger diff, so I'm happy to keep this as-is, but I think this is actually quite a nice use-case for `util::Expected` to improve the ownership semantics of `Enqeue`. Ownership can be transferred unambiguously with:

<details>
<summary>git diff on faa23738fc</summary>

```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index abfcb455e1..de6c837ed2 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -14,11 +14,13 @@
#include <rpc/protocol.h>
#include <sync.h>
...

💬 stickies-v commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2599169803)
An alternative approach is to first extract `bootstrap_cap` and remove the ownership ambiguity:

<details>
<summary>git diff on faa23738fc</summary>

```diff
diff --git a/src/ipc/test/ipc_test.cpp b/src/ipc/test/ipc_test.cpp
index 506facdecf..6e0a5d19be 100644
--- a/src/ipc/test/ipc_test.cpp
+++ b/src/ipc/test/ipc_test.cpp
@@ -60,12 +60,10 @@ void IpcPipeTest()
auto pipe = loop.m_io_context.provider->newTwoWayPipe();

auto connection_client = std::make_unique<mp::
...

💬 stickies-v commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2598943386)
I don't think kernel is currently using this? It seems like this should be in `util/CMakeLists.txt`, and only added to the kernel lib when the files it includes actually use it? `util/expected.cpp` is currently empty of course, so currently should all behave the same way.

🤔 janb84 reviewed a pull request: "build: Introduce internal kernel library"
(https://github.com/bitcoin/bitcoin/pull/28690#pullrequestreview-3552815631)
ACK 4c387e938015de197860215d94b9c9aee9a82da8

Looks good to me, next to several builds & tests, I did a (subsection of a ) Guix build to see if there where any issues with that. It ran fine, see below.

my Guix Build Output

**Host architecture:** `aarch64`
**Commit:** `4c387e938015`

```shell
0eea1fa55868cf288544de481d7ae5e8285f068074536723e927c20b9ece0518 guix-build-4c387e938015/output/dist-archive/bitcoin-4c387e938015.tar.gz
bf648f3ef27667e011b2c4547987527111c3d5c286933d092ad826
...

🤔 sipa reviewed a pull request: "precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442#pullrequestreview-3552798362)
Approach ACK