🚀 hebasto merged a pull request: "guix: accomodate migration to codeberg"
(https://github.com/bitcoin/bitcoin/pull/32439)

💬 hebasto commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#discussion_r2109748436)
Guix's `cmake-minimal` remains at version [3.24.2](https://codeberg.org/guix/guix/src/commit/b868ae741e989d56dbaf8dd481c66a1c00e45e25/gnu/packages/cmake.scm#L179).

👍 pinheadmz approved a pull request: "blocks: avoid recomputing block header hash in `ReadBlock`"
(https://github.com/bitcoin/bitcoin/pull/32487#pullrequestreview-2871920816)
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0

Built and tested on macos/arm64 and reviewed code. This is a simple optimization that deduplicates a call to `Block.GetHash()` when reading a block from disk by `BlockManager`. The hash is checked against the expected result from a `CBlockIndex`. Covered by a new unit test.

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
-----BEGIN PGP SIGNATURE-
...

🤔 theStack reviewed a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2871820534)
Reviewed thoroughly up to ff7476ec32f4dbee07bf04169e741e2aea5a9ab7, looks good to me (with one refactoring suggestion below). Still need to go again through the parsing code as there is a lot going on with all the conditions on what is allowed and what not w.r.t. derivations.

💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2109683327)
in commit ff7476ec32f4dbee07bf04169e741e2aea5a9ab7: can't this be simplified to
```suggestion
out += pubkey->ToString(type);
```
? It's a bit confusing that there are two `StringType` enum classes within different classes, (`PubkeyProvider::StringType` and `DescriptorImpl::StringType`), but the code here seems to deal only with the former. I guess whether the diff makes sense or not depends on if it's possible in the future that the default arguments for pubkey providers `ToStrin
...

💬 luke-jr commented on pull request "gui: Add a menu action to restore then migrate a legacy wallet":
(https://github.com/bitcoin-core/gui/pull/877#issuecomment-2913426259)
>by adding a menu item in the "Migrate Wallet" menu

IMO this is bad UX. Users will be looking to restore the wallet, and the details of how it does that shouldn't require unintuitive steps.

💬 maflcko commented on pull request "Mining: Avoid copying template CBlocks":
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2913466098)
Can be turned into draft while the CI is red?

🤔 rkrux reviewed a pull request: "wallet: Always set descriptor cache upgraded flag for new wallets"
(https://github.com/bitcoin/bitcoin/pull/32597#pullrequestreview-2872012392)
Concept ACK, code review 69f588a99a7a79d1d72300bc0f2c8475f95f6c6a

> Newly created wallets will always have an upgraded descriptor cache, so set those.

Besides the logical accuracy, is there any other advantage of doing this that is used in #32489? Probably can mention it in the PR description.

💬 rkrux commented on pull request "wallet: Always set descriptor cache upgraded flag for new wallets":
(https://github.com/bitcoin/bitcoin/pull/32597#discussion_r2109834391)
I don't suppose it's a big issue but while going through the creation flow it seems odd that this flag is set prior to even the setup of the SPKMs. I have not tried it but from a code flow perspective a call to `UpgradeDescriptorCache` can come right after `SetupDescriptorScriptPubKeyMans` that will set both the cache keys in the database along with this flag.

💬 rkrux commented on pull request "wallet: Always set descriptor cache upgraded flag for new wallets":
(https://github.com/bitcoin/bitcoin/pull/32597#discussion_r2109813437)
In 69f588a99a7a79d1d72300bc0f2c8475f95f6c6a

> Since new wallets will always being using the upgraded cache, there's no
reason to wait to set the flag, so set it when the wallet flags are
being initialized for new wallets.

Conceptually, it makes sense to me that the `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED` is set at the time of wallet creation.

> Although WalletBatch::LoadWallet performs the descriptor cache upgrade,
because new wallets do not have the descriptor flag set yet, the upgr
...

💬 achow101 commented on pull request "rpc: Note in fundrawtransaction doc, fee rate is for package":
(https://github.com/bitcoin/bitcoin/pull/32607#issuecomment-2913475287)
> Is this actually intended behaviour?

Yes

💬 romanz commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2109843001)
Reading the undo data directly from storage (i.e. without re-serialization) results in ~3.2x requests per second, and even a bit less data send over the network:
```
Document Path: /rest/spenttxouts/00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054.bin
Document Length: 184971 bytes

Concurrency Level: 1
Time taken for tests: 8.060 seconds
Complete requests: 10000
Failed requests: 0
Keep-Alive requests: 10000
Total transferred: 1
...

💬 achow101 commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#issuecomment-2913490825)
> is parent_descs field useful only when the unspent is not solvable?

No. The purpose is to allow identifying the actual descriptor in the wallet that produced the address. These are supposed to be the same descriptors as output by `listdescriptors false`.

🤔 BrandonOdiwuor reviewed a pull request: "test: Add missing ipc subtree to lint"
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2872073697)
Re-ACK fa4b8b16c37830bf7ee1419aae94e99f294f0e9f

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2109851713)
Fixed

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2109851898)
Good point, done.

💬 romanz commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2109863900)
> However, with this suggestion, and a future change in the storage serialization, there will be a breaking change on the rest interface, or alternatively a re-serialization again (back to what this pull is doing).

I think that non-stable REST output format is OK, in case it enables performant data retrieval.

💬 romanz commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2109866540)
Thanks, good catch! Will update on the next rebase.

💬 davidgumberg commented on pull request "test: Add missing ipc subtree to lint":
(https://github.com/bitcoin/bitcoin/pull/32623#issuecomment-2913526219)
ACK https://github.com/bitcoin/bitcoin/pull/32623/commits/fa4b8b16c37830bf7ee1419aae94e99f294f0e9f

💬 achow101 commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2109897969)
Hmm, yes, I was confused by the weird indentation here.

I've put the backwards compatibility stuff back and changed the docs to say that the field is no longer used.

Also fixed the indentation.