💬 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.
💬 glozow commented on pull request "chainparams: remove dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3461383230)
> I'd highly suggest waiting until @luke-jr can comment and weigh in on this directly before even considering hastily merging this.
We have previously given operators time to respond when we noticed problems with their DNS seed. I'm not sure what is giving people the impression that we're not doing the same here; Luke has been pinged both on this PR and directly through other platforms.
@luke-jr please update us here instead of on twitter.
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3461383230)
> I'd highly suggest waiting until @luke-jr can comment and weigh in on this directly before even considering hastily merging this.
We have previously given operators time to respond when we noticed problems with their DNS seed. I'm not sure what is giving people the impression that we're not doing the same here; Luke has been pinged both on this PR and directly through other platforms.
@luke-jr please update us here instead of on twitter.
💬 maflcko commented on pull request "ci: run native fuzz with MSAN job":
(https://github.com/bitcoin/bitcoin/pull/33626#discussion_r2432198820)
```suggestion
cirrus-runner: 'ghcr.io/cirruslabs/ubuntu-runner-amd64:24.04-md'
```
I wonder if md gives a speedup, similar to the valgrind task? https://github.com/bitcoin/bitcoin/pull/33461#discussion_r2451180232
Also on GHA runners, the timeout is short a few minutes of being hit?
(https://github.com/bitcoin/bitcoin/pull/33626#discussion_r2432198820)
```suggestion
cirrus-runner: 'ghcr.io/cirruslabs/ubuntu-runner-amd64:24.04-md'
```
I wonder if md gives a speedup, similar to the valgrind task? https://github.com/bitcoin/bitcoin/pull/33461#discussion_r2451180232
Also on GHA runners, the timeout is short a few minutes of being hit?