💬 fjahr commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1938345053)
Hm, I am not seeing why having two `GetPrivKey`s is better than one. The description of the commit only talks about the other one, not this one. For this one the caller still cares about the return value it seems. Can't they be unified under the `std::optional` returning variant? It seems like the variant using `FlatSigningProvider` is just used in one place in `ExpandPrivate` and it just saves us one line there.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2629115868)
@hodlinator

Thank you for your feedback!


> ### Ninja
>
> PR summary:
>
> > _IMPORTANT._ Don't forget to install Ninja.
>
> I was able to build on Linux without ninja in my path. Guix/Windows install it automatically, so maybe the wording is a bit drastic?

Please see [this](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2399781582) comment.

💬 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
...