💬 fanquake commented on issue "build: cmake --install fails after --target deploy":
(https://github.com/bitcoin/bitcoin/issues/32007#issuecomment-2705974429)
Note that this isn't Windows specific. The same happens for macOS build.
(https://github.com/bitcoin/bitcoin/issues/32007#issuecomment-2705974429)
Note that this isn't Windows specific. The same happens for macOS build.
💬 hodlinator commented on pull request "miniscript: fixes #29098 by only use first k valid signatures":
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1984747128)
Thanks! Although you are still inserting the loop in the middle of the old comment. How about this? (Or putting the `nsat_return`-block with comment last, not sure what makes most sense).
```C++
// for nsat_return, it expects node.keys.size() number of ZEROs, thus adding this for loop
InputStack nsat_return;
for (size_t i = 0; i < node.keys.size(); ++i) {
nsat_return = std::move(nsat_return) + ZERO;
}
auto& nsat{nsat_return};
// The dissatisfaction consists of as many empty vectors a
...
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1984747128)
Thanks! Although you are still inserting the loop in the middle of the old comment. How about this? (Or putting the `nsat_return`-block with comment last, not sure what makes most sense).
```C++
// for nsat_return, it expects node.keys.size() number of ZEROs, thus adding this for loop
InputStack nsat_return;
for (size_t i = 0; i < node.keys.size(); ++i) {
nsat_return = std::move(nsat_return) + ZERO;
}
auto& nsat{nsat_return};
// The dissatisfaction consists of as many empty vectors a
...
💬 Sjors commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2705984429)
Should we just add a release note about this issue? IIUC it's a regression in our move to cmake, but I suspect that anyone who actually wants to use `-O3` can use master (once it's fixed) or will be happy to review a backport.
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2705984429)
Should we just add a release note about this issue? IIUC it's a regression in our move to cmake, but I suspect that anyone who actually wants to use `-O3` can use master (once it's fixed) or will be happy to review a backport.
💬 Sjors commented on issue "build: `-static-pie` builds no-longer working with CMake":
(https://github.com/bitcoin/bitcoin/issues/31843#issuecomment-2705993383)
Same suggestion as https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2705984429 to document this as a known issue in release notes.
(https://github.com/bitcoin/bitcoin/issues/31843#issuecomment-2705993383)
Same suggestion as https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2705984429 to document this as a known issue in release notes.
📝 vasild opened a pull request: "net: replace manual reference counting of CNode with shared_ptr"
(https://github.com/bitcoin/bitcoin/pull/32015)
Before this change the code used to count references to `CNode` objects manually via `CNode::nRefCount`. Unneeded `CNode`s were scheduled for deletion by putting them in `CConnman::m_nodes_disconnected` and were deleted after their reference count reached zero. Deleting consists of calling `PeerManager::FinalizeNode()` and destroying the `CNode` object.
Replace this scheme with `std::shared_ptr`. This simplifies the code and removes:
`CNode::nRefCount`
`CNode::GetRefCount()`
`CNode::AddRef
...
(https://github.com/bitcoin/bitcoin/pull/32015)
Before this change the code used to count references to `CNode` objects manually via `CNode::nRefCount`. Unneeded `CNode`s were scheduled for deletion by putting them in `CConnman::m_nodes_disconnected` and were deleted after their reference count reached zero. Deleting consists of calling `PeerManager::FinalizeNode()` and destroying the `CNode` object.
Replace this scheme with `std::shared_ptr`. This simplifies the code and removes:
`CNode::nRefCount`
`CNode::GetRefCount()`
`CNode::AddRef
...
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2706012277)
cc @willcl-ark, @dergoegge, @theuni
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2706012277)
cc @willcl-ark, @dergoegge, @theuni
💬 Sjors commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2706014011)
If I understand the remaining criticism correctly, it seems best to merge this before branch-off and improve things later.
The number of users that will be confused if we delay this change until _after_ branch-off is potentially much larger than the number of "everyday developer" folks impacted by the silent breakage here. For people who build the release from source, it's better if we educate them about the cmake changes and the new binary locations at the same time.
> the silent failures
...
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2706014011)
If I understand the remaining criticism correctly, it seems best to merge this before branch-off and improve things later.
The number of users that will be confused if we delay this change until _after_ branch-off is potentially much larger than the number of "everyday developer" folks impacted by the silent breakage here. For people who build the release from source, it's better if we educate them about the cmake changes and the new binary locations at the same time.
> the silent failures
...
💬 michaelfolkson commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2706016378)
> I encourage more general, conceptual discussion to happen on [Delving Bitcoin](https://delvingbitcoin.org/) and not on this pull request.
> @jamesob I'd be happy to help you keep this thread focused by moderating off-topic comments and pointing them to the appropriate forums.
To state the obvious if you push "conceptual discussion" (a critical part of the Bitcoin Core review process for a PR) to external forums (why not an accompanying issue in this repo?) the consideration of whether to
...
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2706016378)
> I encourage more general, conceptual discussion to happen on [Delving Bitcoin](https://delvingbitcoin.org/) and not on this pull request.
> @jamesob I'd be happy to help you keep this thread focused by moderating off-topic comments and pointing them to the appropriate forums.
To state the obvious if you push "conceptual discussion" (a critical part of the Bitcoin Core review process for a PR) to external forums (why not an accompanying issue in this repo?) the consideration of whether to
...
💬 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?