👍 brunoerg approved a pull request: "[29.x] 33106 backport and final changes for rc2"
(https://github.com/bitcoin/bitcoin/pull/33226#pullrequestreview-3138505376)
crACK 0034dcfba9dc599449e7569ed1b30e58d4f4434f
(https://github.com/bitcoin/bitcoin/pull/33226#pullrequestreview-3138505376)
crACK 0034dcfba9dc599449e7569ed1b30e58d4f4434f
🤔 w0xlt reviewed a pull request: "cli: Handle arguments that can be either JSON or string"
(https://github.com/bitcoin/bitcoin/pull/33230#pullrequestreview-3138552075)
Approach ACK
(https://github.com/bitcoin/bitcoin/pull/33230#pullrequestreview-3138552075)
Approach ACK
📝 w0xlt opened a pull request: "net: Prevent node from binding to the same `CService`"
(https://github.com/bitcoin/bitcoin/pull/33231)
Currently, if the node starts with repeated `-bind` options (e.g. `./build/bin/bitcoind -listen -bind=0.0.0.0 -bind=0.0.0.0`), the user will receive a misleading message followed by the node shutdown:
```
[net:error] Unable to bind to 0.0.0.0:8333 on this computer. Bitcoin Core is probably already running.
[error] Unable to bind to 0.0.0.0:8333 on this computer. Bitcoin Core is probably already running.
```
And the user might spend some time looking for a `bitcoind` process or what appl
...
(https://github.com/bitcoin/bitcoin/pull/33231)
Currently, if the node starts with repeated `-bind` options (e.g. `./build/bin/bitcoind -listen -bind=0.0.0.0 -bind=0.0.0.0`), the user will receive a misleading message followed by the node shutdown:
```
[net:error] Unable to bind to 0.0.0.0:8333 on this computer. Bitcoin Core is probably already running.
[error] Unable to bind to 0.0.0.0:8333 on this computer. Bitcoin Core is probably already running.
```
And the user might spend some time looking for a `bitcoind` process or what appl
...
💬 davidgumberg commented on pull request "wallet: Remove isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2289457295)
> This is the current behavior anyways.
That's surprising!
Shouldn't we do something like:
```cpp
entry.pushKV("spendable", pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
```
instead?
Otherwise RPC docs should be more like @pablomartin4btc [below](https://github.com/bitcoin/bitcoin/pull/32523/commits/6a7aa015747e2634fe5a4b2f7fa0d104eb75c796#r2274128180) can be done in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2289457295)
> This is the current behavior anyways.
That's surprising!
Shouldn't we do something like:
```cpp
entry.pushKV("spendable", pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
```
instead?
Otherwise RPC docs should be more like @pablomartin4btc [below](https://github.com/bitcoin/bitcoin/pull/32523/commits/6a7aa015747e2634fe5a4b2f7fa0d104eb75c796#r2274128180) can be done in a follow-up.
💬 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
(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.
(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...")
(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.
(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
(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.
(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
(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.
(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)
(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
...
(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
...
(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.
(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
(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.
(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
...
(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
...
(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
...