💬 TheCharlatan commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2859864794)
Re https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2858637715
> Do you mean compared to the alternative of having a single file for all blocks? I would imagine that would create I/O problems, since the operating system wouldn't know which part of the big file changed. And it can't defragment it.
Yes, that is what I meant. We never change block files, so that is not a problem. I'm also not sure how real this problem actually is. A bunch of databases just maintain one big file and
...
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2859864794)
Re https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2858637715
> Do you mean compared to the alternative of having a single file for all blocks? I would imagine that would create I/O problems, since the operating system wouldn't know which part of the big file changed. And it can't defragment it.
Yes, that is what I meant. We never change block files, so that is not a problem. I'm also not sure how real this problem actually is. A bunch of databases just maintain one big file and
...
💬 achow101 commented on pull request "refactor: Removals after bdb removal":
(https://github.com/bitcoin/bitcoin/pull/32438#discussion_r2078288424)
In fadaa8baafc27fbe29721300569e58777643bb1f "test: remove unused extra_args"
These args are used as the 28.0 node used in this test needs them to make the legacy wallets for testing.
(https://github.com/bitcoin/bitcoin/pull/32438#discussion_r2078288424)
In fadaa8baafc27fbe29721300569e58777643bb1f "test: remove unused extra_args"
These args are used as the 28.0 node used in this test needs them to make the legacy wallets for testing.
🚀 achow101 merged a pull request: "test: remove bdb assert in tool_wallet.py"
(https://github.com/bitcoin/bitcoin/pull/32440)
(https://github.com/bitcoin/bitcoin/pull/32440)
📝 MozirDmitriy opened a pull request: "Fix broken Kyoto Cabinet link in leveldb benchmark doc"
(https://github.com/bitcoin/bitcoin/pull/32441)
Update the broken link to Kyoto Cabinet specifications from http://fallabs.com/kyotocabinet/spex.html to https://dbmx.net/kyotocabinet/spex.html in the leveldb benchmark documentation. The original domain is no longer available, but the documentation is now hosted at dbmx.net.
(https://github.com/bitcoin/bitcoin/pull/32441)
Update the broken link to Kyoto Cabinet specifications from http://fallabs.com/kyotocabinet/spex.html to https://dbmx.net/kyotocabinet/spex.html in the leveldb benchmark documentation. The original domain is no longer available, but the documentation is now hosted at dbmx.net.
✅ fanquake closed a pull request: "Fix broken Kyoto Cabinet link in leveldb benchmark doc"
(https://github.com/bitcoin/bitcoin/pull/32441)
(https://github.com/bitcoin/bitcoin/pull/32441)
💬 achow101 commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2859906234)
> Quick comment - the PR title and description would now need to be updated to use `unused` instead of `void`
Done
> Also, some documentation in [descriptors.md file](https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md) would be nice.
I don't think that `unused()` is really a descriptor that we people should be importing by themselves. I'm not planning on standardizing it with a BIP as I think it's more of an internal implementation detail rather than something that actual
...
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2859906234)
> Quick comment - the PR title and description would now need to be updated to use `unused` instead of `void`
Done
> Also, some documentation in [descriptors.md file](https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md) would be nice.
I don't think that `unused()` is really a descriptor that we people should be importing by themselves. I'm not planning on standardizing it with a BIP as I think it's more of an internal implementation detail rather than something that actual
...
💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2078307648)
Fixed
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2078307648)
Fixed
💬 TheCharlatan commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2859921949)
Re https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2858716091
> How worried are we about file corruption here?
I was hoping to provoke a discussion about this as I alluded to in the PR description - thanks for providing your thoughts on this. I think the proof of work and integrity checks done on loading the index already provide fairly solid guarantees on load, but agree that we should do better. I have also talked to some other people about it offline, and there seems to be s
...
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2859921949)
Re https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2858716091
> How worried are we about file corruption here?
I was hoping to provoke a discussion about this as I alluded to in the PR description - thanks for providing your thoughts on this. I think the proof of work and integrity checks done on loading the index already provide fairly solid guarantees on load, but agree that we should do better. I have also talked to some other people about it offline, and there seems to be s
...
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2078319009)
Done
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2078319009)
Done
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2078319098)
Done
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2078319098)
Done
💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2078319199)
Done
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2078319199)
Done
💬 brunoerg commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2078320533)
> changed back to just `num_txs` in [7c80dcd](https://github.com/bitcoin/bitcoin/pull/32243/commits/7c80dcd81a844837b5823eb33501e9511cbe3d05)
I think that using just `num_txs` is not correct since it's a position, you could add something like `num_txs > 0 ? num_txs - 1 : 0`.
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2078320533)
> changed back to just `num_txs` in [7c80dcd](https://github.com/bitcoin/bitcoin/pull/32243/commits/7c80dcd81a844837b5823eb33501e9511cbe3d05)
I think that using just `num_txs` is not correct since it's a position, you could add something like `num_txs > 0 ? num_txs - 1 : 0`.
💬 kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2078329613)
that works with me! updated in [0fe4e7d](https://github.com/bitcoin/bitcoin/pull/32243/commits/0fe4e7d1863afb32608bb0e8ffacffd7e3866f30)
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2078329613)
that works with me! updated in [0fe4e7d](https://github.com/bitcoin/bitcoin/pull/32243/commits/0fe4e7d1863afb32608bb0e8ffacffd7e3866f30)
💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2078353954)
Done and squashed the commits.
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2078353954)
Done and squashed the commits.
💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2078354485)
Moved to the destructor.
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2078354485)
Moved to the destructor.
💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2078354648)
Squashed.
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2078354648)
Squashed.
💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2078354747)
Done
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2078354747)
Done
🤔 caesrcd reviewed a pull request: "config: allow setting -proxy per network"
(https://github.com/bitcoin/bitcoin/pull/32425#pullrequestreview-2823048212)
ACK 4af5b4d4298b4c69cd9131e87fe50e9386def13a
I have tested the code.
(https://github.com/bitcoin/bitcoin/pull/32425#pullrequestreview-2823048212)
ACK 4af5b4d4298b4c69cd9131e87fe50e9386def13a
I have tested the code.
💬 stringintech commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2078426205)
Small nit: I was thinking maybe we could make the template parameter more specific like:
```cpp
template <typename T>
unsigned int GetP2SHSigOpCount(const CTransaction& tx, const std::span<T> coins)
```
This might better align with the "Sorted span" mentioned in the docs, where T would then be either `const Coin` or `std::reference_wrapper<const Coin>`. It might also make the implementation slightly more readable when calling iterator methods like `begin()` and `end()` on the `coins` pa
...
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2078426205)
Small nit: I was thinking maybe we could make the template parameter more specific like:
```cpp
template <typename T>
unsigned int GetP2SHSigOpCount(const CTransaction& tx, const std::span<T> coins)
```
This might better align with the "Sorted span" mentioned in the docs, where T would then be either `const Coin` or `std::reference_wrapper<const Coin>`. It might also make the implementation slightly more readable when calling iterator methods like `begin()` and `end()` on the `coins` pa
...
💬 laanwj commented on pull request "Fix broken Kyoto Cabinet link in leveldb benchmark doc":
(https://github.com/bitcoin/bitcoin/pull/32441#issuecomment-2860255728)
You should file this upstream https://github.com/google/leveldb
(https://github.com/bitcoin/bitcoin/pull/32441#issuecomment-2860255728)
You should file this upstream https://github.com/google/leveldb