💬 Juma-creator commented on issue "build: `-static-pie` builds no-longer working with CMake":
(https://github.com/bitcoin/bitcoin/issues/31843#issuecomment-2705905947)
. **Replace Project Name and Version:**
- Change `project(MyProject VERSION 1.0)` to your project's name and version.
- Example:
```cmake
project(YourProjectName VERSION 2.0)
```
2. **Specify C++ Standard:**
- Adjust the C++ standard if necessary.
- Example:
```cmake
set(CMAKE_CXX_STANDARD 20)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
```
3. **Organize Project Structure:**
- Adjust the directory structure to match your project. Ensure your source fil
...
(https://github.com/bitcoin/bitcoin/issues/31843#issuecomment-2705905947)
. **Replace Project Name and Version:**
- Change `project(MyProject VERSION 1.0)` to your project's name and version.
- Example:
```cmake
project(YourProjectName VERSION 2.0)
```
2. **Specify C++ Standard:**
- Adjust the C++ standard if necessary.
- Example:
```cmake
set(CMAKE_CXX_STANDARD 20)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
```
3. **Organize Project Structure:**
- Adjust the directory structure to match your project. Ensure your source fil
...
💬 fanquake commented on issue "build: document `BITCOIN_GENBUILD_NO_GIT` environment variable?":
(https://github.com/bitcoin/bitcoin/issues/31999#issuecomment-2705911816)
> Gentoo, I assume: https://bugs.gentoo.org/476294.
Reading that thread, it's not really clear. The last comment (7 years ago) suggests that there was no issue with 0.13.0, even though the fix from https://github.com/bitcoin/bitcoin/pull/7522 only went into 0.15.0? It'd be good if someone could demonstrate an issue today, with the current source code.
(https://github.com/bitcoin/bitcoin/issues/31999#issuecomment-2705911816)
> Gentoo, I assume: https://bugs.gentoo.org/476294.
Reading that thread, it's not really clear. The last comment (7 years ago) suggests that there was no issue with 0.13.0, even though the fix from https://github.com/bitcoin/bitcoin/pull/7522 only went into 0.15.0? It'd be good if someone could demonstrate an issue today, with the current source code.
💬 fanquake commented on issue "build: `-static-pie` builds no-longer working with CMake":
(https://github.com/bitcoin/bitcoin/issues/31843#issuecomment-2705919305)
> introducing a dedicated build option, such as ENABLE_STATIC_PIE, if that's acceptable.
I don't think so. We shouldn't have to introduce our own options, to map to compiler options, to workaround CMake issues. This seems like something that needs to be solved in CMake itself.
(https://github.com/bitcoin/bitcoin/issues/31843#issuecomment-2705919305)
> introducing a dedicated build option, such as ENABLE_STATIC_PIE, if that's acceptable.
I don't think so. We shouldn't have to introduce our own options, to map to compiler options, to workaround CMake issues. This seems like something that needs to be solved in CMake itself.
💬 Sjors commented on pull request "Remove Taproot activation height":
(https://github.com/bitcoin/bitcoin/pull/26201#discussion_r1984719254)
Done
(https://github.com/bitcoin/bitcoin/pull/26201#discussion_r1984719254)
Done
💬 dergoegge commented on pull request "ci: Do not try to install for fuzz builds":
(https://github.com/bitcoin/bitcoin/pull/32014#discussion_r1984723062)
Why does this need to change? The regular msan job is not broken afaik.
(https://github.com/bitcoin/bitcoin/pull/32014#discussion_r1984723062)
Why does this need to change? The regular msan job is not broken afaik.
💬 fanquake commented on pull request "Implement OP_CHECKSIGFROMSTACK(VERIFY)":
(https://github.com/bitcoin/bitcoin/pull/29270#issuecomment-2705951177)
@reardencode It's no-longer possible to re-open:
You'll need to open a new PR.
(https://github.com/bitcoin/bitcoin/pull/29270#issuecomment-2705951177)
@reardencode It's no-longer possible to re-open:
You'll need to open a new PR.
💬 Sjors commented on issue "CI: Cmake warnings should be errors":
(https://github.com/bitcoin/bitcoin/issues/31476#issuecomment-2705965427)
This doesn't seem release blocking to me.
The only open PR that points to this issue is #31665 which isn't marked for v29.
(https://github.com/bitcoin/bitcoin/issues/31476#issuecomment-2705965427)
This doesn't seem release blocking to me.
The only open PR that points to this issue is #31665 which isn't marked for v29.
💬 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.