💬 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
...

💬 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]
...

💬 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).

💬 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
...

💬 hebasto commented on pull request "crypto: disable ASan for sha256_sse4 with Clang":
(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.

🤔 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.

💬 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`.

💬 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

💬 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

💬 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
...

💬 vasild commented on pull request "config: allow setting -proxy per network":
(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.

💬 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

💬 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

💬 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
...

💬 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.

💬 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.

💬 Sjors commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2858435213)
ACK 8b73630c6fe64158bf123da5e22bd0a71ab305c9.

> Windows 11 against HWI 3.1.0 on testnet4.
>
> It crashes immediately after creating a new wallet from a Trezor model T.

This still happens, but it seems only on testnet4, e.g. not on mainnet and not on signet (didn't try testnet3). It also happens on macOS 15.4.1, so it's clearly unrelated (will open an issue).

On Windows 11 I was able to create a new wallet with the GUI, verify an address and make a transaction.

I did not test handl
...

💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077537805)
done