💬 hodlinator commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2321157369)
Done.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2321157369)
Done.
💬 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_r2321223622)
I see, I had missed the connection between the two commits.
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2321223622)
I see, I had missed the connection between the two commits.
💬 maflcko commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3252448008)
When testing the GHA caching, it seems down (at least yesterday and today):
https://github.com/maflcko/bitcoin-core-qa-assets/actions/runs/17436331249/job/49574416227#step:7:173
```
+ ./ci/test/02_run_container.sh
+ '[' -z '' ']'
+ MAYBE_CPUSET=
+ '[' '' ']'
+ echo 'Creating mirror.gcr.io/ubuntu:24.04 container to run in'
+ docker buildx build --file /home/runner/work/bitcoin-core-qa-assets/bitcoin-core-qa-assets/ci/test_imagefile --build-arg CI_IMAGE_NAME_TAG=mirror.gcr.io/ubuntu:24.04 --buil
...
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3252448008)
When testing the GHA caching, it seems down (at least yesterday and today):
https://github.com/maflcko/bitcoin-core-qa-assets/actions/runs/17436331249/job/49574416227#step:7:173
```
+ ./ci/test/02_run_container.sh
+ '[' -z '' ']'
+ MAYBE_CPUSET=
+ '[' '' ']'
+ echo 'Creating mirror.gcr.io/ubuntu:24.04 container to run in'
+ docker buildx build --file /home/runner/work/bitcoin-core-qa-assets/bitcoin-core-qa-assets/ci/test_imagefile --build-arg CI_IMAGE_NAME_TAG=mirror.gcr.io/ubuntu:24.04 --buil
...
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3252476641)
`4652f75bbf...f400e0bb82`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3252476641)
`4652f75bbf...f400e0bb82`: rebase due to conflicts
💬 maflcko commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3252544945)
Interestingly, the armhf task is using GHA runners even in this repo, and it fails with a different error message:
https://github.com/bitcoin/bitcoin/actions/runs/17447989750/job/49546894060?pr=33300#step:8:154:
```
+ echo 'Creating mirror.gcr.io/ubuntu:24.04 container to run in'
+ docker buildx build --file /home/runner/work/bitcoin/bitcoin/ci/test_imagefile --build-arg CI_IMAGE_NAME_TAG=mirror.gcr.io/ubuntu:24.04 --build-arg FILE_ENV=./ci/test/00_setup_env_arm.sh --build-arg BASE_ROOT_DIR=/h
...
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3252544945)
Interestingly, the armhf task is using GHA runners even in this repo, and it fails with a different error message:
https://github.com/bitcoin/bitcoin/actions/runs/17447989750/job/49546894060?pr=33300#step:8:154:
```
+ echo 'Creating mirror.gcr.io/ubuntu:24.04 container to run in'
+ docker buildx build --file /home/runner/work/bitcoin/bitcoin/ci/test_imagefile --build-arg CI_IMAGE_NAME_TAG=mirror.gcr.io/ubuntu:24.04 --build-arg FILE_ENV=./ci/test/00_setup_env_arm.sh --build-arg BASE_ROOT_DIR=/h
...
💬 maflcko commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3252572043)
For peak confusion, another task claims to have read the cache, even though I don't see any task that has created a cache, nor a cache in https://github.com/bitcoin/bitcoin/actions/caches.
https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49576315120?pr=29641#step:7:153:
```
+ echo 'Creating mirror.gcr.io/ubuntu:24.04 container to run in'
+ docker buildx build --file /home/admin/actions-runner/_work/bitcoin/bitcoin/ci/test_imagefile --build-arg CI_IMAGE_NAME_TAG=mirror.gcr.io/ubu
...
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3252572043)
For peak confusion, another task claims to have read the cache, even though I don't see any task that has created a cache, nor a cache in https://github.com/bitcoin/bitcoin/actions/caches.
https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49576315120?pr=29641#step:7:153:
```
+ echo 'Creating mirror.gcr.io/ubuntu:24.04 container to run in'
+ docker buildx build --file /home/admin/actions-runner/_work/bitcoin/bitcoin/ci/test_imagefile --build-arg CI_IMAGE_NAME_TAG=mirror.gcr.io/ubu
...
👋 fanquake's pull request is ready for review: "[29.x] *san CI backports"
(https://github.com/bitcoin/bitcoin/pull/33294)
(https://github.com/bitcoin/bitcoin/pull/33294)
💬 dergoegge commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#issuecomment-3252595782)
> Is my understanding correct that AFL will run init (create the static dir), then fork into several fuzz processes, which run for N iterations and then shut down and delete the static dir? (On the next re-fork, it will fail to find the static dir)?
We removed the `__AFL_INIT` call, so it will actually fork prior to `init`, so I think all that's needed is to randomize the static dir name as you suggested as well.
(https://github.com/bitcoin/bitcoin/pull/33300#issuecomment-3252595782)
> Is my understanding correct that AFL will run init (create the static dir), then fork into several fuzz processes, which run for N iterations and then shut down and delete the static dir? (On the next re-fork, it will fail to find the static dir)?
We removed the `__AFL_INIT` call, so it will actually fork prior to `init`, so I think all that's needed is to randomize the static dir name as you suggested as well.
💬 dergoegge commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2321358588)
Wondering if we should always wipe the `partialBlock` (i.e. downgrade the request to a regular block request) when `FillBlock` fails. Then it would no longer be possible to process two `blocktxn` messages for the same block.
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2321358588)
Wondering if we should always wipe the `partialBlock` (i.e. downgrade the request to a regular block request) when `FillBlock` fails. Then it would no longer be possible to process two `blocktxn` messages for the same block.
🚀 fanquake merged a pull request: "build: set ENABLE_IPC to OFF when fuzzing"
(https://github.com/bitcoin/bitcoin/pull/33235)
(https://github.com/bitcoin/bitcoin/pull/33235)
💬 ismaelsadeeq commented on pull request "mini miner: enable `Linearize` return package feerates":
(https://github.com/bitcoin/bitcoin/pull/33216#issuecomment-3252627413)
Yes it is used in #30079
(https://github.com/bitcoin/bitcoin/pull/33216#issuecomment-3252627413)
Yes it is used in #30079
💬 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_r2321364576)
I don't believe rescanning is skipped here at the moment. The following assertion passes, whereas putting it in the `unexpected` section throws.
```diff
diff --git a/test/functional/wallet_listtransactions.py b/test/functional/wallet_listtransactions.py
index 714e659465..b590ed95d4 100755
--- a/test/functional/wallet_listtransactions.py
+++ b/test/functional/wallet_listtransactions.py
@@ -352,7 +352,8 @@ class ListTransactionsTest(BitcoinTestFramework):
self.nodes[0].se
...
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2321364576)
I don't believe rescanning is skipped here at the moment. The following assertion passes, whereas putting it in the `unexpected` section throws.
```diff
diff --git a/test/functional/wallet_listtransactions.py b/test/functional/wallet_listtransactions.py
index 714e659465..b590ed95d4 100755
--- a/test/functional/wallet_listtransactions.py
+++ b/test/functional/wallet_listtransactions.py
@@ -352,7 +352,8 @@ class ListTransactionsTest(BitcoinTestFramework):
self.nodes[0].se
...
💬 willcl-ark commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3252638785)
Yes, I've seen GHA caching work, will try and find logs.
to your second comment, nice catch! When we switched back to using a combination of GH runners (for arm 32bit) and cirrus for the rest, we don't handle configuring docker differently. I will make a PR for this.
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3252638785)
Yes, I've seen GHA caching work, will try and find logs.
to your second comment, nice catch! When we switched back to using a combination of GH runners (for arm 32bit) and cirrus for the rest, we don't handle configuring docker differently. I will make a PR for this.
⚠️ fanquake opened an issue: "natpmp: quiet down unconditional logging"
(https://github.com/bitcoin/bitcoin/issues/33301)
Mentioned by @mzumsande here: https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3249980322
> I see unconditional log messages [net:warning] pcp: Could not receive response: Connection refused (111) every 5 minutes (in an environment where PCP is expected to not work), and it's a bit spammy. How about a exponential backoff, similar to how it's done in torcontrol.cpp?
cc @darosior @instagibbs
(https://github.com/bitcoin/bitcoin/issues/33301)
Mentioned by @mzumsande here: https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3249980322
> I see unconditional log messages [net:warning] pcp: Could not receive response: Connection refused (111) every 5 minutes (in an environment where PCP is expected to not work), and it's a bit spammy. How about a exponential backoff, similar to how it's done in torcontrol.cpp?
cc @darosior @instagibbs
💬 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_r2321381955)
Fair point, this suggestion was not correct.
The intent of the suggestion was more for the cosmetic angle so that the reader doesn't wonder why a standalone RPC is here without the presence of an assertion or usage of the returned value. Maybe there is an opportunity to introduce an `assert_no_rpc_error` helper here but need not be done now.
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2321381955)
Fair point, this suggestion was not correct.
The intent of the suggestion was more for the cosmetic angle so that the reader doesn't wonder why a standalone RPC is here without the presence of an assertion or usage of the returned value. Maybe there is an opportunity to introduce an `assert_no_rpc_error` helper here but need not be done now.
📝 willcl-ark opened a pull request: "ci: disable cirrus cache in 32bit arm job"
(https://github.com/bitcoin/bitcoin/pull/33302)
Add an optional matrix field allowing opt-out of configuring cirrus GHA cache when not using cirrus runners.
This is not needed for the cirruslabs/[save|restore]-cache actions, as they automatically fallback based on runner type.
Addresses https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3252638785
(https://github.com/bitcoin/bitcoin/pull/33302)
Add an optional matrix field allowing opt-out of configuring cirrus GHA cache when not using cirrus runners.
This is not needed for the cirruslabs/[save|restore]-cache actions, as they automatically fallback based on runner type.
Addresses https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3252638785
💬 willcl-ark commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3252703194)
Opened https://github.com/bitcoin/bitcoin/pull/33302 which should sort this out.
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3252703194)
Opened https://github.com/bitcoin/bitcoin/pull/33302 which should sort this out.
💬 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_r2321395587)
```
const [Params = <char[15], int>]] [fromme] RescanFromTime: Rescanning last 7 blocks
```
Ah, I see now that last 7 blocks are rescanned and 10 blocks are generated previously - so the transaction is indeed not rescanned.
Maybe the following diff for clarity:
```diff
- # Mock time forward and generate blocks so that the import does not rescan the transaction
+ # Mock time forward and generate enough blocks so that the import does not rescan the transaction
```
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2321395587)
```
const [Params = <char[15], int>]] [fromme] RescanFromTime: Rescanning last 7 blocks
```
Ah, I see now that last 7 blocks are rescanned and 10 blocks are generated previously - so the transaction is indeed not rescanned.
Maybe the following diff for clarity:
```diff
- # Mock time forward and generate blocks so that the import does not rescan the transaction
+ # Mock time forward and generate enough blocks so that the import does not rescan the transaction
```
💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2321395923)
Missing `return ` here.
This will lead to `ReadRawTransaction()` being called anyway and in my testing it returns an empty vector, which we then discover on line 533 and on the next line we try to use `req` again but internal pointers have been reset to null by the line of this comment, leading to a crash.
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2321395923)
Missing `return ` here.
This will lead to `ReadRawTransaction()` being called anyway and in my testing it returns an empty vector, which we then discover on line 533 and on the next line we try to use `req` again but internal pointers have been reset to null by the line of this comment, leading to a crash.
💬 purpleKarrot commented on pull request "cmake: Inherit WERROR setting for secp256k1 build":
(https://github.com/bitcoin/bitcoin/pull/33297#issuecomment-3252744634)
I am going to NACK every PR that adds even more `-Werror`. 30f642952cb5bf39479bdbe467b3950f0d09324a was a mistake and should be reverted. The correct approach to this is [`CMAKE_COMPILE_WARNING_AS_ERROR`](https://cmake.org/cmake/help/latest/variable/CMAKE_COMPILE_WARNING_AS_ERROR.html).
Yes, that requires CMake 3.24 and we allow 3.22 as the minimum. It does not matter. Just accept that the setting will have no effect on Ubuntu 22.04. :man_shrugging:
cc @hebasto
(https://github.com/bitcoin/bitcoin/pull/33297#issuecomment-3252744634)
I am going to NACK every PR that adds even more `-Werror`. 30f642952cb5bf39479bdbe467b3950f0d09324a was a mistake and should be reverted. The correct approach to this is [`CMAKE_COMPILE_WARNING_AS_ERROR`](https://cmake.org/cmake/help/latest/variable/CMAKE_COMPILE_WARNING_AS_ERROR.html).
Yes, that requires CMake 3.24 and we allow 3.22 as the minimum. It does not matter. Just accept that the setting will have no effect on Ubuntu 22.04. :man_shrugging:
cc @hebasto