💬 l0rinc commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2234044885)
nit: The comment repeats the code and has a typo; consider deleting or expanding with a rationale.

💬 l0rinc commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2234045732)
```suggestion
throw BlockTreeStoreError(strprintf("Unable to open file %s\n", fs::PathToString(m_block_files_file_path)));
```

💬 l0rinc commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2234046260)
It seem we need this in multiple places, maybe we could add a sub-helper, something like:
```C++
void BlockTreeStore::CheckMagicAndVersion() const
{
auto check{[](const fs::path& path, uint32_t magic_expected, uint32_t version_expected) {
AutoFile file{fsbridge::fopen(path, "rb")};
if (file.IsNull()) {
throw BlockTreeStoreError(strprintf("Unable to open file %s", fs::PathToString(path)));
}
if (auto magic{ser_readdata32(file)}; magic != ma
...

💬 l0rinc commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2234048984)
To make sure the serialization stays stable (e.g when somebody renames one of them and reorders for some reason):
```suggestion
enum class ValueType : uint32_t {
LAST_BLOCK = 0,
BLOCK_FILE_INFO = 1,
DISK_BLOCK_INDEX = 2,
HEADER_DATA_END = 3,
};
```

💬 l0rinc commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2234046538)
nit: These asserts run at runtime, consider moving to a unit test to avoid overhead.

💬 l0rinc commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2234046731)
This may be a bit more desciptive:
```suggestion
if (header_file_exists != block_files_file_exists) {
```

💬 l0rinc commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2234048113)
Include the bad `value_type` in the error:
```suggestion
default:
throw BlockTreeStoreError(strprintf("Unrecognized value_type (%u) in block tree store", value_type));
}
```

💬 l0rinc commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2234058517)
How would this behave in case of a reorg when the block size differs from the previous one?

nit: other positions were unsigned

💬 l0rinc commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2234065617)
we don't have signed 64 bit reading/writing (we're casting to unsigned and back), would it be simpler to make this unsigned in the first place?

👍 ryanofsky approved a pull request: "wallet: Fix relative path backup during migration."
(https://github.com/bitcoin/bitcoin/pull/32273#pullrequestreview-3063441400)
Code review ACK 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77. Nice changes that (1) fix potential errors when names of wallets being migrated contain slashes, and (2) store migration backups in the top-level `-walletdir` instead of in individual wallet subdirectories.

I left various comments below but none are important. I re-reviewed this PR from the beginning since there were a number of changes since my last review.

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