💬 sedited commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582661911)
Nit: I don't like that we are returning a nullopt for a success code here. If we'd finally have #25665 we could have a proper result type here. Leaving as a nit, because I don't have a good alternative either.

💬 sedited commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582597724)
Nit: I think the extra `are` is a typo.

💬 hebasto commented on pull request "doc: Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#issuecomment-3603896660)
> If that is the case, perhaps then instead of introducing more opinionated fallback heirarchies we should look at re-working how toolchains are defined more holistically? I don't see an obvious/clean approach not involving two sets of env vars though...

I agree that the depends build subsystem should accept two sets of variables: one for host/target-specific tools and another for native tools.

The naming for the former is well established (`CC`, `CXX`, etc) and it makes no senses to use a
...

👍 sedited approved a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-2144994913)
Re-ACK f0aff63b5ad51566e626d5b24eee08eb81df54a1

💬 sedited commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1656917172)
Style nit: `{` on new line.

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