Bitcoin Core Github
44 subscribers
122K links
Download Telegram
fanquake closed a pull request: "Update SECURITY.md"
(https://github.com/bitcoin/bitcoin/pull/32362)
📝 fanquake locked a pull request: "Update SECURITY.md"
(https://github.com/bitcoin/bitcoin/pull/32362)
<!--
*** 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
...
📝 GarmashAlex opened 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
...
📝 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
...
🤔 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.
💬 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.
```
💬 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_
...
💬 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])
...
💬 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.
💬 maflcko commented on pull request "ci: Use previous releases in tests on Windows":
(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?
💬 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?
🤔 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
💬 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
...
💬 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
💬 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
📝 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.
💬 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
...
💬 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
...
👍 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
...