💬 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
...
💬 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
...
(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.
(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.
(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.
(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.
💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2290101120)
Also, it should be fine to re-apply once the CI intermittently fails due to this. So this pull looks good as-is and no change is needed here.
Unrelatedly, if we want to keep/use the CI setting in the future, it must be documented as a hidden option:
https://github.com/bitcoin/bitcoin/blob/7d9789401be4c91a9eb3c1112564a6524bdc4f70/ci/test/02_run_container.py#L29-L34
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2290101120)
Also, it should be fine to re-apply once the CI intermittently fails due to this. So this pull looks good as-is and no change is needed here.
Unrelatedly, if we want to keep/use the CI setting in the future, it must be documented as a hidden option:
https://github.com/bitcoin/bitcoin/blob/7d9789401be4c91a9eb3c1112564a6524bdc4f70/ci/test/02_run_container.py#L29-L34
💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3209306539)
> > [c18b093](https://github.com/bitcoin/bitcoin/commit/c18b0939da00d5851e6adcedbba5100ad791b486) also seems fine to keep, but can be cherry-picked in a follow-up.
>
> Despite this gaining quite some decent-enough runtime performance, I'd prefer to add this in a followup after having gotten TSAN failures twice in this PR.
>
> @fanquake also noted that he saw some issues when using `(2 * nproc)` in local runs, so I think safer to leave for now.
Ah, I see. I guess "some issues" refers to
...
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3209306539)
> > [c18b093](https://github.com/bitcoin/bitcoin/commit/c18b0939da00d5851e6adcedbba5100ad791b486) also seems fine to keep, but can be cherry-picked in a follow-up.
>
> Despite this gaining quite some decent-enough runtime performance, I'd prefer to add this in a followup after having gotten TSAN failures twice in this PR.
>
> @fanquake also noted that he saw some issues when using `(2 * nproc)` in local runs, so I think safer to leave for now.
Ah, I see. I guess "some issues" refers to
...
💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#issuecomment-3209327431)
Rebased, and addressed some of the feedback above.
Note that the last commit (which enables the tests in CI) may need work after moving to be on top of #31802, so that it matches the CI configurations which have IPC enabled.
(https://github.com/bitcoin/bitcoin/pull/33201#issuecomment-3209327431)
Rebased, and addressed some of the feedback above.
Note that the last commit (which enables the tests in CI) may need work after moving to be on top of #31802, so that it matches the CI configurations which have IPC enabled.
💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2290124906)
Done.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2290124906)
Done.
💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2290125154)
Done.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2290125154)
Done.
💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2290125464)
Done.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2290125464)
Done.
💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2290126866)
I opted to use system libcapnp in the description here because it's more likely to work. I've added a comment about it being optional now.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2290126866)
I opted to use system libcapnp in the description here because it's more likely to work. I've added a comment about it being optional now.
💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2290127079)
Done.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2290127079)
Done.
💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2290127497)
Done.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2290127497)
Done.
💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2290127873)
Leaving this for a follow-up.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2290127873)
Leaving this for a follow-up.