💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1934748506)
Problem: " (<CLIENT_NAME> already running?)"-`string` is untranslated.

Would suggest bringing back old strings to reduce translation churn, or ensuring the new version is fully translated. Old strings:
```suggestion
if (err == WSAEADDRINUSE) {
err_msg = strprintf(_("Unable to bind to %s on this computer. %s is probably already running."), to.ToStringAddrPort(), CLIENT_NAME);
} else {
err_msg = strprintf(_("Unable to bind to %s on this computer (bin
...

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1934722181)
nit: The comment adds no information.
```suggestion
std::vector<std::shared_ptr<Sock>> m_listen;
```
Same for `m_unused_i2p_sessions_mutex`.

💬 hebasto commented on issue "build: use UCRT runtime for Windows (release) binaries":
(https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-2629109656)
> - [ ] Add CI to cover new runtime.

I've [added](https://github.com/hebasto/bitcoin-core-nightly/pull/39) a nightly build with UCRT to https://github.com/hebasto/bitcoin-core-nightly.

> Inconsistency in the binaries leading to more workarounds/disabling test code: [#31410](https://github.com/bitcoin/bitcoin/pull/31410).

It seems that the mentioned issue requires further investigation, as the fix is still needed.

💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2629110703)
FWIW, the Windows UCRT-linked cross-compiled builds still [need](https://github.com/hebasto/bitcoin-core-nightly/pull/39) this fix.

🤔 fjahr reviewed a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2588404383)
Everything besides my `GetPrivKey` question looks good to me. Will do a final pass once I get an answer there.

💬 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 :)