🚀 fanquake merged a pull request: "build: Remove manpages when making MacOS app"
(https://github.com/bitcoin/bitcoin/pull/32064)
(https://github.com/bitcoin/bitcoin/pull/32064)
💬 fanquake commented on pull request "contrib: Fix `gen-bitcoin-conf.sh`":
(https://github.com/bitcoin/bitcoin/pull/32049#issuecomment-2723294891)
Backported in #32062.
(https://github.com/bitcoin/bitcoin/pull/32049#issuecomment-2723294891)
Backported in #32062.
⚠️ achow101 reopened an issue: "v29.0 Testing"
(https://github.com/bitcoin/bitcoin/issues/32052)
Umbrella issue for 29.0 testing. Please help testing on a wide variety of supported platforms, as well as interaction with different software.
Let us know which version you tested on which operating system.
If you find an issue, please search Github for known issues first and then open a new Github issue.
This meta issue should not be used to report bugs, as a single thread makes it impossible to track more than one topic.
See [29.0 Release Notes Draft](https://github.com/bitcoin-core/bitcoi
...
(https://github.com/bitcoin/bitcoin/issues/32052)
Umbrella issue for 29.0 testing. Please help testing on a wide variety of supported platforms, as well as interaction with different software.
Let us know which version you tested on which operating system.
If you find an issue, please search Github for known issues first and then open a new Github issue.
This meta issue should not be used to report bugs, as a single thread makes it impossible to track more than one topic.
See [29.0 Release Notes Draft](https://github.com/bitcoin-core/bitcoi
...
💬 Prabhat1308 commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2723306164)
tACK [611151e](https://github.com/bitcoin/bitcoin/pull/32059/commits/611151eae75ed756437442a5d0356f525c73dd00)
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2723306164)
tACK [611151e](https://github.com/bitcoin/bitcoin/pull/32059/commits/611151eae75ed756437442a5d0356f525c73dd00)
🚀 fanquake merged a pull request: "Require sqlite when building the wallet"
(https://github.com/bitcoin/bitcoin/pull/31961)
(https://github.com/bitcoin/bitcoin/pull/31961)
💬 ZKcash-IrishGov commented on something "":
(https://github.com/bitcoin/bitcoin/commit/a24419f8bed5e1145ce171dbbdad957750585471#r153704902)
rbmwgtoj4z9npg9nu8wb9q4dejsy5js35oaicefzg3mbf3ncxzu815o7k9ecfagt6ozqp3rxx4g7a3etqajahm4w4j4cyy51sbe4c8zm
(https://github.com/bitcoin/bitcoin/commit/a24419f8bed5e1145ce171dbbdad957750585471#r153704902)
rbmwgtoj4z9npg9nu8wb9q4dejsy5js35oaicefzg3mbf3ncxzu815o7k9ecfagt6ozqp3rxx4g7a3etqajahm4w4j4cyy51sbe4c8zm
💬 fanquake commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1994664138)
Why do we need to specify platforms here, rather than just `__clang__` (especially if you're now going to add fallbacks)?
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1994664138)
Why do we need to specify platforms here, rather than just `__clang__` (especially if you're now going to add fallbacks)?
💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1994677857)
Added a bunch of checks to the unit test. The fuzz tester checks at a lower level (`ThresholdConditionCache` vs `VersionBitsCache`) so that's not really suitable.
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1994677857)
Added a bunch of checks to the unit test. The fuzz tester checks at a lower level (`ThresholdConditionCache` vs `VersionBitsCache`) so that's not really suitable.
💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1994678038)
Added.
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1994678038)
Added.
💬 maflcko commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2723812624)
> In PR [#31901](https://github.com/bitcoin/bitcoin/pull/31901), Coverage.cpp was introduced
For reference the cpp file was created in fa99c3b544b631cfe34d52fb5e71636aedb1b423, but the code was only moved. You can check this by observing it greyed out via `git show fa99c3b544b631cfe34d52fb5e71636aedb1b423 --color-moved=dimmed-zebra`.
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2723812624)
> In PR [#31901](https://github.com/bitcoin/bitcoin/pull/31901), Coverage.cpp was introduced
For reference the cpp file was created in fa99c3b544b631cfe34d52fb5e71636aedb1b423, but the code was only moved. You can check this by observing it greyed out via `git show fa99c3b544b631cfe34d52fb5e71636aedb1b423 --color-moved=dimmed-zebra`.
💬 maflcko commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1994984199)
in commit "refactor: Improve ResetCoverageCounters function with fallback implem...":
I don't think this is a "refactor" (non-behavior-change), given that CI fails before and passes after.
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1994984199)
in commit "refactor: Improve ResetCoverageCounters function with fallback implem...":
I don't think this is a "refactor" (non-behavior-change), given that CI fails before and passes after.
💬 maflcko commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1994984394)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1994984394)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1994997845)
Thanks and sorry for missing that bash/sh are obviously not used on Windows. I missed the `cmd` section when looking at the link.
Still, it feels a bit uncomfortable that we are using `cmd` on Windows in the CI, which will by default ignore intermediate exit codes.
Given that neither the pull request author, nor any reviewer caught this before someone found it by testing, doesn't increase confidence either.
Thus, I think we should somehow rule out this class of bug, or at least documen
...
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1994997845)
Thanks and sorry for missing that bash/sh are obviously not used on Windows. I missed the `cmd` section when looking at the link.
Still, it feels a bit uncomfortable that we are using `cmd` on Windows in the CI, which will by default ignore intermediate exit codes.
Given that neither the pull request author, nor any reviewer caught this before someone found it by testing, doesn't increase confidence either.
Thus, I think we should somehow rule out this class of bug, or at least documen
...
💬 maflcko commented on pull request "depends: remove `NO_HARDEN` option":
(https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2723888193)
> Would it make sense to also drop `ENABLE_HARDENING`? Currently set `OFF` in the `clang-tidy` ci job, but I [flipped it `ON`](https://github.com/davidgumberg/bitcoin/commit/ce79bea14ae1427395b7eddb0a0e1c21fa78516d) and ran the clang-tidy job locally and it seems to work. (But I have no context for why hardening was disabled in clang-tidy cc: @maflcko)
IIRC it was added in fab24f8c3540b6f1a128cb9d6812df6678472b8d, because I copy-pasted it from another CI task and forgot to remove it. I guess
...
(https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2723888193)
> Would it make sense to also drop `ENABLE_HARDENING`? Currently set `OFF` in the `clang-tidy` ci job, but I [flipped it `ON`](https://github.com/davidgumberg/bitcoin/commit/ce79bea14ae1427395b7eddb0a0e1c21fa78516d) and ran the clang-tidy job locally and it seems to work. (But I have no context for why hardening was disabled in clang-tidy cc: @maflcko)
IIRC it was added in fab24f8c3540b6f1a128cb9d6812df6678472b8d, because I copy-pasted it from another CI task and forgot to remove it. I guess
...
💬 maflcko commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2723935664)
No strong opinion, but it could make sense to reword the pull description. Otherwise, it is less clear what all the effects of this will be. Something like:
> This adds support for macOS to `ResetCoverageCounters`. `ResetCoverageCounters` is used by the unit tests in `g_rng_temp_path_init` to support the deterministic unit test tooling. It is also used in fuzz tests to completely suppress coverage from anything init-related.
> Refer to ... Readme on how to test this for deterministic unit
...
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2723935664)
No strong opinion, but it could make sense to reword the pull description. Otherwise, it is less clear what all the effects of this will be. Something like:
> This adds support for macOS to `ResetCoverageCounters`. `ResetCoverageCounters` is used by the unit tests in `g_rng_temp_path_init` to support the deterministic unit test tooling. It is also used in fuzz tests to completely suppress coverage from anything init-related.
> Refer to ... Readme on how to test this for deterministic unit
...
💬 janb84 commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995079532)
Rebased the cod and changed the PR description based on feedback , thank you for the review!
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995079532)
Rebased the cod and changed the PR description based on feedback , thank you for the review!
💬 janb84 commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995082229)
Good point, I have removed the check in the macro and rebased my commits. Thank you for the review.
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995082229)
Good point, I have removed the check in the macro and rebased my commits. Thank you for the review.
💬 janb84 commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995085087)
Thank you for the suggestion, have found a way to fix the code so that the linker does not break and the functionality is the same. Thank you for the review
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995085087)
Thank you for the suggestion, have found a way to fix the code so that the linker does not break and the functionality is the same. Thank you for the review
💬 maflcko commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1995088556)
This change makes the assert a no-op and turns it undefined behavior.
Previously, the `block.data` presence was asserted before the `for`-loop. Now, the for loop will run into UB when `block.data` is null. (It is true that the for-loop body still checks for the presence, but once UB has happened, it is not possible to recover from it)
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1995088556)
This change makes the assert a no-op and turns it undefined behavior.
Previously, the `block.data` presence was asserted before the `for`-loop. Now, the for loop will run into UB when `block.data` is null. (It is true that the for-loop body still checks for the presence, but once UB has happened, it is not possible to recover from it)
💬 Sjors commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#issuecomment-2723975436)
Rebased after #31649.
(https://github.com/bitcoin/bitcoin/pull/31974#issuecomment-2723975436)
Rebased after #31649.