💬 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).

💬 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.

📝 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
...

🤔 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.

💬 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
...

💬 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
...

🤔 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)
-- |
...

📝 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
...

🤔 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)

💬 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 )

💬 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.

✅ maflcko closed a pull request: "wallet: Validate all descriptors before rescanning in importdescriptors"
(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

👍 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.

💬 l0rinc commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2556431555)
> Sure, lgtm

Thanks, let me know if you can reproduce the results above.

> I'm fine with including or not including a command for mac in the docs.

I do feel strongly about having some kind of support for Macs, otherwise fuzzing will be even more of a black box as it is currently.

⚠️ ryanofsky opened an issue: "Memory leak when using IPC mining interface"
(https://github.com/bitcoin/bitcoin/issues/33940)
_Originally posted by @plebhash in [#33899](https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3568788623)_

> we are getting reports of out-of-memory crashes on https://github.com/stratum-mining/sv2-apps/pull/59#issuecomment-3568252007
>
> initially I suspected there could be thread-related issues similar to what I reported on https://github.com/bitcoin/bitcoin/issues/33923, but it turns out it was the VPS running out of it's 2GB available RAM, which only happened after long (12h+)
...

💬 ryanofsky commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3570988224)
I created a new issue to track the memory leak in #33940 so this discussion can stay focused higher level issues, but thanks for finding & reporting that

📝 Sjors converted_to_draft 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
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556461454)
It's the `map.extract().key().foo` which I deem a non-debugger friendly, not `extact()` itself. I split that into two lines now.

💬 Sjors commented on pull request "mining: pass missing context to createNewBlock() and checkBlock()":
(https://github.com/bitcoin/bitcoin/pull/33936#issuecomment-3570998560)
Although the change is fine, the original motivation is incorrect. There was no `destroy` bug. Will update the PR descriptor later.