💬 vasild commented on pull request "p2p: Improve diversification of new connections":
(https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1490154556)
How? I opened https://github.com/bitcoin/bitcoin/issues/27369 to discuss ideas on how to approach this.

💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1153139259)
https://cirrus-ci.com/task/4526522288046080
seems like the tidy job wants you to add this:
```suggestion
std::vector<uint256> txids_needed;
txids_needed.reserve(m_requested_outpoints_by_txid.size());
```

💬 stratospher commented on issue "Fix net grouping of non-IP networks":
(https://github.com/bitcoin/bitcoin/issues/27369#issuecomment-1490173773)
https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1490154556 - workaround it by not inserting tor/i2p/cjdns address in `setConnected` for now? so `GetGroup` wouldn't be called on these networks.

💬 Sjors commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#discussion_r1153151060)
Oh you're right, I missed the last push.

In the majority of cases we don't use a space after `=*/`, but that's not worth touching the PR for imo.

💬 Sjors commented on pull request "refactor: Drop no longer used `CNetMsgMaker` instances":
(https://github.com/bitcoin/bitcoin/pull/27368#issuecomment-1490183368)
ACK ea7ec7808745805c0a18513d7da271dedb2de3f1

👍 Sjors approved a pull request: "refactor: remove unused param from legacy pubkey interface"
(https://github.com/bitcoin/bitcoin/pull/27274)
ACK 1869310f3cfa4ab26b5090d8a4002eefdc84870e

⚠️ osas90 opened an issue: "Import in Bitcoin Core or a different software? What are the steps to reproduce?"
(https://github.com/bitcoin/bitcoin/issues/27370)
I got a private key in bitcoin on my blockchain.com for one of my receiving addresses. When I went to import it, it imported another address but i already used it as receiving addresses now funds now available...

_Originally posted by @MarcoFalke in https://github.com/bitcoin/bitcoin/issues/17846#issuecomment-570241898_

✅ fanquake closed an issue: "Import in Bitcoin Core or a different software? What are the steps to reproduce?"
(https://github.com/bitcoin/bitcoin/issues/27370)

:lock: fanquake locked an issue: "Import in Bitcoin Core or a different software? What are the steps to reproduce?"
(https://github.com/bitcoin/bitcoin/issues/27370)

💬 ismaelsadeeq commented on pull request "test: refactor: dedup mempool_package_limits.py subtests via decorator":
(https://github.com/bitcoin/bitcoin/pull/27350#issuecomment-1490199581)
ACK e669833943bda13b2840a174dc8e45194187fc8e

💬 Sjors commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1490210669)
This is definitely nicer. When you doubleclick on the zip file it magically reveals the orange icon:

<img width="612" alt="Scherm­afbeelding 2023-03-30 om 14 15 54" src="https://user-images.githubusercontent.com/10217/228833150-647e3460-c174-43d0-b026-c9dbffb10b64.png">

<img width="845" alt="Scherm­afbeelding 2023-03-30 om 14 16 10" src="https://user-images.githubusercontent.com/10217/228833181-c82d0639-91a3-476f-a309-e9732577a657.png">

The only thing we need to remind the user of, is t
...

💬 Sjors commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1490218629)
I'd like to test the Guix build too. @achow101 can you sign it? Or is there a way to self-sign it?

💬 Sjors commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#discussion_r1153198925)
💿

💬 dergoegge commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1153208873)
No reason, it should be in `TxRelay`, which also results in a small resource optimization because that filter now only exists for tx relay peers. Thanks!

🚀 fanquake merged a pull request: "refactor: Drop no longer used `CNetMsgMaker` instances"
(https://github.com/bitcoin/bitcoin/pull/27368)

👍 stickies-v approved a pull request: "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`"
(https://github.com/bitcoin/bitcoin/pull/26261)
re-ACK 30efd6d36

I think my comments aren't too controversial to address but they only affect documentation and style/readability so I'm happy with the status quo and a follow-up, too. We've had quite a bit of back and forth already on this PR.

💬 stickies-v commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1140315012)
There's another such update needed for the `LookupNumeric` docstring which currently reads:

```
* @see Lookup(const std::string&, std::vector<CService>&, uint16_t, bool, unsigned int, DNSLookupFn)
* for additional parameter descriptions.
```

💬 stickies-v commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1153145784)
```suggestion
const CService ui_proxy{ui_proxy_netaddr.value_or(CNetAddr{}), ui->proxyPort->text().toUShort()};
```

💬 stickies-v commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1153136412)
Largely unrelated, so just leaving this here for follow-up. I think this can be cleaned up quite a bit:

<details>
<summary>git diff</summary>

```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 49c2a85e1..46d095633 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -311,17 +311,17 @@ static bool HTTPBindAddresses(struct evhttp* http)
}

// Bind addresses
- for (std::vector<std::pair<std::string, uint16_t> >::iterator i = endpoints.begin(); i
...

💬 stickies-v commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1153170805)
This docstring is updated in ab4257be34209fd61efb409593dc7aa21dba0fb7, but there it still returns a `bool`. The signature is only updated to return `std::optional<CNetAddr>` in 30efd6d36e24c83f5c094e30f8cb25e66e913d73

Moreover, the function never returns `addresses`, it's only ever singular. I also don't like the phrasing of 'empty', so would prefer updating this to:
```suggestion
* @returns The resulting network address to which the specified host
* string resolved or std::null
...