💬 fanquake commented on issue "windows: usage of deprecated `std:wstring_convert`":
(https://github.com/bitcoin/bitcoin/issues/32361#issuecomment-2835002973)
> with use of the subprocess library soon

Note that `subprocess.h` is also using `wstring_convert`: https://github.com/bitcoin/bitcoin/blob/d62c2d82e14d27307d8790fd9d921b740b784668/src/util/subprocess.h#L1071.

💬 fanquake commented on pull request "build: Drop option to disable hardening.":
(https://github.com/bitcoin/bitcoin/pull/32071#issuecomment-2835045019)
Guix Build:
```bash
db95c26122204cf686ae4d0a7d6f2e2f2cd30d7ea0dce425f28bf4fa1af9fa12 guix-build-77e553ab6a0a/output/aarch64-linux-gnu/SHA256SUMS.part
3b5b3fd4b6ca8539e2ea5e50afdea37593f1f2e0c0b26851c20fc0505dd79a42 guix-build-77e553ab6a0a/output/aarch64-linux-gnu/bitcoin-77e553ab6a0a-aarch64-linux-gnu-debug.tar.gz
4c6b3a3949cc70e3f1a82fa38ed5c204c54074a79b267d67c02439279099c2ff guix-build-77e553ab6a0a/output/aarch64-linux-gnu/bitcoin-77e553ab6a0a-aarch64-linux-gnu.tar.gz
b9d2d30e3b5df7ad
...

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2835048806)
> Is making `m_nodes_mutex` non-recursive a goal?

No, I did not think about that before you mentioned it. The goal here is to make the interface around `FindNode()` safer and simpler (don't return a possibly dangling pointer and don't return a pointer at all since callers can do with a boolean).

I did mention dropping the recursive lock because I think less recursive locking is good, even if the mutex remains recursive.

> I'm curious how far we are from a non-recursive `m_nodes_mutex`?
...

💬 maflcko commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#discussion_r2063545701)
style nit: fd's probably can't be negative nor include `+` as prefix? So `ToIntegral<uint64_t>` may be a better fit.

💬 dergoegge commented on pull request "tests: Added progress tracker when running fuzz test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/32354#issuecomment-2835086272)
Concept ~0

> it is hard to tell how far you are in the progress

I think there is only a handful of harnesses that take up most of the time, so a progress bar like you have here won't do much to show progress. It'll look like you're stuck at 9x% percent for a long time, so answering "when will this finish" is about as hard as before.

🚀 fanquake merged a pull request: "build: Drop option to disable hardening."
(https://github.com/bitcoin/bitcoin/pull/32071)

🤔 l0rinc requested changes to a pull request: "cli: return local services in -netinfo"
(https://github.com/bitcoin/bitcoin/pull/31886#pullrequestreview-2798792287)
Concept ACK

I have testes it manually, seems to work correctly, but I would encourage the author to cover it with some simple functional tests and to consider sorting the values to make sure similar values end up closer together

💬 l0rinc commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2063352327)
The new additions don't seem to be covered by any tests https://corecheck.dev/bitcoin/bitcoin/pulls/31886

Actually the whole file is a blood-bath: https://maflcko.github.io/b-c-cov/total.coverage/src/bitcoin-cli.cpp.gcov.html

But since the following functional tests seem to cover this file:
* interface_bitcoin_cli.py
* interface_rpc.py
* rpc_deriveaddresses.py
* rpc_generate.py
* tool_signet_miner.py
* wallet_createwallet.py
* wallet_descriptor.py
* wallet_importdescriptors.py
* w
...

💬 l0rinc commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2063442975)
We're currently getting each string, recreating it as lowercase, iterating it again for potential replacement, adding it to a vector, which we're recreating at the end again as a string.

We could refactor it slightly to be similar to `FormatServices`, by doing the lowercase and replace operations once on the final joined string instead of processing each string individually:
```suggestion
static std::string ServicesList(const UniValue& services)
{
std::string res{services.
...

💬 l0rinc commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2063518813)
should we maybe sort to avoid an order like:
```bash
Local services: network, witness, network limited, p2p v2
```

it may make sense to have:
```bash
Local services: network, network limited, p2p v2, witness
```

💬 maflcko commented on pull request "tests: Added progress tracker when running fuzz test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/32354#issuecomment-2835132455)
Agree. The only case where it'd help is possibly generate, which is based on a fixed time.

If this was done, i'd say it is fine to log all completed futures, and remove the 5 percent "step".

More useful in theory could be to log the name of the remaining fuzz tests, like the functional tests.

💬 hebasto commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2835177926)
> On top of this PR, plus a pile of `EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex)` additions in `net.h`. Should I PR that (separately from the current PR)?

I'd appreciate it. It would be good to cross `m_nodes_mutex` off the [list](https://github.com/bitcoin/bitcoin/issues/19303).

💬 rkrux commented on pull request "test: Force named args for RPCOverloadWrapper optional args":
(https://github.com/bitcoin/bitcoin/pull/32360#discussion_r2063625236)
> avoiding any wrapped method.

Avoiding any of the below 4 wrapped methods?

> add a more general (currently
unused) method rpc_passthrough, which could be used in the future, if
there really is need for it.

It seems to be effectively same as calling the instance returned by `__getattr__ ` above, while improving readability. I don't think it hurts to add this but I sense it will not end up being used if there are not already usages of it in the framework.

💬 maflcko commented on pull request "test: Force named args for RPCOverloadWrapper optional args":
(https://github.com/bitcoin/bitcoin/pull/32360#discussion_r2063636905)
thx, removed

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2063650801)
I think this:

After https://github.com/bitcoin/bitcoin/pull/32338 makes it, then here in this PR:

* rename `AlreadyConnectedToAddress()` to `IsConnectedToAddr()`, compare just the address
* rename the two remaining `FindNode()` to `IsConnectedToAddrPort()`, overload, one taking a string and the other taking a `CService`, both comparing the address and the port.

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2063655007)
this was already checked for earlier in the first if block

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2063660146)
In debug builds at least it will fail, see `// can only fail if main is oversized`

💬 instagibbs commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2063676228)
done

💬 instagibbs commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2063676614)
changed to `push` everywhere

📝 9AeinBagheri9 opened a pull request: "Update SECURITY.md"
(https://github.com/bitcoin/bitcoin/pull/32362)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...