💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1165987641)
Would be good to check that the map contains the element prior accessing it. Otherwise the GUI could be adding a new address book entry by mistake.

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1507577994)
Thanks so much for the great review @vasild I think I addressed all your feedback up to the adding UNIX to CNetAddr. I understand your concerns there and we can chat about it, I'll look at alternatives as well. What I did do in this last push was mostly separate unix sockets from regular sockets, and adding `Proxy.GetSocket()` to decide which socket factory to call.

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1166002075)
I'm a bit intimidated by the syntax `prevector<ADDR_IPV6_SIZE, uint8_t> m_addr{ADDR_IPV6_SIZE, 0x0};` -- is that only 16 bytes?

📝 theuni opened a pull request: "verify-commits: error and exit cleanly when git is too old."
(https://github.com/bitcoin/bitcoin/pull/27461)
Requested by fanquake. Rather than failing with a cryptic error with older git, fail gracefully and mention why.

The new option semantics [are explained here](https://github.com/git/git/commit/1f0c3a29da3515d88537902cd267cc726020eea5).

Note: my local git versions are currently too old to test the new functionality, so I've only verified the failure case.

💬 achow101 commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1507674903)
ACK b5585ba5f97a19d1b435d9ab69b5a55cfd45dd70

🚀 achow101 merged a pull request: "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns"
(https://github.com/bitcoin/bitcoin/pull/27374)

🤔 jonatack reviewed a pull request: "Add wallet method to detect if a key is "active""
(https://github.com/bitcoin/bitcoin/pull/27216#pullrequestreview-1384012329)
ACK with some suggestions below.

The release note and added test coverage could optionally all be in separate commits, provided each commit is hygienic, i.e. builds and all tests pass. It's a bit easier for reviewers if updates needed for tests to pass with a change are in the same commit, while additional coverage is in separate commits.

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1165903530)
```suggestion
address is derived from an active descriptor or HD seed. (#27216)
```

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1165902595)
Per `doc/release-notes-empty-template.md` would suggest

```
Updated RPCs
------------
```

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1165908531)
- Suggestion

```suggestion
- RPC `getaddressinfo` now returns a new boolean field `"isactive"`, which is `true` if the
```

- I think it may be good to briefly explain *why* this field is added, i.e. maybe something roughly like: "Keys become non-active if derived before the wallet was encrypted or imported using RPCs `importprivkey`, `importaddress`, or `importpubkey`. The new field is intended to help users avoid using such keys to receive new transactions, as they can be exposed or com
...

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1165915371)
Unsure, but seems like this doc change could be a separate commit if it is an unrelated fix/improvement.

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1166088185)
nit, prefer named args when multiple

```diff
- addrs = nodes[0].deriveaddresses(desc, [0, 9])
+ addrs = nodes[0].deriveaddresses(descriptor=desc, range=[0, 9])
else:
list_descriptors = nodes[0].listdescriptors()
for desc in list_descriptors["descriptors"]:
if desc['active'] and not desc["internal"] and desc["desc"][:4] == "wpkh":
- addrs = nodes[0].deriveaddresses(desc["desc"], [0, 9])
+
...

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1166089789)
```suggestion
nodes[0].importpubkey("02f893ca95b0d55b4ce4e72ae94982eb679158cb2ebc120ff62c17fedfd1f0700e", "label", rescan=False)
```

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1166089491)
```suggestion
nodes[0].importaddress("bcrt1q95gp4zeaah3qcerh35yhw02qeptlzasdtst55v", "label", rescan=False)
```

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1166092789)
It may be nice to add a comment on why the choice of 50 here.

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1166097250)
My understanding is that [[[nodiscard]]](https://en.cppreference.com/w/cpp/language/attributes/nodiscard) only needs to be declared in the declaration (i.e. in the .h file here), not the definition as well (idem for the other functions where it has been added to the definitions in addition to the declarations).

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1166118176)
Another implementation for fun!

```diff
- int num_script_keys_not_found = 0;
- for (const CScript& script : scripts4) {
- if (!wallet.IsDestinationActive(WitnessV0ScriptHash(script))) {
- ++num_script_keys_not_found;
- }
- }
+ int num_script_keys_not_found = std::accumulate(scripts4.cbegin(), scripts4.cend(), 0, [&wallet] (int n, const CScript& s) {return wallet.IsDestinationActive(WitnessV0ScriptHash(s)) ? n : ++n; });
```

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1507805195)
Update changes:
- Split commit in 2 (as suggested by @stickies-v and @vasild): one for the fix and one for the logic improvement.
- Added some comments to each commit (as suggested by @stickies-v).

✅ 1440000bytes closed a pull request: "doc: remove incorrect line from example"
(https://github.com/bitcoin/bitcoin/pull/27454)

💬 ajtowns commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1507916357)
There are a two cases where the same name is valid both as a positional argument and in the `options` object: `send` and `sendall` both have `conf_target`, `estimate_mode` and `fee_rate` as named/positional parameters as well as `options` members (`fee_rate` explicitly, the others via `FundTxDoc`). Both those RPCs are marked as "EXPERIMENTAL", so perhaps it would be good to change their API now?

I believe:

```diff
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -493,6 +493,14 @@ RPCHel
...