💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1160161770)
re: https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150538791

> This method was removed from the cpp but not from the header.

Thanks applied your suggested change (https://github.com/furszy/bitcoin-core/commit/2195b1e3288f1019f81fc76c584519bd1d158f59)

💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1160160932)
re: https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150535358

Sure, I think it would make sense to clear this flag when a transaction is abandoned. I think this could be a separate issue, and it should be pretty easy to implement anytime IMO

💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1160135250)
re: https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150541159

> Would be good to keep the encapsulation and don't access `m_address_book` from outside of the wallet class (otherwise we are reverting #25337 and, future, #26836).

Thanks, applied your change (https://github.com/furszy/bitcoin-core/commit/2195b1e3288f1019f81fc76c584519bd1d158f59)

💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1159265608)
re: https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150572914

> ```c++
> BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::::LOAD_OK);
> ```

Thanks, added this check

💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1160138608)
re: https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150586523

> ```c++
> BOOST_CHECK(!wallet->IsAddressUsed(PKHash()));
> BOOST_CHECK(!wallet->IsAddressUsed(ScriptHash()));
> ```

Thanks applied your suggested change (https://github.com/furszy/bitcoin-core/commit/2195b1e3288f1019f81fc76c584519bd1d158f59)

💬 theuni commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1160211457)
Makes sense to me but I'd rather keep the changes down in this PR. Let's do as a follow-up?

📝 pinheadmz opened a pull request: "validation: implement MaybeInvalidateFork() and call from rpc getchaintips"
(https://github.com/bitcoin/bitcoin/pull/27434)
Closes https://github.com/bitcoin/bitcoin/issues/8050

If we discover an invalid block on a blockchain branch with otherwise valid headers, all the child headers can be marked as INVALID_CHILD. When invalidating mainchain blocks we can easily mark all the invalid children but on a headers-only branch we just stop on the invalid block and search for the next best tip and branch to validate. On restart, during `LoadBlockIndex()` we iterate through all the headers we know about sorted by height,
...

💬 pinheadmz commented on issue "getchaintips doesn't mark headers-only chain as invalid":
(https://github.com/bitcoin/bitcoin/issues/8050#issuecomment-1499583446)
> I'm looking into this

Draft PR: https://github.com/bitcoin/bitcoin/pull/27434

💬 josibake commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1499652736)
ACK https://github.com/bitcoin/bitcoin/pull/27358/commits/754fb6bb8125317575edec7c20b5617ad27a9bdd

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