👍 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
(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)
(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)
(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
...
(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.
(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.
(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.
(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
...
(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?
(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)
(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
(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.
(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
(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)
(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)
(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).
(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.
(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
(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.
(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.
(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.