💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2706016805)
cc @hodlinator, you might be interested in this as well
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2706016805)
cc @hodlinator, you might be interested in this as well
📝 Sjors opened a pull request: "doc: warn against having qt6 installed on macOS"
(https://github.com/bitcoin/bitcoin/pull/32017)
Document #31009 in time for the v29 release.
(https://github.com/bitcoin/bitcoin/pull/32017)
Document #31009 in time for the v29 release.
💬 Sjors commented on issue "Having qt(@6) breaks build for qt@5 on macOS 15.0 and 13.7":
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-2706033318)
I opened #32017 to document this behavior. I suggest we add that PR to the milestone and remove this issue from it. The actual problem can be solved later.
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-2706033318)
I opened #32017 to document this behavior. I suggest we add that PR to the milestone and remove this issue from it. The actual problem can be solved later.
💬 Sjors commented on pull request "ci: Do not try to install for fuzz builds":
(https://github.com/bitcoin/bitcoin/pull/32014#discussion_r1984790680)
It does seem more correct. Perhaps in the future we can even warn users when they're about to install a debug, sanitizer or otherwise potentially very slow binary.
(https://github.com/bitcoin/bitcoin/pull/32014#discussion_r1984790680)
It does seem more correct. Perhaps in the future we can even warn users when they're about to install a debug, sanitizer or otherwise potentially very slow binary.
💬 willcl-ark commented on pull request "RFC contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2706040342)
Concept ACK.
I don't think a few hundred MB more disk space required for (power-)users doing guix builds will be a great concern to anyone, and I agree with @davidgumberg that having as many steps in the guix workflow as possible be reproducible, is probably best.
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2706040342)
Concept ACK.
I don't think a few hundred MB more disk space required for (power-)users doing guix builds will be a great concern to anyone, and I agree with @davidgumberg that having as many steps in the guix workflow as possible be reproducible, is probably best.
💬 Sjors commented on pull request "ci: Do not try to install for fuzz builds":
(https://github.com/bitcoin/bitcoin/pull/32014#issuecomment-2706052847)
Where in the CMake code is '$GOAL' processed?
I see this in the CI code:
```sh
bash -c "cmake --build . $MAKEJOBS --target all $GOAL"
```
But after that I lost track.
(https://github.com/bitcoin/bitcoin/pull/32014#issuecomment-2706052847)
Where in the CMake code is '$GOAL' processed?
I see this in the CI code:
```sh
bash -c "cmake --build . $MAKEJOBS --target all $GOAL"
```
But after that I lost track.
💬 fanquake commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2706054855)
https://github.com/bitcoin/bitcoin/actions/runs/13718139296/job/38367326902?pr=32015#step:7:1603:
```bash
Run p2p_headers_presync with args ['/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/src/test/fuzz/fuzz', PosixPath('/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/p2p_headers_presync')]net_processing.cpp:295 SetTxRelay: Assertion `!m_tx_relay' failed.
Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_c
...
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2706054855)
https://github.com/bitcoin/bitcoin/actions/runs/13718139296/job/38367326902?pr=32015#step:7:1603:
```bash
Run p2p_headers_presync with args ['/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/src/test/fuzz/fuzz', PosixPath('/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/p2p_headers_presync')]net_processing.cpp:295 SetTxRelay: Assertion `!m_tx_relay' failed.
Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_c
...
💬 fanquake commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2706062430)
> No, the check is not rerun, [the result is cached](https://cmake.org/cmake/help/latest/module/CheckCXXSourceCompiles.html).
Sure, but from the perspective of the builder, if it's producing output from the check, they'll likely think it is being re-run, and shortly after they'll open an issue in our repo, asking why the build still doesn't work, even after they've fixed the missing dependency (given no guidance that the build directory should be removed).
I don't think it's useful for CMake t
...
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2706062430)
> No, the check is not rerun, [the result is cached](https://cmake.org/cmake/help/latest/module/CheckCXXSourceCompiles.html).
Sure, but from the perspective of the builder, if it's producing output from the check, they'll likely think it is being re-run, and shortly after they'll open an issue in our repo, asking why the build still doesn't work, even after they've fixed the missing dependency (given no guidance that the build directory should be removed).
I don't think it's useful for CMake t
...
💬 Sjors commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2706121101)
I would be useful if the PR describes any major intended changes.
It adds several new languages: ast_ES, ay, or, ps, sa, sm, tn, ve, xh, yi
It almost completely drops Dutch, Czech, Danish and perhaps others (Github can barely load the diff).
---
It would be great if, in the future, the strings can have canonical ordering, so the XML files are easier to diff (without going to Transifex)
```
git show 5a52e0946d399fa467073c44752a5ce68b2a9008 src/qt/locale/bitcoin_de.ts
```
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2706121101)
I would be useful if the PR describes any major intended changes.
It adds several new languages: ast_ES, ay, or, ps, sa, sm, tn, ve, xh, yi
It almost completely drops Dutch, Czech, Danish and perhaps others (Github can barely load the diff).
---
It would be great if, in the future, the strings can have canonical ordering, so the XML files are easier to diff (without going to Transifex)
```
git show 5a52e0946d399fa467073c44752a5ce68b2a9008 src/qt/locale/bitcoin_de.ts
```
👍 pablomartin4btc approved a pull request: "qt: 29.0 translations update"
(https://github.com/bitcoin/bitcoin/pull/32004#pullrequestreview-2666901356)
tACK 5a52e0946d399fa467073c44752a5ce68b2a9008
Tested sync'ing `master` ([doc](https://github.com/bitcoin/bitcoin/blob/master/doc/translation_process.md#synchronising-translations)) and generating the `translate` ([doc](https://github.com/bitcoin/bitcoin/blob/master/doc/translation_process.md#writing-code-with-translations)) from both `master` and this PR.
No diffs.
(https://github.com/bitcoin/bitcoin/pull/32004#pullrequestreview-2666901356)
tACK 5a52e0946d399fa467073c44752a5ce68b2a9008
Tested sync'ing `master` ([doc](https://github.com/bitcoin/bitcoin/blob/master/doc/translation_process.md#synchronising-translations)) and generating the `translate` ([doc](https://github.com/bitcoin/bitcoin/blob/master/doc/translation_process.md#writing-code-with-translations)) from both `master` and this PR.
No diffs.
💬 dergoegge commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1984875189)
Now that `~CNode` is responsible for calling `FinalizeNode`, the guarantees around which thread does the actual cleanup are lost. It's basically just whichever thread ends up holding the last instance of a `CNode`. This seems bug prone.
E.g. there are no guarantees (afaict) that `disconnected_nodes` will actually be the last owner.
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1984875189)
Now that `~CNode` is responsible for calling `FinalizeNode`, the guarantees around which thread does the actual cleanup are lost. It's basically just whichever thread ends up holding the last instance of a `CNode`. This seems bug prone.
E.g. there are no guarantees (afaict) that `disconnected_nodes` will actually be the last owner.
💬 moonsettler commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2706175674)
We are going to activate CTV I don't think that's a question. The question is will core merge it in before or after lock-in?
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2706175674)
We are going to activate CTV I don't think that's a question. The question is will core merge it in before or after lock-in?
💬 hebasto commented on pull request "ci: Do not try to install for fuzz builds":
(https://github.com/bitcoin/bitcoin/pull/32014#discussion_r1984884920)
> Why does this need to change? The regular msan job is not broken afaik.
Right. It wasn't my intention. Reverted.
(https://github.com/bitcoin/bitcoin/pull/32014#discussion_r1984884920)
> Why does this need to change? The regular msan job is not broken afaik.
Right. It wasn't my intention. Reverted.
💬 hebasto commented on pull request "ci: Do not try to install for fuzz builds":
(https://github.com/bitcoin/bitcoin/pull/32014#issuecomment-2706182124)
> Where in the CMake code is `$GOAL` processed?
>
> I see this in the CI code:
>
> ```shell
> bash -c "cmake --build . $MAKEJOBS --target all $GOAL"
> ```
>
> But after that I lost track.
This is the only place.
(https://github.com/bitcoin/bitcoin/pull/32014#issuecomment-2706182124)
> Where in the CMake code is `$GOAL` processed?
>
> I see this in the CI code:
>
> ```shell
> bash -c "cmake --build . $MAKEJOBS --target all $GOAL"
> ```
>
> But after that I lost track.
This is the only place.
💬 marcofleon commented on pull request "ci: Do not try to install for fuzz builds":
(https://github.com/bitcoin/bitcoin/pull/32014#issuecomment-2706183177)
> Is it possible to open a PR to the QA repo that demonstrates that this actually fixes the problem?
Trying it here:
https://github.com/bitcoin-core/qa-assets/pull/219
(https://github.com/bitcoin/bitcoin/pull/32014#issuecomment-2706183177)
> Is it possible to open a PR to the QA repo that demonstrates that this actually fixes the problem?
Trying it here:
https://github.com/bitcoin-core/qa-assets/pull/219
💬 michaelfolkson commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2706189931)
> We are going to activate CTV I don't think that's a question. The question is will core merge it in before or after lock-in?
If you don't care about the Bitcoin Core review process, community consensus and causing chain splits probably better you close this PR and attempt to cause chaos in a different repo.
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2706189931)
> We are going to activate CTV I don't think that's a question. The question is will core merge it in before or after lock-in?
If you don't care about the Bitcoin Core review process, community consensus and causing chain splits probably better you close this PR and attempt to cause chaos in a different repo.
⚠️ fanquake opened an issue: "cmake: `makenisis` isn't checked-for before use"
(https://github.com/bitcoin/bitcoin/issues/32018)
Autotools would warn that we wouldn't be able to build the installer if it was missing. Currently, we don't check if it's available at all, and just fail when we first try to use it. This differs from the macOS deploy target, where we do check that `zip` is avilable, and fail to configure if it isn't.
(https://github.com/bitcoin/bitcoin/issues/32018)
Autotools would warn that we wouldn't be able to build the installer if it was missing. Currently, we don't check if it's available at all, and just fail when we first try to use it. This differs from the macOS deploy target, where we do check that `zip` is avilable, and fail to configure if it isn't.
💬 fanquake commented on pull request "depends: set `CMAKE_*_COMPILER_TARGET` in toolchain":
(https://github.com/bitcoin/bitcoin/pull/31849#issuecomment-2706241283)
> For example:
I'm not entirely convinced, given this isn't generic. If duplicate flags is an issue, then we should be doing the same for duplication that already exists (in non-verbose builds), not just for this specific case.
(https://github.com/bitcoin/bitcoin/pull/31849#issuecomment-2706241283)
> For example:
I'm not entirely convinced, given this isn't generic. If duplicate flags is an issue, then we should be doing the same for duplication that already exists (in non-verbose builds), not just for this specific case.
💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2706246199)
> I would be useful if the PR describes any major intended changes.
>
> It adds several new languages: ast_ES, ay, or, ps, sa, sm, tn, ve, xh, yi
>
> It almost completely drops Dutch, Czech, Danish and perhaps others (Github can barely load the diff).
As a PR author, I took 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, w
...
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2706246199)
> I would be useful if the PR describes any major intended changes.
>
> It adds several new languages: ast_ES, ay, or, ps, sa, sm, tn, ve, xh, yi
>
> It almost completely drops Dutch, Czech, Danish and perhaps others (Github can barely load the diff).
As a PR author, I took 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, w
...
💬 fanquake commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1984932418)
This is pulling back in the same spam from #30897.
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1984932418)
This is pulling back in the same spam from #30897.