💬 hebasto commented on pull request "doc: Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2561967012)
I don't think that the `host_$(tool)` variables were ever meant to be set by users, and the same applies to `build_$(tool)`.

(this is my understanding of the code as it was originally introduced; however, @theuni knows better)

More recently, it became [clear](https://github.com/bitcoin/bitcoin/pull/23571) that native packages also need a way to define their toolchains.

As https://github.com/bitcoin/bitcoin/pull/23571 didn’t gain enough support, I ended up using the "internal" `build_$
...

💬 ryanofsky commented on issue "Memory leak when using IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3578402033)
Thanks the the updates! If I'm understanding correctly, it seems like memory usage increasing 160MB overnight and 100MB over next 8 hours, and this would be pretty consistent with block templates not being freed. I think there are two key unknowns here:

- We don't know whether the client is holding onto the block template references. If it is, then the memory usage going up would be expected and this is client bug, otherwise it is a server bug.
- We don't know if server is failing to release bl
...

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2562742653)
The asymmetry is intentional and due to the fact that only the local node will `Add` a tx, so why would we malleate our own witness? `Remove` is called on any tx we receive from peers, so it could have a different wtxid from the tx we added to the private broadcast queue.

🤔 maflcko reviewed a pull request: "Clear out space on CentOS, depends, gui GHA job"
(https://github.com/bitcoin/bitcoin/pull/33514#pullrequestreview-3509596228)
left some nits.

I guess I don't understand why this one takes 5 minutes, but basically the same takes 1 minute in https://github.com/hebasto/bitcoin-core-nightly/actions/runs/19678632177/job/56366955874?pr=91#step:3:1.

I don't have a strong opinion, but I still think this is better run on all tasks, and constantly maintaining which task needs it (or can it have removed) does not seem like a good use of time?

If the time is a concern, all of this could be replaced with a one-line background ta
...

💬 maflcko commented on pull request "Clear out space on CentOS, depends, gui GHA job":
(https://github.com/bitcoin/bitcoin/pull/33514#discussion_r2563905429)
Does this clear any meaningful space? Could maybe remove, if not?

💬 maflcko commented on pull request "Clear out space on CentOS, depends, gui GHA job":
(https://github.com/bitcoin/bitcoin/pull/33514#discussion_r2563885819)
```suggestion
set +o errexit
```

nit: Would be nice to use the long names, so that the code can be understood without looking up the long name in the manual.

💬 maflcko commented on pull request "Clear out space on CentOS, depends, gui GHA job":
(https://github.com/bitcoin/bitcoin/pull/33514#discussion_r2563907311)
Will this then lead to OOM instead?

💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3580138004)
Fixed nit related to moving of `create_empty_fork`.

🤔 janb84 reviewed a pull request: "contrib: Count entry differences in asmap-tool diff summary"
(https://github.com/bitcoin/bitcoin/pull/33939#pullrequestreview-3509684360)
utACK 996660c1e8b0f221012ac0662fd05c3c85b96888

PR changes format of and adds counters of ip entries to te summary.

During my code review I noticed that it uses bit shifting in stead of the built-in property of Python's standard library `ipaddress` module. Since we are already importing it why not use it? (should be slightly more performant also) And we are already touching the code, this would be a nice time to change it. Therefor I added the NIT.

💬 janb84 commented on pull request "contrib: Count entry differences in asmap-tool diff summary":
(https://github.com/bitcoin/bitcoin/pull/33939#discussion_r2563960690)
NIT: since we are touching the code, it would be nice to update the code to be a little bit more performant (and modern)

```suggestion
net = asmap.prefix_to_net(prefix)
num_addrs = net.num_addresses
if isinstance(net, ipaddress.IPv4Network):
ipv4_changed += num_addrs
ipv4_entries_changed += 1
else: # IPv6Network
ipv6_changed += num_addrs
ipv6_entries_changed += 1

...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564079915)
This seems more general discussion about `size_t`. I think the message of `size_t` is "some unsigned integer, don't care about the number of bits, or the architecture". `size_t` is used about 3000 times in the code base already. Maybe have a broader discussion about it on IRC or open a brainstorm issue "should we avoid size_t or when to use size_t"?

I will leave it as `size_t` for now.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564090327)
I stole parts of other tests into this one, did not start it from scratch. That 2017 is from there, I think it is fair.

✅ ismaelsadeeq closed a pull request: "mini miner: enable `Linearize` return package feerates"
(https://github.com/bitcoin/bitcoin/pull/33216)

✅ ismaelsadeeq closed a pull request: "Fee Estimation: Ignore all transactions that are CPFP'd"
(https://github.com/bitcoin/bitcoin/pull/30079)

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-3580359812)
Thanks for your reviews.
After the merge of cluster mempool, there is a better approach.
I will work on it soon, in the meantime this can tagged up for grabs.

🤔 maflcko reviewed a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3509833324)
lgtm, but i think the error handling can be improved?

lgtm ff5c3feceaba496ff25efd8420cfcc32e0864bcc 🌽

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

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=

...

💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2564089445)
```suggestion
Responds with 404 if the block or the byte range doesn't exist.
```

nit

💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2564233993)
I also don't see the code path here. `rest_block` is called in the scope and line below, so why would pruning have any effect here?

💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2564184625)
the rest interface should be fine to expose to remote untrusted parties, no? So i am not sure about allowing them to unconditionally log confusing errors to the debug log.

i think it would be better if the caller could decide if there really was a storage corruption and an error should be logged, or if the user just had a typo in the rest url.

One way to achieve this, would be by returning a failure value.

Unfortunately, `util::Result` does not yet allow this: #25665. So maybe for now, the r
...

💬 Sjors commented on issue "Memory leak when using IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3580478570)
The node log messages should contain a `destroy` entry every time the client releases a template. It's probably also a good idea to have the client emit a log message when it discards a template. If the client waits until disconnect you'll see a flood of destroy methods at that point.

If the latter isn't happening, that could suggest a bug where Bitcoin Core doesn't release the memory after the client disconnects. As suggested by @ryanofsky adding logging to `BlockTemplateImpl` and `~BlockTempl
...