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

💬 Sjors commented on issue "Memory leak when using IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3571010573)
Turns out I was chasing a ghost. The reason `destroy` wasn't called by sv2-tp was not because of a missing `context` param, but because of an unrelated regression.

So the memory leak @plebhash is seeing is probably due to not calling destroy.

💬 ryanofsky commented on issue "Memory leak when using IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3571013068)
re: https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3570523079

> it looks like you found a real bug. Because `BlockTemplate::createNewBlock` doesn't have a context param, it looks like its destroy method is not invoked until sv2-tp disconnects.

I can imagine there's a bug causing the block template not to be freed until the disconnect happens, but I don't think `createNewBlock` method having a context parameter would affect this. Having a context parameter just allows the method t
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556478955)
I still prefer the dumb:
```
if (IsWhatever()) {
++counter;
}
```

✅ rkrux closed a pull request: "wallet: remove redundant sighash calculation in Musig2 signing flow"
(https://github.com/bitcoin/bitcoin/pull/33665)

💬 rkrux commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#issuecomment-3571032651)
> The only situation where it is computed multiple times is if the wallet has multiple keys in the musig, but that should rarely happen.

One of the reasons that made me raise the PR was this case, but as highlighted it'd occur rarely.

Re: https://github.com/bitcoin/bitcoin/pull/33665#issuecomment-3554963168

All these points make sense to me, thus I'm closing this PR.

💬 ryanofsky commented on issue "Memory leak when using IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3571047403)
re: https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3570541428

> 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?

Your understanding is right, I think, but there are a lot of details here and I can imagine there being some bug where just dropping the r
...

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2556512190)
Resolving as this commit has been removed.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556513211)
Maybe an attacker could hush the transaction originator, tricking the originator to stop broadcasting by sending them a bricked transaction and getting them to stop broadcasting the original one. Even without that, it feels more correct to keep broadcasting the transaction until we receive the same transaction back ("same" meaning: both txid and wtxid match).

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2556520957)
Done in 0190ac6622ff017aeead385289918d388a85218a

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2556523653)
Done in 62f2b9e4601ba34e504171f87577f315af51f3c9

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2556526561)
Done in 04d1dc78599362623bdd4b55c7be0c7b0f9f2174

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2556529044)
Resolving since commit message changed so no longer relevant.

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2556535563)
Done in the latest push to 039c3aab3b79e61e8ffe193bffee36b826ca2984

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2556547584)
Done in f07ef4149b89d2c1d71af2cafd1bf29f6661b35a which introduces `rand_path` using strong randomness, and `-fuzzcopydatadir` and uses both in the same commit.