💬 purpleKarrot commented on pull request "cmake: Improve safety and robustness during building `crc32c` subtree":
(https://github.com/bitcoin/bitcoin/pull/31779#issuecomment-2629481474)
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.

💬 purpleKarrot commented on issue "cmake: incorrectly reporting MSVC as using ccache":
(https://github.com/bitcoin/bitcoin/issues/31771#issuecomment-2629506821)
I don't understand the motivation for setting `CMAKE_<lang>_COMPILER_LAUNCHER` from inside the project. Ideally, the project should not interfere with compiler launchers at all.

This gives individual users as well as CI instances the highest flexibility.

Assuming I have `ENV{CMAKE_CXX_COMPILER_LAUNCHER}` set locally to use [`sccache`](https://github.com/mozilla/sccache). What is the expected behavior when `cmake/ccache.cmake` invokes `list(APPEND CMAKE_CXX_COMPILER_LAUNCHER ${CCACHE_EXECUTABLE
...

💬 davidgumberg commented on pull request "cmake: Fix `-pthread` flags in summary":
(https://github.com/bitcoin/bitcoin/pull/31724#discussion_r1938551789)
I agree, but in that situation we would be back to printing generator expressions in the flags summary, which seems to me better than silently printing something that is not true.

💬 vijayabhaskar78 commented on issue "cmake: incorrectly reporting MSVC as using ccache":
(https://github.com/bitcoin/bitcoin/issues/31771#issuecomment-2629512226)
@purpleKarrot Should we remove the CMAKE_*_COMPILER_LAUNCHER assignments from ccache.cmake to prioritize user/CI flexibility? This would:

Avoid conflicts with external launchers like sccache set via ENV{CMAKE_CXX_COMPILER_LAUNCHER}.
Follow CMake best practices by letting users control launchers externally.
Prevent invalid command chains (e.g., sccache;ccache;cl.exe) when multiple launchers are set.

If agreed, I can update the PR to delete these lines and add documentation instead

💬 vijayabhaskar78 commented on issue "cmake: incorrectly reporting MSVC as using ccache":
(https://github.com/bitcoin/bitcoin/issues/31771#issuecomment-2629513923)
I'm a newbie to this repository. Can you help me to understand it?

💬 purpleKarrot commented on issue "cmake: incorrectly reporting MSVC as using ccache":
(https://github.com/bitcoin/bitcoin/issues/31771#issuecomment-2629517352)
@vijayabhaskar78, I am a newbe in this repository too, but I am not a newbie with CMake ;-)
My recommendation would be to remove `cmake/ccache.cmake` for the reason you wrote.
But let the original authors of that file chime in.

💬 vijayabhaskar78 commented on issue "cmake: incorrectly reporting MSVC as using ccache":
(https://github.com/bitcoin/bitcoin/issues/31771#issuecomment-2629524038)
OK let us see

💬 Oztayls commented on issue ""Rolling forward" at startup can take a long time, and is not interruptible":
(https://github.com/bitcoin/bitcoin/issues/11600#issuecomment-2629572914)
Any more on this? I've been stuck on 90% for hours. Wondering if a smaller dbcache is more preventative, albeit slower? My internet provider shut off at midnight over the weekend for maintenance, causing this issue.

✅ pablomartin4btc closed a pull request: "Add new "address type" column to the "receiving tab" address book page"
(https://github.com/bitcoin-core/gui/pull/753)

💬 pablomartin4btc commented on pull request "Add new "address type" column to the "receiving tab" address book page":
(https://github.com/bitcoin-core/gui/pull/753#issuecomment-2629587602)
> Concept ~0 for adding a new feature that has not been vetted by designers, especially in light of development in https://github.com/bitcoin-core/gui-qml.

No problem! I originally worked on this PR as a way to get familiar with the Qt framework since I found the requirement in #646.

Thanks for your feedback - I'll close the PR to avoid any confusions.

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1938953860)
Ok, I think using `MAX_MONEY` as magic value to mean "no threshold" would be nice to avoid but is not a blocker for this PR. Accompanying boolean "has" field is fine too.

🤔 rkrux reviewed a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2589198945)
I reviewed range-diff, asked a question.

```
git range-diff fefe0636d4ae7c246042276cacd60b22f5fc6bb9...c99e1bc359263d44a85460032c04f7c1f3c688c7
```

All the new changes address the previous review comments, namely: removing duplication in the functional test rpc_psbt, adding Taproot input in the test, fixing comments, and changing `FillPSBT()` argument default.

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938961739)
This is the final hex when I run the test, which is same as the one before modifying the `sighash` type as per the test:
```
0200000000010208bd94fdd97de6bc3ec454b8e0088a5341f9e9611bfbe412387e7e0df35207960000000000fdffffff08bd94fdd97de6bc3ec454b8e0088a5341f9e9611bfbe412387e7e0df35207960100000000fdffffff0280f0fa0200000000160014f57919cd765629cd0d2ba39021a146ee065d697508c2f00800000000160014c7109263e9f5db5d2162ce40a1f898f6ac3933ad02473044022067caa211634d7f25bcac48704165ca7d257ff80a7d0b764825877c7a4
...

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938961976)
`provided`

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1938960467)
nit:
```suggestion
// Helper to check if the tip has changed, and also update tip_changed.
```

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1938970989)
`wait_until()` will first call the predicate, so `check_tip_changed() || ` is not needed. The above code is equivalent to:

```cpp
notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
return check_tip_changed() || chainman().m_interrupt;
});
```

✅ hebasto closed a pull request: "MacOS updated Bitcoin-Core gui icon in accordance with Apple design docs"
(https://github.com/bitcoin/bitcoin/pull/31780)

💬 hebasto commented on pull request "MacOS updated Bitcoin-Core gui icon in accordance with Apple design docs":
(https://github.com/bitcoin/bitcoin/pull/31780#issuecomment-2630308019)
GUI-related proposal should be opened in https://github.com/bitcoin-core/gui.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1939003712)
Ah oops, we're going in circles, I forgot about this as well. Will add a comment.

✅ hebasto closed an issue: "Organize the address types on the "Receiving addresses" tab"
(https://github.com/bitcoin-core/gui/issues/646)