💬 theuni commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1499550445)
Thanks for the review @TheCharlatan and @josibake. I've addressed most of the feedback with the exception of a few nits as I don't want this to drag out forever.

I squashed some of the readme changes to keep the number of commits down. The diff from the previous changes can be seen with: `git diff 96344222f3..754fb6bb81` .

🤔 ryanofsky reviewed a pull request: "refactor: Remove CAddressBookData::destdata"
(https://github.com/bitcoin/bitcoin/pull/27224#pullrequestreview-1373581514)
Updated 0ad6a533b93162ec808de092cbe5edc31a27a390 -> d34323544d7283bf46bb154a90b8feca443b103e ([`pr/nodest.18`](https://github.com/ryanofsky/bitcoin/commits/pr/nodest.18) -> [`pr/nodest.19`](https://github.com/ryanofsky/bitcoin/commits/pr/nodest.19), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nodest.18..pr/nodest.19)) applying most of the suggested changes. Thanks for the reviews!

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

> you have a reason to prefer `Span<std::byte>`?

Well it should actually take a span of const bytes, so I fixed this to add const.

Passing a span makes more sense than passing a datastream because the function just needs a read-only array of bytes, it doesn't need a read/write vector or a stream object or anything more complicated.

The other functions are using CDataStream just because they are called by older code w
...

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

> to make this an `rvalue`. there is likely a cleaner way to do this, if you end up going with `DataStream`

Thanks, responded to earlier comment

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

> nit: could use structured bindings.

Thanks, applied suggestion

💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1160150468)
> No longer the case. Same for the comment that is above this one.

Thanks, I basically just replaced "destdata" with "address data" in these comments, but you can let me know if the issue was something else

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