Bitcoin Core Github
44 subscribers
121K links
Download Telegram
πŸ‘ stickies-v approved a pull request: "wallet: reduce unconditional logging during load"
(https://github.com/bitcoin/bitcoin/pull/33299#pullrequestreview-3284021822)
ACK fc861332b351c9390400054ff74193ce26eb0713

nit: Would still prefer doing the logging from `WalletInit::Construct`, `VerifyWallets` feels out of place to me, but no biggie.

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

```diff
diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
index 79e2c9688d..4013e12967 100644
--- a/src/wallet/init.cpp
+++ b/src/wallet/init.cpp
@@ -20,6 +20,7 @@
#include <util/translation.h>
#include <wallet/coincontrol.h>
#include <wallet/walle
...
πŸ’¬ l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2485772199)
Hmm, I'm not sure this is correct.

> Obviously we don't want to use the sorted vector

That's not what I'm getting. You were inserting to the same collection over and over, I'm not sure what we were measuring there.
Adding the collection creation and an assertion to not optimize it away reveals something completely different.

<details>
<summary>updated benchmarking code</summary>

```C++
#include <bench/bench.h>
#include <bench/data/block413567.raw.h>
#include <coins.h>
#include
...
πŸ’¬ rkrux commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2486017553)
In d633db54166497685b80a12c51db6772982e01fe "rpc: add "ischange: true" in wallet gettransaction decoded tx output"

Nit: #IWYU

```diff
diff --git a/src/core_io.h b/src/core_io.h
index 1874c93a05..57ae6a2af7 100644
--- a/src/core_io.h
+++ b/src/core_io.h
@@ -11,6 +11,7 @@
#include <string>
#include <vector>
#include <optional>
+#include <functional>

class CBlock;
class CBlockHeader;
```
πŸ’¬ 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?