💬 maflcko commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2834929279)
Scheduled 10 runs in https://cirrus-ci.com/task/5397894338969600, but it seems they are all passing. Huh

💬 laanwj commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2834937440)
> Scheduled 10 runs in https://cirrus-ci.com/task/5397894338969600, but it seems they are all passing. Huh

It could be 4a6b99b3172fe007328bd260968acdb0dd78c569, though i have no idea why that would make a difference. In any case, nice that it passes now.

💬 maflcko commented on pull request "test: Slim down previous releases bdb check":
(https://github.com/bitcoin/bitcoin/pull/32350#issuecomment-2834978311)
> I have noticed that the 17 release was removed from this test recently in some PR and now 18, 19 are removed.

Correct. For reference, the reasons for removal were different. This removal in this pull request only became possible after the bdb removal removing the test for the v0.19 18075 behavior (https://github.com/bitcoin/bitcoin/issues/18075)

> I'd assume that the 20 release is kept in this test for a considerable time as it marks the last release that didn't contain descriptors (& SQ
...

💬 maflcko commented on pull request "test: Slim down previous releases bdb check":
(https://github.com/bitcoin/bitcoin/pull/32350#discussion_r2063491579)
Correct. Thanks for adding the reference.

💬 maflcko commented on pull request "test: Slim down previous releases bdb check":
(https://github.com/bitcoin/bitcoin/pull/32350#discussion_r2063492156)
thx, may do if I have to re-touch.

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