💬 stickies-v commented on pull request "refactor: C++20 operators":
(https://github.com/bitcoin/bitcoin/pull/33771#discussion_r2487080201)
nit (here and for all other new `operator<=>`): perhaps better to be explicit about the `std::strong_ordering` return type? And perhaps making it `constexpr` while touching?

<details>
<summary>git diff on 48840bfc2d</summary>

```diff
diff --git a/src/prevector.h b/src/prevector.h
index d4d90c7350..595be4a603 100644
--- a/src/prevector.h
+++ b/src/prevector.h
@@ -7,6 +7,7 @@

#include <algorithm>
#include <cassert>
+#include <compare>
#include <cstddef>
#include <cstdint>

...

💬 fanquake commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3496969502)
> For reference, if devs want to copy the output, the job already has the full diff, ready to apply.

I don't think this diff is "ready to apply"? Headers like `errno.h` are wrong, and should be `<cerrno>` (this is enforced by `modernize-deprecated-headers` in the tidy job)? Includes like `#include "net_types.h"` are also wrong, and should be `#include <net_types.h>`?

Given that developers working on this code, have said that this change will likely make development more difficult, should
...

💬 fanquake commented on pull request "Changing the rpcbind argument being ignored to a pop up warning, inst…":
(https://github.com/bitcoin/bitcoin/pull/33813#issuecomment-3501963024)
https://github.com/bitcoin/bitcoin/actions/runs/19159736145/job/54788142845?pr=33813#step:5:171:
```bash
Duplicate include(s) in src/httpserver.cpp:
#include <node/interface_ui.h>
```

Can you also shorten the length of the commit title, and add a proper `prefix:`.

💬 maflcko commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2504767099)
Maybe:

```sh
echo "^^^ ⚠️ Failure generated from IWYU"
echo
echo "Some adjustments to the diff may be needed:"
echo
echo "* Use the C++ headers: E.g. <cerrno> over <errno.h>"
echo '* Use #include <__.h> over #include "__.h" for all headers'
echo "* Sort the headers, so that they are in the right pre-existing section, if"
echo " there are any."

🤔 hebasto reviewed a pull request: "refactor: Add missing include in bitcoinkernel_wrapper.h"
(https://github.com/bitcoin/bitcoin/pull/33825#pullrequestreview-3438418410)
ACK fa1e8d8bad92f5fba2b086d78581df4c8123b098.

[Here](https://github.com/hebasto/bitcoin/commit/11bc5cca61a9edad38d070bd5046355fcf58c7ee#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2) is IWYU's diff based on https://github.com/bitcoin/bitcoin/pull/33810:
```diff
--- a/src/kernel/bitcoinkernel_wrapper.h
+++ b/src/kernel/bitcoinkernel_wrapper.h
@@ -5,12 +5,19 @@
#ifndef BITCOIN_KERNEL_BITCOINKERNEL_WRAPPER_H
#define BITCOIN_KERNEL_BITCOINKERNEL_WRAPPER_H

-#include <kern
...

📝 777genius opened a pull request: "refactor: Add missing include in util/result.h"
(https://github.com/bitcoin/bitcoin/pull/33832)
## Summary

Add missing `#include <type_traits>` for `std::conditional_t` and `std::is_same_v` used in the `Result` template class.

## Motivation

Following the same principle as #33825, headers should directly include standard library headers for types they use in their public interface, rather than relying on transitive includes.

Currently, `src/util/result.h` uses `std::conditional_t` and `std::is_same_v` (line 38) but does not directly include `<type_traits>`. While the code compiles due t
...

⚠️ hebasto opened an issue: "`test_kernel` fails to build on Ubuntu 22.04"
(https://github.com/bitcoin/bitcoin/issues/33846)
```
$ sudo apt update
$ sudo apt install git build-essential cmake pkgconf python3 libevent-dev libboost-dev libsqlite3-dev
$ git clone https://github.com/bitcoin/bitcoin.git && cd bitcoin
$ cmake -B build -DENABLE_IPC=OFF -DBUILD_KERNEL_LIB=ON
$ cmake --build build -j $(nproc)
<snip>
[ 76%] Building CXX object src/test/kernel/CMakeFiles/test_kernel.dir/test_kernel.cpp.o
/bitcoin/src/test/kernel/test_kernel.cpp:17:10: fatal error: format: No such file or directory
17 | #include <format>

...

💬 maflcko commented on issue "`test_kernel` fails to build on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3515396269)
Please remove the include. See https://github.com/bitcoin/bitcoin/actions/runs/19176694315/job/54823183155?pr=33810#step:9:18532 :

```
/home/admin/actions-runner/_work/_temp/src/test/kernel/test_kernel.cpp should remove these lines:
- #include <boost/test/included/unit_test.hpp> // lines 9-9
- #include <cstdlib> // lines 15-15
- #include <format> // lines 17-17

💬 l0rinc commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2513989045)
do we still need `#include <pow.h>` here after the move?

💬 ryanofsky commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#discussion_r2524524789)
In commit "cmake, test: Improve locality of `bitcoin_ipc_test` library description" (866bbb98fd365962840ee99df0d9f7ad557cc025)

This change should be correct, but I'm confused about how it was working before. This change causes includes in generated files such `build/src/ipc/capnp/common.capnp.proxy.h` to be generated correctly relative to `src/`:

```diff
-#include <src/ipc/capnp/common.capnp.h> // IWYU pragma: keep
+#include <capnp/common.capnp.h> // IWYU pragma: keep
```

So that see
...

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2534655978)
Continuing the thread from https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2477534828:

```patch
#include <common/system.h>
+#include <test/util/setup_common.h>
#include <util/string.h>
```

```suggestion
BOOST_CHECK_EXCEPTION(threadPool.Submit([]{ return false; }), std::runtime_error, HasReason{"No active workers; cannot accept new tasks"});
```

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2541836752)
some of these includes seem unused, can you please check?

