💬 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
...
💬 Sjors commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3570785658)
@plebhash IIUC libmultiprocess does this automatically, but only `sv2-tp` uses that library. So it probably depends on how the rust capnp library is implemented. It might be worth testing how that library behaves out of the box, with and without the fix here. Just look for the `IPC server destroy` messages on the Bitcoin Core side (preferably tested against master).
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3570785658)
@plebhash IIUC libmultiprocess does this automatically, but only `sv2-tp` uses that library. So it probably depends on how the rust capnp library is implemented. It might be worth testing how that library behaves out of the box, with and without the fix here. Just look for the `IPC server destroy` messages on the Bitcoin Core side (preferably tested against master).
💬 theStack commented on pull request "test: add `-alertnotify` test for large work invalid chain warning":
(https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2556299085)
Good point, adapted the comment accordingly.
(https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2556299085)
Good point, adapted the comment accordingly.
📝 psam21 opened a pull request: "wallet: Validate all descriptors before rescanning in importdescriptors"
(https://github.com/bitcoin/bitcoin/pull/33938)
Fixes #33655
The importdescriptors RPC was performing expensive blockchain rescans even when descriptor validation failed, wasting time and resources.
## Changes
This PR separates validation logic into a new ValidateDescriptorImport() function that validates all descriptors before any wallet modifications or rescanning occurs.
## Behavior
- **Before**: ProcessDescriptorImport() validated and imported descriptors one by one, triggering a rescan even if some descriptors were invalid
- **After
...
(https://github.com/bitcoin/bitcoin/pull/33938)
Fixes #33655
The importdescriptors RPC was performing expensive blockchain rescans even when descriptor validation failed, wasting time and resources.
## Changes
This PR separates validation logic into a new ValidateDescriptorImport() function that validates all descriptors before any wallet modifications or rescanning occurs.
## Behavior
- **Before**: ProcessDescriptorImport() validated and imported descriptors one by one, triggering a rescan even if some descriptors were invalid
- **After
...
🤔 rkrux reviewed a pull request: "wallet: don't consider unconfirmed TRUC coins with ancestors"
(https://github.com/bitcoin/bitcoin/pull/33528#pullrequestreview-3500321052)
lgtm ACK dcd42d6d8f160ae8bc12c152099a6e6473658e30
Asked a question.
(https://github.com/bitcoin/bitcoin/pull/33528#pullrequestreview-3500321052)
lgtm ACK dcd42d6d8f160ae8bc12c152099a6e6473658e30
Asked a question.
💬 rkrux commented on pull request "wallet: don't consider unconfirmed TRUC coins with ancestors":
(https://github.com/bitcoin/bitcoin/pull/33528#discussion_r2556243671)
There appears to be two different ways now to check for the mempool ancestors and descendants of the transaction here. I tried the following diff and the tests pass. Maybe we can remove the `truc_child_in_mempool` class member altogether in the future?
```diff
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 5654c8f3d4..65c896892f 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -401,16 +401,15 @@ CoinsResult AvailableCoins(const CWallet& wallet,
if (nD
...
(https://github.com/bitcoin/bitcoin/pull/33528#discussion_r2556243671)
There appears to be two different ways now to check for the mempool ancestors and descendants of the transaction here. I tried the following diff and the tests pass. Maybe we can remove the `truc_child_in_mempool` class member altogether in the future?
```diff
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 5654c8f3d4..65c896892f 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -401,16 +401,15 @@ CoinsResult AvailableCoins(const CWallet& wallet,
if (nD
...