🤔 jonatack reviewed a pull request: "Add wallet method to detect if a key is "active""
(https://github.com/bitcoin/bitcoin/pull/27216#pullrequestreview-1391018900)
ACK 8139075244f1f361a14f80920c45d8c4f41d553e modulo eyes from someone more familiar with the wallet.

Commit 2a5774f45c07f0d000f0cbdf0 should probably be renamed to something like `test: cover ScriptPubKeyMan::IsKeyActive and IsDestinationActive`

Re-reviewing this shuffled version was a little trickier due to the unneeded rebase. Please rebase only when you have to. Aside from that, the reorganized commits seem easier to review now.

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1170643262)
fb3d39ad3e391da273a142c36 It may make sense to declare this base class member as a pure virtual function.
```suggestion
virtual bool IsKeyActive(const CScript& script) const = 0;
```

🤔 brunoerg reviewed a pull request: "p2p: update hardcoded mainnet seeds for 25.x"
(https://github.com/bitcoin/bitcoin/pull/27488#pullrequestreview-1391084553)
Perhaps it's time to encourage people to build their own asmap database instead of just dowloading it?
```bash
curl https://bitcoin.sipa.be/asmap-filled.dat > asmap-filled.dat
```

💬 hebasto commented on issue "Replace all of the RecursiveMutex instances with the Mutex ones":
(https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1513950602)
> This may also fix the false-positive
>
> https://github.com/bitcoin/bitcoin/blob/2fa7344aa9bbce8069ebd6bef5f6a22f0b7c5c56/test/sanitizer_suppressions/tsan#L15-L16

FWIW, I'm not able to reproduce "deadlock" TSan warning using clang-14 starting from 9996b1806a189a9632c9f5023489eb47bf87ca05 when depends can be compiled. Last time the warning was observed for clang-10.

💬 jonatack commented on pull request "p2p: update hardcoded mainnet seeds for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27488#issuecomment-1513962484)
> Perhaps it's time to encourage people to build their own asmap database instead of just dowloading it?

That might be tangential to this pull but feel free to propose a concrete diff or a follow-up.

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#issuecomment-1514035359)
added an extra commit to ensure the non-deterministic test will not fail intermittently. will incorporate the outstanding feedback separately

💬 vasild commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#discussion_r1170892009)
I think that's a bit too verbose.

💬 vasild commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#discussion_r1170894501)
Named args at call sites look too verbose to me and here a short `name` is ok because the scope if very limited - only used on one line.

💬 gruve-p commented on pull request "kernel: chainparams updates for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27482#issuecomment-1514238275)
ACK https://github.com/bitcoin/bitcoin/pull/27482/commits/a2bef805c11a4ab391f4ea635bfde7dd2ec9ce81

💬 gruve-p commented on pull request "[24.1] Bump version to 24.1rc2":
(https://github.com/bitcoin/bitcoin/pull/27486#issuecomment-1514240526)
ACK https://github.com/bitcoin/bitcoin/pull/27486/commits/03a16a1da09f45721c799c719e48350dfe12ae88

💬 MarcoFalke commented on issue "Replace all of the RecursiveMutex instances with the Mutex ones":
(https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514259259)
You should be able to see it in the tsan CI task or with libc++ locally?

💬 MarcoFalke commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1170929813)
Yeah, I think it is good to know of false positives and then report them upstream. Otherwise they are less likely to be fixed.

🚀 fanquake merged a pull request: "[24.1] Bump version to 24.1rc2"
(https://github.com/bitcoin/bitcoin/pull/27486)

👍 MarcoFalke approved a pull request: "refactor: Extract common/args from util/system"
(https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1391499392)
review ACK ec51c252de42e11906f114ac05e476fd88ca2176 🦅

<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: review ACK ec51c252de4
...

💬 MarcoFalke commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1170972002)
Any reason for a separate section for this header?

💬 MarcoFalke commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1170983576)
Can you explain this change?

💬 MarcoFalke commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1170994610)
nit: The comment is incomplete (missing AnyPtr stuff etc), and I don't think it is a good use of time to make it complete. Also, the file has less than 10 functions and we don't put those comments in other files, so might as well remove it.

💬 MarcoFalke commented on pull request "depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode":
(https://github.com/bitcoin/bitcoin/pull/27447#issuecomment-1514364903)
This should also fix a segfault with `DEBUG=1`. To reproduce on a fresh install of Ubuntu Jammy:

* `export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core && apt install build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq make automake cmake curl g++-multilib libtool binutils bsdmainutils pkg-config python3 patch biso
...

💬 fanquake commented on issue "Replace all of the RecursiveMutex instances with the Mutex ones":
(https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514372323)
> You should be able to see it in the tsan CI task or with libc++ locally?

I don't currently reproduce on x86_64 with master +:
```diff
--- a/test/sanitizer_suppressions/tsan
+++ b/test/sanitizer_suppressions/tsan
@@ -12,9 +12,6 @@ race:DatabaseBatch
race:zmq::*
race:bitcoin-qt

-# deadlock (TODO fix)
-deadlock:Chainstate::ConnectTip
-
# Intentional deadlock in tests
deadlock:sync_tests::potential_deadlock_detected
```

and

```bash
time FILE_ENV="./ci/test/00_setup_env
...

💬 willcl-ark commented on pull request "doc: Improve documentation of rpcallowip":
(https://github.com/bitcoin/bitcoin/pull/27480#discussion_r1171059750)
Thanks, I did write it like this intentially as I felt that the final 3 are all variants of network/CIDR, but writing them as a sub-list didn't read well to me...

Thinking more about it I think they should be called wildcard addresses, so perhaps we go with:

```cpp
argsman.AddArg("-rpcallowip=<ip>", "Allow JSON-RPC connections from specified source. Valid for <ip> are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0), a network/CIDR (e.g. 1.2.3.4/24), or wildc
...