📝 sipa opened a pull request: "Low-level cluster linearization code"
(https://github.com/bitcoin/bitcoin/pull/30126)
Based on a part of #29625.
This introduces low-level optimized cluster linearization code, including tests and some benchmarks. It is currently not hooked up to anything.
Roughly the commits are organized into 3 groups:
* Repeat of part of #29625.
* Introduce unoptimized versions of candidate finding and linearizations, plus benchmarks and tests.
* Add various optimizations step by step.
(https://github.com/bitcoin/bitcoin/pull/30126)
Based on a part of #29625.
This introduces low-level optimized cluster linearization code, including tests and some benchmarks. It is currently not hooked up to anything.
Roughly the commits are organized into 3 groups:
* Repeat of part of #29625.
* Introduce unoptimized versions of candidate finding and linearizations, plus benchmarks and tests.
* Add various optimizations step by step.
🤔 jonatack reviewed a pull request: "depends: Remove Qt build-time dependencies"
(https://github.com/bitcoin/bitcoin/pull/29923#pullrequestreview-2061817228)
Concept ACK. This looks like amazing work, great!
I'm not sure how to help test. Using arm64 macOS 14.4.1, this branch builds cleanly and the GUI runs fine. Don't see any issues on first read of the code.
```
$ clang --version
Homebrew clang version 18.1.5
Target: arm64-apple-darwin23.4.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin
```
(https://github.com/bitcoin/bitcoin/pull/29923#pullrequestreview-2061817228)
Concept ACK. This looks like amazing work, great!
I'm not sure how to help test. Using arm64 macOS 14.4.1, this branch builds cleanly and the GUI runs fine. Don't see any issues on first read of the code.
```
$ clang --version
Homebrew clang version 18.1.5
Target: arm64-apple-darwin23.4.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin
```
💬 sipa commented on pull request "Update libsecp256k1 subtree to current master":
(https://github.com/bitcoin/bitcoin/pull/30120#issuecomment-2116140509)
I think the 86 kB option is a bit faster, and the change in binary size/memory is immaterial for Bitcoin Core, so from that perspective maybe we want to use that. On the other hand, signing speed is not super important for us, but still, seems like 86 is the no-downside option.
(https://github.com/bitcoin/bitcoin/pull/30120#issuecomment-2116140509)
I think the 86 kB option is a bit faster, and the change in binary size/memory is immaterial for Bitcoin Core, so from that perspective maybe we want to use that. On the other hand, signing speed is not super important for us, but still, seems like 86 is the no-downside option.
👍 TheCharlatan approved a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-2061831963)
Re-ACK d51fbab4b32d56765e8faab6ad01245fb259b0ca
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-2061831963)
Re-ACK d51fbab4b32d56765e8faab6ad01245fb259b0ca
💬 naiyoma commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2116143536)
Test passes for all three scenarios:
- [x] https://github.com/bitcoin/bitcoin/pull/29996/commits/a7501f779eb84effd5579070b8b1c1a3020273d6 loading a snapshot when chain tip isn't ancestor but has less work
- [x] https://github.com/bitcoin/bitcoin/pull/29996/commits/3f033542b869dfbe037cda705c3b8f7faa32da5d loading a snapshot when chain tip isn't ancestor/descendant, has more work and A valid snapshot file & snapshot block, but the block is not on the most-work chain
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2116143536)
Test passes for all three scenarios:
- [x] https://github.com/bitcoin/bitcoin/pull/29996/commits/a7501f779eb84effd5579070b8b1c1a3020273d6 loading a snapshot when chain tip isn't ancestor but has less work
- [x] https://github.com/bitcoin/bitcoin/pull/29996/commits/3f033542b869dfbe037cda705c3b8f7faa32da5d loading a snapshot when chain tip isn't ancestor/descendant, has more work and A valid snapshot file & snapshot block, but the block is not on the most-work chain
🤔 marcofleon reviewed a pull request: "fuzz: wallet, add target for `Crypter`"
(https://github.com/bitcoin/bitcoin/pull/28074#pullrequestreview-2061842202)
My fuzzer is crashing when I run this. I'm going to look more into it tomorrow.
(https://github.com/bitcoin/bitcoin/pull/28074#pullrequestreview-2061842202)
My fuzzer is crashing when I run this. I'm going to look more into it tomorrow.
💬 laanwj commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2116159858)
> I'm not sure how to help test. Using arm64 macOS 14.4.1, this branch builds cleanly, the unit tests pass, and the GUI runs fine. Don't see any issues on first read of the code.
Thanks for testing! To be clear, this shouldn't affect MacOS at all, Qt on MacOS (and Windows) doesn't have any of the huge bag of dependencies that Qt Linux/UNIX has.
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2116159858)
> I'm not sure how to help test. Using arm64 macOS 14.4.1, this branch builds cleanly, the unit tests pass, and the GUI runs fine. Don't see any issues on first read of the code.
Thanks for testing! To be clear, this shouldn't affect MacOS at all, Qt on MacOS (and Windows) doesn't have any of the huge bag of dependencies that Qt Linux/UNIX has.
💬 furszy commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone ":
(https://github.com/bitcoin-core/gui/pull/807#discussion_r1604036526)
> Would suggest maybe moving this struct to wallet/types.h instead of introducing a new header. That file is meant to hold wallet types that are used outside of the wallet library. (For context see https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1543379055 which talks about wallet/types.h, node/types.h, and common/types.h files).
Nice, sure. I'm on the same boat, I just wrote this without thinking on a general convention.
(https://github.com/bitcoin-core/gui/pull/807#discussion_r1604036526)
> Would suggest maybe moving this struct to wallet/types.h instead of introducing a new header. That file is meant to hold wallet types that are used outside of the wallet library. (For context see https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1543379055 which talks about wallet/types.h, node/types.h, and common/types.h files).
Nice, sure. I'm on the same boat, I just wrote this without thinking on a general convention.
🤔 furszy reviewed a pull request: "refactor: interfaces, make 'createTransaction' less error-prone "
(https://github.com/bitcoin-core/gui/pull/807#pullrequestreview-2061884739)
updated per feedback. Thanks @ryanofsky!
(https://github.com/bitcoin-core/gui/pull/807#pullrequestreview-2061884739)
updated per feedback. Thanks @ryanofsky!
💬 naiyoma commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2116200874)
> I splitted the original commit into two commits (one for each test). The second test ([af0f401](https://github.com/bitcoin/bitcoin/commit/af0f401258e0c189799a36f4487eaa751d779e7b)) may be redundant with this one: #29428. The only difference is that my test is executed on a node that has a divergent chain after block 199. I did that to cover this scenario described in the comments
>
> `[...] Loading a snapshot when the current chain tip is: [...] Not an ancestor or a descendant of the
...
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2116200874)
> I splitted the original commit into two commits (one for each test). The second test ([af0f401](https://github.com/bitcoin/bitcoin/commit/af0f401258e0c189799a36f4487eaa751d779e7b)) may be redundant with this one: #29428. The only difference is that my test is executed on a node that has a divergent chain after block 199. I did that to cover this scenario described in the comments
>
> `[...] Loading a snapshot when the current chain tip is: [...] Not an ancestor or a descendant of the
...
🤔 jonatack reviewed a pull request: "include verbose "debug-message" field in testmempoolaccept response"
(https://github.com/bitcoin/bitcoin/pull/28121#pullrequestreview-2062037268)
Concept ACK d2917e7e0b342509d50325cf7f00da15d81b3c65
(https://github.com/bitcoin/bitcoin/pull/28121#pullrequestreview-2062037268)
Concept ACK d2917e7e0b342509d50325cf7f00da15d81b3c65
💬 jonatack commented on pull request "include verbose "debug-message" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1604141501)
c1be9d4
- it might be more user-friendly to call the field `reject-details`?
- maybe just me, but `a message is provided` might sound like user input
```suggestion
{RPCResult::Type::STR, "reject-details", /*optional=*/true, "Rejection details (only present when 'allowed' is false and rejection details exist)"},
```
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1604141501)
c1be9d4
- it might be more user-friendly to call the field `reject-details`?
- maybe just me, but `a message is provided` might sound like user input
```suggestion
{RPCResult::Type::STR, "reject-details", /*optional=*/true, "Rejection details (only present when 'allowed' is false and rejection details exist)"},
```
💬 jonatack commented on pull request "include verbose "debug-message" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1604146476)
d2917e7e0b342509d50325cf7f00da15d81b3c65
```suggestion
The RPC `testmempoolaccept` response now includes a "reject-details" field in some cases
```
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1604146476)
d2917e7e0b342509d50325cf7f00da15d81b3c65
```suggestion
The RPC `testmempoolaccept` response now includes a "reject-details" field in some cases
```
💬 jonatack commented on pull request "include verbose "debug-message" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1604141675)
https://github.com/bitcoin/bitcoin/commit/c1be9d40f8c3044587d1d3480ccbedfc427951fe while touching the next line, `string` here might be a relic from when the field type wasn't auto-generated in the help, ISTM `reason` would be clearer
```suggestion
{RPCResult::Type::STR, "reject-reason", /*optional=*/true, "Rejection reason (only present when 'allowed' is false)"},
```
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1604141675)
https://github.com/bitcoin/bitcoin/commit/c1be9d40f8c3044587d1d3480ccbedfc427951fe while touching the next line, `string` here might be a relic from when the field type wasn't auto-generated in the help, ISTM `reason` would be clearer
```suggestion
{RPCResult::Type::STR, "reject-reason", /*optional=*/true, "Rejection reason (only present when 'allowed' is false)"},
```
💬 jonatack commented on pull request "include verbose "debug-message" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1604145031)
d2917e7e0b342509d50325cf7f00da15d81b3c65 Not sure if `Updated RPCs` would be better here
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1604145031)
d2917e7e0b342509d50325cf7f00da15d81b3c65 Not sure if `Updated RPCs` would be better here
🤔 pablomartin4btc reviewed a pull request: "refactor: interfaces, make 'createTransaction' less error-prone "
(https://github.com/bitcoin-core/gui/pull/807#pullrequestreview-2062071072)
re-ACK 4d941b5ff43e6e7efa83913ded8cd5806ef92dbe
- Changes from my last review: `CreatedTransactionResult` struct moved as [suggested](https://github.com/bitcoin-core/gui/pull/807#discussion_r1598730244) by @ryanofsky [explaining](https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1543379055) reasoning behind the convention. I like the idea of keeping the namespace, not only for [consistency](https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1424337693) but for clarity and makin
...
(https://github.com/bitcoin-core/gui/pull/807#pullrequestreview-2062071072)
re-ACK 4d941b5ff43e6e7efa83913ded8cd5806ef92dbe
- Changes from my last review: `CreatedTransactionResult` struct moved as [suggested](https://github.com/bitcoin-core/gui/pull/807#discussion_r1598730244) by @ryanofsky [explaining](https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1543379055) reasoning behind the convention. I like the idea of keeping the namespace, not only for [consistency](https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1424337693) but for clarity and makin
...
⚠️ ygcool opened an issue: "dumpprivkey error"
(https://github.com/bitcoin/bitcoin/issues/30129)
bitcoin-core version:27.0
execute:
`bitcoin-cli createwallet "wallet1" false false pass`
`bitcoin-cli getnewaddress`
`bitcoin-cli walletpassphrase pass 100`
`bitcoin-cli dumpprivkey "address"`
dumpprivkey Return error:
```
error code: -4
error message:
Only legacy wallets are supported by this command
```
Is there any solution to export the private key of the address?
If I must use dumpprivkey which version of bitcoin core should I use?
thanks
(https://github.com/bitcoin/bitcoin/issues/30129)
bitcoin-core version:27.0
execute:
`bitcoin-cli createwallet "wallet1" false false pass`
`bitcoin-cli getnewaddress`
`bitcoin-cli walletpassphrase pass 100`
`bitcoin-cli dumpprivkey "address"`
dumpprivkey Return error:
```
error code: -4
error message:
Only legacy wallets are supported by this command
```
Is there any solution to export the private key of the address?
If I must use dumpprivkey which version of bitcoin core should I use?
thanks
💬 furszy commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone ":
(https://github.com/bitcoin-core/gui/pull/807#issuecomment-2116475460)
> nit: on 2nd. commit (https://github.com/bitcoin-core/gui/commit/95aaaa4f023c6a6b629c36cfb9d8abd4bab1cb66), you forgot to remove the change from the makefile as it's not longer needed.
Removed. Thanks.
(https://github.com/bitcoin-core/gui/pull/807#issuecomment-2116475460)
> nit: on 2nd. commit (https://github.com/bitcoin-core/gui/commit/95aaaa4f023c6a6b629c36cfb9d8abd4bab1cb66), you forgot to remove the change from the makefile as it's not longer needed.
Removed. Thanks.
💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2116484602)
> There seems to be a conflict with #30098. Maybe rebase?
Thanks! Rebased now.
> If other reviewers or maintainers are not happy with introducing a bash script, I could volunteer some time to translate it.
Yes this could be a good idea. The script started out just being a few lines of bash listing symbols exported by one library and imported by another one. But then it grew as it was extended to support multiple libraries and suppress known errors. So now it's no longer a small script,
...
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2116484602)
> There seems to be a conflict with #30098. Maybe rebase?
Thanks! Rebased now.
> If other reviewers or maintainers are not happy with introducing a bash script, I could volunteer some time to translate it.
Yes this could be a good idea. The script started out just being a few lines of bash listing symbols exported by one library and imported by another one. But then it grew as it was extended to support multiple libraries and suppress known errors. So now it's no longer a small script,
...
💬 edilmedeiros commented on issue "dumpprivkey error":
(https://github.com/bitcoin/bitcoin/issues/30129#issuecomment-2116495430)
Newer version of bitcoin core use [descriptor wallets](https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md).
You can see them using:
```
bitcoin-cli listdescriptors true
```
The `true` argument mean to export the private descriptors, the equivalent of private keys.
I usually use the tool `jq` to extract them to an environment variable like so:
```
DESCRIPTORS=$(bitcoin-cli listdescriptors true | jq -r .descriptors)
```
I import the descriptors to my wallet with:
```
bitcoin-cl
...
(https://github.com/bitcoin/bitcoin/issues/30129#issuecomment-2116495430)
Newer version of bitcoin core use [descriptor wallets](https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md).
You can see them using:
```
bitcoin-cli listdescriptors true
```
The `true` argument mean to export the private descriptors, the equivalent of private keys.
I usually use the tool `jq` to extract them to an environment variable like so:
```
DESCRIPTORS=$(bitcoin-cli listdescriptors true | jq -r .descriptors)
```
I import the descriptors to my wallet with:
```
bitcoin-cl
...