👍 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.
(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
(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
(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.
(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
...
(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.
(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?
(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
(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`.
(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
(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.
(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
...
(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.
(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?
(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
...
(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
(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?
(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.
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1989263069)
ba2042c31f1630678a1f08bb68e01c1d98cc9abc: both `backup_path` and `pubkey` are unused.
💬 Sjors commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2714215851)
Based on `git show --stat`, looking for files with massive deletions:
- `src/qt/locale/bitcoin_cs.ts`
- `src/qt/locale/bitcoin_da.ts`
- `src/qt/locale/bitcoin_nl.ts`
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2714215851)
Based on `git show --stat`, looking for files with massive deletions:
- `src/qt/locale/bitcoin_cs.ts`
- `src/qt/locale/bitcoin_da.ts`
- `src/qt/locale/bitcoin_nl.ts`
💬 l0rinc commented on pull request "Don't zero-after-free `DataStream`: Faster IBD on some configurations":
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-2714223061)
While the performance increase likely isn't a measurable, it could still make sense to remove the memory zeroing - at least for most serializations, if for no other reason, it's doing useless work and it's easier to understand the behavior without it.
For cases where we're not sure if zeroing makes sense, we could keep the old behavior - as suggested by @laanwj - making this a cleanup/refactor, rather than an optimization.
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-2714223061)
While the performance increase likely isn't a measurable, it could still make sense to remove the memory zeroing - at least for most serializations, if for no other reason, it's doing useless work and it's easier to understand the behavior without it.
For cases where we're not sure if zeroing makes sense, we could keep the old behavior - as suggested by @laanwj - making this a cleanup/refactor, rather than an optimization.