💬 jonatack commented on pull request "p2p: update hardcoded mainnet seeds for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27488#issuecomment-1513792371)
https://github.com/sipa/bitcoin-seeder/issues/92 and https://github.com/sipa/bitcoin-seeder/pull/101 could simplify the process when done.

💬 achow101 commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1513814497)
-0

If we want to allow for more configurable signet consensus rules, then I think we need to do it in a more robust way rather than ad-hoc adding options that all users must remember to set if they want to use a particular signet. As these are consensus parameters, forgetting to set the option correctly will result in eventual consensus failure which is generally only resolved by nuking the data directory. This may garner additional and unnecessary issues being opened in this repo which we ge
...

💬 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_r1170596878)
I have implemented this suggestion, I agree that it is much clearer now.

💬 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_r1170597818)
I implemented your next suggestion about simplifying the code in the lambda, so the comments are a bit different now but I did add them 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_r1170598598)
This is simpler and it is a small change so I've implemented it in this commit.

💬 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