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

💬 stutxo commented on pull request "add ignore_incoming_blocks option to ProcessMessage and SendMessages":
(https://github.com/bitcoin/bitcoin/pull/32366#issuecomment-2835737982)
oops opened this PR to the wrong place soz, ignore pls

💬 hebasto commented on pull request "Fix missing error check in `set_clo_on_exec` for FD_CLOEXEC handling":
(https://github.com/bitcoin/bitcoin/pull/32342#issuecomment-2835789138)
> @hebasto I have upstreamed the changes as requested: [arun11299/cpp-subprocess#117](https://github.com/arun11299/cpp-subprocess/pull/117).

Thank you!

I've backported the merged upstream PR in https://github.com/bitcoin/bitcoin/pull/32358.

So this one can be closed, I think.

💬 hebasto commented on pull request "subprocess: Backport upstream changes":
(https://github.com/bitcoin/bitcoin/pull/32358#issuecomment-2835793736)
Added https://github.com/arun11299/cpp-subprocess/pull/117, which is upstreamed https://github.com/bitcoin/bitcoin/pull/32342.

💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-2835804766)
Updated a56468ab0bf2154159919a2c3feba9974486d10e -> 0b72656ecf363d9fc4820a9e3e22afb2c98d4e75 ([`pr/ipc-stop.2`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-stop.2) -> [`pr/ipc-stop.3`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-stop.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-stop.2..pr/ipc-stop.3)) to try to fix init test CI failures https://github.com/bitcoin/bitcoin/actions/runs/14670251419/job/41176416988?pr=32345 and https://cirrus-ci.com/task/60942682
...

💬 theuni commented on pull request "net: remove unnecessary check from AlreadyConnectedToAddress()":
(https://github.com/bitcoin/bitcoin/pull/32338#discussion_r2064058927)
I agree this is overkill.

📝 hebasto opened a pull request: "cmake: Check user-defined `APPEND_*FLAGS` variables early"
(https://github.com/bitcoin/bitcoin/pull/32367)
For the purpose of checks performed by the build system, we strive to handle user-defined `APPEND_*FLAGS` in the same way as flags provided by other standard means. In particular, they are considered when the `try_compile()` command is used.

However, these flags are not considered during `enable_language()` command invocation due to design limitations, which might cause issues, such as https://github.com/bitcoin/bitcoin/issues/32323.

This PR addresses the issue by introducing an additional
...

👍 theuni approved a pull request: "net: remove unnecessary check from AlreadyConnectedToAddress()"
(https://github.com/bitcoin/bitcoin/pull/32338#pullrequestreview-2799968661)
utACK f1b142856a4ecd0a0d90bc3d73ef5137997b14ff

As I mentioned above, I believe this was needed at one point when connecting to an IP through a proxy, or when using addnode for an IP, as those were stored as strings rather than resolved addresses.

But now that we always attempt to resolve ips, `m_addr_name` is only interesting for non-ip connections. So the check here should be redundant and unnecessary.

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

- Between blocks 877325 (01 Jan 2025) and 894289 (today) there's 30 non-std OP_RETURN txs out of 7 million, or 0.004246903% of the total.

- That's a 99.995753097% success rate for the 83 byte limit spam filter in 2025.