💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126736075)
'outpoints' naming shadows the function's arg. Could
```c++
for (const auto& [txid, _] : m_requested_outpoints_by_txid) {
```

💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1456850195)
ReACK 6a302d4
Thanks for addressing feedback

📝 amitiuttarwar opened a pull request: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213)
This is joint work with mzumsande.

The current logic that makes automatic outbound connections doesn’t try to diversify with respect to reachable networks - the decision to connect to a particular address from `AddrMan` is based purely on the frequency of available addresses.

For instance, if `AddrMan` is populated primarily with IPv4 and IPv6 addresses and only a handful of onion addresses, it is likely that we won't establish any automatic outbound connections to Tor, even if we're cap
...

📝 amitiuttarwar opened a pull request: "addrman: Enable selecting addresses by network "
(https://github.com/bitcoin/bitcoin/pull/27214)
For the full context & motivation of this patch, see #27213

This is joint work with mzumsande.

This PR adds functionality to `AddrMan::Select` to enable callers to specify a network they are interested in.

Along the way, it refactors the function to deduplicate the logic, updates the local variables to match modern conventions, adds test coverage for both the new and existing `Select` logic, and adds bench tests for the worst case performance of both the new and existing `Select` logic.
...

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1456867760)
We have 3 open questions that we are interested in hearing feedback from reviewers. We have considered many potential interactions and have laid out a couple specific ones along with tradeoffs of different approaches.

### IBD
The first is regarding the interactions between this patch & IBD. Peers on privacy network might be slower than clearnet peers (eg. Tor or I2P). Our current proposal does not have any special case logic for disabling the network prioritization during IBD.

This coul
...

💬 brunoerg commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#issuecomment-1456947717)
@theStack have in mind re-ACK it?

📝 achow101 opened a pull request: "wallet: Turn `destdata` entries into `CAddressBookData` fields"
(https://github.com/bitcoin/bitcoin/pull/27215)
`destdata` is a generic strings map in `CAddressBookEntry`. It is opaque and thus hard to know what is actually being stored in a `CAddressBookEntry`. It is a map with fixed fields, but these are not documented nor are they immediately obvious when looking at the class definition. This PR changes those to be member variables inside of `CAddressBookEntry` to make it easier to understand what kind of data is being stored.

👍 theStack approved a pull request: "wallet: group outputs only once, decouple it from Coin Selection"
(https://github.com/bitcoin/bitcoin/pull/25806)
re-ACK 6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44 :coconut:

(verified via `git range-diff 0bb90cab...6a302d40` that since my last ACK, most review comments by Murch and S3RK have been tackled, mostly trivial refactor + terminology/naming improvements and [a minor unit test adaption](https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126088720))

📝 pinheadmz opened a pull request: "Add wallet method to detect if a key is "fresh""
(https://github.com/bitcoin/bitcoin/pull/27216)
This is a first step towards closing https://github.com/bitcoin/bitcoin/issues/3314 by implementing functions in Wallet and ScriptPubKeyMan that determine if a key is "still in the keypool" aka "fresh". This wording and the function names may need some brainstorming because the concept of "used" keys already has a few meanings, especially the `avoid_reuse` flag.

A key is "still in the keypool" or "fresh" if:
- it has never been seen on chain
- its index does not precede any keys seen on cha
...

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1127069573)
Great, thanks

💬 brunoerg commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1127073061)
nit (for 9bf078f66c8f286e1ab5e34b8eeed7d80290a897):
```cpp
diff --git a/src/addrman.cpp b/src/addrman.cpp
index ec5b0213b..f608d60f0 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -722,15 +722,13 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool newOnly) const
if (newOnly && nNew == 0) return {};

// Decide if we are going to search the new or tried table
- bool search_tried;
+ bool search_tried{false};

// Use a 50% chance for choosing betw
...

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1127076941)
Good catch

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1127077341)
Okay, added

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1127079196)
You’re right, fixed

💬 brunoerg commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1127079909)
perhaps rename `newOnly` to `new_only`?

💬 brunoerg commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1127083746)
Suggestion:
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index 8c36af13f..66d54d2b4 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -746,8 +746,7 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool newOnly, std::optiona
search_tried = insecure_rand.randbool();
}

- int bucket_count;
- search_tried ? bucket_count = ADDRMAN_TRIED_BUCKET_COUNT : bucket_count = ADDRMAN_NEW_BUCKET_COUNT;
+ const int bucket_count{search_tried ? ADDRMAN_TRIE
...

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1127091583)
I decided to leave it as is for now.

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1457092354)
Changes since https://github.com/bitcoin/bitcoin/commit/32123cfc11ad22cdd7fe5ef26ac0550e1ab2fe7b:
- Addressed remaining review from @LarryRuane
- Fixed crash bug surfaced by @dergoegge
- Addressed review by @furszy

📝 achow101 opened a pull request: "wallet: Replace use of purpose strings with an enum"
(https://github.com/bitcoin/bitcoin/pull/27217)
Instead of storing and passing around fixed strings for the purpose of an address, use an enum.

💬 achow101 commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1457199020)
ACK 6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44