💬 mzumsande commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1160180267)
This moves the creation of the ZMQ interface into a `for` loop, which I think can be executed twice under some special conditions (`-reindex` is activated by a GUI user).
However, `g_zmq_notification_interface` is just a simple pointer, not a `std::unique_ptr`, so I think that there could be a memory leak with the first object never being deleted if we create `CZMQNotificationInterface::Create` a second time.

🤔 jonatack reviewed a pull request: "Add wallet method to detect if a key is "active""
(https://github.com/bitcoin/bitcoin/pull/27216#pullrequestreview-1373758349)
Concept ACK, tested and reviewed each commit. Some thoughts and questions follow.

> This PR also patches PKDescriptor from https://github.com/bitcoin/bitcoin/pull/22051 where matching public keys were not returned.

I didn't see where/how; can you help or explain?

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1159068107)
45d565073cab12b6107aae9d08048 nit, remove unnecessary local object while still keeping code readability.

(Also if we're not sure the element exists or its absence would be a logic error, maybe `s/keys[0]/keys.at(0)` to have bounds checks, as accessing a nonexistent element with `vector#operator[]` is UB).

```diff
- CKeyID id = keys[0].GetID();
- out.pubkeys.emplace(id, keys[0]);
+ out.pubkeys.emplace(/*CKeyID=*/keys[0].GetID(), /*CPubKey=*/keys[0]);
```

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

- add missing conditional brackets (see developer notes)
- avoid unneeded `const CKeyMetadata& meta` overhead in the loop, or...

```diff
LOCK(cs_KeyStore);
-
- // Not in the keystore at all
- if (!IsMine(script)) return false;
+ if (!IsMine(script)) return false; // not in the keystore at all

for (const auto& key_id : GetAffectedKeys(script, *this)) {
- auto it = mapKeyMe
...

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1160257900)
https://github.com/bitcoin/bitcoin/commit/45d565073cab12b6107aae9d08048d5310d752d4 If they are thread-safe, should the `IsKeyActive` member functions be `const`?

<details><summary>diff</summary><p>

```diff
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 9afd2538b0c..87ab7cc034b 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp

-bool LegacyScriptPubKeyMan::IsKeyActive(const CScript& script)
+bool LegacyScriptPubKeyMa
...

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1160272170)
81f29f03a607cbfb7162705 I'm not sure if or when legacy wallet support is expected to be phased out, but if isactive is only different from ismine in legacy wallets, is it worth adding a field, versus just updating the ismine field documentation in the help for descriptor wallets? (When I'm double-checking an address with getaddressinfo before using it to receive, ismine is a field I'm already checking.)

Otherwise, it might be handy for cli users to order isactive just after ismine, and in gen
...

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1160291829)
in general can use `BOOST_CHECK_EQUAL` for equality checks, if you prefer
```suggestion
BOOST_CHECK_EQUAL(scripts1.size(), 84);
```

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

- avoid copy

```suggestion
const CScript& script{GetScriptForDestination(dest)};
```

- remove extra newline, and another implementation for the fun of it

```diff
bool CWallet::IsDestinationActive(const CTxDestination& dest) const
{
- CScript script = GetScriptForDestination(dest);
- for (const auto& spk_man : GetActiveScriptPubKeyMans()) {
- if (spk_man->IsKeyActive(script))
...

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1160329162)
Might be good to explain this "or will be" change.

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1160298313)
81f29f03a607cbfb7162705 conditional brackets and prefix increment operator per developer notes, drop unneeded localvar

```diff
- WitnessV0ScriptHash scripthash(script);
- if (!wallet.IsDestinationActive(scripthash))
- num_script_keys_not_found++;
+ if (!wallet.IsDestinationActive(WitnessV0ScriptHash(script))) {
+ ++num_script_keys_not_found;
+ }
```

💬 jonatack commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1160327631)
https://github.com/bitcoin/bitcoin/commit/81f29f03a607cbfb7162705c1c1618ca7b59640e style nit, either omit the `== False` and the 3 `== True` above, or use `assert_equal`

👍 theStack approved a pull request: "wallet: coin selection, don't return results that exceed the max allowed weight"
(https://github.com/bitcoin/bitcoin/pull/26720#pullrequestreview-1375685000)
Code-review ACK 25ab14712b9d80276016f9fc9bff7fb9c1d09635

💬 ajtowns commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1499931992)
> Right, so I'd be correct in saying that based on blocks from all mining pools generally containing about a dozen Extra Transactions on your miningpool.observer site, blocks generally contain about a dozen transactions not previously broadcast that compact blocks can't accellerate.

No, you wouldn't. Of the last 460 blocks, 375 (81%) required no additional txs, and an additional 67 (14%) required only one or two additional transactions. All of the 8 Luxor pool blocks in that set required betw
...

💬 1440000bytes commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1499987244)
> The [BIP35](https://github.com/bitcoin/bips/blob/master/bip-0035.mediawiki) mempool P2P message can be used to share the txids in a node's mempool (motivation in BIP35). However it is no longer used (?), is bad for privacy, and we can simplify our P2P code by removing it.

It is still possible to guess mempool transactions using `getdata` by maintaining mempool with no restrictions. I dont think sharing mempool affects privacy particularly in this case as its behind `NetPermissionFlags::Memp
...

🚀 fanquake merged a pull request: "ci: Run base install at most once"
(https://github.com/bitcoin/bitcoin/pull/27429)

💬 fanquake commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1500021194)
We can have a followup, to address any further issues/improvements.

🚀 fanquake merged a pull request: "contrib: allow multi-sig binary verification v2"
(https://github.com/bitcoin/bitcoin/pull/27358)

💬 josibake commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1160562186)
gotcha, thanks for the explanation!

💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1500100974)
> > If we overshoot the 8 connections (like described above), then for some time we will have more, until eviction kicks in. How much time is that exactly? If that is short then it would be sub-optimal to open a IPv4 connection and drop it shortly after in cases where Tor connectivity is working smoothly. If it is long then we have practically increased the limit of 8 which will cause more traffic.
>
> Since we already have short-lived connections through feelers, rotating extra block relay o
...

🤔 josibake reviewed a pull request: "MiniTapscript: port Miniscript to Tapscript"
(https://github.com/bitcoin/bitcoin/pull/27255#pullrequestreview-1376054310)
Left some style nits, nothing important. Might want to run `clang-tidy`, as it ended up reformatting a lot of the new code. Also got a few warnings, not quite sure about the `FromPKBytes` warning

Still trying to wrap my head around the logic; I'll follow up with a (hopefully) more helpful review