💬 l0rinc commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556093636)
> This comment led me to remove the steps entirely
Hmmm, so what happened to "best effort"?
> which is partly what we're trying to avoid with this effort in the first place
I thought we were aiming to provide Mac users a way to have at least basic fuzzing support.
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556093636)
> This comment led me to remove the steps entirely
Hmmm, so what happened to "best effort"?
> which is partly what we're trying to avoid with this effort in the first place
I thought we were aiming to provide Mac users a way to have at least basic fuzzing support.
💬 plebhash commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3570541428)
that's good to know!
but do we have to explicitly call `destroy` or is it sufficient to drop the reference from memory on the client side?
from my understanding of capnp, I believe it should be sufficient to drop it from memory, but on the other hand there must be a reason for `destroy` to exist?
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3570541428)
that's good to know!
but do we have to explicitly call `destroy` or is it sufficient to drop the reference from memory on the client side?
from my understanding of capnp, I believe it should be sufficient to drop it from memory, but on the other hand there must be a reason for `destroy` to exist?
💬 maflcko commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556116259)
I think it is fine to keep the libfuzzer-nosan command, but no strong opinion. Seems like it works without the extra flags for most people, and any extra flags can be added in the future, if there is need to.
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556116259)
I think it is fine to keep the libfuzzer-nosan command, but no strong opinion. Seems like it works without the extra flags for most people, and any extra flags can be added in the future, if there is need to.
📝 psam21 opened a pull request: "net: Execute Discover() when bind=0.0.0.0 or :: is set"
(https://github.com/bitcoin/bitcoin/pull/33935)
Fixes #31293
When using `-bind=0.0.0.0:port` or `-bind=[::]:port` (or their `-whitebind` equivalents), the `Discover()` function was not being executed because `bind_on_any` was set to false when explicit bind addresses were provided.
This meant that nodes using explicit "any" bind addresses would not discover and advertise their own local addresses to peers, even though they were effectively listening on all interfaces.
This commit checks if any of the explicitly provided `-bind` or `-
...
(https://github.com/bitcoin/bitcoin/pull/33935)
Fixes #31293
When using `-bind=0.0.0.0:port` or `-bind=[::]:port` (or their `-whitebind` equivalents), the `Discover()` function was not being executed because `bind_on_any` was set to false when explicit bind addresses were provided.
This meant that nodes using explicit "any" bind addresses would not discover and advertise their own local addresses to peers, even though they were effectively listening on all interfaces.
This commit checks if any of the explicitly provided `-bind` or `-
...
💬 l0rinc commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556122623)
Wouldn't the version with extra flags work for even more people?
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556122623)
Wouldn't the version with extra flags work for even more people?
💬 marcofleon commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556129865)
This works on my mac M3:
```bash
cmake -B fuzzbuild \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_BUILD_TYPE=Debug \
-DBUILD_FOR_FUZZING=ON \
-DSANITIZERS=fuzzer
-DAPPEND_LDFLAGS=-Wl,-no_warn_duplicate_libraries
```
I remember copying this from the original documentation when we transitioned to cmake (before the presets). It seems like everyone has their own way to make it work, s
...
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556129865)
This works on my mac M3:
```bash
cmake -B fuzzbuild \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_BUILD_TYPE=Debug \
-DBUILD_FOR_FUZZING=ON \
-DSANITIZERS=fuzzer
-DAPPEND_LDFLAGS=-Wl,-no_warn_duplicate_libraries
```
I remember copying this from the original documentation when we transitioned to cmake (before the presets). It seems like everyone has their own way to make it work, s
...
📝 Sjors opened a pull request: "mining: pass missing context to createNewBlock() and checkBlock()"
(https://github.com/bitcoin/bitcoin/pull/33936)
Adding a `context` parameter ensures that these methods are run in their own thread and don't block other calls.
Additionally this is required for `destroy` handling to work.
This fixes a bug where we would not release any (stale) templates until the client disconnects. This was first observed in https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3568788623.
The issue can be confirmed running [sv2-tp](https://github.com/stratum-mining/sv2-tp) and noticing in the log that destr
...
(https://github.com/bitcoin/bitcoin/pull/33936)
Adding a `context` parameter ensures that these methods are run in their own thread and don't block other calls.
Additionally this is required for `destroy` handling to work.
This fixes a bug where we would not release any (stale) templates until the client disconnects. This was first observed in https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3568788623.
The issue can be confirmed running [sv2-tp](https://github.com/stratum-mining/sv2-tp) and noticing in the log that destr
...
💬 marcofleon commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556140203)
Although, I thought the point was to reduce the number of issues that were constantly being opened wrt to mac fuzzing. In that case, I think it would make the most sense to have no instructions at all, as anything we put there will invite future issues/PRs.
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556140203)
Although, I thought the point was to reduce the number of issues that were constantly being opened wrt to mac fuzzing. In that case, I think it would make the most sense to have no instructions at all, as anything we put there will invite future issues/PRs.
👍 TheCharlatan approved a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3500228951)
Re-ACK 78d6402458d7d14533444d893c989f0534a3896f
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3500228951)
Re-ACK 78d6402458d7d14533444d893c989f0534a3896f
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3570652179)
If the important but breaking change in #33936 lands, then I might as well add the `getCoinbaseTx()` deletion here. It would be still be a separate commit, because it's useful to demonstrate equivalence before deletion.
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3570652179)
If the important but breaking change in #33936 lands, then I might as well add the `getCoinbaseTx()` deletion here. It would be still be a separate commit, because it's useful to demonstrate equivalence before deletion.
💬 maflcko commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556196974)
Yeah, no strong opinion. I just thought that the fuzzer+sanitizer version never worked on recent macOS versions, so removing it makes sense. Though, the libfuzzer-nosan worked for almost everyone, so it seems fine to keep. Having to deal with a pull/issue for that every few months seems fine, but I don't really expect a flood of those.
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556196974)
Yeah, no strong opinion. I just thought that the fuzzer+sanitizer version never worked on recent macOS versions, so removing it makes sense. Though, the libfuzzer-nosan worked for almost everyone, so it seems fine to keep. Having to deal with a pull/issue for that every few months seems fine, but I don't really expect a flood of those.
✅ maflcko closed a pull request: "Update GitHub Actions"
(https://github.com/bitcoin/bitcoin/pull/33934)
(https://github.com/bitcoin/bitcoin/pull/33934)
💬 maflcko commented on pull request "Update GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/33934#issuecomment-3570673314)
thx, but you'll need to follow the contrib guidelines
(https://github.com/bitcoin/bitcoin/pull/33934#issuecomment-3570673314)
thx, but you'll need to follow the contrib guidelines
✅ maflcko closed a pull request: "net: Execute Discover() when bind=0.0.0.0 or :: is set"
(https://github.com/bitcoin/bitcoin/pull/33935)
(https://github.com/bitcoin/bitcoin/pull/33935)
💬 maflcko commented on pull request "net: Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/33935#issuecomment-3570682484)
Thx, but instead of opening a third (3rd) pull request to fix this, it would be better to review the first two and possibly explain why they are insufficient and why a third pull request is needed.
(https://github.com/bitcoin/bitcoin/pull/33935#issuecomment-3570682484)
Thx, but instead of opening a third (3rd) pull request to fix this, it would be better to review the first two and possibly explain why they are insufficient and why a third pull request is needed.
💬 l0rinc commented on pull request "fix assumevalid is ignored during reindex":
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3570695985)
I have checked if it fixes the problem I have noticed a year ago and now that https://github.com/bitcoin/bitcoin/pull/33336 landed it's even simpler to do a reproducer:
```bash
for COMMIT in 509dc91db143fe2ebb4e910680aca97ba62233b9 b59ca7602bf9efaad80b3465129685ee1d09a028; do
(echo ""; git fetch -q origin $COMMIT >/dev/null 2>&1 && git checkout -q $COMMIT && git log -1 --pretty='%h %s' || exit 1) && \
cmake -B build >/dev/null 2>&1 && cmake --build build -j$(nproc) >/dev/null 2>&1 && \
...
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3570695985)
I have checked if it fixes the problem I have noticed a year ago and now that https://github.com/bitcoin/bitcoin/pull/33336 landed it's even simpler to do a reproducer:
```bash
for COMMIT in 509dc91db143fe2ebb4e910680aca97ba62233b9 b59ca7602bf9efaad80b3465129685ee1d09a028; do
(echo ""; git fetch -q origin $COMMIT >/dev/null 2>&1 && git checkout -q $COMMIT && git log -1 --pretty='%h %s' || exit 1) && \
cmake -B build >/dev/null 2>&1 && cmake --build build -j$(nproc) >/dev/null 2>&1 && \
...
💬 Sammie05 commented on pull request "ci: Remove redundant busybox option":
(https://github.com/bitcoin/bitcoin/pull/33903#issuecomment-3570704778)
Concept ACK
Removing the USE_BUSYBOX option is appropriate.
(https://github.com/bitcoin/bitcoin/pull/33903#issuecomment-3570704778)
Concept ACK
Removing the USE_BUSYBOX option is appropriate.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556237729)
Ok, then I think it is best to leave it as it is.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556237729)
Ok, then I think it is best to leave it as it is.
📝 psam21 opened a pull request: "depends: Fix native_capnp to respect build_CC and build_CXX"
(https://github.com/bitcoin/bitcoin/pull/33937)
Fixes #33859
native_capnp was ignoring CC/CXX environment variables and always using the system default compiler. This caused build failures on systems with older GCC versions when users tried to specify a different compiler.
Added config_env settings in native_capnp.mk to properly use build_CC and build_CXX variables, following the same pattern used in native_qt.mk.
(https://github.com/bitcoin/bitcoin/pull/33937)
Fixes #33859
native_capnp was ignoring CC/CXX environment variables and always using the system default compiler. This caused build failures on systems with older GCC versions when users tried to specify a different compiler.
Added config_env settings in native_capnp.mk to properly use build_CC and build_CXX variables, following the same pattern used in native_qt.mk.
💬 l0rinc commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556264196)
> Although, I thought the point was to reduce the number of issues that were constantly being opened wrt to mac fuzzing
Wasn't the point to actually do a best effort of solving their problem instead of just silencing the issues?
> This works on my mac M3:
Can you show the exact command because this is missing at least a backslash.
As mentioned, having the latest dependencies and running:
```bash
cmake -B fuzzbuild \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMA
...
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556264196)
> Although, I thought the point was to reduce the number of issues that were constantly being opened wrt to mac fuzzing
Wasn't the point to actually do a best effort of solving their problem instead of just silencing the issues?
> This works on my mac M3:
Can you show the exact command because this is missing at least a backslash.
As mentioned, having the latest dependencies and running:
```bash
cmake -B fuzzbuild \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMA
...