💬 TheCharlatan commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2938977867)
Thanks for giving this a try @ismaelsadeeq! I'm working on adding a write ahead log at the moment, so will draft this PR in the meantime. Bit surprised that you immediately ran into some corruption, maybe it is caused by the library attempting to still write some data like the genesis block? I think it would be good to have a test for parallel reads/writes here as well as a demo branch for the library.
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2938977867)
Thanks for giving this a try @ismaelsadeeq! I'm working on adding a write ahead log at the moment, so will draft this PR in the meantime. Bit surprised that you immediately ran into some corruption, maybe it is caused by the library attempting to still write some data like the genesis block? I think it would be good to have a test for parallel reads/writes here as well as a demo branch for the library.
📝 TheCharlatan converted_to_draft a pull request: "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store"
(https://github.com/bitcoin/bitcoin/pull/32427)
Opening this PR mostly to get concept/approach feedback.
This is motivated by the kernel library, where the internal usage of leveldb is a limiting factor to its future use cases. Specifically it is not possible to share leveldb databases between two processes. A notable use-case for the kernel library is accessing and analyzing existing block data. Currently this can only be done by first shutting down the node writing this data. Moving away from leveldb opens the door towards doing this in
...
(https://github.com/bitcoin/bitcoin/pull/32427)
Opening this PR mostly to get concept/approach feedback.
This is motivated by the kernel library, where the internal usage of leveldb is a limiting factor to its future use cases. Specifically it is not possible to share leveldb databases between two processes. A notable use-case for the kernel library is accessing and analyzing existing block data. Currently this can only be done by first shutting down the node writing this data. Moving away from leveldb opens the door towards doing this in
...
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2125949575)
You could also be more strict and disallow any duplicate non-hardened path. Or even duplicate origin fingerprint.
The bigger problem may be that whatever we choose might be slightly different than what implementations do. So we should probably change the descriptor standard to pick an extreme on either side: allow duplicate keys or more thorowly prevent them.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2125949575)
You could also be more strict and disallow any duplicate non-hardened path. Or even duplicate origin fingerprint.
The bigger problem may be that whatever we choose might be slightly different than what implementations do. So we should probably change the descriptor standard to pick an extreme on either side: allow duplicate keys or more thorowly prevent them.
💬 l0rinc commented on pull request "clang-tidy: Apply modernize-deprecated-headers":
(https://github.com/bitcoin/bitcoin/pull/32673#issuecomment-2939069298)
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
The new commit is only moving includes (not deleting any) and changing one remaining copyright header.
(https://github.com/bitcoin/bitcoin/pull/32673#issuecomment-2939069298)
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
The new commit is only moving includes (not deleting any) and changing one remaining copyright header.
💬 l0rinc commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#issuecomment-2939092052)
I retested the scenario with `"address\n;)"`, running
```bash
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug && ninja -C build && build/test/functional/test_runner.py rpc_help
```
and got:
```
Internal bug detected: s.m_left.find('\n') == std::string::npos
```
on master it's:
```
rpc_help.py | ✓ Passed
```
👍
(https://github.com/bitcoin/bitcoin/pull/32660#issuecomment-2939092052)
I retested the scenario with `"address\n;)"`, running
```bash
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug && ninja -C build && build/test/functional/test_runner.py rpc_help
```
and got:
```
Internal bug detected: s.m_left.find('\n') == std::string::npos
```
on master it's:
```
rpc_help.py | ✓ Passed
```
👍
💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2126008619)
I would also prefer to keep this binary focused on testing / demoing the mining interface.
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2126008619)
I would also prefer to keep this binary focused on testing / demoing the mining interface.
💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2939117270)
Now that #31375 landed, can you add `bitcoin mine` as a command?
Other than that 27c6576aba5c59402b8844703e04face2f41578c looks good and works.
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2939117270)
Now that #31375 landed, can you add `bitcoin mine` as a command?
Other than that 27c6576aba5c59402b8844703e04face2f41578c looks good and works.
💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2126039658)
hi @achow101 I've added more test cases.
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2126039658)
hi @achow101 I've added more test cases.
🚀 fanquake merged a pull request: "build: add -Wthread-safety-pointer"
(https://github.com/bitcoin/bitcoin/pull/32647)
(https://github.com/bitcoin/bitcoin/pull/32647)
💬 Sjors commented on issue "external signer: PSBT error code `EXTERNAL_SIGNER_NOT_FOUND` is never returned":
(https://github.com/bitcoin/bitcoin/issues/32426#issuecomment-2939202952)
I made a note to refactor `GetExternalSigner()` and `ExternalSignerScriptPubKeyMan::FillPSBT` so the latter can return `PSBTError::EXTERNAL_SIGNER_NOT_FOUND`.
We call GetExternalSigner in three scenarios:
1. During initial wallet setup to fetch keys from the device
2. When attempting to display an address on the device
3. When signing a PSBT
For scenario (1) and (2) the current `std::runtime_error` approach seems fine.
In particular it's unlikely, and definitely an error, in scenario (1) tha
...
(https://github.com/bitcoin/bitcoin/issues/32426#issuecomment-2939202952)
I made a note to refactor `GetExternalSigner()` and `ExternalSignerScriptPubKeyMan::FillPSBT` so the latter can return `PSBTError::EXTERNAL_SIGNER_NOT_FOUND`.
We call GetExternalSigner in three scenarios:
1. During initial wallet setup to fetch keys from the device
2. When attempting to display an address on the device
3. When signing a PSBT
For scenario (1) and (2) the current `std::runtime_error` approach seems fine.
In particular it's unlikely, and definitely an error, in scenario (1) tha
...
💬 GregTonoski commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2939296718)
> [@GregTonoski](https://github.com/GregTonoski) Just curious, how do you get the pruned node of 550MB? I'd like to explore a little more about this issue but searched on the internet and not able to find a easy solution to get a pruned node to start with.
A pruned node could be setup by copying data (a.k.a. snapshot of UTXO/chainstate and blocks) to directories, see e.g. [https://github.com/Blockchains-Download/Bitcoin/releases](https://github.com/Blockchains-Download/Bitcoin/releases) or [htt
...
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2939296718)
> [@GregTonoski](https://github.com/GregTonoski) Just curious, how do you get the pruned node of 550MB? I'd like to explore a little more about this issue but searched on the internet and not able to find a easy solution to get a pruned node to start with.
A pruned node could be setup by copying data (a.k.a. snapshot of UTXO/chainstate and blocks) to directories, see e.g. [https://github.com/Blockchains-Download/Bitcoin/releases](https://github.com/Blockchains-Download/Bitcoin/releases) or [htt
...
💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#issuecomment-2939315304)
I understand the difficulties of guessing the correct memory usage of such an app (especially when the user cannot even provide total memory usage requirements, just `dbcache` ones), trying to find a formula that is good enough on all OSs, compilers, versions, 32/64 bits, architectures, etc.
I ran a simple benchmark to see if this memory accounting changed the behavior of IBD (simulated via a `reindex-chainstate` with default `dbcache=450` until block 888,888).
For reference, the commits I u
...
(https://github.com/bitcoin/bitcoin/pull/28531#issuecomment-2939315304)
I understand the difficulties of guessing the correct memory usage of such an app (especially when the user cannot even provide total memory usage requirements, just `dbcache` ones), trying to find a formula that is good enough on all OSs, compilers, versions, 32/64 bits, architectures, etc.
I ran a simple benchmark to see if this memory accounting changed the behavior of IBD (simulated via a `reindex-chainstate` with default `dbcache=450` until block 888,888).
For reference, the commits I u
...
💬 fanquake commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2126217083)
#32568 landed.
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2126217083)
#32568 landed.
💬 willcl-ark commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2126226110)
Dropped in 64986e1a44e108628b245ca977d2b8eb69ed5580
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2126226110)
Dropped in 64986e1a44e108628b245ca977d2b8eb69ed5580
👋 willcl-ark's pull request is ready for review: "doc: add alpine build instructions"
(https://github.com/bitcoin/bitcoin/pull/32559)
(https://github.com/bitcoin/bitcoin/pull/32559)
💬 Sjors commented on issue "wallet: render BIP388 wallet policies":
(https://github.com/bitcoin/bitcoin/issues/32659#issuecomment-2939449631)
One caveat is that a wallet policy typically uses `/**` to indicate a combined receive and change descriptor `<0;1>`. Although we can import such combined descriptors, we store them seperately and forget about the unified original. So the `listdescriptors` command would return a useless policy value.
We could make `listdescriptors` smart, but it's perhaps better if don't give it a `policy` and `keys` field and instead introduce a new `listwalletpolicies` RPC that _is_ smart (and only considers
...
(https://github.com/bitcoin/bitcoin/issues/32659#issuecomment-2939449631)
One caveat is that a wallet policy typically uses `/**` to indicate a combined receive and change descriptor `<0;1>`. Although we can import such combined descriptors, we store them seperately and forget about the unified original. So the `listdescriptors` command would return a useless policy value.
We could make `listdescriptors` smart, but it's perhaps better if don't give it a `policy` and `keys` field and instead introduce a new `listwalletpolicies` RPC that _is_ smart (and only considers
...
💬 petertodd commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2939451159)
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
Nits in things like the exact wording of the release notes can be handled in a followup pull-req if necessary. The basic functionality clearly works.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2939451159)
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
Nits in things like the exact wording of the release notes can be handled in a followup pull-req if necessary. The basic functionality clearly works.
💬 l0rinc commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-2939458958)
I've run the command on a `Raspberry Pi 4 Model B - Cortex-A72 4-Core ARM with SanDisk SSD PLUS 1TB (USB 3.0)` with to have data about lower-end hardware as well.
<details>
<summary>full command</summary>
```bash
git remote add sipa https://github.com/sipa/bitcoin.git || true && git fetch sipa bench_sfl_css && git switch -C bench_sfl_css sipa/bench_sfl_css && \
git clean -fxd && git reset --hard && \
cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release && cmake --build build -j"
...
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-2939458958)
I've run the command on a `Raspberry Pi 4 Model B - Cortex-A72 4-Core ARM with SanDisk SSD PLUS 1TB (USB 3.0)` with to have data about lower-end hardware as well.
<details>
<summary>full command</summary>
```bash
git remote add sipa https://github.com/sipa/bitcoin.git || true && git fetch sipa bench_sfl_css && git switch -C bench_sfl_css sipa/bench_sfl_css && \
git clean -fxd && git reset --hard && \
cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release && cmake --build build -j"
...
💬 petertodd commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2939461753)
@AMoynahan89
> Why is this pr still open?
>
> "It's abundantly clear that this PR is controversial and, in its current state, has no hope of reaching a conclusion that is acceptable to everyone." -Achow101
>
> [](https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1878853896)
Pretty much every change to Bitcoin Core is controversial in the sense that there's someone out there who doesn't want the change to happen, or wants it to happen in a different way. Segwit, for example,
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2939461753)
@AMoynahan89
> Why is this pr still open?
>
> "It's abundantly clear that this PR is controversial and, in its current state, has no hope of reaching a conclusion that is acceptable to everyone." -Achow101
>
> [](https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1878853896)
Pretty much every change to Bitcoin Core is controversial in the sense that there's someone out there who doesn't want the change to happen, or wants it to happen in a different way. Segwit, for example,
...
💬 fjahr commented on pull request "index: Fix coinstats overflow and introduce index versioning":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2126292213)
No, it wasn't deleting it because it sounded like it should be kept around but yeah, deleting makes sense startin from a future version. I will just go with >=31 for now, if this gets backported the version check can be adapted accordingly.
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2126292213)
No, it wasn't deleting it because it sounded like it should be kept around but yeah, deleting makes sense startin from a future version. I will just go with >=31 for now, if this gets backported the version check can be adapted accordingly.