🤔 achow101 reviewed a pull request: "Added rescan option for import descriptors"
(https://github.com/bitcoin/bitcoin/pull/31668#pullrequestreview-2788566889)
There should also be a test where multiple things are imported, one with a timestamp, and one with `never`. There should be a rescan in this case.

💬 achow101 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2056798497)
While this verifies that the address was imported, what we care about here is whether a rescan occurred. It would be better to send to the address for the descriptor being imported, then check to see that after importing, the transaction is not detected.

🤔 mzumsande reviewed a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2788360332)
Looks good, except for an issue with commit 00fe040eca4b77d05ec5652b4d494b0ded183653

💬 mzumsande commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2056670803)
This function is declared here, but not implemented or used anywhere. Is this meant to be called somewhere in `UnloadWallets`? Or does UnloadWallets already close the db somewhere (where?) and this can be deleted?

💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056813556)
Unlike, [`assert_not_equal`](https://github.com/bitcoin/bitcoin/blob/6b4b7ac2113ed969bd31ce42190b96bd2e1b9c97/test/functional/test_framework/util.py#L79-L81), `assert_equal` [does not support](https://github.com/bitcoin/bitcoin/blob/6b4b7ac2113ed969bd31ce42190b96bd2e1b9c97/test/functional/test_framework/util.py#L72-L77) passing an error message. Re: the conceptual change; while it might have some advantages over the style currently used, as far as I can tell it is almost never the case that we l
...

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2056818503)
Just forgot to remove it, done now.

The db is closed when a `CWallet` is destroyed.

💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056837989)
We are already asserting the transaction id exists in the migrated wallet, and that the wallet has the correct balance, I'm not totally opposed to checking the tx amount as opposed to the wallet balance, but I'm not sure that this change really does add any guarantees, or help with debugging when the test fails, so I will err on the side of keeping this as-is so it [parallels](https://github.com/bitcoin/bitcoin/blob/9a4c92eb9ac29204df3d826f5ae526ab16b8ad65/test/functional/wallet_migration.py#L53
...

💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056839286)
I'll leave this as-is for now.

💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056840721)
See https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056813556.

💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2056844763)
If `open` fails an exception will be thrown and the block below it will not be executed.

https://docs.python.org/3/reference/compound_stmts.html#with

🚀 achow101 merged a pull request: "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan"
(https://github.com/bitcoin/bitcoin/pull/32023)

💬 achow101 commented on pull request "validation: set BLOCK_FAILED_CHILD correctly":
(https://github.com/bitcoin/bitcoin/pull/31835#issuecomment-2825477728)
ACK 3c3548a70eedb8dcf6a4a8d605a4a12e814c7cac

🚀 achow101 merged a pull request: "validation: set BLOCK_FAILED_CHILD correctly"
(https://github.com/bitcoin/bitcoin/pull/31835)

💬 mzumsande commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2825517598)
Looks like `WalletMigration` bench fails on this branch (which is now run in the CI).

💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2056922934)
Reworked and upstreamed the fix in https://github.com/arun11299/cpp-subprocess/pull/112.

💬 furszy commented on issue "ci: failure in Windows cross-test":
(https://github.com/bitcoin/bitcoin/issues/32291#issuecomment-2825587643)
Can be closed.

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2825599793)
Fixed CI

✅ hebasto closed an issue: "ci: failure in Windows cross-test"
(https://github.com/bitcoin/bitcoin/issues/32291)

💬 hebasto commented on issue "ci: failure in Windows cross-test":
(https://github.com/bitcoin/bitcoin/issues/32291#issuecomment-2825623597)
Fixed in https://github.com/bitcoin/bitcoin/pull/32309.

📝 w0xlt opened a pull request: "refactor: Update `XOnlyPubKey::GetKeyIDs()` to return a pair of pubkeys"
(https://github.com/bitcoin/bitcoin/pull/32332)
Currently, `XOnlyPubKey::GetKeyIDs()` has `vector` as return type, but always returns a pair of public key IDs.

This PR changes the code for readability reasons.
There may be negligible efficiency gains, but the goal is to make the code more readable and to make the intent of the function clear.
There is no change in behavior.