💬 m3dwards commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2180607503)
Setting to draft while I fix the job, will test on my own fork and re-open when ready for review.
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2180607503)
Setting to draft while I fix the job, will test on my own fork and re-open when ready for review.
📝 m3dwards converted_to_draft a pull request: "ci: add option for running tests without volume"
(https://github.com/bitcoin/bitcoin/pull/30310)
Fixes: https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1645950272
Cache wasn't being saved when run on GHA because the default behaviour of the CI script was to store cache items in a docker volume. This works on Cirrus CI as the volumes are shared but it does not work on Github Actions in which each run is ephemeral.
Kept the default behaviour the same so hopefully this continues to work for the Cirrus CI jobs.
(https://github.com/bitcoin/bitcoin/pull/30310)
Fixes: https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1645950272
Cache wasn't being saved when run on GHA because the default behaviour of the CI script was to store cache items in a docker volume. This works on Cirrus CI as the volumes are shared but it does not work on Github Actions in which each run is ephemeral.
Kept the default behaviour the same so hopefully this continues to work for the Cirrus CI jobs.
💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2180646372)
Could split "ci: clarify Cirrus self-hosted workers setup " into a separate doc-only PR? Should be easy to review and merge.
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2180646372)
Could split "ci: clarify Cirrus self-hosted workers setup " into a separate doc-only PR? Should be easy to review and merge.
💬 m3dwards commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#discussion_r1647581108)
This was made as a step and not in the `env:` section of the job as ${{ runner.temp }} is only available in the run steps.
(https://github.com/bitcoin/bitcoin/pull/30310#discussion_r1647581108)
This was made as a step and not in the `env:` section of the job as ${{ runner.temp }} is only available in the run steps.
👍 maflcko approved a pull request: "ci: add option for running tests without volume"
(https://github.com/bitcoin/bitcoin/pull/30310#pullrequestreview-2130444346)
lgtm.
Maybe wait for the CI to pass, then address the nits to test the created cache?
(https://github.com/bitcoin/bitcoin/pull/30310#pullrequestreview-2130444346)
lgtm.
Maybe wait for the CI to pass, then address the nits to test the created cache?
💬 maflcko commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#discussion_r1647584227)
```suggestion
--mount "${CI_PREVIOUS_RELEASES_MOUNT}" \
```
nit: Could remove the docker mention?
(https://github.com/bitcoin/bitcoin/pull/30310#discussion_r1647584227)
```suggestion
--mount "${CI_PREVIOUS_RELEASES_MOUNT}" \
```
nit: Could remove the docker mention?
💬 maflcko commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#discussion_r1647584771)
nit: Could keep the order as-is and not move this?
(https://github.com/bitcoin/bitcoin/pull/30310#discussion_r1647584771)
nit: Could keep the order as-is and not move this?
💬 m3dwards commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#discussion_r1647590152)
`RUNNER_TEMP` was used over `${{ github.workspace }}` (as is used in the MacOS job) because the CI container mounts that as readonly which means it can't be used to write cache items to.
(https://github.com/bitcoin/bitcoin/pull/30310#discussion_r1647590152)
`RUNNER_TEMP` was used over `${{ github.workspace }}` (as is used in the MacOS job) because the CI container mounts that as readonly which means it can't be used to write cache items to.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2180745584)
`057c79365c...7a7e7d189d`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2180745584)
`057c79365c...7a7e7d189d`: rebase due to conflicts
💬 vasild commented on pull request "fuzz: make FuzzedDataProvider usage deterministic":
(https://github.com/bitcoin/bitcoin/pull/29043#issuecomment-2180755507)
Has two ACKs, @TheCharlatan, maybe you want to take a look at this since you :+1:'ed the OP.
(https://github.com/bitcoin/bitcoin/pull/29043#issuecomment-2180755507)
Has two ACKs, @TheCharlatan, maybe you want to take a look at this since you :+1:'ed the OP.
🤔 maflcko reviewed a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2128595258)
lgtm ACK f946b083fe1431fb8d5d2c080ed3c2938116c37a 📪
<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 comment: lgtm ACK f946b083fe1431fb
...
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2128595258)
lgtm ACK f946b083fe1431fb8d5d2c080ed3c2938116c37a 📪
<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 comment: lgtm ACK f946b083fe1431fb
...
💬 maflcko commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1647123546)
nit in 594189567956063fa1a44bef9e86e72c14207027:
```
script/sigcache.h should remove these lines:
- #include <cstdint> // lines 18-18
- #include <utility> // lines 20-20
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1647123546)
nit in 594189567956063fa1a44bef9e86e72c14207027:
```
script/sigcache.h should remove these lines:
- #include <cstdint> // lines 18-18
- #include <utility> // lines 20-20
💬 maflcko commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1647518394)
nit in /f946b083fe1431fb8d5d2c080ed3c2938116c37a: Why use a pointer when a reference is better?
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1647518394)
nit in /f946b083fe1431fb8d5d2c080ed3c2938116c37a: Why use a pointer when a reference is better?
💬 maflcko commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1647514586)
nit in /f946b083fe1431fb8d5d2c080ed3c2938116c37a: Unrelated change? The commit message is "De-globalize signature cache", but this hunk makes the script execution cache private.
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1647514586)
nit in /f946b083fe1431fb8d5d2c080ed3c2938116c37a: Unrelated change? The commit message is "De-globalize signature cache", but this hunk makes the script execution cache private.
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2180775603)
I split the commit that introduces `sv2_messages.h` in two parts:
* 9fcc95d837dbd20c1003b4c2d3c90b202e0629bd: introduces a single simple message `Sv2CoinbaseOutputDataSizeMsg` and the plumbing it needs
* 00d1408cc7cb263fb6e956649a41e0b02e2378aa: adds the other messages
This makes it easier to introduce `Sv2Transport` in a separate PR, which only needs the first commit.
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2180775603)
I split the commit that introduces `sv2_messages.h` in two parts:
* 9fcc95d837dbd20c1003b4c2d3c90b202e0629bd: introduces a single simple message `Sv2CoinbaseOutputDataSizeMsg` and the plumbing it needs
* 00d1408cc7cb263fb6e956649a41e0b02e2378aa: adds the other messages
This makes it easier to introduce `Sv2Transport` in a separate PR, which only needs the first commit.
💬 kevkevinpal commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2180781920)
Is there a reason why we don't want to just have cirrus behave in the same way we'd do it on GHA, seems like GHA cannot store to shared cache but cirrus can act the same was as GHA. Just thinking maybe theres a way to reduce complexity in `ci/test/02_run_container.sh` and have more shared logic between the two ci's
if I were to set `DANGER_CI_ON_HOST_CACHE_FOLDERS = 1` on the cirrus pipeline would the ci fail?
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2180781920)
Is there a reason why we don't want to just have cirrus behave in the same way we'd do it on GHA, seems like GHA cannot store to shared cache but cirrus can act the same was as GHA. Just thinking maybe theres a way to reduce complexity in `ci/test/02_run_container.sh` and have more shared logic between the two ci's
if I were to set `DANGER_CI_ON_HOST_CACHE_FOLDERS = 1` on the cirrus pipeline would the ci fail?
📝 guilhermelinosp opened a pull request: "Update README.md"
(https://github.com/bitcoin/bitcoin/pull/30313)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/30313)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1647632350)
58 minutes for the second run, which is acceptable.
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1647632350)
58 minutes for the second run, which is acceptable.
📝 fanquake locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/30313)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/30313)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...