💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1983212714)
Now that `out` is not being used in this function.

```diff
-std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider& out) const override
+std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider&) const override
```

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1983271123)
In the function doc:

```diff
-/** Derive a public key.
+/** Derive a public key and put it into out
```

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1983307885)
This variable is unused now.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1983252231)
Same for other `MakeScripts` functions where it's applicable - `WPKHDescriptor`.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1983325137)
I agree with filling `out` with `pubkeys` and `origins` together in a function.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1983318081)
> The benefit is in the MuSig2 PRs where GetPrivKey can insert multiple private keys to the FlatSigningProvider, none of which are the private key for the MuSig2 aggregate pubkey itself.

Something around this can be mentioned either in the commit message and/or the PR description.

💬 Sjors commented on pull request "Remove Taproot activation height":
(https://github.com/bitcoin/bitcoin/pull/26201#issuecomment-2703851551)
Rebased after #31978.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1983393187)
I am inclining to agree with @achow101 here. Although I don't like a function that is prefixed with `Get` returning nothing but at the moment it's a common pattern in the codebase to `Get` something and update a reference passed in one of the function arguments. The confusion, as highlighted above, that might arise with prefixing with `Fill` makes me not to want to use that.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1983402091)
> like GetConstPrivKey maybe

This is already inside `ConstPubkeyProvider` class, using `Const` again could feel repetitive. I think the function overloading (with different argument types) is differentiating enough. Though, some documentation for the new function would be helpful for the reader.

💬 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_r1983410849)
I know it's not parseable right now, that's why suggested only a TODO that'd be done after PSBTv2 gets merged. The assumption I have is this PR will get merged before #21283. I didn't notice any such test in the PSBT PR, which is fine because that PR is already big enough, and adding a TODO here can be a reminder for later.

📝 xiaobei0715 opened a pull request: "chore: update copyright year to present"
(https://github.com/bitcoin/bitcoin/pull/32008)

✅ fanquake closed a pull request: "chore: update copyright year to present"
(https://github.com/bitcoin/bitcoin/pull/32008)

💬 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#issuecomment-2703968323)
The functional test failure seems unrelated to me. A re-run should fix it?

`test_preferred_tiebreaker_inv()` in `p2p_tx_download.py`

💬 brunoerg commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1983442025)
ba2042c31f1630678a1f08bb68e01c1d98cc9abc: Is this verification reachable? The scenario I can see is: someone unloaded the wallet using an old version, upgraded and then tried to load it again. However, before reaching here it would fail during the database creation (`MakeDatabase`).

💬 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_r1983453607)
Hmm I think you might be correct, I will make a note of this for it to be done separately. If there was a bug in the newer implementation of `RemoveUnnecessaryTransactions` the existing test using `walletprocesspsbt` would have caught it.

💬 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_r1983477824)
Nice to see this now, it was bugging me earlier (but could not prove to myself until I found it in the spec) that how it can finalise to the same hex with a different sighash when underneath `PSBTInputSignedAndVerified` uses the same `interpreter.cpp::VerifyScript()` that is used in other script validation flows as well.

💬 fanquake commented on issue "guix: Unable to reproduce macOS SDK tarball on Fedora 40":
(https://github.com/bitcoin/bitcoin/issues/31873#issuecomment-2704041240)
@dongcarl Nice find. I wonder if we can just set `mtime` to 1, and avoid the delegation.

💬 laanwj commented on issue "guix: re-enable exported symbol checking for RISC-V":
(https://github.com/bitcoin/bitcoin/issues/28095#issuecomment-2704078274)
i tried to figure this out today but haven't got very far. Curiously, there are even more (weak) symbols exported by the current RISC-V build than mentioned in the OP-
```
0000000000091fd8 w DF .text 0000000000000998 Base _ZZSt8to_arrayINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEELm35EESt5arrayINSt9remove_cvIT_E4typeEXT0_EEOAT0__S8_ENKUlTpTnmSt16integer_sequenceImJXspT_EEEE_clIJLm0ELm1ELm2ELm3ELm4ELm5ELm6ELm7ELm8ELm9ELm10ELm11ELm12ELm13ELm14ELm15ELm16ELm17ELm18ELm19ELm20ELm21
...

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1983515428)
> This comment is too positive. If the node undergoes through an unclean shutdown and restart here, the wallet crashes again.

If there is any shutdown here (clean or unclean), the node will be off anyway, and all RPC calls will fail.

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1983507204)
> Technically correct but since the block was supposed to be invalidated and with the assumption that the likelihood for it to be invalidated again (successfully this time) is high, this immature unspent will not be blocked for long.

Usually, yes.
Yet, there is another edge case. "Invalidated" means a reorg, which could also be reversed. That is, a block reorged out of the active chain before the unclean shutdown could become part of the active chain again when the node restarts (due to an e
...