💬 davidgumberg commented on pull request "wallet: Remove isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-3208497666)
crACK https://github.com/bitcoin/bitcoin/commit/be776a1443fdf1a72e0d363c1566d71cb0cda8b5

🤔 jonatack reviewed a pull request: "net: Prevent node from binding to the same `CService`"
(https://github.com/bitcoin/bitcoin/pull/33231#pullrequestreview-3138666437)
Concept ACK

> This PR proposes that repeated -bind options have no effect.

Only no effect for repeated *duplicate* -bind options, yes? Would it be preferable to give the user feedback instead?

Would be good to add test coverage.

💬 jonatack commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2289594951)
Would need to update the doxygen ("A vector...")

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3208833064)
recrACK 50e61f448eedfe0ccfa07f3124aeef9c7762a69b

I only left nits, I'm fine with merging it as is.

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2289730918)
nit: as mentioned this should be synchronized with the other PR to simplify the merge conflicts

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2289739420)
another super-nit: `CheckHeadersPow` -> `CheckHeadersPoW`. Only change if you have to change for any other reason.

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2289725133)
nit: if you touch again, you could explain in the commit message why the `LookupBlockIndex` cannot be eliminated as well

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2289729638)
nit: if touching again, the commit message should probably mention the `explicit` as well. No that important, just noticed it.

✅ nervana21 closed an issue: "Crash on launch in PruneBlockIndexCandidates"
(https://github.com/bitcoin/bitcoin/issues/33129)

💬 ajtowns commented on pull request "net: Provide block templates to peers on request":
(https://github.com/bitcoin/bitcoin/pull/33191#issuecomment-3208988596)
> 1. How do nodes build the block template they share with peers?

The same way they build a block template for the `getblocktemplate` RPC (well, except ignoring config options that might otherwise reduce the block size). See [here](https://github.com/ajtowns/bitcoin/blob/355e0b2665952c4c16d98a4212d1f74df8bf5263/src/net_processing.cpp#L5192-L5203).

> When they use only their mempool to build those templates, I don’t think that the transactions with diverging mempool policy will relay th
...

💬 ajtowns commented on pull request "net: Provide block templates to peers on request":
(https://github.com/bitcoin/bitcoin/pull/33191#issuecomment-3208998238)
> Maybe a stupid question and I'm not understanding something, but what is the point of this if we have to wait for all transactions to be announced? Isn't the whole idea to make sure our peers know about what we think will be the next block?

If you include a transaction in a template you send to a peer, that implicitly announces the tx to that peer.

The reason we delay announcing txs to a peer is to add some uncertainty about when precisely we received a transaction, and the order in whic
...

💬 ajtowns commented on pull request "net: Provide block templates to peers on request":
(https://github.com/bitcoin/bitcoin/pull/33191#issuecomment-3209091546)
Bumped the size of templates up to 2MvB, and added `latest_template_weight` and `latest_template_tx` to `gettemplateinfo` to report on the size of the current template.

💬 yuvicc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#issuecomment-3209170419)
Concept ACK

💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2290007939)
Ah, ok. I am not familiar with the GitHub Actions Docker Caching Buildx setup interactions with the docker containers that are run later. But given that this is documented as a buildx option (https://docs.docker.com/reference/cli/docker/buildx/create/#driver-opt) it seems plausible that it may not apply to the docker container.

💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2290021504)
When running locally, the image, the container and the volumes are named after it:

```
ci/test/02_run_container.sh: docker volume create "${CONTAINER_NAME}_ccache" || true
ci/test/02_run_container.sh: docker volume create "${CONTAINER_NAME}_depends" || true
ci/test/02_run_container.sh: docker volume create "${CONTAINER_NAME}_depends_sources" || true
ci/test/02_run_container.sh: docker volume create "${CONTAINER_NAME}_previous_releases" || true
```

So I think it is fine to use the
...

🤔 maflcko reviewed a pull request: "ci: Migrate CI to hosted Cirrus Runners"
(https://github.com/bitcoin/bitcoin/pull/32989#pullrequestreview-3139298640)
only change is some env var shuffling

re-ACK c2d18db38de6d5f71d48437422b93b8a962d3935 🛄

<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=
trusted commen
...

💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2290063942)
style nit in 5d652ca1ef996cbe7ad9e94a4c6d29b11a7f35e9: Not sure about the distinction, but `PREVIOUS_RELEASES_DIR` is also a CI dir (config). See the output in https://github.com/bitcoin/bitcoin/actions/runs/16960628990/job/48072349653?pr=32989#step:9:114:

```
+ ./ci/test/02_run_container.py
Export only allowed settings:
+ bash -c 'grep export ./ci/test/00_setup_env*.sh'
+ cat /tmp/env-admin-ci_native_previous_releases
CCACHE_TEMPDIR=/tmp/.ccache-temp
CCACHE_MAXSIZE=500M
...
PREVIOUS
...

💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3209279969)
🎉 thanks for all the reviews!

I'll stay on top of followup PR's over the next few weeks.

💬 Sjors commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3209295373)
Concept ACK

This also makes it easier to document instructions for miners, they just need to drop the "d" from `bitcoind` and then `-ipcbind` behaves like any other setting.

The help text should explain that `-m` is assumed if any IPC arguments are passed.

💬 Sjors commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#issuecomment-3209298414)
@sipa let me know if you don't have to time to rebase, in which case I'll open a fresh PR next week.