📝 hebasto reopened a pull request: "ci: Use previous releases in tests on Windows"
(https://github.com/bitcoin/bitcoin/pull/32363)
Currently, none of the CI jobs running on Windows (Windows native and Windows test cross-built) use previous releases for testing. This is causing problems with tests like `wallet_migration.py` which require previous releases to be available.
This PR fixes issue #32192 by implementing the following changes:
1. Added Windows binaries to the SHA256_SUMS dictionary in get_previous_releases.py
2. Modified the check_host function to recognize Windows hosts and map them to the "win64" platform
...
(https://github.com/bitcoin/bitcoin/pull/32363)
Currently, none of the CI jobs running on Windows (Windows native and Windows test cross-built) use previous releases for testing. This is causing problems with tests like `wallet_migration.py` which require previous releases to be available.
This PR fixes issue #32192 by implementing the following changes:
1. Added Windows binaries to the SHA256_SUMS dictionary in get_previous_releases.py
2. Modified the check_host function to recognize Windows hosts and map them to the "win64" platform
...
🤔 Sjors reviewed a pull request: "Remove arbitrary limits on OP_Return (datacarrier) outputs"
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2799281057)
cd7872ca543d31d20f419fd2203138b8301c2e68 looks good other than what I said above: we should make sure the push-only standardness rule is both tested and illustrated. See inline suggestions.
This PR removes more (test) code than I expected, which is good.
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2799281057)
cd7872ca543d31d20f419fd2203138b8301c2e68 looks good other than what I said above: we should make sure the push-only standardness rule is both tested and illustrated. See inline suggestions.
This PR removes more (test) code than I expected, which is good.
💬 Sjors commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2063718969)
```cpp
// Until v30 OP_RETURN was limited to 83 bytes (80 bytes of data, +1 for
// OP_RETURN, +2 for the pushdata opcodes). Add one more byte.
```
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2063718969)
```cpp
// Until v30 OP_RETURN was limited to 83 bytes (80 bytes of data, +1 for
// OP_RETURN, +2 for the pushdata opcodes). Add one more byte.
```
💬 Sjors commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2063646275)
I found myself confused why this restart is still needed. It's because the previous test used `-persistmempool=0` and we need to forget about the transaction it created.
```diff
diff --git a/test/functional/mining_basic.py b/test/functional/mining_basic.py
index cbb1cf789e..85c0f38291 100755
--- a/test/functional/mining_basic.py
+++ b/test/functional/mining_basic.py
@@ -186,6 +186,9 @@ class MiningTest(BitcoinTestFramework):
assert tx_below_min_feerate['txid'] not in block_
...
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2063646275)
I found myself confused why this restart is still needed. It's because the previous test used `-persistmempool=0` and we need to forget about the transaction it created.
```diff
diff --git a/test/functional/mining_basic.py b/test/functional/mining_basic.py
index cbb1cf789e..85c0f38291 100755
--- a/test/functional/mining_basic.py
+++ b/test/functional/mining_basic.py
@@ -186,6 +186,9 @@ class MiningTest(BitcoinTestFramework):
assert tx_below_min_feerate['txid'] not in block_
...
💬 Sjors commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2063714504)
```diff
diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
index f4b21cd204..609d826a6f 100755
--- a/test/functional/mempool_accept.py
+++ b/test/functional/mempool_accept.py
@@ -327,6 +327,26 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
rawtxs=[tx.serialize().hex()],
)
+ # OP_RETURN followed by non-push
+ tx = tx_from_hex(raw_tx_reference)
+ tx.vout[0].scriptPubKey = CScript([OP_RETURN, OP_HASH160])
...
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2063714504)
```diff
diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
index f4b21cd204..609d826a6f 100755
--- a/test/functional/mempool_accept.py
+++ b/test/functional/mempool_accept.py
@@ -327,6 +327,26 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
rawtxs=[tx.serialize().hex()],
)
+ # OP_RETURN followed by non-push
+ tx = tx_from_hex(raw_tx_reference)
+ tx.vout[0].scriptPubKey = CScript([OP_RETURN, OP_HASH160])
...
💬 l0rinc commented on pull request "test: avoid stack overflow in `FindChallenges` via manual iteration":
(https://github.com/bitcoin/bitcoin/pull/32351#issuecomment-2835394763)
> Refactoring changes, such as introducing the switch statement, could be added in a separate commit
Good idea, done in latest push.
I have also checked https://corecheck.dev/bitcoin/bitcoin/pulls/32351 and saw that Sonar was also compaining about https://sonarcloud.io/organizations/aureleoules/rules?open=cpp%3AS4998&rule_key=cpp%3AS4998, addressed it also in a separate commit.
(https://github.com/bitcoin/bitcoin/pull/32351#issuecomment-2835394763)
> Refactoring changes, such as introducing the switch statement, could be added in a separate commit
Good idea, done in latest push.
I have also checked https://corecheck.dev/bitcoin/bitcoin/pulls/32351 and saw that Sonar was also compaining about https://sonarcloud.io/organizations/aureleoules/rules?open=cpp%3AS4998&rule_key=cpp%3AS4998, addressed it also in a separate commit.
💬 maflcko commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2063756157)
no-op?
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2063756157)
no-op?
💬 maflcko commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2063748613)
not sure about running the test separately and then excluding it separately below. Just remove the exclusion?
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2063748613)
not sure about running the test separately and then excluding it separately below. Just remove the exclusion?
💬 maflcko commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2063753216)
I don't think any wallet files are opened in this test, let alone without a context manager. Why would this be needed?
(https://github.com/bitcoin/bitcoin/pull/32363#discussion_r2063753216)
I don't think any wallet files are opened in this test, let alone without a context manager. Why would this be needed?
🤔 sipa reviewed a pull request: "test: avoid stack overflow in `FindChallenges` via manual iteration"
(https://github.com/bitcoin/bitcoin/pull/32351#pullrequestreview-2799484506)
utACK 7e8ef959d0637ca5543ed33d3919937e0d053e70
(https://github.com/bitcoin/bitcoin/pull/32351#pullrequestreview-2799484506)
utACK 7e8ef959d0637ca5543ed33d3919937e0d053e70
💬 wizkid057 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2835467933)
> Moderation suggestion: close to non-contributors.
This is far more than a small technical change, the broader community should not be shut out of the discussion or shuffled off to the mailing list. This is a fundamental change to the nature of what the Bitcoin network itself is in its entirety.
- Merging this PR means Bitcoin is no longer a decentralized currency.
- Merging this PR means Bitcoin is no longer money.
- Merging this PR means Bitcoin is no longer a store of value.
- Merg
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2835467933)
> Moderation suggestion: close to non-contributors.
This is far more than a small technical change, the broader community should not be shut out of the discussion or shuffled off to the mailing list. This is a fundamental change to the nature of what the Bitcoin network itself is in its entirety.
- Merging this PR means Bitcoin is no longer a decentralized currency.
- Merging this PR means Bitcoin is no longer money.
- Merging this PR means Bitcoin is no longer a store of value.
- Merg
...
💬 GarmashAlex commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#issuecomment-2835493635)
@maflcko I've made some corrections
(https://github.com/bitcoin/bitcoin/pull/32363#issuecomment-2835493635)
@maflcko I've made some corrections
💬 maflcko commented on pull request "ci: Use previous releases in tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/32363#issuecomment-2835504950)
Please don't ask for review before CI is passing
Please link to a passing CI in your project.
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
(https://github.com/bitcoin/bitcoin/pull/32363#issuecomment-2835504950)
Please don't ask for review before CI is passing
Please link to a passing CI in your project.
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
📝 stickies-v opened a pull request: "refactor: validation: mark CheckBlockIndex as const "
(https://github.com/bitcoin/bitcoin/pull/32364)
While reviewing another PR, I [noticed](https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2056509235) that `ChainstateManager::CheckBlockIndex()` is not a `const` method. To try and assert that this method was not causing any side-effects, I modified the method to make it `const`. It did not surface any errors, but I think it would be good to merge this change regardless, even if `CheckBlockIndex` is only used in regtest.
(https://github.com/bitcoin/bitcoin/pull/32364)
While reviewing another PR, I [noticed](https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2056509235) that `ChainstateManager::CheckBlockIndex()` is not a `const` method. To try and assert that this method was not causing any side-effects, I modified the method to make it `const`. It did not surface any errors, but I think it would be good to merge this change regardless, even if `CheckBlockIndex` is only used in regtest.
💬 ryanofsky commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2835521038)
I'm seeing a failure in this CI job that just says `*** 3 failures are detected in the test module "Bitcoin Core Test Suite"` but doesn't seem to give a more specific indication of where the failures are happening:
https://github.com/bitcoin/bitcoin/actions/runs/14670251419/job/41176416988?pr=32345#step:5:1701
Is this expected? Am I maybe missing some clues in the output? Could I maybe add some logging to my PR or to the CI job generally that would make this failure or other failures easie
...
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2835521038)
I'm seeing a failure in this CI job that just says `*** 3 failures are detected in the test module "Bitcoin Core Test Suite"` but doesn't seem to give a more specific indication of where the failures are happening:
https://github.com/bitcoin/bitcoin/actions/runs/14670251419/job/41176416988?pr=32345#step:5:1701
Is this expected? Am I maybe missing some clues in the output? Could I maybe add some logging to my PR or to the CI job generally that would make this failure or other failures easie
...
💬 pinheadmz commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2835535199)
@wizkid057 your comment borders on off-topic, but I'm not going to hide it because at this point in the PR thread it is new information. This will be last comment allowed with this information though -- meaning all future comments talking about the end of decentralized money as we know it will be considered redundant brigadeing. Everyone else who agrees with you can 👍 your comment instead of rewriting it in their own words. I also encourage you to take longer arguments like this to the mailing
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2835535199)
@wizkid057 your comment borders on off-topic, but I'm not going to hide it because at this point in the PR thread it is new information. This will be last comment allowed with this information though -- meaning all future comments talking about the end of decentralized money as we know it will be considered redundant brigadeing. Everyone else who agrees with you can 👍 your comment instead of rewriting it in their own words. I also encourage you to take longer arguments like this to the mailing
...
👍 darosior approved a pull request: "Remove arbitrary limits on OP_Return (datacarrier) outputs"
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2799422333)
ACK cd7872ca543d31d20f419fd2203138b8301c2e68
You missed a mention of `-datacarriersize` which i think should be deleted. Although `IsStandard` is already checked for these outputs in the unit tests, i think it is worth having a functional test which exercises both cases as well as tests the bounds (here hitting the maximum standard transaction size). Here is a diff which adds those cases:
<details>
<summary>Click to see diff</summary>
```diff
diff --git a/test/functional/mempool_accep
...
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2799422333)
ACK cd7872ca543d31d20f419fd2203138b8301c2e68
You missed a mention of `-datacarriersize` which i think should be deleted. Although `IsStandard` is already checked for these outputs in the unit tests, i think it is worth having a functional test which exercises both cases as well as tests the bounds (here hitting the maximum standard transaction size). Here is a diff which adds those cases:
<details>
<summary>Click to see diff</summary>
```diff
diff --git a/test/functional/mempool_accep
...
💬 darosior commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2063731685)
Could remove the documented requirement in `fill_mempool()` as well. https://github.com/bitcoin/bitcoin/blob/f409444d024340d58fb3a7d6c44329e7dbdb3857/test/functional/test_framework/mempool_util.py#L44
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2063731685)
Could remove the documented requirement in `fill_mempool()` as well. https://github.com/bitcoin/bitcoin/blob/f409444d024340d58fb3a7d6c44329e7dbdb3857/test/functional/test_framework/mempool_util.py#L44
💬 fanquake commented on pull request "depends: Fix cross-compiling `qt` package from macOS to Windows":
(https://github.com/bitcoin/bitcoin/pull/32357#issuecomment-2835570768)
Guix Build:
```bash
8ba76371ec4b297da8d29674c7ec31a01eead8131a8e0bbf9bc7e66fb4e1adff guix-build-35e57fbe336c/output/aarch64-linux-gnu/SHA256SUMS.part
d16137ebd3ca5eabb9614a6d4c322e05c43727621d1b8eca98ef4321c5964f21 guix-build-35e57fbe336c/output/aarch64-linux-gnu/bitcoin-35e57fbe336c-aarch64-linux-gnu-debug.tar.gz
3f0078d7df0a54497a52541f2252a52d5b50737b96b83f88d8f2aecd76b54cca guix-build-35e57fbe336c/output/aarch64-linux-gnu/bitcoin-35e57fbe336c-aarch64-linux-gnu.tar.gz
778792dcea8b0e82
...
(https://github.com/bitcoin/bitcoin/pull/32357#issuecomment-2835570768)
Guix Build:
```bash
8ba76371ec4b297da8d29674c7ec31a01eead8131a8e0bbf9bc7e66fb4e1adff guix-build-35e57fbe336c/output/aarch64-linux-gnu/SHA256SUMS.part
d16137ebd3ca5eabb9614a6d4c322e05c43727621d1b8eca98ef4321c5964f21 guix-build-35e57fbe336c/output/aarch64-linux-gnu/bitcoin-35e57fbe336c-aarch64-linux-gnu-debug.tar.gz
3f0078d7df0a54497a52541f2252a52d5b50737b96b83f88d8f2aecd76b54cca guix-build-35e57fbe336c/output/aarch64-linux-gnu/bitcoin-35e57fbe336c-aarch64-linux-gnu.tar.gz
778792dcea8b0e82
...
💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2835578794)
> Is this expected?
The GitHub CI log handling is horrible, because you can't use normal browser workflows to search for errors in the log. You'll have to download the raw log or use the GitHub log search feature. The string you are looking for is `error: in`.
This gives: https://github.com/bitcoin/bitcoin/actions/runs/14670251419/job/41176416988?pr=32345#step:5:1004
```
./test/net_tests.cpp(707): error: in "net_tests/LimitedAndReachable_Network": check g_reachable_nets.Contains(NET_ON
...
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2835578794)
> Is this expected?
The GitHub CI log handling is horrible, because you can't use normal browser workflows to search for errors in the log. You'll have to download the raw log or use the GitHub log search feature. The string you are looking for is `error: in`.
This gives: https://github.com/bitcoin/bitcoin/actions/runs/14670251419/job/41176416988?pr=32345#step:5:1004
```
./test/net_tests.cpp(707): error: in "net_tests/LimitedAndReachable_Network": check g_reachable_nets.Contains(NET_ON
...