💬 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.
💬 mzumsande commented on issue "Fully validated AssumeUTXO starts revalidating after restart":
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2714230274)
I'm pretty sure this is being done on purpose, see [this comment](https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/node/chainstate.cpp#L185-L193), so I wouldn't call it a bug.
But that doesn't mean it's ideal - if we want to skip the second UTXO stats check after restart, I think it would be wrong to just skip it from the init code path - instead I think we'd need to persist additional validation progress to disk (so that a user that Ctrl+C's out of the firs
...
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2714230274)
I'm pretty sure this is being done on purpose, see [this comment](https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/node/chainstate.cpp#L185-L193), so I wouldn't call it a bug.
But that doesn't mean it's ideal - if we want to skip the second UTXO stats check after restart, I think it would be wrong to just skip it from the init code path - instead I think we'd need to persist additional validation progress to disk (so that a user that Ctrl+C's out of the firs
...
🤔 BrandonOdiwuor reviewed a pull request: "qa: Enable feature_init.py on Windows"
(https://github.com/bitcoin/bitcoin/pull/32021#pullrequestreview-2674560829)
Code Review ACK 59c4930394cafc939eb396224b3d60d01ba0ce37
(https://github.com/bitcoin/bitcoin/pull/32021#pullrequestreview-2674560829)
Code Review ACK 59c4930394cafc939eb396224b3d60d01ba0ce37
💬 Sjors commented on issue "Fully validated AssumeUTXO starts revalidating after restart":
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2714283026)
@mzumsande from that code comment it seems cleaning up is intentional, but not redoing validation.
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2714283026)
@mzumsande from that code comment it seems cleaning up is intentional, but not redoing validation.
💬 fjahr commented on issue "Fully validated AssumeUTXO starts revalidating after restart":
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2714303553)
> I'm pretty sure this is being done on purpose, see [this comment](https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/node/chainstate.cpp#L185-L193), so I wouldn't call it a bug.
Yes, and there is more context in the comment on the implementation: https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/validation.cpp#L6102
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2714303553)
> I'm pretty sure this is being done on purpose, see [this comment](https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/node/chainstate.cpp#L185-L193), so I wouldn't call it a bug.
Yes, and there is more context in the comment on the implementation: https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/validation.cpp#L6102
👍 brunoerg approved a pull request: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2674582400)
code review ACK ba2042c31f1630678a1f08bb68e01c1d98cc9abc
no blockers!
- 8afd005cbf9209e90a5cc1bcdb2c0ff400c2de33: checked it removes disable RPCs related to legacy wallets (import/dump stuff) - I had same doubt of https://github.com/bitcoin/bitcoin/pull/31250/commits/8afd005cbf9209e90a5cc1bcdb2c0ff400c2de33#r1985126112
- 56e9bef655673becb3e265948143a9e860a5c68b: checked it removes legacy wallet functional tests and get rid off --descriptor --legacy-wallet. Left a nit about a comment that m
...
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2674582400)
code review ACK ba2042c31f1630678a1f08bb68e01c1d98cc9abc
no blockers!
- 8afd005cbf9209e90a5cc1bcdb2c0ff400c2de33: checked it removes disable RPCs related to legacy wallets (import/dump stuff) - I had same doubt of https://github.com/bitcoin/bitcoin/pull/31250/commits/8afd005cbf9209e90a5cc1bcdb2c0ff400c2de33#r1985126112
- 56e9bef655673becb3e265948143a9e860a5c68b: checked it removes legacy wallet functional tests and get rid off --descriptor --legacy-wallet. Left a nit about a comment that m
...