💬 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

💬 sipa commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2599180166)
In commit "optimization: introduce PresaltedSipHasher for repeated hashing"

Unrelated change, which does not match style guide.

💬 sipa commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2599216213)
In commit "refactor: extract shared SipHash state into SipSalt"

These could be doxygen comments (`/** Equivalent to ... */`).

💬 sipa commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2599207202)
In commit "refactor: extract shared SipHash state into SipSalt"

Given that instances of this class are apprently used for the internal state in `CSipHasher` and `PresaltedSipHasher`, maybe it would be more appropriate to call it `SipHashState` or so?

💬 darosior commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2599232376)
I hadn't seen your other comment. Makes sense.

🤔 sipa reviewed a pull request: "validation: periodically flush dbcache during reindex-chainstate"
(https://github.com/bitcoin/bitcoin/pull/32414#pullrequestreview-3552937494)
utACK c1e554d3e5834a140f2a53854018499a3bfe6822

🤔 sipa reviewed a pull request: "refactor: C++20 operators"
(https://github.com/bitcoin/bitcoin/pull/33771#pullrequestreview-3552981158)
ACK 48840bfc2d7beeac0ddf56a3c26b243156ec8936

🤔 sipa reviewed a pull request: "netbase: Remove "tor" as a network specification"
(https://github.com/bitcoin/bitcoin/pull/34031#pullrequestreview-3553002765)
ACK 7efb18c38150344ca6f7efbcd8441792a2ea921f

14 major releases ought to be a long enough deprecation period.

🚀 fanquake merged a pull request: "refactor: C++20 operators"
(https://github.com/bitcoin/bitcoin/pull/33771)

💬 fanquake commented on pull request "net: Waste less time in socket handling":
(https://github.com/bitcoin/bitcoin/pull/34025#issuecomment-3628023796)
cc @theuni @vasild