💬 hodlinator commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#discussion_r1988781317)
I think the code is noisy, but like that the text is precise, could skip the example though. Tried to make the code less noisy too but best I could come up with while still using `echo` was:
```
COMMAND ${CMAKE_COMMAND} -E echo &&
${CMAKE_COMMAND} -E echo "Error: NSIS not found." &&
${CMAKE_COMMAND} -E echo "Please install NSIS and/or ensure that its executable is accessible to the find_program() command." &&
${CMAKE_COMMAND} -E echo "
...

💬 l0rinc commented on issue "Fully validated AssumeUTXO starts revalidating after restart":
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2713478902)
> all traces of AssumeUtxo are only gone after the next restart

Yes, after fully validating I have restarted the node and it wants to validate again, which I haven't done (cancelled the process)

👍 pablomartin4btc approved a pull request: "qt: 29.0 translations update"
(https://github.com/bitcoin/bitcoin/pull/32004#pullrequestreview-2673743665)
re-ACK d95d1ddd173fef2654fa0678d1c1f0c9c5e40d93

Minor diffs against sync'ed (updated translations) `master` on:
```
src/qt/locale/bitcoin_et.ts
src/qt/locale/bitcoin_ru.ts
src/qt/locale/bitcoin_sv.ts
```
Checked that the changes on those files were done after the latest PR's refresh.

🤔 polespinasa reviewed a pull request: "test: Use rpc_deprecated only for testing deprecation"
(https://github.com/bitcoin/bitcoin/pull/31977#pullrequestreview-2673757627)
ACK b1c80fab0c026a40f2901958abdeedb9dd36c30d

💬 polespinasa commented on pull request "test: Use rpc_deprecated only for testing deprecation":
(https://github.com/bitcoin/bitcoin/pull/31977#discussion_r1988884259)
nit: update copyright

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988909339)
Yes, the argument `CNode& node` is always heap allocated. Not sure about "guaranteed". If not heap allocated, then this will be a gross error, resulting in an immediate crash or an error report by Valgrind or a memory sanitizer.

Note that the semantic of this piece of code is not changed by this PR. It was the same before as well - it would eventually `delete` the object.

📝 Lubov66 opened a pull request: "docs: added a badge to the workflow"
(https://github.com/bitcoin/bitcoin/pull/32032)
<!--
*** 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
...

💬 hebasto commented on issue "build: `-static-pie` builds no-longer working with CMake":
(https://github.com/bitcoin/bitcoin/issues/31843#issuecomment-2713769991)
> > This seems like something that needs to be solved in CMake itself.
>
> Asked here: https://discourse.cmake.org/t/static-pie-is-incompatible-with-checkpiesupported/13696

Upstream issue: https://gitlab.kitware.com/cmake/cmake/-/issues/26757.

💬 hebasto commented on issue "build: `-static-pie` builds no-longer working with CMake":
(https://github.com/bitcoin/bitcoin/issues/31843#issuecomment-2713779158)
> You can make things work using `-DAPPEND_LDFLAGS`, but using this (non-standard workaround) shouldn't be necessary.

It has been clarified that this is a CMake issue, and we have a workaround for it.

Should this issue still be a blocker for 29.0?

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2713885677)
`af622d00ba...d233c7e999`: address https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988085255

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1989085326)
Removed the `const`.

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2713906394)
`d233c7e999...4492abbf0a`: rebase and address https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988125455

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1989097485)
Right! Removed.

💬 Sjors commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2713946284)
> > It almost completely drops Dutch, Czech, Danish and perhaps others (Github can barely load the diff).
>
> As a PR author, I take responsibility for committing changes that are generated by the [`bitcoin-maintainer-tools/update-translations.py`](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/update-translations.py) script, which in turn fetches translations from the Transifex website. When a translation file is removed, I do check whether a translation has been vandaliz
...

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1989115901)
I agree with @hodlinator that this has lifetime issue in `master` and in this PR it does not. That issue was also discussed in another PR. Good to resolve it.

💬 fanquake commented on pull request "docs: added a badge to the workflow":
(https://github.com/bitcoin/bitcoin/pull/32032#issuecomment-2713993252)
@maflcko is Draht meant to be editing the PR description, or leaving a comment with the feedback?

💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2714035321)
> > > It almost completely drops Dutch, Czech, Danish and perhaps others (Github can barely load the diff).
> >
> >
> > As a PR author, I take responsibility for committing changes that are generated by the [`bitcoin-maintainer-tools/update-translations.py`](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/update-translations.py) script, which in turn fetches translations from the Transifex website. When a translation file is removed, I do check whether a translation has b
...

💬 Sjors commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2714180106)
> people who care about a particular language's translation usually restore the damage

They'd have to know about it first and understand how Transifex works. Is it safe to omit these large changes from this PR entirely? I'd rather have a slightly stale transaction than a completely broken one.

cc @laanwj

💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2714194607)
> > people who care about a particular language's translation usually restore the damage
>
> They'd have to know about it first and understand how Transifex works.

Caring about a particular language's translation implies both, doesn’t it?

> Is it safe to omit these large changes from this PR entirely? I'd rather have a slightly stale translation than a completely broken one.

I believe it is safe. Which languages are referring to?

💬 brunoerg commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1989263069)
ba2042c31f1630678a1f08bb68e01c1d98cc9abc: both `backup_path` and `pubkey` are unused.