💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2629144602)
@achow101 @sipa @sr-gi I noticed you acked the [PR](https://github.com/bitcoin/bitcoin/pull/27877) that added this test, so I think you can probably quickly review this since you understand the code. friendly ping to review.

The test claims to be testing that a "good solution" is found, although in actuality, there's only an assertion that _any_ solution is found. Coin-grinder is looking for a solution with the lowest weight and all UTXOs in the test have the same weight. Therefore, the sol
...

👍 Prabhat1308 approved a pull request: "test: check `scanning` field from `getwalletinfo`"
(https://github.com/bitcoin/bitcoin/pull/31768#pullrequestreview-2588431111)
ACK [e7f5955](https://github.com/bitcoin/bitcoin/pull/31768/commits/e7f5955ee0f1ede3854acdada97f90624524e8db)

confirms the duration of rescanning to be greater than 0

🤔 fjahr 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-2588430991)
Concept ACK

💬 fjahr 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_r1938366105)
It looks like there is no test coverage for this yet. Is there coverage for this in a follow-up PR (maybe Musig2) or are you writing a test? Otherwise I can give it a shot.

💬 achow101 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_r1938385780)
I don't think there's any specific coverage for this, feel free to tackle it.

📝 volkanutkuurl opened a pull request: "wkl.wlkbase58.h"
(https://github.com/bitcoin/bitcoin/pull/31777)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ volkanutkuurl closed a pull request: "wkl.wlkbase58.h"
(https://github.com/bitcoin/bitcoin/pull/31777)

💬 volkanutkuurl commented on pull request "wkl.wlkbase58.h":
(https://github.com/bitcoin/bitcoin/pull/31777#issuecomment-2629227568)
SIN grrowk

📝 kcalvinalvin opened a pull request: "refactor: simplify GetAncestor"
(https://github.com/bitcoin/bitcoin/pull/31778)
The if statement in GetAncestor was quite hard to make sense of.
Simplifying it improves readability and the added test ensures that
the performance remains the same.

👍 hebasto approved a pull request: "psbt: Use SIGHASH_DEFAULT when signing PSBTs"
(https://github.com/bitcoin-core/gui/pull/850#pullrequestreview-2588510404)
ACK 3e97ff9c5eaa3160426ba112930b047404c54c9e, I have reviewed the code and it looks OK.

🚀 hebasto merged a pull request: "psbt: Use SIGHASH_DEFAULT when signing PSBTs"
(https://github.com/bitcoin-core/gui/pull/850)

💬 hebasto 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-2629332355)
> > I don't know how useful this is. Why would users care to see only a certain type, or to see what type addresses are like this?
>
> Well, since QT offers to the user the feature to create certain types, it'd make sense to list them properly identifying the original intention.

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.

💬 vijayabhaskar78 commented on issue "cmake: incorrectly reporting MSVC as using ccache":
(https://github.com/bitcoin/bitcoin/issues/31771#issuecomment-2629339404)
Hey, can I contribute to this issue

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

> Hey, can I contribute to this issue

Sure! No need to ask :)

💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2629344469)
Rebased

💬 vijayabhaskar78 commented on issue "cmake: incorrectly reporting MSVC as using ccache":
(https://github.com/bitcoin/bitcoin/issues/31771#issuecomment-2629345112)
Proposing a fix for #31771:

Set USE_CCACHE=OFF for MSVC in cmake/ccache.cmake.
Update status reporting in CMakeLists.txt.

Questions:

Does this align with project conventions?
Any edge cases (MinGW/cross-compile) to test?

💬 hebasto commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#discussion_r1938474813)
Unset the `srcs` variable at the end to avoid polluting the global namespace?

👍 hebasto approved a pull request: "cmake: add optional source files to bitcoin_crypto directly"
(https://github.com/bitcoin/bitcoin/pull/31268#pullrequestreview-2588535898)
ACK 7a53ce742207c5dfaeda7858a098049627b03d55, tested on Ubuntu 24.04, both x86_64 and aarch64.

These changes effectively revert https://github.com/hebasto/bitcoin/pull/171 and https://github.com/hebasto/bitcoin/pull/172.

Could you please mention the changes to `cmake/crc32c.cmake` in the PR title and description, and separate them into their own commit?

Additionally, it seems reasonable to mention [this](https://github.com/bitcoin/bitcoin/pull/31268/files#r1926593281) in the PR descript
...

💬 hebasto commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#discussion_r1938474356)
style nit: Keep the `PRIVATE` keyword on its own line with proper indentation, as in all surrounding code?

💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2629391951)
Re-ACK bdec262bf661db9d46fa9b4e3fb42508751b9ea8

Only rebased since last review.

📝 hebasto opened a pull request: "cmake: Improve safety and robustness during building `crc32c` subtree"
(https://github.com/bitcoin/bitcoin/pull/31779)
It was [agreed](https://github.com/bitcoin/bitcoin/pull/30905#issuecomment-2426263301) not to adopt the upstream buildsystem the `crc32c` subtree.

So this PR ensures that our custom build script properly handles potentially harmful situations, including:
1. The upstream buildsystem changes remaining unnoticed during the subtree update, even when some of those changes need to be reflected in `crc32c.cmake`.

2. The order of `include(cmake/introspection.cmake)` and `include(cmake/crc32c.cmak
...