💬 maflcko commented on issue "intermittent issue in wallet_listsinceblock.py ( test_framework.test_node.FailedToStartError: [node 3] bitcoind exited with status -6 during initialization.)":
(https://github.com/bitcoin/bitcoin/issues/32435#issuecomment-2858030829)
Not sure how to debug this further, so closing for now. However, further input is still very welcome.
(https://github.com/bitcoin/bitcoin/issues/32435#issuecomment-2858030829)
Not sure how to debug this further, so closing for now. However, further input is still very welcome.
💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-2858043588)
Re https://github.com/bitcoin/bitcoin/pull/32317#pullrequestreview-2785006540
> nit: The name SpendBlock may not be clear (unless this concept is already known and I'm missing the point).
Maybe BIP30Validation, something like that, would be clearer.
It is doing more than BIP-30 validation, so I don't think that would accurate either. I thought the name was pretty clear for what it does, but I'd be open to other suggestions.
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-2858043588)
Re https://github.com/bitcoin/bitcoin/pull/32317#pullrequestreview-2785006540
> nit: The name SpendBlock may not be clear (unless this concept is already known and I'm missing the point).
Maybe BIP30Validation, something like that, would be clearer.
It is doing more than BIP-30 validation, so I don't think that would accurate either. I thought the name was pretty clear for what it does, but I'd be open to other suggestions.
📝 theStack opened a pull request: "test: refactor: negate signature-s using libsecp256k1"
(https://github.com/bitcoin/bitcoin/pull/32436)
This small PR gets rid of manual mod-n inversion of the ECDSA signature-s part in unit tests (introduced a long time ago in #5256, triggered by https://github.com/bitcoin-core/secp256k1/pull/69) by using secp256k1 instead. The function wasn't available at that time, but was introduced about three years later, see https://github.com/bitcoin-core/secp256k1/pull/408. Note that as the name suggests, `secp256k1_ec_seckey_negate` is meant to be used for secret keys, but it obviously works in general f
...
(https://github.com/bitcoin/bitcoin/pull/32436)
This small PR gets rid of manual mod-n inversion of the ECDSA signature-s part in unit tests (introduced a long time ago in #5256, triggered by https://github.com/bitcoin-core/secp256k1/pull/69) by using secp256k1 instead. The function wasn't available at that time, but was introduced about three years later, see https://github.com/bitcoin-core/secp256k1/pull/408. Note that as the name suggests, `secp256k1_ec_seckey_negate` is meant to be used for secret keys, but it obviously works in general f
...
💬 Crypt-iQ commented on pull request "fuzz: Remove unused TimeoutExpired catch in fuzz runner":
(https://github.com/bitcoin/bitcoin/pull/32388#issuecomment-2858082209)
crACK fa4804009ceba96926edd7f55ea22442ebdc665d
Just curious, why did adding the `read_file` fallback cause the timeout to be unused? I can't really speak to the Windows side-effect as I don't totally understand it.
(https://github.com/bitcoin/bitcoin/pull/32388#issuecomment-2858082209)
crACK fa4804009ceba96926edd7f55ea22442ebdc665d
Just curious, why did adding the `read_file` fallback cause the timeout to be unused? I can't really speak to the Windows side-effect as I don't totally understand it.
👍 rkrux approved a pull request: "docs: Improve `keypoolrefill` RPC docs"
(https://github.com/bitcoin/bitcoin/pull/32429#pullrequestreview-2821296596)
ACK cd00fb85e9e0997120715e68374462cff0dad7eb given the suggestions are addressed.
I checked that by default there are 4 scriptpubkeymans setup on the first run of the wallet inside `SetupDescriptorScriptPubKeyMans`. Also, ack because the lack of this information has caused an issue to be created.
(https://github.com/bitcoin/bitcoin/pull/32429#pullrequestreview-2821296596)
ACK cd00fb85e9e0997120715e68374462cff0dad7eb given the suggestions are addressed.
I checked that by default there are 4 scriptpubkeymans setup on the first run of the wallet inside `SetupDescriptorScriptPubKeyMans`. Also, ack because the lack of this information has caused an issue to be created.
🚀 fanquake merged a pull request: "fuzz: Remove unused TimeoutExpired catch in fuzz runner"
(https://github.com/bitcoin/bitcoin/pull/32388)
(https://github.com/bitcoin/bitcoin/pull/32388)
📝 fanquake opened a pull request: "crypto: disable ASan for sha256_sse4 with Clang"
(https://github.com/bitcoin/bitcoin/pull/32437)
This also fails to compile when optimisations are being used, see: https://github.com/bitcoin/bitcoin/issues/31913.
So just disable ASan under any optimisation level.
Closes #31913.
(https://github.com/bitcoin/bitcoin/pull/32437)
This also fails to compile when optimisations are being used, see: https://github.com/bitcoin/bitcoin/issues/31913.
So just disable ASan under any optimisation level.
Closes #31913.
💬 rkrux commented on pull request "lint: Remove string exclusion from locale check":
(https://github.com/bitcoin/bitcoin/pull/32434#issuecomment-2858137223)
> > The exclusion isn't needed. In fact, it prevents detection of "bla" + wrong().
>
> i wonder if the same is true for the comment exclusion. Would it detect `/* bla */ wrong()`.
It seems to.
```bash
➜ bitcoin git:(keypoolrefill_docs) ✗ ./test/lint/lint-locale-dependence.py
The locale dependent function std::to_string(...) appears to be used:
src/wallet/rpc/addresses.cpp: "By default, descriptor wallets have 4 active ranged descriptors (\"legacy\", \"p2sh-segwit\", \
...
(https://github.com/bitcoin/bitcoin/pull/32434#issuecomment-2858137223)
> > The exclusion isn't needed. In fact, it prevents detection of "bla" + wrong().
>
> i wonder if the same is true for the comment exclusion. Would it detect `/* bla */ wrong()`.
It seems to.
```bash
➜ bitcoin git:(keypoolrefill_docs) ✗ ./test/lint/lint-locale-dependence.py
The locale dependent function std::to_string(...) appears to be used:
src/wallet/rpc/addresses.cpp: "By default, descriptor wallets have 4 active ranged descriptors (\"legacy\", \"p2sh-segwit\", \
...
🤔 rkrux reviewed a pull request: "lint: Remove string exclusion from locale check"
(https://github.com/bitcoin/bitcoin/pull/32434#pullrequestreview-2821339543)
Maybe can mention the `to_string` function in the docs as well? It's already a part of the linter.
https://github.com/bitcoin/bitcoin/blob/59d3e4ed34eb55cb40928d524cb0bd5e183ed85a/doc/developer-notes.md?plain=1#L1022-L1042
(https://github.com/bitcoin/bitcoin/pull/32434#pullrequestreview-2821339543)
Maybe can mention the `to_string` function in the docs as well? It's already a part of the linter.
https://github.com/bitcoin/bitcoin/blob/59d3e4ed34eb55cb40928d524cb0bd5e183ed85a/doc/developer-notes.md?plain=1#L1022-L1042
💬 maflcko commented on pull request "lint: Remove string exclusion from locale check":
(https://github.com/bitcoin/bitcoin/pull/32434#issuecomment-2858207860)
> > The exclusion isn't needed. In fact, it prevents detection of "bla" + wrong().
>
> i wonder if the same is true for the comment exclusion. Would it detect `/* bla */ wrong()`.
Yes, I can see it may happen with named args. Fixed.
Though, Regex isn't really the right tool to parse C++ code. Better would be to write a clang-tidy plugin. See https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools/bitcoin-tidy
> Maybe can mention the `to_string` function in the docs as
...
(https://github.com/bitcoin/bitcoin/pull/32434#issuecomment-2858207860)
> > The exclusion isn't needed. In fact, it prevents detection of "bla" + wrong().
>
> i wonder if the same is true for the comment exclusion. Would it detect `/* bla */ wrong()`.
Yes, I can see it may happen with named args. Fixed.
Though, Regex isn't really the right tool to parse C++ code. Better would be to write a clang-tidy plugin. See https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools/bitcoin-tidy
> Maybe can mention the `to_string` function in the docs as
...
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2858241771)
@ryanofsky CI complains about `std::move` in `libmultiprocess`: https://cirrus-ci.com/task/6187773452877824, e.g.:
```
/ci_container_base/src/ipc/libmultiprocess/include/mp/proxy-io.h:252:16: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors]
[05:29:23.932] 252 | m_fn = std::move(fn);
[05:29:23.932] | ^~~~~~~~~
[05:29:23.932]
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2858241771)
@ryanofsky CI complains about `std::move` in `libmultiprocess`: https://cirrus-ci.com/task/6187773452877824, e.g.:
```
/ci_container_base/src/ipc/libmultiprocess/include/mp/proxy-io.h:252:16: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors]
[05:29:23.932] 252 | m_fn = std::move(fn);
[05:29:23.932] | ^~~~~~~~~
[05:29:23.932]
...
💬 maflcko commented on pull request "fuzz: Remove unused TimeoutExpired catch in fuzz runner":
(https://github.com/bitcoin/bitcoin/pull/32388#issuecomment-2858245569)
> Just curious, why did adding the `read_file` fallback cause the timeout to be unused?
`read_file` was implemented in https://github.com/bitcoin/bitcoin/commit/f59bee3fb242c9e02781a35272cf9644f37e7fc1. Previously, it would just call `read_stdin`, which then may time out (because nothing is fed into stdin).
(https://github.com/bitcoin/bitcoin/pull/32388#issuecomment-2858245569)
> Just curious, why did adding the `read_file` fallback cause the timeout to be unused?
`read_file` was implemented in https://github.com/bitcoin/bitcoin/commit/f59bee3fb242c9e02781a35272cf9644f37e7fc1. Previously, it would just call `read_stdin`, which then may time out (because nothing is fed into stdin).
💬 laanwj commented on pull request "lint: Remove string exclusion from locale check":
(https://github.com/bitcoin/bitcoin/pull/32434#issuecomment-2858283813)
Code review ACK fa24fdcb7f474e6959ae4d8fe9759d365c6c021b.
> Though, Regex isn't really the right tool to parse C++ code. Better would be to write a clang-tidy plugin. See https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools/bitcoin-tidy
Agree. The current script is more of a band-aid, there are likely better ways to check if certain functions aren't used in C++ code.
What came to mind is "just check the linker output, the linker knows". However this won't catch inline / templ
...
(https://github.com/bitcoin/bitcoin/pull/32434#issuecomment-2858283813)
Code review ACK fa24fdcb7f474e6959ae4d8fe9759d365c6c021b.
> Though, Regex isn't really the right tool to parse C++ code. Better would be to write a clang-tidy plugin. See https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools/bitcoin-tidy
Agree. The current script is more of a band-aid, there are likely better ways to check if certain functions aren't used in C++ code.
What came to mind is "just check the linker output, the linker knows". However this won't catch inline / templ
...
💬 hebasto commented on pull request "crypto: disable ASan for sha256_sse4 with Clang":
(https://github.com/bitcoin/bitcoin/pull/32437#issuecomment-2858284934)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/32437#issuecomment-2858284934)
Concept ACK.
👍 laanwj approved a pull request: "crypto: disable ASan for sha256_sse4 with Clang"
(https://github.com/bitcoin/bitcoin/pull/32437#pullrequestreview-2821466599)
Code review ACK 4e8ab5e00fa72016a7ec0e0505ca025d4e59e4d8
Have not tested.
(https://github.com/bitcoin/bitcoin/pull/32437#pullrequestreview-2821466599)
Code review ACK 4e8ab5e00fa72016a7ec0e0505ca025d4e59e4d8
Have not tested.
🤔 rkrux reviewed a pull request: "Remove the legacy wallet and BDB dependency"
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2821415265)
Couple of points in 8ede6dea0c55bb4afefa790b83dd4da48a2f84da, can be done in follow up if no more changes being in this PR.
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2821415265)
Couple of points in 8ede6dea0c55bb4afefa790b83dd4da48a2f84da, can be done in follow up if no more changes being in this PR.
💬 rkrux commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2077427167)
Besides the typo `s/an/a` above, the comment "Import a private key" doesn't go well with `rescanblockchain`.
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2077427167)
Besides the typo `s/an/a` above, the comment "Import a private key" doesn't go well with `rescanblockchain`.
💬 rkrux commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2077459615)
In #31243, `IsSingleKey` was added with a TODO to remove it after removing legacy wallets.
I checked that no other usage of `IsSingleKey` is left, it can be removed.
https://github.com/bitcoin/bitcoin/blob/6d5edfcc585bab3374ae14aa7918b3e178e016aa/src/script/descriptor.h#L114-L117
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2077459615)
In #31243, `IsSingleKey` was added with a TODO to remove it after removing legacy wallets.
I checked that no other usage of `IsSingleKey` is left, it can be removed.
https://github.com/bitcoin/bitcoin/blob/6d5edfcc585bab3374ae14aa7918b3e178e016aa/src/script/descriptor.h#L114-L117
💬 instagibbs commented on pull request "test: refactor: overhaul (w)txid determination for `CTransaction` objects":
(https://github.com/bitcoin/bitcoin/pull/32421#issuecomment-2858353344)
concept ACK either way; we lose mental cycles every time we need to figure this out
(https://github.com/bitcoin/bitcoin/pull/32421#issuecomment-2858353344)
concept ACK either way; we lose mental cycles every time we need to figure this out
💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2858366754)
re-ACK de054df6dc32cbd8b127c6761d9c65d95025e08 🔗
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK de054df6dc32cbd8b127c
...
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2858366754)
re-ACK de054df6dc32cbd8b127c6761d9c65d95025e08 🔗
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK de054df6dc32cbd8b127c
...