🤔 Xekyo reviewed a pull request: "wallet: coin selection, don't return results that exceed the max allowed weight"
(https://github.com/bitcoin/bitcoin/pull/26720#pullrequestreview-1375347359)
reACK 25ab14712b9d80276016f9fc9bff7fb9c1d09635

📝 Sjors opened a pull request: "getblocktemplate improvements for segwit and sigops"
(https://github.com/bitcoin/bitcoin/pull/27433)
**Sigops**

Two recent F2Pool violated the sigops limit. Both by 3.

I suspect they were not using `getblocktemplate`. If you look at the [template](https://miningpool.observer/template-and-block/00000000000000000004e0ec4f27bd3347381e8e19ed98d7f918e8c1c292ae97) right before the valid block at the same height, which was produced two minutes later, you'll see that it matches the block with 3 small transactions difference. In other words, the valid block producer likely did use `getblocktemplat
...

💬 LarryRuane commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1499452625)
ACK 93c70287a6434c6c665a211dc4dfbbd9c3db4083

💬 stratospher commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1499478379)
Thanks a lot @sdaftuar! I've used the simplified approach in your patch!

> Counting multiple sources from the same alternative network, they can fill up a theoretical maximum of 256 buckets (~16k addresses) or 1/4 of the new tables I think (but due to collisions from the the modulo operations in GetNewBucket and GetBucketPosition that depend on nKey, it is typically more like ~200 buckets in reality).
On the other hand, multiple IPv4 and IPv6 sources can fill up all 1024 buckets in the new
...

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