💬 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.

💬 achow101 commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2109899317)
Removed

💬 achow101 commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2109899458)
Done

💬 achow101 commented on pull request "wallet: Always set descriptor cache upgraded flag for new wallets":
(https://github.com/bitcoin/bitcoin/pull/32597#discussion_r2109902489)
There can be no DB entries when upgrade is run on a newly created wallet as the descriptors don't even exist yet.

💬 achow101 commented on pull request "wallet: Always set descriptor cache upgraded flag for new wallets":
(https://github.com/bitcoin/bitcoin/pull/32597#discussion_r2109907898)
The only behavior that is conditional on this flag is whether the upgrade needs to be run. It exists only as a startup optimization to ensure that we aren't re-running the same upgrade over and over. The `DescriptorScriptPubKeyMan` will always write the records for the upgraded cache, regardless of whether the flag exists. There is no need to upgrade the cache either as the upgraded cache is always used regardless of the flag.

💬 achow101 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-2913572120)
> 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.

While it is an extra click, I don't think it is unintuitive.

When the user goes to restore a legacy wallet, they will be informed that the restore failed and that they will need to migrate the wallet via the corresponding menu. When the user goes there, this option to restore and migrate will be presented to them.

💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#issuecomment-2913670535)
Updated the description with recent benchmarks.