💬 fanquake commented on pull request "kernel: Remove args, chainparams, chainparamsbase from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27576#issuecomment-1543685914)
```bash
src/common/settings.h seems to be missing the expected include guard:
#ifndef BITCOIN_COMMON_SETTINGS_H
#define BITCOIN_COMMON_SETTINGS_H
...
#endif // BITCOIN_COMMON_SETTINGS_H
```

💬 jonatack commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1271484187)
c75d9de0683a91151eb4a508cb64a8937ca92 `s/PAYMENT_H/PAYMENTS_H/` in `ifndef/define/endif` or rename the files to `silentpayment.{h,cpp}`

```bash
$ test/lint/lint-include-guards.py
src/wallet/silentpayments.h seems to be missing the expected include guard:
#ifndef BITCOIN_WALLET_SILENTPAYMENTS_H
#define BITCOIN_WALLET_SILENTPAYMENTS_H
...
#endif // BITCOIN_WALLET_SILENTPAYMENTS_H
```

💬 fanquake commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1288431124)
```bash
src/script/solver.h seems to be missing the expected include guard:
#ifndef BITCOIN_SCRIPT_SOLVER_H
#define BITCOIN_SCRIPT_SOLVER_H
...
#endif // BITCOIN_SCRIPT_SOLVER_H
```

💬 hebasto commented on pull request "build: Patch Qt to handle minimum macOS version properly":
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1790792236)
> Can you explain the patch? What is the actual problem, and how does this fix it? Seems a bit odd to have to patch version detection checks inside Qt, can the correct values not be passed in from the outside?

In macOS SDK, the problematic `os/activity.h` header relies on the `OS_LOG_TARGET_HAS_10_12_FEATURES` macro:
```c++
#if OS_LOG_TARGET_HAS_10_12_FEATURES
#ifndef OS_ACTIVITY_OBJECT_API
#define OS_ACTIVITY_OBJECT_API 1
#endif // OS_ACTIVITY_OBJECT_API
#else
#if OS_ACTIVITY_OBJECT_A
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1542970441)
I think so, but this drags some further changes, are you ok with those:

<details>
<summary>[patch] move TxBroadcastMethod from primitives/transaction.h to node/transaction.h</summary>

```diff
diff --git i/src/interfaces/chain.h w/src/interfaces/chain.h
index 315211ae49..eeb5fef42b 100644
--- i/src/interfaces/chain.h
+++ w/src/interfaces/chain.h
@@ -4,12 +4,13 @@

#ifndef BITCOIN_INTERFACES_CHAIN_H
#define BITCOIN_INTERFACES_CHAIN_H

#include <blockfilter.h>
#include <comm
...

💬 maflcko commented on pull request "refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options":
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2208611605)
E.g.:

```diff
commit 94ed6bf4575abee5200e7fc7054a47d66bebd56c
Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Date: Wed Jul 3 18:05:21 2024 +0200

move-only: Default values in MemPoolLimits

diff --git a/src/kernel/mempool_limits.h b/src/kernel/mempool_limits.h
index 8d4495c3cb..eeeaedd233 100644
--- a/src/kernel/mempool_limits.h
+++ b/src/kernel/mempool_limits.h
@@ -4,9 +4,17 @@
#ifndef BITCOIN_KERNEL_MEMPOOL_LIMITS_H
#define BITCOIN_KERNEL_MEMPOOL_LIMITS_H

-#includ
...

💬 nicholas-1vbw commented on issue "Risczero Fit":
(https://github.com/bitcoin/bitcoin/issues/30747#issuecomment-2320560690)
We compiled a scratch main.cpp that includes `assumptions.h` successfully, but fails with `static_assert(std::is_same_v<int, int32_t>);`

Actually, the toolchain from risc0 has different type of "int" and "int32_t", see https://github.com/risc0/toolchain/releases/tag/2024.01.05

int32_t is defined in "riscv32-unknown-elf/include/sys/_stdint.h"
```
#ifdef ___int32_t_defined
#ifndef _INT32_T_DECLARED
typedef __int32_t int32_t ;
#define _INT32_T_DECLARED
#endif
#ifndef _UINT32_T_DECLARED
...

🤔 hodlinator reviewed a pull request: "test: increase FromUserHex FUZZ and unit testing coverage"
(https://github.com/bitcoin/bitcoin/pull/30618#pullrequestreview-2274132462)
Concept ACK a42afbdcfe02b2a872fe9bfa6cd98287b1fa9a50

TIL: one can compare `optional` against a value without `.value()` (C++17 addition).

<details>
<summary>
Would suggest something closer to this:
</summary>

```diff
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 6c1b2a57f1..d1eae29003 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -5,7 +5,6 @@
#ifndef BITCOIN_TEST_UTIL_SETUP_COMMON_H
#define BITCOIN_TEST_UTIL_
...

💬 fanquake commented on pull request "build: work around issue with Boost <= 1.80 and Clang >= 18":
(https://github.com/bitcoin/bitcoin/pull/30821#discussion_r1745578011)
(depending on what GCC does, over the lifetime of the 28.x branch, at some point, it may turn in and #ifndef MSVC etc)

💬 rkrux commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2042044806)
Few lines below.

https://github.com/bitcoin/bitcoin/pull/31250/commits/150a83ca4fcad4e158cef3e6752b3a84ffb7d0b6#diff-7b13e03c457b62251e26d2c366180c262a3fe9da45995277a9a9c1c5abbaae6cR415-R419

```cpp
#ifndef USE_BDB
if (!(flags & WALLET_FLAG_DESCRIPTORS)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without bdb support (required for legacy wallets)");
}
#endif
```

I don't believe this is required anymore, the inner condition can never be true now as `WALLET_FLAG_
...

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112726648)
In commit "feature: Add TxOrphanageImpl"

Many of the includes here seem unused:

```diff
--- a/src/node/txorphanage_impl.h
+++ b/src/node/txorphanage_impl.h
@@ -5,33 +5,20 @@
#ifndef BITCOIN_NODE_TXORPHANAGE_IMPL_H
#define BITCOIN_NODE_TXORPHANAGE_IMPL_H

-#include <coins.h>
-#include <consensus/amount.h>
-#include <indirectmap.h>
#include <logging.h>
-#include <net.h>
#include <policy/policy.h>
#include <primitives/transaction.h>
-#include <sync.h>
-#include <util/epoc
...

💬 maflcko commented on pull request "clang-tidy: Apply modernize-deprecated-headers":
(https://github.com/bitcoin/bitcoin/pull/32673#discussion_r2124553705)
there are several, so maybe a separate pull request or linter could be added for this? Although, I don't see a risk here.

```
src/base58.h:14:#ifndef BITCOIN_BASE58_H
src/bech32.h:14:#ifndef BITCOIN_BECH32_H
src/common/messages.h:11:#ifndef BITCOIN_COMMON_MESSAGES_H
src/common/types.h:13:#ifndef BITCOIN_COMMON_TYPES_H
src/net_permissions.h:12:#ifndef BITCOIN_NET_PERMISSIONS_H
src/node/types.h:13:#ifndef BITCOIN_NODE_TYPES_H
src/txgraph.h:14:#ifndef BITCOIN_TXGRAPH_H
src/wallet/types.h
...