💬 sedited commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1656947427)
Nit: Call this DstContructed like in the template declaration?

💬 sedited commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3603908712)
Re https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3597867646

> I'd expect both classes to be useful: Result class for higher level code, std::expected for low-level code.

I think I'd be fine with that, but would appreciate to just have a consistent way to propagate our errors. I was more suggesting it, because it seems like this PR is not compelling for everybody. But maybe this can get over the line now :)

💬 ryanofsky commented on issue "Memory leak when using IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3604081211)
Following up on https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3578402033, I added log statements to the `BlockTemplateImpl` constructor and destructor and wrote a python test creating a block template object and calling the destroy method or not (`DESTROY` variable), and deleting the python template variable or not (`DEL` variable). As soon as either of these things were done, I could see the `~BlockTemplateImpl` destructor being called. I could also see the destructor being calle
...

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

> after block [`0000000000000000000077e34472247575f4858357541269d56a8df5b429862b`](https://mempool.space/block/0000000000000000000077e34472247575f4858357541269d56a8df5b429862b), we destroyed 84 stale templates:
> [...]
> all of this come from the execution of [`TemplateData::destroy_ipc_client`](https://github.com/stratum-mining/sv2-apps/blob/ecf5f258766ac9e08bd8aaaa53b45b1c1bf961d8/bitcoin-core-sv2/src/template_data.rs
...

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582956040)
> If we'd finally have https://github.com/bitcoin/bitcoin/pull/25665 we could have a proper result type here.

Agree - but maybe meanwhile we can use the following?
https://github.com/bitcoin/bitcoin/compare/6826375e85a690d583cdeae3ff68d8703b1802ca..f07c765aa6bdba2511ceec56aa7f9755fa29a81e
(changed the return type back to be boolean + added an output parameter for the error enum)

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582956676)
Thanks! Removed in https://github.com/bitcoin/bitcoin/compare/6826375e85a690d583cdeae3ff68d8703b1802ca..f07c765aa6bdba2511ceec56aa7f9755fa29a81e

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582956967)
Thanks! Added in https://github.com/bitcoin/bitcoin/compare/6826375e85a690d583cdeae3ff68d8703b1802ca..f07c765aa6bdba2511ceec56aa7f9755fa29a81e.

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582961930)
If f07c765aa6bdba2511ceec56aa7f9755fa29a81e is OK, I will split it into the following commits:
1. adding support for returning error details via a new `ReadRawBlock()` overload.
2. adding support for range reads to the previously added `ReadRawBlock()` overload.
3. adding new REST endpoint using the previously added `ReadRawBlock()` overload.

WDYT?

🤔 mzumsande reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3531872272)
Code Review ACK 2d398050ee31db05e9222149b5e60572ac31d803

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2582595332)
I wonder if there is any software out there that will send pings and expect pongs back (before processing any other messages), that this would be incompatible with.

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2582770939)
Why not simply call `NumToOpenAdd(num_for_rebroadcast)`. Given that we only reattempt to reconnect every 12.5 minutes (and we add just 1 more attempt per tx, compared to the initial 3 attempts), which seem both quite conservative choices, wouldn't it be better to overshoot? If we are currently doing other (non-stale) private broadcasts (which will have fewer attempts and therefore higher priority), these could otherwise prevent the rebroadcast of stale transactions.

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2582800611)
I think it would be better to have a bunch randomly created addresses (obviously with a valid checksum) instead of addrs that at sometime belonged to actual node runners on mainnet - less confusing (node runners may search the internet for their node) and in case of a future bug that leads to actual connections, it's less likely that actual nodes are contacted.

💬 pablomartin4btc commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#discussion_r2583352655)
Done.

💬 pablomartin4btc commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#discussion_r2583352810)
Done.

💬 pablomartin4btc commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#discussion_r2583553569)
Done in 2nd. commit: _"gui: Update QMessageBox::Icon with chaintype image"_.

💬 Sjors commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3605610816)
Here's a slightly more realistic plot from last night on a well connected node running on an Intel i5-8400:

<img width="3842" height="1369" alt="getmemoryload-scatter" src="https://github.com/user-attachments/assets/f1ae2d88-73fa-4d2c-bd5f-bce5a168eeee" />

It's connected to DMND pool, declaring custom templates and getting them approved, but not actually mining. Due to their rate limiting I set `-sv2interval=20`, so if fees go up, it waits at least 20 seconds before generating a new templa
...

💬 willcl-ark commented on pull request "guix: reduce allowed exported symbols":
(https://github.com/bitcoin/bitcoin/pull/33950#issuecomment-3605815880)
Guix hashes:

```
x86_64
b507c965417a1adcd6b10000f9a8bae29e18ba9a1e76060c38af7fe1fbbcd0e4 guix-build-7b90b4f5bb10/output/aarch64-linux-gnu/SHA256SUMS.part
c60c33e4ececbc650bbeda06c7aa7dad714ace312a08c804afd750fc0a0c3d68 guix-build-7b90b4f5bb10/output/aarch64-linux-gnu/bitcoin-7b90b4f5bb10-aarch64-linux-gnu-debug.tar.gz
77bdfc199c87064ee166c6c124aa9d586fb24b144d8408bdd3d0e2e0065577a5 guix-build-7b90b4f5bb10/output/aarch64-linux-gnu/bitcoin-7b90b4f5bb10-aarch64-linux-gnu.tar.gz
5b6c18817
...

👍 rkrux approved a pull request: "doc: improvements to doc/descriptors.md"
(https://github.com/bitcoin/bitcoin/pull/33986#pullrequestreview-3534021842)
lgtm ACK fc33626f66e7f93f39989fbbabfbf00222762071

💬 Sjors commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#discussion_r2584340016)
That's useful. So far `interface_ipc.py` never had a transaction appear in multiple templates, so the destructor would always remove the last reference.

I adjusted the test so that it does. Now your mutation causes a crash during this test.

👍 rkrux approved a pull request: "contrib: fix manpage generation"
(https://github.com/bitcoin/bitcoin/pull/33996#pullrequestreview-3534104599)
tACK 10e6417b3b33a7df18a61d657153f3d90b89dbf2

The regex seems a little loose but ok.