💬 sipa commented on issue "Consensus code without testing":
(https://github.com/bitcoin/bitcoin/issues/32156#issuecomment-2759549868)
That PR is not a consensus change.
(https://github.com/bitcoin/bitcoin/issues/32156#issuecomment-2759549868)
That PR is not a consensus change.
✅ maflcko closed an issue: "Consensus code without testing"
(https://github.com/bitcoin/bitcoin/issues/32156)
(https://github.com/bitcoin/bitcoin/issues/32156)
💬 maflcko commented on issue "Consensus code without testing":
(https://github.com/bitcoin/bitcoin/issues/32156#issuecomment-2759550054)
The pull request was opened two hours ago. It would be surprising if anyone looked at it, let alone review and test. So it not being reviewed/tested/merged is normal and expected and there is no need to open an issue about it.
(https://github.com/bitcoin/bitcoin/issues/32156#issuecomment-2759550054)
The pull request was opened two hours ago. It would be surprising if anyone looked at it, let alone review and test. So it not being reviewed/tested/merged is normal and expected and there is no need to open an issue about it.
💬 brunoerg commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2759560870)
I think we have two different issues here: @l0rinc is getting "libc++abi: terminating due to uncaught exception of type std::__1::ios_base::failure: DataStream::read(): end of data: unspecified iostream_category error". Me and @fjahr were getting the `AddressSanitizer: container-overflow` one. For both nosan should work, but I think that the container-overflow is a false positive so might be good to document the `ASAN_OPTIONS=detect_container_overflow=0`.
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2759560870)
I think we have two different issues here: @l0rinc is getting "libc++abi: terminating due to uncaught exception of type std::__1::ios_base::failure: DataStream::read(): end of data: unspecified iostream_category error". Me and @fjahr were getting the `AddressSanitizer: container-overflow` one. For both nosan should work, but I think that the container-overflow is a false positive so might be good to document the `ASAN_OPTIONS=detect_container_overflow=0`.
💬 1440000bytes commented on issue "Consensus code without testing":
(https://github.com/bitcoin/bitcoin/issues/32156#issuecomment-2759562121)
> It would be surprising if anyone looked at it, let alone review and test. So it not being reviewed/tested/merged is normal and expected and there is no need to open an issue about it.
I am the only bitcoin core contributor who cannot comment tin the pull request. So it makes sense for me to create a new issue.
I might do it in future as well.
(https://github.com/bitcoin/bitcoin/issues/32156#issuecomment-2759562121)
> It would be surprising if anyone looked at it, let alone review and test. So it not being reviewed/tested/merged is normal and expected and there is no need to open an issue about it.
I am the only bitcoin core contributor who cannot comment tin the pull request. So it makes sense for me to create a new issue.
I might do it in future as well.
💬 l0rinc commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2759566670)
> might be good to document the ASAN_OPTIONS=detect_container_overflow=0
Already pushed, please review the changes.
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2759566670)
> might be good to document the ASAN_OPTIONS=detect_container_overflow=0
Already pushed, please review the changes.
💬 fjahr commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2759567984)
Yeah, I do still get the error that @l0rinc sees when I test `psbt_base64_decode`. When I test `mini_miner_selection` which I am working on then it is fixed with the ASAN option with and without corpus.
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2759567984)
Yeah, I do still get the error that @l0rinc sees when I test `psbt_base64_decode`. When I test `mini_miner_selection` which I am working on then it is fixed with the ASAN option with and without corpus.
💬 l0rinc commented on issue "Consensus code without testing":
(https://github.com/bitcoin/bitcoin/issues/32156#issuecomment-2759577547)
> I might do it in future as well
Try finding a way that isn't so hostile, not everything's an attack...
(https://github.com/bitcoin/bitcoin/issues/32156#issuecomment-2759577547)
> I might do it in future as well
Try finding a way that isn't so hostile, not everything's an attack...
🤔 fjahr reviewed a pull request: "doc: document workaround and fallback for macOS fuzzing"
(https://github.com/bitcoin/bitcoin/pull/32084#pullrequestreview-2723470081)
Left some comments but I am still ~0 on this change and would much rather prefer that someone tries to spend more time on investigating the issue. I tried for a bit but wasn't successful so far.
(https://github.com/bitcoin/bitcoin/pull/32084#pullrequestreview-2723470081)
Left some comments but I am still ~0 on this change and would much rather prefer that someone tries to spend more time on investigating the issue. I tried for a bit but wasn't successful so far.
💬 fjahr commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2017655643)
Why remove the `brew` hint here? In other places the examples still include `brew` references.
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2017655643)
Why remove the `brew` hint here? In other places the examples still include `brew` references.
💬 fjahr commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2017656659)
Do I see correctly that the content hasn't chnaged here? It's annoying to review this reformatting when it doesn't have any effect.
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2017656659)
Do I see correctly that the content hasn't chnaged here? It's annoying to review this reformatting when it doesn't have any effect.
💬 fjahr commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2017662287)
I liked the old version better where it's only there in case you run into problems. There is no need to send people there preemptively. For example maybe the issues are fixed with the next llvm version or so. Then we would save users time with the old comment.
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2017662287)
I liked the old version better where it's only there in case you run into problems. There is no need to send people there preemptively. For example maybe the issues are fixed with the next llvm version or so. Then we would save users time with the old comment.
💬 hodlinator commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2017664612)
Would be more eager to ACK if code introduced by the PR followed bitcoin/test/functional/README.md as noted above. If you don't want to change the phrasing towards what's in the nit-part that's fine.
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2017664612)
Would be more eager to ACK if code introduced by the PR followed bitcoin/test/functional/README.md as noted above. If you don't want to change the phrasing towards what's in the nit-part that's fine.
💬 brunoerg commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2759614457)
If we're going to put `ASAN_OPTIONS=detect_container_overflow=0` into the documentation, I think it would be good to mention https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow#false-positives
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2759614457)
If we're going to put `ASAN_OPTIONS=detect_container_overflow=0` into the documentation, I think it would be good to mention https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow#false-positives
💬 l0rinc commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2017666023)
we're repeating the same a few lines below
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2017666023)
we're repeating the same a few lines below
💬 1440000bytes commented on issue "Consensus code without testing":
(https://github.com/bitcoin/bitcoin/issues/32156#issuecomment-2759627001)
Sorry @sipa @maflcko
(https://github.com/bitcoin/bitcoin/issues/32156#issuecomment-2759627001)
Sorry @sipa @maflcko
💬 l0rinc commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2017666745)
I've fixed a typo (`non-systems clang`) and rearranged the lines based on sentence structure
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2017666745)
I've fixed a typo (`non-systems clang`) and rearranged the lines based on sentence structure
💬 l0rinc commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2017668145)
I don't think we can run these instructions on a mac currently - can we? I always had to jump to the mac section.
> maybe the issues are fixed with the next llvm version or so
You mean we will be able to remove the special mac section in that case? We can remove the comment as well in that case, but I think we always have to jump over if on a mac - please correct me if I'm wrong.
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2017668145)
I don't think we can run these instructions on a mac currently - can we? I always had to jump to the mac section.
> maybe the issues are fixed with the next llvm version or so
You mean we will be able to remove the special mac section in that case? We can remove the comment as well in that case, but I think we always have to jump over if on a mac - please correct me if I'm wrong.
💬 EthanHeilman commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#discussion_r2017669534)
This approach is deliberate. My thinking here was that this wasn't a test failure, i.e. functionality isn't behaving as expected, but rather a failure in the tests themselves.
That said, as you have shown, the boost error alone is more helpful for finding the fault. I'll remove it.
(https://github.com/bitcoin/bitcoin/pull/31640#discussion_r2017669534)
This approach is deliberate. My thinking here was that this wasn't a test failure, i.e. functionality isn't behaving as expected, but rather a failure in the tests themselves.
That said, as you have shown, the boost error alone is more helpful for finding the fault. I'll remove it.
💬 l0rinc commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2759660152)
> it would be good to mention [Wiki: AddressSanitizerContainerOverflow (false positives) (google/sanitizers)](https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow#false-positives)
I've put this in the description already, but added it to the doc as well now
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2759660152)
> it would be good to mention [Wiki: AddressSanitizerContainerOverflow (false positives) (google/sanitizers)](https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow#false-positives)
I've put this in the description already, but added it to the doc as well now