💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3128247787)
Thank you so much for the review @stickies-v! Picked off the smaller items here, while we are still experimenting with the rest:
Updated 938767d957b7669accfb554a7cbb25141f7e8632 -> 4eb1c66dbdaf35cbc480cb201f6019bc0f5fde95 ([kernelApi_46](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_46) -> [kernelApi_47](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_47), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_46..kernelApi_47)).
* Addressed @stickies-v's [comm
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3128247787)
Thank you so much for the review @stickies-v! Picked off the smaller items here, while we are still experimenting with the rest:
Updated 938767d957b7669accfb554a7cbb25141f7e8632 -> 4eb1c66dbdaf35cbc480cb201f6019bc0f5fde95 ([kernelApi_46](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_46) -> [kernelApi_47](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_47), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_46..kernelApi_47)).
* Addressed @stickies-v's [comm
...
💬 marcofleon commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#issuecomment-3128251488)
Most recent push updates the `m_tx_inventory_to_send` comment and adds a clarifying comment as to why we create the inv before checking `m_tx_inventory_known_filter`.
I'm still considering https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2236858410.
(https://github.com/bitcoin/bitcoin/pull/33005#issuecomment-3128251488)
Most recent push updates the `m_tx_inventory_to_send` comment and adds a clarifying comment as to why we create the inv before checking `m_tx_inventory_known_filter`.
I'm still considering https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2236858410.
💬 marcofleon commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2237365239)
Done.
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2237365239)
Done.
💬 marcofleon commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2237365568)
Good looks, fixed.
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2237365568)
Good looks, fixed.
🤔 l0rinc reviewed a pull request: "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store"
(https://github.com/bitcoin/bitcoin/pull/32427#pullrequestreview-2822815289)
Concept ACK, not yet sure about the approach
### LevelDB migration
Moving away from unmaintained LevelDB makes sense and aligns with other modularization and optimization efforts.
Whether that means switching to SQLite or a custom solution like this one is debatable, but removing LevelDB already paves the way for further migrations - which still seem somewhat taboo at this point.
Before merging something like this, I'd be interested in seeing a full migration story (including other ind
...
(https://github.com/bitcoin/bitcoin/pull/32427#pullrequestreview-2822815289)
Concept ACK, not yet sure about the approach
### LevelDB migration
Moving away from unmaintained LevelDB makes sense and aligns with other modularization and optimization efforts.
Whether that means switching to SQLite or a custom solution like this one is debatable, but removing LevelDB already paves the way for further migrations - which still seem somewhat taboo at this point.
Before merging something like this, I'd be interested in seeing a full migration story (including other ind
...
💬 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.
(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)));
```
(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
...
(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,
};
```
(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.
(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) {
```
(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));
}
```
(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
(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?
(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.
(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.
(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
...
(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>
/
...
(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
...
(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.
(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.