💬 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
...
(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
...
(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
(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) :
...
(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
...
(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>
```
(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>
```
💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592629697)
Requires `#include <cassert>`.
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592629697)
Requires `#include <cassert>`.
💬 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>
...
(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>
...
💬 hodlinator commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2607135191)
~nit: Could use `optional` instead of `monostate`?~ Ah, I see that was already discussed https://github.com/bitcoin/bitcoin/pull/34032#issuecomment-3632418418.
<details><summary>Diff</summary>
```diff
--- a/src/util/expected.h
+++ b/src/util/expected.h
@@ -10,6 +10,7 @@
#include <cassert>
#include <exception>
+#include <optional>
#include <utility>
#include <variant>
@@ -119,16 +120,16 @@ template <class E>
class Expected<void, E>
{
private:
- std::variant<std:
...
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2607135191)
~nit: Could use `optional` instead of `monostate`?~ Ah, I see that was already discussed https://github.com/bitcoin/bitcoin/pull/34032#issuecomment-3632418418.
<details><summary>Diff</summary>
```diff
--- a/src/util/expected.h
+++ b/src/util/expected.h
@@ -10,6 +10,7 @@
#include <cassert>
#include <exception>
+#include <optional>
#include <utility>
#include <variant>
@@ -119,16 +120,16 @@ template <class E>
class Expected<void, E>
{
private:
- std::variant<std:
...
💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2611463509)
> So the -Wswitch wouldn't catch them? I have checked multiple and they all failed the build if I left out any enum value
No. See https://godbolt.org/z/6zGqrcnax
```cpp
#include <iostream>
enum class Color : uint8_t { Red, Green, Blue };
int to_int(Color c) {
switch (c) {
case Color::Red:
return 1;
case Color::Green:
return 2;
case Color::Blue:
return 3;
}
return -1; // this should never happen?
}
i
...
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2611463509)
> So the -Wswitch wouldn't catch them? I have checked multiple and they all failed the build if I left out any enum value
No. See https://godbolt.org/z/6zGqrcnax
```cpp
#include <iostream>
enum class Color : uint8_t { Red, Green, Blue };
int to_int(Color c) {
switch (c) {
case Color::Red:
return 1;
case Color::Green:
return 2;
case Color::Blue:
return 3;
}
return -1; // this should never happen?
}
i
...
💬 hebasto commented on pull request "kernel: Remove non-kernel module includes":
(https://github.com/bitcoin/bitcoin/pull/34079#issuecomment-3660095503)
@sedited
Could you please elaborate on the motivation for the last commit? Is it for avoiding `#include <interfaces/...>` in kernel sources?
(https://github.com/bitcoin/bitcoin/pull/34079#issuecomment-3660095503)
@sedited
Could you please elaborate on the motivation for the last commit? Is it for avoiding `#include <interfaces/...>` in kernel sources?
💬 sedited commented on pull request "kernel: Remove non-kernel module includes":
(https://github.com/bitcoin/bitcoin/pull/34079#issuecomment-3660133778)
> Could you please elaborate on the motivation for the last commit? Is it intended to avoid #include <interfaces/...> in kernel sources?
Yes, interfaces in turn include a bunch of stuff that should not belong in the library. I elaborated a bit in the commit message.
(https://github.com/bitcoin/bitcoin/pull/34079#issuecomment-3660133778)
> Could you please elaborate on the motivation for the last commit? Is it intended to avoid #include <interfaces/...> in kernel sources?
Yes, interfaces in turn include a bunch of stuff that should not belong in the library. I elaborated a bit in the commit message.
💬 janb84 commented on pull request "kernel: Remove non-kernel module includes":
(https://github.com/bitcoin/bitcoin/pull/34079#discussion_r2626416456)
NIT: I think `#include <consensus/validation.h>` can also be removed, IWYU does not complain after removal and compiles + tests fine.
(https://github.com/bitcoin/bitcoin/pull/34079#discussion_r2626416456)
NIT: I think `#include <consensus/validation.h>` can also be removed, IWYU does not complain after removal and compiles + tests fine.
💬 janb84 commented on pull request "kernel: Remove non-kernel module includes":
(https://github.com/bitcoin/bitcoin/pull/34079#discussion_r2628339958)
NIT: Also ` #include <util/strencodings.h>` should be fine to remove compiles, tests etc. fine.
(https://github.com/bitcoin/bitcoin/pull/34079#discussion_r2628339958)
NIT: Also ` #include <util/strencodings.h>` should be fine to remove compiles, tests etc. fine.
💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2630789857)
1ff59a15407cb5fdc2ae06358e45134166db2448: Besides a unit test for the aggregate, I think a fuzz testing would be nice, e.g.:
```cpp
// Copyright (c) 2025-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <random.h>
#include <test/fuzz/FuzzedDataProvider.h>
#include <test/fuzz/fuzz.h>
#include <test/fuzz/util.h>
#include <test/util/setup_common.h>
#in
...
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2630789857)
1ff59a15407cb5fdc2ae06358e45134166db2448: Besides a unit test for the aggregate, I think a fuzz testing would be nice, e.g.:
```cpp
// Copyright (c) 2025-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <random.h>
#include <test/fuzz/FuzzedDataProvider.h>
#include <test/fuzz/fuzz.h>
#include <test/fuzz/util.h>
#include <test/util/setup_common.h>
#in
...
💬 hebasto commented on pull request "kernel: Remove non-kernel module includes":
(https://github.com/bitcoin/bitcoin/pull/34079#discussion_r2637033403)
```diff
--- a/src/kernel/chain.cpp
+++ b/src/kernel/chain.cpp
@@ -2,8 +2,9 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
-#include <chain.h>
#include <kernel/chain.h>
+
+#include <chain.h>
#include <kernel/types.h>
#include <kernel/cs_main.h>
#include <sync.h>
```
(https://github.com/bitcoin/bitcoin/pull/34079#discussion_r2637033403)
```diff
--- a/src/kernel/chain.cpp
+++ b/src/kernel/chain.cpp
@@ -2,8 +2,9 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
-#include <chain.h>
#include <kernel/chain.h>
+
+#include <chain.h>
#include <kernel/types.h>
#include <kernel/cs_main.h>
#include <sync.h>
```
📝 hebasto opened a pull request: "iwyu: Add `pragma: always_keep` to `bitcoin-build-config.h`"
(https://github.com/bitcoin/bitcoin/pull/34127)
Rather than use `// IWYU pragma: keep` with every `#include <bitcoin-build-config.h>`, apply a dedicated [`always_keep`](https://github.com/include-what-you-use/include-what-you-use/blob/clang_21/docs/IWYUPragmas.md#iwyu-pragma-always_keep) pragma to the `bitcoin-build-config.h` header itself.
(https://github.com/bitcoin/bitcoin/pull/34127)
Rather than use `// IWYU pragma: keep` with every `#include <bitcoin-build-config.h>`, apply a dedicated [`always_keep`](https://github.com/include-what-you-use/include-what-you-use/blob/clang_21/docs/IWYUPragmas.md#iwyu-pragma-always_keep) pragma to the `bitcoin-build-config.h` header itself.