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

💬 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_r1166323704)
I think that this is a good idea, but would out of the scope of this PR and would warrant more discussion, since then it would be notifying for tx state updates that it previously did not.

💬 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_r1166323841)
Done

🤔 ajtowns reviewed a pull request: "rpc: Add importmempool RPC"
(https://github.com/bitcoin/bitcoin/pull/27460#pullrequestreview-1384593990)
Approach ACK

💬 ajtowns commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166274761)
Maybe reference `prioritisetransaction` here explicitly?

💬 ajtowns commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166325126)
Should this also include the ability to override the (added to the mempool) `nTime` data with the current time (or an explicitly nominated time)?

Having a child transaction with an earlier `nTime` than its parent transaction might be confusing. I think the only way that can currently happen is with system/mock time shenanigans.