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

💬 stickies-v commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1153157545)
A bit more readable and avoids default initializing CServices:

```suggestion
services.reserve(addresses.size());
for (const auto& addr : addresses)
services.emplace_back(addr, port);
```

💬 stickies-v commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1153142035)
The comment was on an older commit, so apologies if I'm missing something, but I'm not sure it's fully addressed? In `if (i->first.empty() || (addr.has_value() && addr->IsBindAny()))`, I think the first `i->first.empty()` is indeed still superfluous as theStack pointed out.

🚀 fanquake merged a pull request: "ci: use LLVM/clang-16 in native_fuzz (ASAN) job"
(https://github.com/bitcoin/bitcoin/pull/27363)