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

👍 rkrux approved a pull request: "test: Force named args for RPCOverloadWrapper optional args"
(https://github.com/bitcoin/bitcoin/pull/32360#pullrequestreview-2799342118)
tACK fa48be3ba443d2436f754265b86331f84b866130

This pull makes the optional looking arguments as named args instead of being positional in the 4 wrapped methods of `RPCOverloadWrapper` class in the functional test framework. I, too, have a preference for named args over positional as I feel it makes the function call sites easier on the eyes.

There are couple removals of unused fields and methods of `RPCOverloadWrapper` class. Some formatting changes are also there that distracts a bit but
...

💬 rkrux commented on pull request "test: Force named args for RPCOverloadWrapper optional args":
(https://github.com/bitcoin/bitcoin/pull/32360#discussion_r2063682773)
Note: This named arg here is passed down to the non-wrapped methods of `RPCOverloadWrapper` class as well because those are present in `rpc_calls` list, but _after_ the positional args.

💬 9AeinBagheri9 commented on pull request "Update SECURITY.md":
(https://github.com/bitcoin/bitcoin/pull/32362#issuecomment-2835286354)
From f116ff8d5839933ee717f99de11b0aa3f05bdef5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=D8=A2=D8=A6=DB=8C=D9=86=20=D8=A8=D8=A7=D9=82=D8=B1=DB=8C?=
=?UTF-8?q?=20=D8=AC=D9=88=D8=B1=D8=A2=D8=A8=DB=8C?=
<bagheriaein18@gmail.com>
Date: Mon, 28 Apr 2025 17:09:48 +0330
Subject: [PATCH] Update SECURITY.md

---
SECURITY.md | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/SECURITY.md b/SECURITY.md
index fd4c61d176dc3..64d0b49e0abc1 100644
--- a/SECURITY.md
+++ b/SECURIT
...

💬 9AeinBagheri9 commented on pull request "Update SECURITY.md":
(https://github.com/bitcoin/bitcoin/pull/32362#issuecomment-2835288579)
diff --git a/SECURITY.md b/SECURITY.md
index fd4c61d176dc3..64d0b49e0abc1 100644
--- a/SECURITY.md
+++ b/SECURITY.md
@@ -1,4 +1,4 @@
-# Security Policy
+ # Security Policy

## Supported Versions

✅ fanquake closed a pull request: "Update SECURITY.md"
(https://github.com/bitcoin/bitcoin/pull/32362)

📝 fanquake locked 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
...

📝 GarmashAlex opened a pull request: "ci: Use previous releases in tests on Windows"
(https://github.com/bitcoin/bitcoin/pull/32363)
Currently, none of the CI jobs running on Windows (Windows native and Windows test cross-built) use previous releases for testing. This is causing problems with tests like `wallet_migration.py` which require previous releases to be available.

This PR fixes issue #32192 by implementing the following changes:

1. Added Windows binaries to the SHA256_SUMS dictionary in get_previous_releases.py
2. Modified the check_host function to recognize Windows hosts and map them to the "win64" platform
...