<details>
<summary>iwyu_tool.py</summary>

```
python3 /opt/homebrew/bin/iwyu_tool.py -p build-iwyu src/test/threadpool_tests.cpp -- -Xiwyu --cxx17ns -Xiwyu --mapping_file=$PWD/contrib/devtools/iwyu/bitcoin.core.i
mp -Xiwyu --max_line_length=160 -Xiwyu --check_also=$PWD/src/util/threadpool.h

/Users/lorinc/IdeaProjects/bitcoin/src/util/threadpool.h should add these lines:
#include <__vector/vector.h> // for vector

/Users/
...

💬 ajtowns commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#discussion_r2546566487)
FYI, you could also make this a `string_view` literal, and leave the function as expecting a `span<const char>`, with

```c++
#include <string_view>

using namespace std::string_view_literals;

auto descs = Parse("pk(0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798)"sv, keys, err, /*require_checksum=*/false);
```

Using `string_view` in the function declaration seems better though.

💬 diegoviola commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-3574067005)
@pablomartin4btc like I said, this variation when applied to your branch also makes it work:

```diff
diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp
index 48e41bc352..72c9a65cd3 100644
--- a/src/qt/transactionview.cpp
+++ b/src/qt/transactionview.cpp
@@ -36,6 +36,7 @@
#include <QScrollBar>
#include <QSettings>
#include <QString>
+#include <QWindow>
#include <QTableView>
#include <QTimer>
#include <QUrl>
@@ -671,7 +672,9 @@ bool TransactionView::detailsA
...

💬 sedited commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2571742382)
It would be nice if these could stay where they are. How about the following:
<details>
<summary>Protected Method Wrapper</summary>

```diff
diff --git a/src/test/fuzz/block_index_tree.cpp b/src/test/fuzz/block_index_tree.cpp
index 037d168e0e..04fbbc5473 100644
--- a/src/test/fuzz/block_index_tree.cpp
+++ b/src/test/fuzz/block_index_tree.cpp
@@ -14,0 +15 @@
+#include <test/util/validation.h>
@@ -44 +45 @@ FUZZ_TARGET(block_index_tree, .init = initialize_block_index_tree)
- Chainst
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2577299367)
Looks like this needs more work, it's not as easy as I though - we could use an `std::deque` instead of a `vector` here:
```patch
Subject: [PATCH] std::deque<InputToFetch> m_inputs
---
diff --git a/src/coinsviewcacheasync.h b/src/coinsviewcacheasync.h
--- a/src/coinsviewcacheasync.h (revision a3f56354d6e3f64eaca84a16e4951e6073090f60)
+++ b/src/coinsviewcacheasync.h (revision 462408b897197de3a7067dcbdee318ad9dc1e546)
@@ -17,6 +17,7 @@
#include <atomic>
#include <barrier>
#include <cs
...

💬 laanwj commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3607180028)
This is a minimum C++ file that will reproduce the issue:
```c++
#include <filesystem>
#include <string>
#include <fstream>
#include <vector>

#include <cstring> // Unused, but removing this makes it determinstic

namespace fs {

using namespace std::filesystem;

// Dummy class. Using "path = std::filesystem::path" makes it deterministic.
class path : public std::filesystem::path
{
public:
using std::filesystem::path::path;

path(std::filesystem::path path) : std::file
...

💬 hebasto commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#discussion_r2589020763)
> But I don't understand why the `#include <src/...>` lines were working previously?

This code is responsible for adding the required include path:https://github.com/bitcoin/bitcoin/blob/75baff98fcf987735437196a4db1919e390c4bd2/src/ipc/libmultiprocess/cmake/TargetCapnpSources.cmake#L96-L103

💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3612808384)
Minified a little more (https://github.com/fanquake/bitcoin/tree/repro_33775_minimal):
```cpp
#include <filesystem>
#include <string>
#include <vector>

#include <cstring> // Unused, but removing this makes it determinstic

namespace fs {

using namespace std::filesystem;

// Dummy class. Using "path = std::filesystem::path" makes it deterministic.
class path : public std::filesystem::path
{
public:
using std::filesystem::path::path;

path(std::filesystem::path path) :
...

💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3616532437)
Slightly more minified:
```cpp
#include <filesystem>
#include <string>
#include <vector>
#include <cstring> // Unused, but removing this makes it determinstic

namespace fs {

using namespace std::filesystem;

// Dummy class. Using "path = std::filesystem::path" makes it deterministic.
class path : public std::filesystem::path
{
public:
using std::filesystem::path::path;

path(std::filesystem::path path) : std::filesystem::path::path(std::move(path)) {}
};

}

bool
...

💬 hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2592510783)
> nit: Looking at the output, I think this is already done correctly?

I don't think so. The generated diffs still [suggest](https://github.com/bitcoin/bitcoin/actions/runs/19938614368/job/57170256836) the C headers:
```diff
+#include <errno.h>
```