💬 Emzy commented on pull request "p2p: update hardcoded mainnet seeds for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27488#issuecomment-1513738537)
ACK.
Verified that my nodes are added.
They are long running. And I plan to run them for the foreseeable future.

💬 ryanofsky commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1513771771)
re: https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1507916357

@ajtowns I like the safety check you suggested, and agree it could catch some programming errors. But I also think what `send` and `sendall` functions are trying to do to provide compatibility is reasonable, and adding the check would limit what they are able to do.

`send` and `sendall` currently accept `conf_target`, `estimate_mode` and `fee_rate` values as positional parameters (1), or as named parameters (2), or a
...

💬 achow101 commented on pull request "p2p: update hardcoded mainnet seeds for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27488#issuecomment-1513776313)
What criteria is used to choose the manually selected nodes?

💬 ryanofsky commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1170558620)
> I checked it and its history again and can't think of a good reason either. Should I amend here, or go for the follow up?

Either way seems fine. Would be nice to turn these into simple includes, but I was asking mostly out of curiosity

💬 jonatack commented on pull request "p2p: update hardcoded mainnet seeds for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27488#issuecomment-1513791528)
> What criteria is used to choose the manually selected nodes?

Reachability, uptime, returned by getnodeaddresses (!IsTerrible), and service bit 1. Some are run by colleagues or known persons in the space, in some cases since a long time.

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