💬 l0rinc commented on pull request "chainparams: remove *petertodd.net":
(https://github.com/bitcoin/bitcoin/pull/33730#issuecomment-3460872871)
NACK, seems like trolling, please provide proper motivation
(https://github.com/bitcoin/bitcoin/pull/33730#issuecomment-3460872871)
NACK, seems like trolling, please provide proper motivation
💬 pinheadmz commented on pull request "chainparams: remove *petertodd.net":
(https://github.com/bitcoin/bitcoin/pull/33730#issuecomment-3460874678)
This PR description is unacceptable, please expand it with technical details or references to specific, documented project policy.
(https://github.com/bitcoin/bitcoin/pull/33730#issuecomment-3460874678)
This PR description is unacceptable, please expand it with technical details or references to specific, documented project policy.
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2472541351)
Resolved this by rebasing on the merged manifest changes and dropped the entire change.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2472541351)
Resolved this by rebasing on the merged manifest changes and dropped the entire change.
💬 fanquake commented on pull request "[wip] [nomerge] [draft] 2510 msan zero":
(https://github.com/bitcoin/bitcoin/pull/33686#issuecomment-3460935528)
Close here now it's been added to the nightly repo (https://github.com/bitcoin/bitcoin/issues/33668#issuecomment-3458046048)?
(https://github.com/bitcoin/bitcoin/pull/33686#issuecomment-3460935528)
Close here now it's been added to the nightly repo (https://github.com/bitcoin/bitcoin/issues/33668#issuecomment-3458046048)?
💬 jlopp commented on pull request "chainparams: remove dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3460938455)
> The DNS seed results must consist exclusively of fairly selected and functioning Bitcoin nodes from the public network to the best of the operator's understanding and capability.
This may be considered more of a meta comment, but the practice of a DNS seed dropping peers based upon their useragent is problematic in my view.
A useragent by no means defines how a node operates; it's trivial to set one's node to advertise any arbitrary useragent. If a DNS seed only accepts specific "approve
...
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3460938455)
> The DNS seed results must consist exclusively of fairly selected and functioning Bitcoin nodes from the public network to the best of the operator's understanding and capability.
This may be considered more of a meta comment, but the practice of a DNS seed dropping peers based upon their useragent is problematic in my view.
A useragent by no means defines how a node operates; it's trivial to set one's node to advertise any arbitrary useragent. If a DNS seed only accepts specific "approve
...
⚠️ dergoegge opened an issue: "RFC: Do we want to support fuzzing on MacOS?"
(https://github.com/bitcoin/bitcoin/issues/33731)
Fuzzing on MacOS (i.e. actual fuzzing not just running the inputs through the `fuzz` binary) is known to be brittle and we've had plenty of issues reported to us showcasing this:
- https://github.com/bitcoin/bitcoin/issues/33667
- https://github.com/bitcoin/bitcoin/issues/32089
- https://github.com/bitcoin/bitcoin/issues/31049
- https://github.com/bitcoin/bitcoin/issues/27550
- https://github.com/bitcoin/bitcoin/issues/19789
The solution usually involves something along the lines of waiting fo
...
(https://github.com/bitcoin/bitcoin/issues/33731)
Fuzzing on MacOS (i.e. actual fuzzing not just running the inputs through the `fuzz` binary) is known to be brittle and we've had plenty of issues reported to us showcasing this:
- https://github.com/bitcoin/bitcoin/issues/33667
- https://github.com/bitcoin/bitcoin/issues/32089
- https://github.com/bitcoin/bitcoin/issues/31049
- https://github.com/bitcoin/bitcoin/issues/27550
- https://github.com/bitcoin/bitcoin/issues/19789
The solution usually involves something along the lines of waiting fo
...
💬 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.