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

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

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

💬 maflcko commented on pull request "Revert "gui, qt: brintToFront workaround for Wayland"":
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3571111965)
Instead of linking to a bunch of threads, it could make sense to just say that the issue has been fixed upstream in Qt6, so that the workaround is no longer needed?

Ref: https://codereview.qt-project.org/c/qt/qtwayland/+/418094

lgtm, but I haven't tested this.

review ACK 0672e727bf1db5995562e9656d18b699aeba5fe0 🎩

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_an
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556578753)
Ok, if a transaction that we are trying to broadcast gets double spent then this will be handled by the `else` branch below:

https://github.com/bitcoin/bitcoin/blob/73ea6405da30725358cd2973a8898d9b0ba719cd/src/net_processing.cpp#L1676-L1693

🤔 hodlinator reviewed a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3499037428)
Reviewed 78d6402458d7d14533444d893c989f0534a3896f

Main issue blocking A-C-K is in doc/REST-interface.md (see inline comment).

Left minor comment/question in related presentation: https://docs.google.com/presentation/d/1Zez-6DApKRu59kke4i_g9jwxQlaFKKRpOPdYFYsFXfA/edit?disco=AAABwl3JY0U
(I was re-reading it to remind myself why switching from a tx hash to a block hash would be more efficient, re-realized that mapping block height=>block hash is quite natural).

Experimented with further optimiza
...

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2555214226)
Incorrect names:
```suggestion
- `GET /rest/blockpart/<BLOCK-HASH>.<bin|hex>?offset=X&size=Y`
```

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2555229245)
nit: Consistency
```suggestion
* - `/blockpart/` via `rest_block_part()` (doesn't support JSON response, so `tx_verbosity` is unset)
```

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2555223914)
nit: I think it's worth including the parameters to make the reason behind adding this more understandable.
```suggestion
- A new REST API endpoint (`/rest/blockpart/BLOCKHASH?offset=X&size=Y`) has been introduced
```

💬 plebhash commented on issue "Memory leak when using IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3571209737)
I ran two sessions with https://github.com/bitcoin/bitcoin/pull/33936 at a3a6861e131120eabf6e2f7ecd15e5ea805c66b2, where the client was our Sv2 Rust code

both of them had at least one chain tip update, which should trigger templates being flushed from memory (so I'd expect to see a sharp decline in RAM consumption)

in the first one, we're not calling `destroy`:

<img width="640" height="480" alt="Image" src="https://github.com/user-attachments/assets/3e3cb655-8069-4107-8532-bed5455189dd" />

i
...

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

> do we have to explicitly call `destroy` or is it sufficient to drop the reference from memory on the client side?

Following up on this, it's good practice to call `destroy` methods on objects which have them, to guarantee the objects are destroyed right away instead of asynchronously. This is most important if objects have nontrivial destructors, or if it matters what order objects get destroyed.

Block templates usi
...

👍 fanquake approved a pull request: "ci: Use latest Xcode that the minimum macOS version allows"
(https://github.com/bitcoin/bitcoin/pull/33932#pullrequestreview-3500865738)
ACK https://github.com/bitcoin/bitcoin/pull/33932/commits/fa9537cde10120b12c96061cbc3f79a7680f9d64 - seems fine.