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

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2063932237)
> any reason why the verification flags don't also include SCRIPT_VERIFY_LOW_S?

I had been wondering about this as well. I think this flag should be included as the core wallet always produces low R/S values ECDSA signatures already ever since this change: #13666.

If I am not missing anything, might as well the wallet reject such PSBTs while unserializing?

📝 D33r-Gee opened a pull request: "interface: Expose load utxo snapshot functionality"
(https://github.com/bitcoin-core/gui/pull/869)
Expose load/activate AssumeUTXO snapshot functionaility so that it can be laoded through the GUI.

This can be tested and viewed in action in the qml repository with legacy code (https://github.com/bitcoin-core/gui-qml/pull/424)

💬 D33r-Gee commented on pull request "interface: Expose load utxo snapshot functionality":
(https://github.com/bitcoin-core/gui/pull/869#issuecomment-2835661664)
friendly ping @Sjors

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2063944522)
Also, I realise that it would be cool to pass a reference to the last argument to `CheckSignatureEncoding` that captures the error so that we can relay it forward to the user; the current error message (or its variation) can remain as a prefix.

🤔 jarolrod reviewed a pull request: "interface: Expose load utxo snapshot functionality"
(https://github.com/bitcoin-core/gui/pull/869#pullrequestreview-2799797924)
There's no consumer of this on the bitcoin-core/gui side, can you show this works with some sort of POC on the bitcoin-core/gui side?

How do we know this works

📝 stutxo opened a pull request: "add ignore_incoming_blocks option to ProcessMessage and SendMessages"
(https://github.com/bitcoin/bitcoin/pull/32366)

✅ stutxo closed a pull request: "add ignore_incoming_blocks option to ProcessMessage and SendMessages"
(https://github.com/bitcoin/bitcoin/pull/32366)

👍 hodlinator approved a pull request: "test: avoid stack overflow in `FindChallenges` via manual iteration"
(https://github.com/bitcoin/bitcoin/pull/32351#pullrequestreview-2799813040)
re-ACK 7e8ef959d0637ca5543ed33d3919937e0d053e70

nit: Would switch order of:
* e400ac53524d143467740e2f59698a7c94644c21 refactor: simplify repeated comparisons in `FindChallenges`
* f670836112c01feb3cb71618192e9c0c2e55767f test: remove old recursive `FindChallenges_recursive` implementation

This way CI should compare the `switch`-statement iterative version against the recursive one before it is removed. (Something I liked about what I last reviewed).

💬 hodlinator commented on pull request "test: avoid stack overflow in `FindChallenges` via manual iteration":
(https://github.com/bitcoin/bitcoin/pull/32351#discussion_r2063969451)
nit:
If you re-touch, it would be nicer to abide by the Sonar suggestion "...or a reference" with:
```suggestion
const auto challenges{FindChallenges(*node)}; // Find all challenges in the generated miniscript.
```
and
```C++
std::set<Challenge> FindChallenges(const Node& root)
```
to signify `nullptr` being unsupported.