💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125437820)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111949140
> IIRC we decided to not include spawning support just yet to keep it simple, so the comment may be outdated.
Yes the comment was just out of date and should be fixed now. Support for spawning was dropped in
https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2231676827 but implemented in the branch mentioned in the PR description: https://github.com/ryanofsky/bitcoin/commits/pr/mine-detach
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125437820)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111949140
> IIRC we decided to not include spawning support just yet to keep it simple, so the comment may be outdated.
Yes the comment was just out of date and should be fixed now. Support for spawning was dropped in
https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2231676827 but implemented in the branch mentioned in the PR description: https://github.com/ryanofsky/bitcoin/commits/pr/mine-detach
💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125432171)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111970868
> In "ipc: Add bitcoin-mine test program" [6841bae](https://github.com/bitcoin/bitcoin/commit/6841bae335d109e0207aed002635cd12e609f9a8)
>
> The valid address probably shouldn’t be hardcoded here. It would be better to have a function that returns all valid addresses, with a default constant for unix.
This seems like a nice idea for a future change, but I don't think there is a very straightforward way to implement
...
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125432171)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111970868
> In "ipc: Add bitcoin-mine test program" [6841bae](https://github.com/bitcoin/bitcoin/commit/6841bae335d109e0207aed002635cd12e609f9a8)
>
> The valid address probably shouldn’t be hardcoded here. It would be better to have a function that returns all valid addresses, with a default constant for unix.
This seems like a nice idea for a future change, but I don't think there is a very straightforward way to implement
...
💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125446117)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2112177316
> Should validate the address first?
This should not be necessary because connectAddress will validate it. The error string is in the exception printed below.
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125446117)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2112177316
> Should validate the address first?
This should not be necessary because connectAddress will validate it. The error string is in the exception printed below.
💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125422549)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111981130
> Maybe specify the default datadir
This is a nice idea, and there is a GetDefaultDataDir() function that could potentially be called here. Only issue is this -datadir option documentation is copied from existing documentations and I think it is best to not introduce a discrepency. A PR to update all the -datadir options together though might be a good idea for a separate PR.
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2125422549)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111981130
> Maybe specify the default datadir
This is a nice idea, and there is a GetDefaultDataDir() function that could potentially be called here. Only issue is this -datadir option documentation is copied from existing documentations and I think it is best to not introduce a discrepency. A PR to update all the -datadir options together though might be a good idea for a separate PR.
💬 ajtowns commented on pull request "init: deprecate `-blockmaxweight` startup option":
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-2938701306)
> > (TBH, I think these options should move into the template request, rather than being node startup options)
>
> Yes, the `BlockCreateOptions` have `block_reserved_weight` variable. From recent @Sjors comment I think there's no need for the `blockmaxweight` option for TP that's why it was not added.
> > I think it makes sense to combine the two values into one internal option that controls the maximum cumulative tx weight.
>
> That sounds like a good idea. Could you expand on that?
...
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-2938701306)
> > (TBH, I think these options should move into the template request, rather than being node startup options)
>
> Yes, the `BlockCreateOptions` have `block_reserved_weight` variable. From recent @Sjors comment I think there's no need for the `blockmaxweight` option for TP that's why it was not added.
> > I think it makes sense to combine the two values into one internal option that controls the maximum cumulative tx weight.
>
> That sounds like a good idea. Could you expand on that?
...
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2125788051)
I don't think this is right. This can lead to missing includes:
```
bash-5.2# cat map.imp
[
{ include: [ "<iterator>", public, "<utility>", public ] },
]
bash-5.2# cat repr.cpp
#include <utility>
#include <iterator>
void A(){
int a{};
int b{std::move(a)};
auto* it{&b};
std::advance(it,0);
}
bash-5.2# include-what-you-use -Xiwyu --mapping_file=map.imp repr.cpp
repr.cpp should add these lines:
repr.cpp should remove these lines:
- #include <iterator> // lines 2
...
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2125788051)
I don't think this is right. This can lead to missing includes:
```
bash-5.2# cat map.imp
[
{ include: [ "<iterator>", public, "<utility>", public ] },
]
bash-5.2# cat repr.cpp
#include <utility>
#include <iterator>
void A(){
int a{};
int b{std::move(a)};
auto* it{&b};
std::advance(it,0);
}
bash-5.2# include-what-you-use -Xiwyu --mapping_file=map.imp repr.cpp
repr.cpp should add these lines:
repr.cpp should remove these lines:
- #include <iterator> // lines 2
...
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2125793574)
I don't think this is right. Can't this lead to missing the tuple include when it is required in the future?
The correct fix is to just add the includes iwyu is asking for
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2125793574)
I don't think this is right. Can't this lead to missing the tuple include when it is required in the future?
The correct fix is to just add the includes iwyu is asking for
💬 maflcko commented on pull request "build: add -Wthread-safety-pointer":
(https://github.com/bitcoin/bitcoin/pull/32647#issuecomment-2938885589)
lgtm ACK 83bfe1485c37d407de7eed11b8ad769a05f78b66
(https://github.com/bitcoin/bitcoin/pull/32647#issuecomment-2938885589)
lgtm ACK 83bfe1485c37d407de7eed11b8ad769a05f78b66
💬 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
...