💬 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#discussion_r2599076399)
It's a bit strange to use an imperative form in release notes, given that it's describing changes that have been made already.

Suggestion:

> CLI `-addrinfo` now returns the full set of known addresses. In previous versions (v22.0 - v30.0) the set of returned addresses was filtered for quality and recency. This was changed since it does not match the logic for selecting peers to connect to, which does not filter.

---

Also worth mentioning that the CLI feature is now imcompatible with
...

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