💬 ryanofsky commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2237011818)
In commit "wallet: Fix migration of wallets with pathnames." (70f1c99c901de64d6ccea793b7e267e20dfa49cf)

IMO, it's confusing for this comment to distinguish between the case when the wallet is a directory and the case when it's a file, when there is no distinction in the code below. The code below is always returning the last path component no matter what kind of path is provided.

💬 ryanofsky commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2237137742)
In commit "wallet: migration: Make backup in walletdir" (f6ee59b6e2995a3916fb4f0d4cbe15ece2054494)

Notes about this change (mostly for myself to avoid getting confused later)

- This commit changes location of wallet backups created during migrations when the wallets being migrated are directories. Instead of placing backups inside those directories, backups are placed in the top level `-walletdir` (`<datadir>/wallets`) so they are easier to find and don't need to copied around if migration
...

💬 ryanofsky commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2237028524)
In commit "wallet: Fix migration of wallets with pathnames." (70f1c99c901de64d6ccea793b7e267e20dfa49cf)

This looks good, but just for comparison here is an updated version of the suggestion from: https://github.com/bitcoin/bitcoin/pull/32273#pullrequestreview-2983599593

<details><summary>diff</summary>
<p>

```diff
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4231,14 +4231,15 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>

/
...

💬 ryanofsky commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2237317368)
In commit "test: Migration of a wallet ending in `../`" (76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77)

Note: there's a minor typo in the commit message. `../` should just be `..` since the path doesn't end in a slash.

Also this commit seems very similar to previous commit. Could be nice to combine these into a single function like:

```py
self.test_wallet_with_path("path/to/mywallet/")
self.test_wallet_with_path("path/that/ends/in/..")
```

With a `fail_migration: bool` option, this cou
...

💬 ryanofsky commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2237231516)
In commit "wallet: Fix migration of wallets with pathnames." (70f1c99c901de64d6ccea793b7e267e20dfa49cf)

Note: This line of code was broken before this commit when `wallet_name` contained slashes. It would either cause the `BackupWallet` call below to fail, or to write backup files to unexpected places, instead of writing them to `-walletdir` as intended after the previous commit.

💬 ryanofsky commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3128322864)
This has 3 acks and looks ready to merge. @davidgumberg free free to indicate if you want to make any updates based on later comments (also LLM linter suggestions in https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2803681753) or just merge as-is.

🤔 jonatack reviewed a pull request: "doc: Add legacy wallet removal release notes"
(https://github.com/bitcoin/bitcoin/pull/33075#pullrequestreview-3064033670)
Thanks for picking this up -- looks good modulo a few suggestions.

💬 jonatack commented on pull request "doc: Add legacy wallet removal release notes":
(https://github.com/bitcoin/bitcoin/pull/33075#discussion_r2237460175)
nit, would sort the PRs from oldest to newest

💬 jonatack commented on pull request "doc: Add legacy wallet removal release notes":
(https://github.com/bitcoin/bitcoin/pull/33075#discussion_r2237461703)
```suggestion
the newer descriptor wallet format. Refer to the `migratewallet` RPC for more
```

💬 jonatack commented on pull request "doc: Add legacy wallet removal release notes":
(https://github.com/bitcoin/bitcoin/pull/33075#discussion_r2237465193)
```suggestion
`importwallet`, `newkeypool`, `sethdseed`, and `upgradewallet`, have been removed.
```

💬 jonatack commented on pull request "doc: Add legacy wallet removal release notes":
(https://github.com/bitcoin/bitcoin/pull/33075#discussion_r2237467117)
Nice touch sorting the RPCs by alphabetical order; maybe do the same for these options.

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2237491835)
Done

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2237492581)
Done, will add additional tests for these RPCs as well.

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2237493859)
Done

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2237494375)
Done

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2237495065)
Done

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2237496037)
Added description to commit message.

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2237499169)
Done

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2237499397)
Done

💬 theuni commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237499919)
Concept ACK. I suggested the same thing :)