💬 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_r2061250917)
Nit: Maybe can nest these 2 checks within a `utxo.scriptPubKey.IsPayToTaproot()` check because these are taproot specific sigs. Using `SIGHASH_DEFAULT` in the above `if` implies it's a taproot input but this `else` can contain either.

💬 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_r2061248409)
IMO it'd be nice to extract this if-else piece out in a function called `ValidatePSBTInputSigsWithSighash` or similar that takes the `input` and the `sighash` as arguments.

Though making `SignPSBTInput` shorter is a benefit, suggesting this mostly to emphasise (to the reader) on what's happening here.

💬 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_r2061251688)
I gather from the interpreter code that these 2 checks are testing for the fixed length of 64 in case of Schnorr signatures if sighash is absent, which is ok but it'd be nice to have a constant for `64` to make it explicit to the reader.
https://github.com/bitcoin/bitcoin/blob/de90b47ea09826d52043f4ce40c734847187c075/src/script/interpreter.cpp#L1673-L1698

Maybe an enum `SCHNORR_SIG_LENGTH`?

```cpp
enum SCHNORR_SIG_LENGTH: size_t {
WITHOUT_HASHTYPE = 64,
WITH_HASHTYPE = 65,
}
```

👍 rkrux approved a pull request: "test: Test that migration automatically repairs corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124#pullrequestreview-2795939653)
re-ACK c7e2b9e2644442b147880becb8a659f3d00092d9

```
git range-diff bfcf5c5ab1fdba46d492aa0872c3d5f9a4a87633..c7e2b9e2644442b147880becb8a659f3d00092d9
```

The range-diff contains only rebase changes now that #31250 is merged.

📝 hebasto opened a pull request: "cmake: Do not override flags set by the user when replacing them"
(https://github.com/bitcoin/bitcoin/pull/32356)
This PR addresses [this](https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2542140874) comment:
> I suppose that should only happen if the `-O3` isn't coming from an explicitly set `CMAKE_CXX_FLAGS_RELEASE`.

With this PR:
```
$ cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O3"
<snip>
C++ compiler flags .................... -O3 -std=c++20 -fPIC -fno-extended-identifiers -fdebug-prefix-map=/home/hebasto/dev/bitcoin/src=. -fmacro-prefix-map=/home/hebasto/d
...

💬 hebasto commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2832056951)
@theuni

> Ah, I see. It's: `replace_cxx_flag_in_config(Release -O3 -O2)`
>
> I suppose that should only happen if the `-O3` isn't coming from an explicitly set `CMAKE_CXX_FLAGS_RELEASE`.

Fixed in https://github.com/bitcoin/bitcoin/pull/32356.

💬 hebasto commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2832107817)
> Using master @ [d73f37d](https://github.com/bitcoin/bitcoin/commit/d73f37dda221835b5109ede1b84db2dc7c4b74a1).
>
> The following seems incorrect, because
> ```
> make -C depends/ NO_QT=1 NO_WALLET=1 NO_ZMQ=1 -j18 CFLAGS="-O3" CXXFLAGS="-O3"
> cmake -B build --toolchain /root/ci_scratch/depends/aarch64-unknown-linux-gnu/toolchain.cmake
> <snip>
> C++ compiler flags .................... -O3 -O2 -g
> Linker flags .......................... -O3 -O2 -g
> ```
> is overriding the user-provided `-O3`.
...

👋 Bue-von-hon's pull request is ready for review: "rpc: Support v3 raw transactions creation"
(https://github.com/bitcoin/bitcoin/pull/31936)

💬 Bue-von-hon commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2832113486)
@luke-jr @instagibbs @maflcko @rkrux @glozow @adamandrews1 @w0xlt @1440000bytes
PTAL 🙏

💬 hebasto commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2832116385)
> Is making `m_nodes_mutex` non-recursive a goal? That sounds nice to me and I'm not opposed, but I'd rather not make changes just to get halfway there.

I'm curious how far we are from a non-recursive `m_nodes_mutex`?

👍 hebasto approved a pull request: "Crash fix, disconnect numBlocksChanged() signal during shutdown"
(https://github.com/bitcoin-core/gui/pull/864#pullrequestreview-2795975241)
ACK 71656bdfaa6bfe08ce9651246a3ef606f923351b, I have reviewed the code and it looks OK.

💬 l0rinc commented on pull request "cmake: Do not override flags set by the user when replacing them":
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2832127762)
Concept ACK

Nit: the title is a bit ambiguous, not sure what "replacing them" mean in this context

✅ hebasto closed an issue: "Segfault during shutdown in SendCoinsDialog::updateCoinControlState"
(https://github.com/bitcoin-core/gui/issues/862)

🚀 hebasto merged a pull request: "Crash fix, disconnect numBlocksChanged() signal during shutdown"
(https://github.com/bitcoin-core/gui/pull/864)

💬 hebasto commented on pull request "[29.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/32292#issuecomment-2832134070)
> Could also include [bitcoin-core/gui#864](https://github.com/bitcoin-core/gui/pull/864), possibly?

+1

💬 hebasto commented on pull request "cmake: Do not override flags set by the user when replacing them":
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2832136978)
> Concept ACK
>
> Nit: the title is a bit ambiguous, not sure what "replacing them" mean in this context

I meant "we try to replace `-O3` with `-O2` but only when `CMAKE_CXX_FLAGS_RELEASE` is not specified by the user explicitly". I'll be happy to adjust wording for a better suggestion.

💬 Eunovo commented on pull request "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor":
(https://github.com/bitcoin/bitcoin/pull/32344#discussion_r2061300483)
Calling AddWalletDescriptor with a descriptor that's already in wallet, calls UpdateWalletDescriptor which calls CanUpdateWalletDescriptor. This is what happens on the second call to AddWalletDescriptor.

💬 l0rinc commented on pull request "cmake: Do not override flags set by the user when replacing them":
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2832203043)
How about:
```
cmake: Respect user-provided compiler optimization level
```

💬 jamesob commented on pull request "BIP-348 (OP_CHECKSIGFROMSTACK) (regtest only)":
(https://github.com/bitcoin/bitcoin/pull/32247#issuecomment-2832242691)
Thanks @instagibbs; I've compressed your test changes into a single commit and added that here.

🤔 theStack reviewed a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2796051128)
Concept ACK

Seems like the commit message for 2e6dcdbc8055660a2e20ba81b62b7d26ae0ccb05 ("Add MuSig2 Keyagg Cache class and functions") is out-of-sync, as there is no such class added and also the mentioned `MuSig2KeyAggCacheImpl` doesn't exist.

For the newly introduced parameters of the `Const` and `Split` functions (commits fe02d7cb237c00de6abe1776b3101342ffddf757 and 4a1eeee27a64c4be7293740f8fff839879b88d86), it would be nice to have unit test coverage, but this can be also done in a fol
...