💬 darosior commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1955143126)
Why? If we are interrupted the `while()` clause above will continue here and we won't tell the router to remove the mappings and just return instead.
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1955143126)
Why? If we are interrupted the `while()` clause above will continue here and we won't tell the router to remove the mappings and just return instead.
💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1955147830)
This still needs `LDFLAGS ${SANITIZER_LDFLAGS}` as before in order for the check to pass, so I think the `sanitizer_interface` changes above should be undone.
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1955147830)
This still needs `LDFLAGS ${SANITIZER_LDFLAGS}` as before in order for the check to pass, so I think the `sanitizer_interface` changes above should be undone.
💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2657611235)
@jonasschnelli Good point. Yes. A BIP159 node MUST be able to serve at least the latest 288 blocks, but SHOULD not serve more. Will update.
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2657611235)
@jonasschnelli Good point. Yes. A BIP159 node MUST be able to serve at least the latest 288 blocks, but SHOULD not serve more. Will update.
💬 darosior commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2657611766)
Thanks for the upload, i'll test the Mac ARM this weekend.
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2657611766)
Thanks for the upload, i'll test the Mac ARM this weekend.
💬 murchandamus commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2657614810)
Nice catch, @furszy. Thanks!
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2657614810)
Nice catch, @furszy. Thanks!
👍 hodlinator approved a pull request: "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2616050635)
re-ACK a85e8c0e6158fad2408bda5cb1e36da707eb081b
Resolved trivial conflict with #31767 since [my other ACK](https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2546259926).
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2616050635)
re-ACK a85e8c0e6158fad2408bda5cb1e36da707eb081b
Resolved trivial conflict with #31767 since [my other ACK](https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2546259926).
💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1955153140)
GoogleTest which seems like a slightly more desirable framework than Boost Unit Tests also uses [`ASSERT_*`-style](https://google.github.io/googletest/reference/assertions.html) statements I realize, so maybe keeping with that term is good in the long run.
What we're missing may be generic `assert_true(x)` & others that can be used where the special Python `assert x` feature is a bad fit.
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1955153140)
GoogleTest which seems like a slightly more desirable framework than Boost Unit Tests also uses [`ASSERT_*`-style](https://google.github.io/googletest/reference/assertions.html) statements I realize, so maybe keeping with that term is good in the long run.
What we're missing may be generic `assert_true(x)` & others that can be used where the special Python `assert x` feature is a bad fit.
💬 theuni commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2657633117)
Summarizing:
Ignoring the current impl details, the main question to be answered here is: Do we consider `-O3` to be harmful/dangerous. A ~decade ago it was commonly accepted that compiler bugs/UB were more likely to be exposed at that level, but I'm not sure how relevant that is these days.
Historically we've defaulted everything to `-O2`, but if a user set their CXXFLAGS to `-O3` then that's what they'd get. It would've been intentional.
It's a bit different with CMake because the default s
...
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2657633117)
Summarizing:
Ignoring the current impl details, the main question to be answered here is: Do we consider `-O3` to be harmful/dangerous. A ~decade ago it was commonly accepted that compiler bugs/UB were more likely to be exposed at that level, but I'm not sure how relevant that is these days.
Historically we've defaulted everything to `-O2`, but if a user set their CXXFLAGS to `-O3` then that's what they'd get. It would've been intentional.
It's a bit different with CMake because the default s
...
👍 willcl-ark approved a pull request: "cmake: Improve compatibility with Python version managers"
(https://github.com/bitcoin/bitcoin/pull/31421#pullrequestreview-2616146538)
ACK dead9086543671b07e6ce041821e4d2a6627075b
Tested with `uv` and `pyenv` on linux and macOS.
(https://github.com/bitcoin/bitcoin/pull/31421#pullrequestreview-2616146538)
ACK dead9086543671b07e6ce041821e4d2a6627075b
Tested with `uv` and `pyenv` on linux and macOS.
💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955172547)
It was a left-over of a previous approach and I left it in initially in case it might be interesting for testing, experiments etc. just because it was already there. But the PR has grown in scope a lot and it's becoming annoying to deal with for me and reviewers, so I dropped it now.
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955172547)
It was a left-over of a previous approach and I left it in initially in case it might be interesting for testing, experiments etc. just because it was already there. But the PR has grown in scope a lot and it's becoming annoying to deal with for me and reviewers, so I dropped it now.
💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955172814)
done
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955172814)
done
💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955180588)
This is neat but I found it to be not as straight forward as it seemed initially. Aside from the additional include I found that I needed to bump `template-depth` and `constexpr-steps` in order to make this work at compile time with `clang`. I am still a bit unclear on what the trade-offs are for this and I will open a separate PR with this so that this discussion doesn't extend the scope here further.
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955180588)
This is neat but I found it to be not as straight forward as it seemed initially. Aside from the additional include I found that I needed to bump `template-depth` and `constexpr-steps` in order to make this work at compile time with `clang`. I am still a bit unclear on what the trade-offs are for this and I will open a separate PR with this so that this discussion doesn't extend the scope here further.
💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2657659979)
Addressed @sipa 's feedback and rebased on the latest version of #30901.
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2657659979)
Addressed @sipa 's feedback and rebased on the latest version of #30901.
💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1955183145)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1955183145)
Fixed.
💬 fjahr commented on pull request "cmake: Revamp handling of data files":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2657674233)
tACK 18cc0a55595f9dc1f2e561743201a05754996b64
Again, I am not an expert on cmake but I don't see any downsides in the latest changes and I can confirm it works. I am using this latest version in #28792 now. And I find the change on function names and args is making things a bit nicer.
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2657674233)
tACK 18cc0a55595f9dc1f2e561743201a05754996b64
Again, I am not an expert on cmake but I don't see any downsides in the latest changes and I can confirm it works. I am using this latest version in #28792 now. And I find the change on function names and args is making things a bit nicer.
💬 hebasto commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1955190524)
Would you mind elaborating on your suggestion?
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1955190524)
Would you mind elaborating on your suggestion?
💬 achow101 commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1955192539)
This function basically duplicates `GetRNDRRS()`; it should instead be refactored to avoid duplicating.
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1955192539)
This function basically duplicates `GetRNDRRS()`; it should instead be refactored to avoid duplicating.
📝 TheCharlatan opened a pull request: "init: Take lock on blocks directory in BlockManager ctor"
(https://github.com/bitcoin/bitcoin/pull/31860)
This ensures consistent directory locking for users of the kernel library by disallowing mistakenly creating multiple BlockManagers that might write into the same block or undo files.
The change here makes `LockDirectory` more strict: It now returns an error even if it is called again on the same directory from the same process. However from the description in https://github.com/bitcoin/bitcoin/pull/12422 , where this feature was introduced, it is not immediately clear what its usage was at t
...
(https://github.com/bitcoin/bitcoin/pull/31860)
This ensures consistent directory locking for users of the kernel library by disallowing mistakenly creating multiple BlockManagers that might write into the same block or undo files.
The change here makes `LockDirectory` more strict: It now returns an error even if it is called again on the same directory from the same process. However from the description in https://github.com/bitcoin/bitcoin/pull/12422 , where this feature was introduced, it is not immediately clear what its usage was at t
...
💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955196258)
Yeah, given that the file isn't committed its size doesn't matter that much. And it may be that `constexpr` evaluating a huge amount of data is non-trivial for the compiler, unsure. This isn't that important.
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955196258)
Yeah, given that the file isn't committed its size doesn't matter that much. And it may be that `constexpr` evaluating a huge amount of data is non-trivial for the compiler, unsure. This isn't that important.
📝 fjahr opened a pull request: "RFC: Generated headers with ""_hex user-defined literal"
(https://github.com/bitcoin/bitcoin/pull/31861)
This is based on a [suggestion by sipa](https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1934116502) in #28792.
The generated header could be a lot smaller when using the ""_hex user-defined literal defined in `util/strencodings.h`. Primarily this would be nice for an embedded asmap file but since we already generate these headers it's a conversation that doesn't have to be tied to the asmap PR.
I am looking for feedback on:
- Do people take issue with including `util/strencoding
...
(https://github.com/bitcoin/bitcoin/pull/31861)
This is based on a [suggestion by sipa](https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1934116502) in #28792.
The generated header could be a lot smaller when using the ""_hex user-defined literal defined in `util/strencodings.h`. Primarily this would be nice for an embedded asmap file but since we already generate these headers it's a conversation that doesn't have to be tied to the asmap PR.
I am looking for feedback on:
- Do people take issue with including `util/strencoding
...