💬 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
...
💬 fjahr commented on issue "Fully validated AssumeUTXO starts revalidating after restart":
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2714343635)
I guess that the revalidation is expected could be added in the design doc here as well: https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/doc/design/assumeutxo.md#bitcoind-restarts-sometime-after-snapshot-validation-has-completed
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2714343635)
I guess that the revalidation is expected could be added in the design doc here as well: https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/doc/design/assumeutxo.md#bitcoind-restarts-sometime-after-snapshot-validation-has-completed
💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2714345545)
> 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`
>
>
> When I undo the changes to those files, at least Dutch looks fine to me.
Thanks! Updated.
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2714345545)
> 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`
>
>
> When I undo the changes to those files, at least Dutch looks fine to me.
Thanks! Updated.
📝 fjahr opened a pull request: "test: Check datadir cleanup after assumeutxo was successful"
(https://github.com/bitcoin/bitcoin/pull/32033)
I noticed that the proper datadir cleanup after a successful restart of an assumutxo node does not seem to be covered in our tests. This is added here.
(https://github.com/bitcoin/bitcoin/pull/32033)
I noticed that the proper datadir cleanup after a successful restart of an assumutxo node does not seem to be covered in our tests. This is added here.
💬 ryanofsky commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2714361350)
> but the wine tests seem to have been fundamentally broken anyways, both brittle and not catching real [bugs](https://github.com/bitcoin/bitcoin/commit/7b16ae23fba38eaf58c0efec4d1a47214e4b1930), so not a real solution.
It's true that wine tests can't catch every bug that could happen on windows since Wine is another platform and "Is Not an Emulator." But calling the wine tests "fundamentally broken" would seem to imply that there were bugs in wine or in the wine test setup, and I haven't see
...
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2714361350)
> but the wine tests seem to have been fundamentally broken anyways, both brittle and not catching real [bugs](https://github.com/bitcoin/bitcoin/commit/7b16ae23fba38eaf58c0efec4d1a47214e4b1930), so not a real solution.
It's true that wine tests can't catch every bug that could happen on windows since Wine is another platform and "Is Not an Emulator." But calling the wine tests "fundamentally broken" would seem to imply that there were bugs in wine or in the wine test setup, and I haven't see
...
💬 laanwj commented on issue "guix: re-enable exported symbol checking for RISC-V":
(https://github.com/bitcoin/bitcoin/issues/28095#issuecomment-2714382203)
It almost looks like ld flag `--no-export-dynamic`, which is the default, doesn't work on RISC-V. Not even if added explicitly. Without `--exclude-libs ALL`, all weak symbols imported from libraries end up in the exported binary. With `--exclude-libs ALL` only the ones that are exported by the `.o` file (the behavior we're seeing).
Passing `--export-dynamic` to the x86_64 build approximates what we're seeing on RISC-V. But not entirely, we get more symbols than that. So *some* filtering is happ
...
(https://github.com/bitcoin/bitcoin/issues/28095#issuecomment-2714382203)
It almost looks like ld flag `--no-export-dynamic`, which is the default, doesn't work on RISC-V. Not even if added explicitly. Without `--exclude-libs ALL`, all weak symbols imported from libraries end up in the exported binary. With `--exclude-libs ALL` only the ones that are exported by the `.o` file (the behavior we're seeing).
Passing `--export-dynamic` to the x86_64 build approximates what we're seeing on RISC-V. But not entirely, we get more symbols than that. So *some* filtering is happ
...
🚀 glozow merged a pull request: "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/31960)
(https://github.com/bitcoin/bitcoin/pull/31960)
👍 pinheadmz approved a pull request: "wallet: fix crash on double block disconnection"
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2674634683)
ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda
Built on arm/macos. Confirmed the functional tests passes on the branch, on master bitcoind fails the test with a crash:
> stderr Assertion failed: (TxStateSerializedIndex(wtx.m_state) == TxStateSerializedIndex(state)), function AddToWallet, file wallet.cpp, line 1114.
Also confirmed that if the assertion is commented out, the test still passes on both master and branch, indicating that the new state (abandoned) is already correct.
The cod
...
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2674634683)
ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda
Built on arm/macos. Confirmed the functional tests passes on the branch, on master bitcoind fails the test with a crash:
> stderr Assertion failed: (TxStateSerializedIndex(wtx.m_state) == TxStateSerializedIndex(state)), function AddToWallet, file wallet.cpp, line 1114.
Also confirmed that if the assertion is commented out, the test still passes on both master and branch, indicating that the new state (abandoned) is already correct.
The cod
...