💬 fanquake commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3461142954)
> 1. Keep the current approach and fix/document as issues are reported.
If we do do this, it should be clear what is supported (i.e just the latest version of LLVM/Clang shipped via brew, using `lld` to link?) , otherwise (as you metion above) we'll be documenting many different "workarounds" depending on the version of macOS, version of Apple clang/binutils install, if brew LLVM is being used, and it's version, using `ld64` to link, vs `lld`.
> I'd prefer option 2).
Concept ACK. Getting fuzz
...
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3461142954)
> 1. Keep the current approach and fix/document as issues are reported.
If we do do this, it should be clear what is supported (i.e just the latest version of LLVM/Clang shipped via brew, using `lld` to link?) , otherwise (as you metion above) we'll be documenting many different "workarounds" depending on the version of macOS, version of Apple clang/binutils install, if brew LLVM is being used, and it's version, using `ld64` to link, vs `lld`.
> I'd prefer option 2).
Concept ACK. Getting fuzz
...
💬 fanquake commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#discussion_r2472734365)
> we maintain our own package
If we are going to copy-paste this from Guix, and maintain it outself, seems like we should simplify it for our usage? i.e we know we are building winpthreads.
(https://github.com/bitcoin/bitcoin/pull/33593#discussion_r2472734365)
> we maintain our own package
If we are going to copy-paste this from Guix, and maintain it outself, seems like we should simplify it for our usage? i.e we know we are building winpthreads.
💬 fanquake commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#discussion_r2472736627)
Any reason to not use the v13.0.0 headers: https://www.mingw-w64.org/changelog/?
(https://github.com/bitcoin/bitcoin/pull/33593#discussion_r2472736627)
Any reason to not use the v13.0.0 headers: https://www.mingw-w64.org/changelog/?
💬 fanquake commented on pull request "guix: Build for macOS using Clang only":
(https://github.com/bitcoin/bitcoin/pull/32764#discussion_r2472750419)
> This can be dropped after commit 2877c75dc5db0dbf664fb6170d5754068e941d91.
Would be good for this to explain why (is libunwind causing build issues?). Could also mention that this is copy-pasted from Guix, and I assume the intention isn't to change it here, if we plan on dropping it?
(https://github.com/bitcoin/bitcoin/pull/32764#discussion_r2472750419)
> This can be dropped after commit 2877c75dc5db0dbf664fb6170d5754068e941d91.
Would be good for this to explain why (is libunwind causing build issues?). Could also mention that this is copy-pasted from Guix, and I assume the intention isn't to change it here, if we plan on dropping it?
💬 fanquake commented on pull request "guix: Build for macOS using Clang only":
(https://github.com/bitcoin/bitcoin/pull/32764#discussion_r2472753741)
> as some native tools
If we are re-introducing this, I think we can be explicit about which tools. Can we just build them statically to not need to do this?
(https://github.com/bitcoin/bitcoin/pull/32764#discussion_r2472753741)
> as some native tools
If we are re-introducing this, I think we can be explicit about which tools. Can we just build them statically to not need to do this?
📝 maflcko opened a pull request: "ci: Call docker exec from Python script to fix word splitting"
(https://github.com/bitcoin/bitcoin/pull/33732)
The remaining `ci/test/02_run_container.sh` is fine, but has a bunch of shellcheck SC2086 word splitting violations.
This is fine currently, because the only place that needed them had additional escaping, and all other commands happened to split fine on spaces.
However, this may change in the future. So fix it now, by rewriting it in Python, which is recommended in the dev notes.
(https://github.com/bitcoin/bitcoin/pull/33732)
The remaining `ci/test/02_run_container.sh` is fine, but has a bunch of shellcheck SC2086 word splitting violations.
This is fine currently, because the only place that needed them had additional escaping, and all other commands happened to split fine on spaces.
However, this may change in the future. So fix it now, by rewriting it in Python, which is recommended in the dev notes.
💬 Crypt-iQ commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3461199887)
I have a mac and prefer option 2 also. The fuzz tests usually break each macOS update and I don't think it makes sense to maintain documentation for it. I occasionally find it useful just to see that a harness is working, but I can fix issues on my own as they come up.
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3461199887)
I have a mac and prefer option 2 also. The fuzz tests usually break each macOS update and I don't think it makes sense to maintain documentation for it. I occasionally find it useful just to see that a harness is working, but I can fix issues on my own as they come up.
💬 marcofleon commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3461226122)
Concept ACK on option 2. I remember struggling on my mac when I first started fuzzing and the section we have in the docs didn't really help. Ended up just figuring it out on my own. These days pretty much all of my fuzzing happens on Linux.
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3461226122)
Concept ACK on option 2. I remember struggling on my mac when I first started fuzzing and the section we have in the docs didn't really help. Ended up just figuring it out on my own. These days pretty much all of my fuzzing happens on Linux.
🤔 stickies-v reviewed a pull request: "contrib: turn off compression of macOS SDK to fix determinism (across distros)"
(https://github.com/bitcoin/bitcoin/pull/32009#pullrequestreview-3393052598)
utACK 71b0f474eee3aceb8c17b9ffa7fa866ecb3a93af, Concept ACK on disabling compression to increase determinism.
71b0f474eee3aceb8c17b9ffa7fa866ecb3a93af seems like a strict win, no need to include what we don't use. I think conceptually 0a9b63b1f447bc31e6b56410f7c9c25abc7f5047 also makes sense: the increased size of the tarball should not be a blocked for increased determinism. However, on my machines (arm64 macOS 15.6 and x86_64 Debian Bookworm ) determinism seems to be _reduced_ without compr
...
(https://github.com/bitcoin/bitcoin/pull/32009#pullrequestreview-3393052598)
utACK 71b0f474eee3aceb8c17b9ffa7fa866ecb3a93af, Concept ACK on disabling compression to increase determinism.
71b0f474eee3aceb8c17b9ffa7fa866ecb3a93af seems like a strict win, no need to include what we don't use. I think conceptually 0a9b63b1f447bc31e6b56410f7c9c25abc7f5047 also makes sense: the increased size of the tarball should not be a blocked for increased determinism. However, on my machines (arm64 macOS 15.6 and x86_64 Debian Bookworm ) determinism seems to be _reduced_ without compr
...
💬 maflcko commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3461244223)
No strong opinion, but I don't mind having docs that just say they only works with a specific brew llvm version and macOS version, and have them updated when they break.
Though, saying that libFuzzer is not supported on macOS seems also fine.
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3461244223)
No strong opinion, but I don't mind having docs that just say they only works with a specific brew llvm version and macOS version, and have them updated when they break.
Though, saying that libFuzzer is not supported on macOS seems also fine.
💬 maflcko commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#issuecomment-3461248758)
Done in https://github.com/bitcoin/bitcoin/pull/33732
(https://github.com/bitcoin/bitcoin/pull/33677#issuecomment-3461248758)
Done in https://github.com/bitcoin/bitcoin/pull/33732
🤔 naiyoma reviewed a pull request: "wallet: Improve wallet creation error message with usage hint"
(https://github.com/bitcoin/bitcoin/pull/33124#pullrequestreview-3393091125)
I'm not sure this is necessary since the correct syntax can be found using `-help `
(https://github.com/bitcoin/bitcoin/pull/33124#pullrequestreview-3393091125)
I'm not sure this is necessary since the correct syntax can be found using `-help `
💬 Crypt-iQ commented on pull request "chainparams: remove dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3461280539)
crACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3461280539)
crACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
🤔 TheCharlatan reviewed a pull request: "test: Add bitcoin-chainstate test for assumeutxo functionality"
(https://github.com/bitcoin/bitcoin/pull/33728#pullrequestreview-3392815899)
Concept ACK
Consolidating the tests into the regtest case seems fine to me, especially since this is just a utility to showcase and doogfood some of the kernel work.
(https://github.com/bitcoin/bitcoin/pull/33728#pullrequestreview-3392815899)
Concept ACK
Consolidating the tests into the regtest case seems fine to me, especially since this is just a utility to showcase and doogfood some of the kernel work.
💬 TheCharlatan commented on pull request "test: Add bitcoin-chainstate test for assumeutxo functionality":
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2472836534)
It is weaker, yes, but we similarly guard in other situations. We also still have the assertion in ActivateSnapshot to guard this condition at a more critical point in time.
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2472836534)
It is weaker, yes, but we similarly guard in other situations. We also still have the assertion in ActivateSnapshot to guard this condition at a more critical point in time.
💬 TheCharlatan commented on pull request "test: Add bitcoin-chainstate test for assumeutxo functionality":
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2472614198)
Since this is an optional positional argument, I think I would drop the `-` prefix.
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2472614198)
Since this is an optional positional argument, I think I would drop the `-` prefix.
✅ maflcko closed a pull request: "wallet: Improve wallet creation error message with usage hint"
(https://github.com/bitcoin/bitcoin/pull/33124)
(https://github.com/bitcoin/bitcoin/pull/33124)
💬 maflcko commented on pull request "wallet: Improve wallet creation error message with usage hint":
(https://github.com/bitcoin/bitcoin/pull/33124#issuecomment-3461323619)
Yeah, tend to agree. One will have to read the full help anyway to understand it.
(https://github.com/bitcoin/bitcoin/pull/33124#issuecomment-3461323619)
Yeah, tend to agree. One will have to read the full help anyway to understand it.
✅ maflcko closed a pull request: "[wip] [nomerge] [draft] 2510 msan zero"
(https://github.com/bitcoin/bitcoin/pull/33686)
(https://github.com/bitcoin/bitcoin/pull/33686)
💬 maflcko commented on pull request "[wip] [nomerge] [draft] 2510 msan zero":
(https://github.com/bitcoin/bitcoin/pull/33686#issuecomment-3461341894)
yeah, closing for now. It is a bit faster with the fast machines from the CI here, but it isn't going to pass in 6 hours, even with them, so moving to dedicated hardware without timeouts seems the only option.
(https://github.com/bitcoin/bitcoin/pull/33686#issuecomment-3461341894)
yeah, closing for now. It is a bit faster with the fast machines from the CI here, but it isn't going to pass in 6 hours, even with them, so moving to dedicated hardware without timeouts seems the only option.