💬 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
...
💬 willcl-ark commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2556213666)
IMO it should be fine to convert the existing task(s) to a matrix instead of duplicating explicitly. I made a commit [in this branch](https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:refs/heads/pr-33764) to see ~ what it would look like, and I think it's pretty simple.
@maflcko is correct though that with a matrix the `windows-*-test` jobs would both depend on both `windows-*-cross` builds finishing, which might contribute to slow CI runtimes.
According to the PR desc
...
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2556213666)
IMO it should be fine to convert the existing task(s) to a matrix instead of duplicating explicitly. I made a commit [in this branch](https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:refs/heads/pr-33764) to see ~ what it would look like, and I think it's pretty simple.
@maflcko is correct though that with a matrix the `windows-*-test` jobs would both depend on both `windows-*-cross` builds finishing, which might contribute to slow CI runtimes.
According to the PR desc
...
🤔 janb84 reviewed a pull request: "fuzz: wallet: add target for `TransactionCanBeBumped`"
(https://github.com/bitcoin/bitcoin/pull/33916#pullrequestreview-3500413492)
ACK 2df9f10755ea7de94ecc9a17e6dd6dc89de0a637
PR adds new fuzzing target. Target `wallet_tx_can_be_bumped`.
Ran fuzzer for 2 hours without issues,
Codecoverage equals to what is reported.
New Target is NOT deterministic, but I'm also not sure what to change to make it deterministic (if it's even feasible)
This fuzzer increases fuzzing coverage :
| Totals master: | 69.81% (5082/7280) | 41.91% (10784/25729) | 62.64% (57301/91479) | 56.83% (39387/69312) | 56.96% (21311/37411)
-- |
...
(https://github.com/bitcoin/bitcoin/pull/33916#pullrequestreview-3500413492)
ACK 2df9f10755ea7de94ecc9a17e6dd6dc89de0a637
PR adds new fuzzing target. Target `wallet_tx_can_be_bumped`.
Ran fuzzer for 2 hours without issues,
Codecoverage equals to what is reported.
New Target is NOT deterministic, but I'm also not sure what to change to make it deterministic (if it's even feasible)
This fuzzer increases fuzzing coverage :
| Totals master: | 69.81% (5082/7280) | 41.91% (10784/25729) | 62.64% (57301/91479) | 56.83% (39387/69312) | 56.96% (21311/37411)
-- |
...
📝 fjahr opened a pull request: "contrib: Count entry differences in asmap-tool diff summary"
(https://github.com/bitcoin/bitcoin/pull/33939)
Currently the output of `asmap-tool.py diff` returns the total number of addresses that has changed at the end of the list.
Example output currently:
```
2602:feda:c0::/48 AS1029 # was AS43126
2604:7c00:100::/40 AS29802 # was AS40244
# 0 IPv4 addresses changed; 79552154633921058212365205504 (2^96.01) IPv6 addresses changed
```
This is good indicator but in case of a longer list I would like the number of changed entries as well, since that is an easier number to parse and for debugg
...
(https://github.com/bitcoin/bitcoin/pull/33939)
Currently the output of `asmap-tool.py diff` returns the total number of addresses that has changed at the end of the list.
Example output currently:
```
2602:feda:c0::/48 AS1029 # was AS43126
2604:7c00:100::/40 AS29802 # was AS40244
# 0 IPv4 addresses changed; 79552154633921058212365205504 (2^96.01) IPv6 addresses changed
```
This is good indicator but in case of a longer list I would like the number of changed entries as well, since that is an easier number to parse and for debugg
...
🤔 janb84 reviewed a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3500427026)
Concept ACK c34bc01b2ff2fc91ed4020288c5fa15f0c5b075e
PR Changes fuzzng readme to nudge MacOS users to fuzz under linux.
Given the number of issues I think it's best to change the fuzzing.md to suggest the MacOS users to fuzz under Linux.
Have added small NIT given my personal experiences by using nix-shell to run the Fuzzing without trouble. (Not expecting that it will be added)
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3500427026)
Concept ACK c34bc01b2ff2fc91ed4020288c5fa15f0c5b075e
PR Changes fuzzng readme to nudge MacOS users to fuzz under linux.
Given the number of issues I think it's best to change the fuzzing.md to suggest the MacOS users to fuzz under Linux.
Have added small NIT given my personal experiences by using nix-shell to run the Fuzzing without trouble. (Not expecting that it will be added)
💬 janb84 commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556335903)
NIT: non-blocking, maybe add nix-shell as an alternative because of native speed alternative.
```suggestion
best results. On macOS this can be done within Docker, a virtual machine or a nix-shell.
```
I'm running the fuzzer just fine on MacOS, Apple silicon in a nix-shell to the point that I'm actually questioning myself "am I doing it right?" seeing all the responses here and in #33731. ( Results are the same as a linux run, so i'm ok )
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556335903)
NIT: non-blocking, maybe add nix-shell as an alternative because of native speed alternative.
```suggestion
best results. On macOS this can be done within Docker, a virtual machine or a nix-shell.
```
I'm running the fuzzer just fine on MacOS, Apple silicon in a nix-shell to the point that I'm actually questioning myself "am I doing it right?" seeing all the responses here and in #33731. ( Results are the same as a linux run, so i'm ok )
💬 marcofleon commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556363890)
Sure, lgtm. As I said, I'm fine with including or not including a command for mac in the docs.
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556363890)
Sure, lgtm. As I said, I'm fine with including or not including a command for mac in the docs.
✅ maflcko closed a pull request: "wallet: Validate all descriptors before rescanning in importdescriptors"
(https://github.com/bitcoin/bitcoin/pull/33938)
(https://github.com/bitcoin/bitcoin/pull/33938)
💬 maflcko commented on pull request "wallet: Validate all descriptors before rescanning in importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33938#issuecomment-3570910802)
thx, but LLM generated pull requests, where the author does not understand the changes themselves, are not accepted:
* There are no tests
* The code does not even compile
* There is a already a pull request and you'd have to review/reflect that one first, as already mentioned previously
(https://github.com/bitcoin/bitcoin/pull/33938#issuecomment-3570910802)
thx, but LLM generated pull requests, where the author does not understand the changes themselves, are not accepted:
* There are no tests
* The code does not even compile
* There is a already a pull request and you'd have to review/reflect that one first, as already mentioned previously
👍 TheCharlatan approved a pull request: "Add a "tx output spender" index"
(https://github.com/bitcoin/bitcoin/pull/24539#pullrequestreview-3500536121)
Block hash addition looks good to me. Just needs a small iwyu fix to get the CI to pass again.
(https://github.com/bitcoin/bitcoin/pull/24539#pullrequestreview-3500536121)
Block hash addition looks good to me. Just needs a small iwyu fix to get the CI to pass again.