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

💬 ajtowns commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166327231)
Change "the file" to "a mempool.dat file" ?

Maybe reference the "savemempool" rpc as a way of updating mempool.dat with the current mempool?

💬 ajtowns commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1166274209)
Why would either of these "corrupt" the mempool state? They definitely might be undesirable ("unbroadcast" txs might reveal which node just loaded the mempool state, and "fee delta" may allow an attacker to pin txs to your mempool that will never be mined, or to get you to mine unprofitable txs), but the problems should be localised and not result in "corruption", no?

💬 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_r1166333715)
> The lambda function's wtx shadows the function's wtx.

I have renamed the lambda function's `wtx`.
> Could just provide disconnect_height.

By this do you mean providing the `disconnect_height` of the block to `RecursiveUpdateTxState` or providing the wallet tx's `conflicting_block_height` to `try_updating_state` instead of the whole wallet tx by reference?

💬 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#issuecomment-1507990460)
Thanks for the review @achow101 and @furszy!
> Haven't finished checking the latest changes but, if you like it, https://github.com/furszy/bitcoin-core/commit/8bb14c1eaf09b0bca9efa78e1a7a7a00c61c0a29 is all yours. During the review, the functional test wasn't clear enough for me so I spent a bit of time making it more friendly.

Thanks, I'll use these improvements here once I review them.