💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2321775620)
nit: Any reason to use `md` here? Most of the time 7/8 CPUs will be idle (to download the caches or to upload the artifact). Also, the similar mac-cross build is using `sm`.
Suggested diff:
```diff
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 6876b8328d..2efe17a04e 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -302,7 +302,7 @@ jobs:
windows-cross:
name: 'Linux->Windows cross, no tests'
needs: runners
- runs-on: ${
...
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2321775620)
nit: Any reason to use `md` here? Most of the time 7/8 CPUs will be idle (to download the caches or to upload the artifact). Also, the similar mac-cross build is using `sm`.
Suggested diff:
```diff
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 6876b8328d..2efe17a04e 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -302,7 +302,7 @@ jobs:
windows-cross:
name: 'Linux->Windows cross, no tests'
needs: runners
- runs-on: ${
...
💬 rkrux commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2321725927)
Interesting - took me some time to find out.
I see that the transactions are marked dirty when the descriptor is imported that breaks the transaction caches. I confirmed this by commenting the following portion from the `MarkDirty` function after which the test fails:
https://github.com/bitcoin/bitcoin/pull/33268/commits/113a4228229baedda2a730e097f2d59ad58a4b0d#diff-d41d68c5a65d67956c91b33ca86da7df1981d84a0b15b4a186deea566563fed5R344
```cpp
m_cached_from_me = std::nullopt;
```
I
...
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2321725927)
Interesting - took me some time to find out.
I see that the transactions are marked dirty when the descriptor is imported that breaks the transaction caches. I confirmed this by commenting the following portion from the `MarkDirty` function after which the test fails:
https://github.com/bitcoin/bitcoin/pull/33268/commits/113a4228229baedda2a730e097f2d59ad58a4b0d#diff-d41d68c5a65d67956c91b33ca86da7df1981d84a0b15b4a186deea566563fed5R344
```cpp
m_cached_from_me = std::nullopt;
```
I
...
👍 rkrux approved a pull request: "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet"
(https://github.com/bitcoin/bitcoin/pull/33268#pullrequestreview-3184703865)
lgtm ACK 113a4228229baedda2a730e097f2d59ad58a4b0d
Generally, I find it easy to miss these `Cached*` functions because they lie in this `wallet/receive.cpp` file that I don't find intuitive; `wallet/transaction.cpp` could be more intuitive because most of these functions update the `CWallet` object somehow - this PR need not do anything about it though.
Ok with addition of `m_cached_from_me` within `WalletTx` in this PR but I find `Cached*` functions are too many to keep track of.
(https://github.com/bitcoin/bitcoin/pull/33268#pullrequestreview-3184703865)
lgtm ACK 113a4228229baedda2a730e097f2d59ad58a4b0d
Generally, I find it easy to miss these `Cached*` functions because they lie in this `wallet/receive.cpp` file that I don't find intuitive; `wallet/transaction.cpp` could be more intuitive because most of these functions update the `CWallet` object somehow - this PR need not do anything about it though.
Ok with addition of `m_cached_from_me` within `WalletTx` in this PR but I find `Cached*` functions are too many to keep track of.
💬 rkrux commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2321729980)
In https://github.com/bitcoin/bitcoin/commit/009b273df8ad8a7470276a31f3cb900e480aa99b "test: Test wallet 'from me' status change"
Nit if retouched:
```diff
- self.log.info("Test gettransaction after changing a transaction's 'from me' status")
+ self.log.info("Test gettransaction after changing a transaction's 'from me' status due to importing descriptor")
```
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2321729980)
In https://github.com/bitcoin/bitcoin/commit/009b273df8ad8a7470276a31f3cb900e480aa99b "test: Test wallet 'from me' status change"
Nit if retouched:
```diff
- self.log.info("Test gettransaction after changing a transaction's 'from me' status")
+ self.log.info("Test gettransaction after changing a transaction's 'from me' status due to importing descriptor")
```
💬 janb84 commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253317106)
> If I understand correctly - the expected behaviour for `actions/checkout@v5` for `pull_request` is to checkout a temporary merge commit onto the merge branch, which should be re-evaluated every time the workflow is reran (e.g. a manual trigger), which is what we want to avoid silent merge conflicts. But you've observed that in some cases, the merge branch (`GITHUB_REF` [aka](https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request) `refs/pul
...
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253317106)
> If I understand correctly - the expected behaviour for `actions/checkout@v5` for `pull_request` is to checkout a temporary merge commit onto the merge branch, which should be re-evaluated every time the workflow is reran (e.g. a manual trigger), which is what we want to avoid silent merge conflicts. But you've observed that in some cases, the merge branch (`GITHUB_REF` [aka](https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request) `refs/pul
...
💬 maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253384950)
I am not familiar with the action written in typescript and I don't want to touch it upstream. However, I am happy to close this pull, if someone else does.
The issue is that the action does not use the ref, but the commit to fetch: https://github.com/actions/checkout/blob/ff7abcd0c3c05ccf6adc123a8cd1fd4fb30fb493/src/ref-helper.ts#L96
You can also see this here: https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49579638898?pr=29641#step:3:59:
```
/usr/bin/git -c protoco
...
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253384950)
I am not familiar with the action written in typescript and I don't want to touch it upstream. However, I am happy to close this pull, if someone else does.
The issue is that the action does not use the ref, but the commit to fetch: https://github.com/actions/checkout/blob/ff7abcd0c3c05ccf6adc123a8cd1fd4fb30fb493/src/ref-helper.ts#L96
You can also see this here: https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49579638898?pr=29641#step:3:59:
```
/usr/bin/git -c protoco
...
💬 hebasto commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253418623)
> Currently, the `actions/checkout@v5` checks out pull requests merged against master, which is what we want.
>
> However, sometimes, it checks out ancient/stale merge commits. For example:
>
> * https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49579638898?pr=29641#step:9:914 compiles with IPC=ON, even though latest master is at [ed2ff3c](https://github.com/bitcoin/bitcoin/commit/ed2ff3c63d83e275b0785a695fa4db38870abf1a)
>
> * [ci: Migrate CI to hosted Cirrus Runner
...
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253418623)
> Currently, the `actions/checkout@v5` checks out pull requests merged against master, which is what we want.
>
> However, sometimes, it checks out ancient/stale merge commits. For example:
>
> * https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49579638898?pr=29641#step:9:914 compiles with IPC=ON, even though latest master is at [ed2ff3c](https://github.com/bitcoin/bitcoin/commit/ed2ff3c63d83e275b0785a695fa4db38870abf1a)
>
> * [ci: Migrate CI to hosted Cirrus Runner
...
💬 maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253428949)
> This appears to be an issue with GitHub itself rather than `actions/checkout@v5`, as the CI logs show it switching to a `refs/remotes/pull/<PR_NUMBEER>/merge` branch.
The `refs/remotes/...` is just the local name used by the checkout action. It does not reflect the stuff that was fetched, see my previous comment.
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253428949)
> This appears to be an issue with GitHub itself rather than `actions/checkout@v5`, as the CI logs show it switching to a `refs/remotes/pull/<PR_NUMBEER>/merge` branch.
The `refs/remotes/...` is just the local name used by the checkout action. It does not reflect the stuff that was fetched, see my previous comment.
💬 maflcko commented on pull request "rpc: require integer verbosity; remove boolean 'verbose'":
(https://github.com/bitcoin/bitcoin/pull/33214#discussion_r2321934674)
why is this moved? should be a separate commit with rationale, or reverted?
(https://github.com/bitcoin/bitcoin/pull/33214#discussion_r2321934674)
why is this moved? should be a separate commit with rationale, or reverted?
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3253467866)
Updated e022142b52d8160abdee9a47e6ad7d493d521390 -> 82c503641a3a848234bba5c9979b9c4df5e53ea7 ([kernelApi_61](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_61) -> [kernelApi_62](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_62), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_61..kernelApi_62))
* Lock cs_main when manipulating the logging objects to ensure they are thread-safe
* Ran clang-format
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3253467866)
Updated e022142b52d8160abdee9a47e6ad7d493d521390 -> 82c503641a3a848234bba5c9979b9c4df5e53ea7 ([kernelApi_61](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_61) -> [kernelApi_62](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_62), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_61..kernelApi_62))
* Lock cs_main when manipulating the logging objects to ensure they are thread-safe
* Ran clang-format
💬 Crypt-iQ commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2321960953)
@instagibbs suggested this as well. I looked into it and the main issue is that it allows a peer to stall block download:
1. malicious peer sends compact block, reconstruction is required
2. malicious peer sends blocktxn and reconstruction fails, fallback to regular block request with this peer (and wiping `partialBlock`)
3. malicious peer goes back to step 1. This does not work in current master because of this [else statement](https://github.com/bitcoin/bitcoin/blob/2562fe1b2b63c3a510735ba4
...
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2321960953)
@instagibbs suggested this as well. I looked into it and the main issue is that it allows a peer to stall block download:
1. malicious peer sends compact block, reconstruction is required
2. malicious peer sends blocktxn and reconstruction fails, fallback to regular block request with this peer (and wiping `partialBlock`)
3. malicious peer goes back to step 1. This does not work in current master because of this [else statement](https://github.com/bitcoin/bitcoin/blob/2562fe1b2b63c3a510735ba4
...
💬 maflcko commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3253515372)
Created https://github.com/maflcko/DrahtBot/issues/59, because I don't know how to fetch the pull request number from a check suite or check run.
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3253515372)
Created https://github.com/maflcko/DrahtBot/issues/59, because I don't know how to fetch the pull request number from a check suite or check run.
💬 hebasto commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253540953)
> The issue is that the action does not use the ref, but the commit to fetch: https://github.com/actions/checkout/blob/ff7abcd0c3c05ccf6adc123a8cd1fd4fb30fb493/src/ref-helper.ts#L96
>
> You can also see this here: https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49579638898?pr=29641#step:3:59:
>
> ```
> /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin +ee059f57f059c7210da3a3f63ec433b29a35da0e:refs/remotes/pull/29641/merge
...
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253540953)
> The issue is that the action does not use the ref, but the commit to fetch: https://github.com/actions/checkout/blob/ff7abcd0c3c05ccf6adc123a8cd1fd4fb30fb493/src/ref-helper.ts#L96
>
> You can also see this here: https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49579638898?pr=29641#step:3:59:
>
> ```
> /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin +ee059f57f059c7210da3a3f63ec433b29a35da0e:refs/remotes/pull/29641/merge
...
🤔 janb84 reviewed a pull request: "ci: Checkout latest merged pulls"
(https://github.com/bitcoin/bitcoin/pull/33303#pullrequestreview-3185092065)
ACK fa8f944eaa1955e4e2c376ce36f1b1cbb1897769
The example given in the PR description https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49579638898?pr=29641#step:9:914 is a "re-run"
> Re-run workflows use the privileges of the actor who initially triggered the workflow, not the privileges of the actor who initiated the re-run. The workflow will also use the same GITHUB_SHA (commit SHA) and GITHUB_REF (git ref) of the original event that triggered the workflow run.
At a fir
...
(https://github.com/bitcoin/bitcoin/pull/33303#pullrequestreview-3185092065)
ACK fa8f944eaa1955e4e2c376ce36f1b1cbb1897769
The example given in the PR description https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49579638898?pr=29641#step:9:914 is a "re-run"
> Re-run workflows use the privileges of the actor who initially triggered the workflow, not the privileges of the actor who initiated the re-run. The workflow will also use the same GITHUB_SHA (commit SHA) and GITHUB_REF (git ref) of the original event that triggered the workflow run.
At a fir
...
📝 fanquake opened a pull request: "depends: strip when installing qt binaries"
(https://github.com/bitcoin/bitcoin/pull/33304)
Otherwise we end up with ~1.5GB of binaries (Linux) when `DEBUG=1`. This isn't great generally, but is worse in the CI, where disk may be limited (#33293).
(https://github.com/bitcoin/bitcoin/pull/33304)
Otherwise we end up with ~1.5GB of binaries (Linux) when `DEBUG=1`. This isn't great generally, but is worse in the CI, where disk may be limited (#33293).
👍 fanquake approved a pull request: "ci: Remove redundant RUN_UNIT_TESTS_SEQUENTIAL"
(https://github.com/bitcoin/bitcoin/pull/33136#pullrequestreview-3185106147)
ACK fae610d8581a1a0624b57fe0c2595c9695d677c8
(https://github.com/bitcoin/bitcoin/pull/33136#pullrequestreview-3185106147)
ACK fae610d8581a1a0624b57fe0c2595c9695d677c8
💬 fanquake commented on issue "ci: GHA fallback centos task runs out of space":
(https://github.com/bitcoin/bitcoin/issues/33293#issuecomment-3253562126)
> Yes we can try to free up space. I'll take a look where the space is being used and see what we can free.
#33304 should free up ~1.5GB of disk usage, an take some pressure off caching generally.
(https://github.com/bitcoin/bitcoin/issues/33293#issuecomment-3253562126)
> Yes we can try to free up space. I'll take a look where the space is being used and see what we can free.
#33304 should free up ~1.5GB of disk usage, an take some pressure off caching generally.
🚀 fanquake merged a pull request: "ci: Remove redundant RUN_UNIT_TESTS_SEQUENTIAL"
(https://github.com/bitcoin/bitcoin/pull/33136)
(https://github.com/bitcoin/bitcoin/pull/33136)
💬 hebasto commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253566957)
> The PR changes "re-run" default behaviour. Forcing it to use the new state instead of running the old state again.
I suggest making this clearer in the PR description.
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253566957)
> The PR changes "re-run" default behaviour. Forcing it to use the new state instead of running the old state again.
I suggest making this clearer in the PR description.
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3253651928)
`f400e0bb82...9dcbf6ccde`: fix a new case of non-loopback traffic from `node_init_tests/init_test`.
This PR originally contained a few fixes of tests that generated network traffic + a CI change to catch such future cases in CI. Then the tests fixes were moved to https://github.com/bitcoin/bitcoin/pull/31646 and merged. Then the activity here waned.
Now there is a new case of network traffic generated by a test which went in `master` unnoticed. I have fixed that and included it here.
`9
...
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3253651928)
`f400e0bb82...9dcbf6ccde`: fix a new case of non-loopback traffic from `node_init_tests/init_test`.
This PR originally contained a few fixes of tests that generated network traffic + a CI change to catch such future cases in CI. Then the tests fixes were moved to https://github.com/bitcoin/bitcoin/pull/31646 and merged. Then the activity here waned.
Now there is a new case of network traffic generated by a test which went in `master` unnoticed. I have fixed that and included it here.
`9
...