💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1499500913)
Fixed co-authorship attribution to @furszy, no other changes.

💬 hebasto commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1160163588)
You're right. Going to amend the patch.

💬 theuni commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1160166441)
Thanks!

💬 theuni commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1160167278)
Thanks. I'll squashed this into another commit.

💬 Sjors commented on pull request "getblocktemplate improvements for segwit and sigops":
(https://github.com/bitcoin/bitcoin/pull/27433#issuecomment-1499509064)
The "rules" field was introduced in #7935. It's not part of [BIP 22](https://github.com/bitcoin/bips/blob/master/bip-0022.mediawiki), [BIP 23](https://github.com/bitcoin/bips/blob/master/bip-0023.mediawiki) or [BIP 145](https://github.com/bitcoin/bips/blob/master/bip-0145.mediawiki) which describe the expected behaviour of `getblocktemplate`.

It's unclear to me how it is intented to be used, both as an argument and in the results. I'm also not sure how to interpret this wording in BIP 145:

...

💬 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