💬 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
...

💬 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

💬 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
...

💬 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
...

👍 fanquake approved a pull request: "depends: Fix cross-compiling `qt` package from macOS to Windows"
(https://github.com/bitcoin/bitcoin/pull/32357#pullrequestreview-2799684867)
ACK 35e57fbe336cdcb348ff088fc04216f1f5cf2742

✅ fanquake closed an issue: "build: Windows cross Qt broken on macOS host"
(https://github.com/bitcoin/bitcoin/issues/32346)

🚀 fanquake merged a pull request: "depends: Fix cross-compiling `qt` package from macOS to Windows"
(https://github.com/bitcoin/bitcoin/pull/32357)

📝 maflcko opened a pull request: " descriptors: Reject + sign while parsing unsigned"
(https://github.com/bitcoin/bitcoin/pull/32365)
As a follow-up to https://github.com/bitcoin/bitcoin/pull/30577, reject `+` for unsigned values in key-path parsing and multi threshold parsing as well.

Both of those are using unsigned, and Bitcoin Core would never serialize a descriptor string with a stray `+`. Accepting stray `+` signs could lead to checksum mismatches, or other incompatibilities later on.

Just like https://github.com/bitcoin/bitcoin/pull/30577, both changes are breaking changes on the RPC interface, but hopefully no on
...

💬 ryanofsky commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2835616043)
Thank you! Learned something new. I did try searching for error, but I guess I used the wrong search, and somehow I missed those lines when I was scrolling through manually. Glad to see the CI job is actually providing enough information and I just didn't understand the UI. Agree it could be nice if the script made the errors more obvious, but current behavior is also reasonably good.

💬 theuni commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2835618558)
> I'd appreciate it. It would be good to cross `m_nodes_mutex` off the [list](https://github.com/bitcoin/bitcoin/issues/19303).

Yeah, agreed. That's a good justification for the change to non-recursive here.

Looking at that diff, it's not clear to me if it's safe to move that hunk out of `cs_main`, but we can review/test/argue about it in a new PR.

💬 Fiach-Dubh commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2835635859)
NACK

Removing long time, policy level, end user config option choice is an extreme option.

Core shouldn't be in the business of compelling speech at a policy level from it's users unless it's interested in losing users, which is already happening for related reasons.

When you remove an end users choice to configure their policy you are doing harm to that user. Please don't harm end users by nixing these config options altogether.

Thank you for the work you all do.

💬 wizkid057 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2835638919)
> @wizkid057 your comment borders on off-topic, but I'm not going to hide it

Thanks. As a general note I want to briefly point out that I've personally been a long time contributor to the Bitcoin ecosystem as a whole. Longer than many/most active devs here. While my contributions haven't been directly to Bitcoin Core code to-date (although there is certainly code of mine passed through to it from me through others from long before I had any need for recognition for such things), there's 10,00
...