💬 luke-jr commented on pull request "Remove confusing "Dust" label from coincontrol dialog":
(https://github.com/bitcoin-core/gui/pull/719#issuecomment-1616149064)
Concept ACK, but I see no change from the second commit. Only the coincontrol dialog's "Dust" field disappears - nothing else differs in my screenshots.

💬 luke-jr commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1248991477)
Probably not good to ignore it never returning... it's leaking resources. Maybe kill it and/or tell the user somehow? :/

💬 luke-jr commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1248993583)
Won't this still make an externally-visible DNS request?

💬 Crypt-iQ commented on issue "depends build fails macOS intel":
(https://github.com/bitcoin/bitcoin/issues/27977#issuecomment-1616218854)
I guess the issue is that I figured depends would work out of the box without me trying to mess with anything?

💬 Crypt-iQ commented on issue "bitcoind hangs waiting for `g_requests.empty()`":
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1616244103)
> [Thread 0x7ffec0f4b700 (LWP 1828755) exited]
> bitcoin-qt: httpserver.cpp:223: http_request_cb(evhttp_request*

Do you happen to know what libevent version you were on?

💬 dscotese commented on issue "depends build fails macOS intel":
(https://github.com/bitcoin/bitcoin/issues/27977#issuecomment-1616266896)
I think it should too, but since you're on github, it makes sense to explore when expectations aren't met and propose solutions as we find them.

💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1616438227)
Looks like a slightly non trivial rebase. For context, which PR's triggered it?

💬 Sjors commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1616447514)
A bisect would be ideal. Though you could start with recent pull requests that touch net(_processing).cpp and were not backported to the v25.0 release. E.g. #27626 and #27625.

📝 JayBitron opened a pull request: "exclude ipc scheme from port check "
(https://github.com/bitcoin/bitcoin/pull/28020)
Previous PR https://github.com/bitcoin/bitcoin/pull/22087 cause new error, makes it impossible to use ipc protocol using zmq, this patch will exclude port checking on ipc urls.

💬 nberlin1980 commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1249523602)
This will remove `NET_UNROUTABLE ` and `NET_INTERNAL`, after which they cannot be re-added. Is that intentional (the function name implies it is intentional)? I ask because the other accessors guard against their removal and insertion.

💬 nberlin1980 commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1249529700)
Perhaps add test cases for `RemoveAll` as it relates to these two.

💬 mzumsande commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1616786805)
now the previously unaffected coinstatsindex_tests fails intermittently:
https://github.com/bitcoin/bitcoin/pull/27746/checks?check_run_id=14710590806
(doesn't seem to be related to the PR)
```
72023-07-02T07:40:59.796756Z (mocktime: 2020-08-31T15:34:test/util/index.cpp:15 IndexWaitSynced: Assertion `timeout > SteadyClock::now()' failed.
```

💬 Ayush170-Future commented on pull request "init: adding check for : for -torcontrol flag":
(https://github.com/bitcoin/bitcoin/pull/28018#issuecomment-1616813496)
Overall, I think the code is good. However, there have been some earlier remarks in this [comment](https://github.com/bitcoin/bitcoin/pull/23605#issuecomment-980045955) on where to place this code.

Someone with a better understanding of the entire codebase should comment on the placement of this.

In any case, I'm testing it locally and will report back on whether it meets the requirements soon.

💬 luke-jr commented on pull request "Prevent file descriptor exhaustion from too many RPC calls":
(https://github.com/bitcoin/bitcoin/pull/27731#discussion_r1249842846)
I'm thinking of the case where you build with 2.2.1, then try to run it with 2.1.12. (If that's not supported, the runtime check is unnecessary)

💬 luke-jr commented on pull request "init: deduplicate added connections":
(https://github.com/bitcoin/bitcoin/pull/27804#issuecomment-1617016570)
Weak approach NACK. This feels like a hacky workaround for what is ultimately a race condition. Rather we just address the cause of the issue, instead of introducing weird variables (eg, what happens if the `-connect`ion is lost, and the user use the `addnode` RPC right at the correct moment?).

💬 luke-jr commented on pull request "Blocking arguments -nohelp, -noh, and -no?":
(https://github.com/bitcoin/bitcoin/pull/27814#issuecomment-1617019869)
nit: Commit summary is too long. Would also be nice to rebase on top of 8afa602f308ef003bb6893718eae1fe5a830690c

💬 luke-jr commented on pull request "CLI: Only one Request Handler can be specified.":
(https://github.com/bitcoin/bitcoin/pull/27815#issuecomment-1617024007)
nit: Commit summary is too long. Would also be nice to rebase on top of 42af9596ce85a541988abee54eed8a9b271a46a1

🤔 luke-jr requested changes to a pull request: "Supporting parameter "h" and "?" in -netinfo."
(https://github.com/bitcoin/bitcoin/pull/27830#pullrequestreview-1510054476)
Concept NACK. This seems to be for `bitcoin-cli -netinfo h`, which is weird and there's no reason to expect that to work.

💬 kevkevinpal commented on pull request "init: adding check for : for -torcontrol flag":
(https://github.com/bitcoin/bitcoin/pull/28018#issuecomment-1617123715)
> Overall, I think the code is good. However, there have been some earlier remarks in this [comment](https://github.com/bitcoin/bitcoin/pull/23605#issuecomment-980045955) on where to place this code.
>
> Someone with a better understanding of the entire codebase should comment on the placement of this.
>
> In any case, I'm testing it locally and will report back on whether it meets the requirements soon (one suggestion I think you should avoid using `/` in the branch name)

Yea I could t
...

👍 TheCharlatan approved a pull request: "test: move remaining rand code from util/setup_common to util/random"
(https://github.com/bitcoin/bitcoin/pull/27425#pullrequestreview-1510492158)
ACK 5b3f6a49e6bda9f5a0ec261f3f8900e4978e3a1a