💬 shahsb 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_r2061229822)
I do not see the call to the function `DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor()` being made in this test case?

💬 shahsb 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#issuecomment-2831976141)
Concept ACK

💬 hebasto commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2831979533)
> [@hebasto](https://github.com/hebasto) can you followup with all the questions asked in this thread:
>
> [#31491 (comment)](https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2573886240)
>
> It'd be good to get some clarification on which behaviour is meant to work.

@theuni

> [@hebasto](https://github.com/hebasto) Have you experimented with using regular cache variables as opposed to the _INIT ones?

Cache variables are designed to be set by the user. With the suggested approach
...

💬 polespinasa commented on pull request "Fees: add Fee rate Forecaster Manager":
(https://github.com/bitcoin/bitcoin/pull/31664#discussion_r2061240293)
I like the new approach!

What do you think about saving the hash and not the heigh? In case of a reorg we could be at the same height and think that nothing happened.

```
if (chache_estimate && activeTip->phashBlock == last_tip)
...

📝 Bue-von-hon converted_to_draft a pull request: "rpc: Support v3 raw transactions creation"
(https://github.com/bitcoin/bitcoin/pull/31936)
Added support for creating v3 raw transaction:
- Overloaded to include additional parameter

this resolves #31348

🤔 rkrux reviewed a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2795872932)
Code review a7f76b28a1293929d05f15e0a9276d4ec3687c92.
I reviewed focussed on new commits that are added and will do a subsequent review that finalises my understanding of the whole diff.

💬 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_r2061247627)
I feel this assert should come right after line 421 because that's where `sighash` can be set last, after that its value is only read.

💬 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_r2061250526)
These `if` blocks here in this `else` can possibly check for `SIGHASH_ALL` for taproot inputs, which seems ok to me as I gather from the functional test and I have not found anything yet in the [BIP 341](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki) that contradicts it.
Also, the `CheckSchnorrSignature` function disallows only `SIGHASH_DEFAULT` to be explicitly mentioned, so `SIGHASH_ALL` is fine here I believe.

However, I believe it'd be nice to check the size of the sign
...

💬 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_r2061258027)
Nit (& for 2a721dcfae44cb0fef6825e773e5895cb87a91a5 commit message): Because it's easier to get the context from the log message instead of having to find in the test.

```diff
- self.log.info("Test sighash type mismatches")
+ self.log.info("Test sighash type mismatches in process psbt rpcs")
```

💬 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_r2061262878)
Note for this commit: 14e35e675c576c770641160e982713dda4032bd8

I _think_ this is added because later in the commit 94fb539f4e22ed4ec9a65f90db26cb81e5b8749a, the last element of the partial sig is used to check for the sighash type match while signing the PSBT input for which this signature validity guarantee is needed.

It'd be nice to mention something around this in the commit message to aid review (& for future reference).

💬 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`?