💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1170599011)
I have added it back.

💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1170599147)
Done

💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1170599313)
Done

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1170606402)
Interesting, though only 3 of the 8 seem to be extraneous annotations (two in `src/base58.cpp` and one in `src/node/eviction.cpp`, and the one in `src/script/descriptor.cpp` should just have `static` added).

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1170627585)
fb3d39ad3e391da273a142c36 Might make more sense to define this as a pure virtual function.
```suggestion
[[nodiscard]] virtual bool IsKeyActive(const CScript& script) const = 0;
```

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