💬 hebasto commented on pull request "cmake: Improve safety and robustness during building `crc32c` subtree":
(https://github.com/bitcoin/bitcoin/pull/31779#discussion_r1939241677)
Fixed in the recent push.

👍 vasild approved a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283#pullrequestreview-2589664657)
ACK 276ce2eea38865154017047f3761225c2b504cf6

Two things that would be nice to figure out before merge:

1. There could be a low-hanging fruit to avoid iterating over all transactions and use `-nFee` which should be significantly faster: https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937311760

2. Just to confirm that the `Assume()` can't happen in normal circumstances: https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939240601

💬 hebasto commented on pull request "cmake: Improve safety and robustness during building `crc32c` subtree":
(https://github.com/bitcoin/bitcoin/pull/31779#issuecomment-2630727456)
@purpleKarrot
> Whether the upstream `CMakeLists.txt` file is used or not, the `crc32c` subtree should be added with `add_subdirectory` and not `include`. The latter prevents variables from leaking into the parent directory.

Certainly, I agree with keeping things as scoped as possible.

The initial plan was to replace `include(cmake/crc32c.cmake)` with `add_subdirectory(src/crc32c)`. However, the perspective has since [changed](https://github.com/bitcoin/bitcoin/pull/30905#issuecomment-24
...

🤔 rkrux reviewed a pull request: "wallet: Automatically repair corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124#pullrequestreview-2589566638)
Build and tests successful at e92dc1a3d36be02a8b16eed26159a43cfbe3dbcc
Some initial comments, will give it another pass again.

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1939235575)
Can this happen for internal keychain as well?

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1939203297)
I had been wondering why such a path can't occur besides this case, this comment in `DeriveNewChildKey` function answers it: https://github.com/bitcoin/bitcoin/blob/28.x/src/wallet/scriptpubkeyman.cpp#L1129

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1939235210)
I spent some time trying to understand why this one is picked here only to realise later that 9 is the last index at this chain because of keypool size set to 10 at the top of this test here: https://github.com/bitcoin/bitcoin/pull/29124/commits/e92dc1a3d36be02a8b16eed26159a43cfbe3dbcc#diff-b5055a65809b46e2775f6e28aa6eb8171e3fcc7a535cce5687a548b9ff233669R44

This 9 and 10 can be tied to each other with a constant for better readability.

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1939245322)
Reference for line format: https://github.com/bitcoin/bitcoin/blob/28.x/src/wallet/rpc/backup.cpp#L778-L802

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1939186315)
For others, reference: https://github.com/bitcoin/bitcoin/blob/28.x/src/wallet/rpc/wallet.cpp#L521

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1939237181)
Similar tying to a constant here if possible.

💬 vijayabhaskar78 commented on issue "cmake: incorrectly reporting MSVC as using ccache":
(https://github.com/bitcoin/bitcoin/issues/31771#issuecomment-2630755334)
@fanquake @hebasto @purpleKarrot

### **Problem Summary**

- **False Reporting:** CMake incorrectly reports `ccache` as "ON" for MSVC builds despite excluding it in `cmake/ccache.cmake`.
- **Launcher Conflicts:** Forced `CMAKE_*_COMPILER_LAUNCHER` assignments override user/CI settings (e.g., `sccache`).

### **Proposed Fix**

#### **1. Delete `cmake/ccache.cmake`**

```bash
rm cmake/ccache.cmake
```

- **Why?** Removes forced `ccache` logic conflicting with external launchers.
- **
...

💬 fanquake commented on pull request "cmake: Improve safety and robustness during building `crc32c` subtree":
(https://github.com/bitcoin/bitcoin/pull/31779#issuecomment-2630755945)
~0.

> However, the perspective has since https://github.com/bitcoin/bitcoin/pull/30905#issuecomment-2426263301.

#30905 wasn't merged because it just took upstreams compilation flags, which didn't make sense for us, and because switching came with pretty much no benefit (and still couldn't be done without other modifications to the subtree).

I think time would be better spent modifying our subtree, so we could use the CMake file (if that's what we really want to do), rather than trying t
...

💬 fanquake commented on pull request "Pass custom DEP_OPTS and CONFIG_FLAGS to guix-build":
(https://github.com/bitcoin/bitcoin/pull/31763#issuecomment-2630760306)
~0. The benefit here seems limited, as the project has no need for these options in its release process. Outside of that, anyone wanting to do a "custom" release build, is likely already modifying the source, so can change the script for their own needs. As mentioned in the PR description, arbitrary changes to the CMake config, also don't really make sense, because they will cause failures for various reasons (i.e disabling the GUI would then cause failures at the packaging steps), and if you ne
...

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939287479)
I'd rather remove the line, it might have had a use before that no longer applies.

If we want to get fees via the CBLockTemplate then it should just get a field for that.

But I don't think it's useful to do any of that, because this (duplicate) code will likely go away soon: https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937451223

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939291428)
Step (3) doesn't happen. A shutdown doesn't null out `m_tip_block`.

💬 hebasto commented on pull request "build: simplify by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2630847614)
It seems I lack a deep understanding of CMake's underlying behaviour.

The comment in c4e498300c7e6b23dc464eca75a2bc9f86270dab states:
> Specifically, this prevents generated test headers (such as
test/data/base58_encode_decode.json.h) from depending on the
dependencies of test_bitcoin (libcrc32c.a libcrc32c_sse42.a libleveldb.a)
>
> Note that this is currently only relevant for Ninja.

And this aligns perfectly with the CMake [docs](https://cmake.org/cmake/help/latest/command/add_custo
...

💬 laanwj commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1939354206)
i guess it is to make it link qt6 statically

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1939369971)
`MaybeFlipIPv6toCJDNS` only does something if `addr_(bind_)` `IsIPv6()`, which checks `m_net == NET_IPV6`.

This is the default for a `CNetAddr`, however deeply buried in `-ThreadI2PAccept` - > `i2p::sam::Session::Accept` -> `DestB64ToAddr` -> `DestBinToAddr` -> `CNetAddr::SetSpecial` -> `SetI2P` do we find `m_net = NET_I2P`. So the flip won't interfere with an I2P connection (in the case its prefix coincidentally matches the IPv6 CJDNS prefix).

💬 laanwj commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1939378508)
Wait, X11 is still the only backend we support after this change, right?

Do you have X installed at all, or is the system wayland-only?

`libwayland-cursor0` is for native wayland usage, not for using xserver-wayland. XCB is still required if you want to use emulated X.

💬 laanwj commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1939393293)
Oh, that list has all of the other libxcb components, just not -cursor
```
libxcb-damage0:amd64 1.15-1ubuntu2
libxcb-dri2-0:amd64 1.15-1ubuntu2
libxcb-dri3-0:amd64 1.15-1ubuntu2
libxcb-glx0:amd64 1.15-1ubuntu2
libxcb-icccm4:amd64 0.4.1-1.1build3
libxcb-image0:amd64 0.4.0-2build1
libxcb-keysyms1:amd64 0.4.0-1build4
libxcb-present0:amd64 1.15-1ubuntu2
libxcb-randr0:amd64 1.15-1ubuntu2
libxcb-render-util0:amd64 0.3.9-1build4
libxcb-render0:amd64 1.15-1ubuntu2
libxcb-res0:amd64 1.15-1ub
...