💬 hodlinator commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2559145317)
nit: Seems to compile fine without this on Clang:
```suggestion
if (!CheckStandardAsmap(asmap)) {
```

💬 hodlinator commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2559059898)
Thank you for taking the patch! :handshake:

Agreed, it builds on https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2545416624

💬 hodlinator commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2559163190)
meganit in dc0c4310bae257236d3cd6a06de8071276d4548b: Empty line is removed only to be re-added in next commit.

👍 theStack approved a pull request: "test: Fix reorg patterns in tests to use proper fork-based approach"
(https://github.com/bitcoin/bitcoin/pull/32587#pullrequestreview-3507213147)
re-ACK 2a64851c34fcf9955a673eb77463f68ab583a7bd

meta nit: the `create_empty_fork` removal slipped again from first into second commit. Still fine to merge as-is, as far as I'm concerned.

💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#issuecomment-3577923878)
> To build more confidence of the bool -> byte transition, I developed a C++ test with more readable asmap input data - https://github.com/bitcoin/bitcoin/commit/cf34bb9806e95116d4b65b6dc80dc11a1f723a51 (https://github.com/hodlinator/bitcoin/tree/pr/33878_suggestions2 for interleaved branch). Not sure whether it's worth including in some form.

Thanks, looks like a very nice visualization at least. I will consider including it and addressing your other comments until I have to retouch here. If
...

💬 fjahr commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2561897010)
Right, I did have a wrong understanding here. I guess I didn't review the musig descriptors PR and I went by my assumptions, but I have looked at the PR again and at BIP390 and it seems like hitting this error should be impossible. Thanks!

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