💬 pinheadmz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3603546302)
push to 22ec2f95eecddd5dc6ea53f670f232a7e425a8d0:
- fix conflicts with master, which were all logging fixes from https://github.com/bitcoin/bitcoin/pull/33960

👍 hodlinator approved a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3531762528)
re-ACK 6826375e85a690d583cdeae3ff68d8703b1802ca

💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582602195)
there's a **lot** happening in a single commit - please consider splitting the low risk changes such as this line from introducing a new feature or a doc change. It would help guide the reviewers if it were split into trivial-to-review chunks where the commit message explains the thinking.

👍 sedited approved a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3531852343)
Re-ACK 6826375e85a690d583cdeae3ff68d8703b1802ca

💬 sedited commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582578921)
Nit: It would be good to assert that we don't fall through here, as per the [developer-notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures), by adding an `assert(false)` here.

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