Instead of a gethostname hack, which is not guaranteed to return all addresses, use the official way of calling GetAdaptersAddresses to get local network addresses on Windows.
Do the same checks as the UNIX path: interface is up, interface is not loopback.
Suggested by Ava Chow.
Addiional changes:
FromSockAddr in netif.cpp from MacOS and use it everywhere appropriate. This avoids code duplication.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31014.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | davidgumberg, achow101 |
| Concept ACK | 1440000bytes, jonatack |
| Stale ACK | sipa |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
Instead of a gethostname hack, use the official way of calling GetAdaptersAddresses to get local network addresses on Windows.
Concept ACK
268@@ -276,9 +269,46 @@ std::vector<CNetAddr> GetLocalAddresses()
269 {
270 std::vector<CNetAddr> addresses;
271 #ifdef WIN32
272- char pszHostName[256] = "";
273- if (gethostname(pszHostName, sizeof(pszHostName)) != SOCKET_ERROR) {
274- addresses = LookupHost(pszHostName, 0, true);
275+ DWORD status = 0;
276+ constexpr size_t MAX_ADAPTER_ADDR_SIZE = 4 * 1000 * 1000; // Absolute maximum size of adapter addresses structure we're willing to handle, as a precaution.
Can anyone review here please?
Mind that the first commit “Add optional length checking to CService::SetSockAddr” is also part of #31022.
Edit: needs rebasing for #31022 (review) assuming that goes in first
Edit.2: done, took over the same commit, network address length determination logic was moved to FromSockAddr for the unknown-length case
296+ if (status == ERROR_BUFFER_OVERFLOW && out_buf.size() < MAX_ADAPTER_ADDR_SIZE) {
297+ // If status == ERROR_BUFFER_OVERFLOW, out_buf_len will contain the needed size.
298+ // Unfortunately, this cannot be fully relied on, because another process may have added interfaces.
299+ // So to avoid getting stuck due to a race condition, double the buffer size at least
300+ // once before retrying (but only up to the maximum allowed size).
301+ do {
The loops seems unnecessary to me.
0out_buf.resize(std::min(std::max(out_buf_len, out_buf.size() * 2), MAX_ADAPTER_ADDR_SIZE));
and have not tested it.
FWIW, i have tested this on a windows machine. Though a virtual one on amazon EC. Both IPv4 and IPv6 addrsses were picked up.
Of course, more testing is very welcome.
30@@ -31,6 +31,30 @@
31
32 namespace {
33
34+//! Return CNetAddr for the specified OS-level network address. If a length is provided, it is checked for validity.
0If a length is provided, it is checked for validity.
Maybe it would be good to mention that it is checked for validity inside of SetSockAddr(), or is this detail that the caller of FromSockAddr() does not need to know?
Right, the logic here has been swapped since this comment was written 😄
i think that sentence is redundant. That the length is checked is implied, why pass it otherwise? The line below already describes which length is used when it’s not passed.
Tested crACK https://github.com/bitcoin/bitcoin/pull/31014/commits/851085a7f2761584402080b1767328d86cde4d94
Nice refactor in reusing FromSockAddr() and probably makes local address discovery on Windows less brittle.
Ran this branch and master on a local Windows 10 22H2 machine with multiple network adapters, both Discover()‘ed the same IPv6 addresses from all adapters, which matched up with what ifconfig reported as the machine’s addresses. The device does not have any IPv4 addresses which pass CNetAddr::IsRoutable() so no Discover() messages were logged for IPv4 so I have not tested IPv4.
Thanks for testing!
DrahtBot shouldn’t have re-requested review, i first need to address sipa’s comment in the first place.
DrahtBot shouldn’t have re-requested review, i first need to address sipa’s comment in the first place.
For outstanding review comment the bot waits for the next ack to happen, after which it assumes anything outstanding to be addressed. It is not perfect, but I can’t come up with something better, and it seems fine as long any other reviewer notices.
For outstanding review comment the bot waits for the next ack to happen, after which it assumes anything outstanding to be addressed. It is not perfect, but I can’t come up with something better, and it seems fine as long any other reviewer notices.
Yes the heuristic’s usually correct, don’t know anything better. Was just noting it, as github has no way to un-request review.
GetAdaptersAddresses (@sipa)FromSockAddr more concise (@davidgumberg)
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/37163426494
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Instead of a `gethostname` hack, use the official way of calling
`GetAdaptersAddresses` to get local network addresses on Windows.
As additional cleanup, move out `FromSockAddr` from MacOS and use it
everywhere appropriate.
Suggested by Ava Chow.
@davidgumberg Looks like you reviewed a stale commit?
Oops.
utreACK https://github.com/bitcoin/bitcoin/commit/b9d4d5f66a5a35c47e7abc9ec6ef5ab242b3f1e1
Recent rebase makes a comment change, and removes an unnecessary loop, addressing reviewer feedback.
