💬 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
...
💬 vasild commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2077497713)
Done.
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2077497713)
Done.
💬 vasild commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2077498609)
Done and also used `<` and `>` for the other fields.
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2077498609)
Done and also used `<` and `>` for the other fields.
💬 1440000bytes commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2858379376)
> but not allowing a node runner to configure limits on their mempool to make it feasible to run a node (in terms of RAM) goes against our core value of making nodes easy to run for personal use
https://github.com/bitcoin/bitcoin/pull/32406
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2858379376)
> but not allowing a node runner to configure limits on their mempool to make it feasible to run a node (in terms of RAM) goes against our core value of making nodes easy to run for personal use
https://github.com/bitcoin/bitcoin/pull/32406
💬 maflcko commented on pull request "crypto: disable ASan for sha256_sse4 with Clang":
(https://github.com/bitcoin/bitcoin/pull/32437#issuecomment-2858381159)
lgtm ACK 4e8ab5e00fa72016a7ec0e0505ca025d4e59e4d8
I haven't tested this, but the diff looks plausible
(https://github.com/bitcoin/bitcoin/pull/32437#issuecomment-2858381159)
lgtm ACK 4e8ab5e00fa72016a7ec0e0505ca025d4e59e4d8
I haven't tested this, but the diff looks plausible
💬 vasild commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#issuecomment-2858386981)
`6a9f04688f...2059eaa692`: address suggestions
> If the user sets `-proxy=addr:port=tor` then `-onion=` shouldn't be allowed, no? Otherwise one could set 2 diff proxies for Tor(?).
I think that is ok, assuming the expectation is that later options override earlier ones. For example, without this PR, `-proxy=x -onion=y` is supposed to set the Tor proxy to `y`, overriding the earlier `x`. Updated `tor.md` to mention that.
> So, shouldn't `-proxy=0=cjdns` be the default if `-cjdnsreachable
...
(https://github.com/bitcoin/bitcoin/pull/32425#issuecomment-2858386981)
`6a9f04688f...2059eaa692`: address suggestions
> If the user sets `-proxy=addr:port=tor` then `-onion=` shouldn't be allowed, no? Otherwise one could set 2 diff proxies for Tor(?).
I think that is ok, assuming the expectation is that later options override earlier ones. For example, without this PR, `-proxy=x -onion=y` is supposed to set the Tor proxy to `y`, overriding the earlier `x`. Updated `tor.md` to mention that.
> So, shouldn't `-proxy=0=cjdns` be the default if `-cjdnsreachable
...
💬 hebasto commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077521444)
What alternatives are you suggesting for configuring the Developer Command Prompt?
FWIW, we've been using this action since https://github.com/bitcoin/bitcoin/pull/28173.
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077521444)
What alternatives are you suggesting for configuring the Developer Command Prompt?
FWIW, we've been using this action since https://github.com/bitcoin/bitcoin/pull/28173.
💬 pinheadmz commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2858416006)
> > but not allowing a node runner to configure limits on their mempool to make it feasible to run a node (in terms of RAM) goes against our core value of making nodes easy to run for personal use
>
>
>
> https://github.com/bitcoin/bitcoin/pull/32406
There's also `-maxmempool` specifically for this. And if you still have memory problems running a full node open an issue.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2858416006)
> > but not allowing a node runner to configure limits on their mempool to make it feasible to run a node (in terms of RAM) goes against our core value of making nodes easy to run for personal use
>
>
>
> https://github.com/bitcoin/bitcoin/pull/32406
There's also `-maxmempool` specifically for this. And if you still have memory problems running a full node open an issue.