👍 andrewtoth approved a pull request: "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline"
(https://github.com/bitcoin/bitcoin/pull/32279#pullrequestreview-3063819192)
ACK d5104cfbaeb82081e4b00a5084516555e446dcdc
(https://github.com/bitcoin/bitcoin/pull/32279#pullrequestreview-3063819192)
ACK d5104cfbaeb82081e4b00a5084516555e446dcdc
🤔 stratospher reviewed a pull request: "test: refactor mempool_accept_wtxid"
(https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3063408783)
Concept ACK. I like how you've used `MiniWallet` to remove the signing bits (`signrawtransactionwithkey`) which needed the private key.
You can simplify this further. `ValidWitnessMalleatedTx` returned parent transaction and children transaction in 2 different steps because the children transactions needed the parent's txid which required signing with private key. Now since we don't do `signrawtransactionwithkey`, you can replace class `ValidWitnessMalleatedTx` with a function `valid_witnes
...
(https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3063408783)
Concept ACK. I like how you've used `MiniWallet` to remove the signing bits (`signrawtransactionwithkey`) which needed the private key.
You can simplify this further. `ValidWitnessMalleatedTx` returned parent transaction and children transaction in 2 different steps because the children transactions needed the parent's txid which required signing with private key. Now since we don't do `signrawtransactionwithkey`, you can replace class `ValidWitnessMalleatedTx` with a function `valid_witnes
...
💬 stratospher commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2236986215)
```suggestion
parent = wallet.send_to(from_node=node, scriptPubKey=txgen.get_script_pubkey(), amount=int(9.99998 * COIN), fee=1000)
```
style nit: space after , (here and in other places too)
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2236986215)
```suggestion
parent = wallet.send_to(from_node=node, scriptPubKey=txgen.get_script_pubkey(), amount=int(9.99998 * COIN), fee=1000)
```
style nit: space after , (here and in other places too)
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237313429)
Right, there is also no test for this :(. Will fix and add a test.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237313429)
Right, there is also no test for this :(. Will fix and add a test.
💬 stickies-v commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2237359282)
Sorry, need to read up on this thread properly in the next few days, but quickly wanted to add: moving `info_for_relay` to `net_processing.cpp` absolutely makes sense to me, that logic should not be in mempool interface. However, leaking mempool internals into p2p (through `mempool.GetIter(id)`) I really don't like.
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2237359282)
Sorry, need to read up on this thread properly in the next few days, but quickly wanted to add: moving `info_for_relay` to `net_processing.cpp` absolutely makes sense to me, that logic should not be in mempool interface. However, leaking mempool internals into p2p (through `mempool.GetIter(id)`) I really don't like.
💬 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.